fix: rename 'Subtotal' to 'Subvalue' in pivot tables#37458
Conversation
github-actions
Bot
added
i18n
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #6b3610
Actionable Suggestions - 4
-
superset/translations/zh_TW/LC_MESSAGES/messages.po - 1
- Missing translation for Subvalue · Line 10512-10512
-
superset/translations/en/LC_MESSAGES/messages.po - 1
- Incorrect subtotal translation · Line 9686-9686
-
superset/translations/it/LC_MESSAGES/messages.po - 1
- Missing Italian translation for Subvalue · Line 10467-10467
-
superset/translations/uk/LC_MESSAGES/messages.po - 1
- Incorrect translation for subtotal · Line 10558-10559
Additional Suggestions - 1
-
superset/translations/pt/LC_MESSAGES/messages.po - 1
-
Missing Portuguese translation for pivot table subtotal · Line 10599-10599The msgid was correctly updated from "Subtotal" to "Subvalue" to match the code's translation key, but the msgstr remains empty. This will cause Portuguese users to see the untranslated "Subvalue" instead of "Subtotal" in pivot table subtotals. Based on translations in Spanish ("Subtotal") and Brazilian Portuguese ("Subtotal"), "Subtotal" is the appropriate Portuguese term here.
Code suggestion
@@ -10600,1 +10600,1 @@ - msgstr "" + msgstr "Subtotal"
-
Review Details
-
Files reviewed - 25 · Commit Range:
3f6c695..3f6c695- superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.jsx
- superset/charts/client_processing.py
- superset/translations/ar/LC_MESSAGES/messages.po
- superset/translations/ca/LC_MESSAGES/messages.po
- superset/translations/de/LC_MESSAGES/messages.po
- superset/translations/en/LC_MESSAGES/messages.po
- superset/translations/es/LC_MESSAGES/messages.po
- superset/translations/fa/LC_MESSAGES/messages.po
- superset/translations/fr/LC_MESSAGES/messages.po
- superset/translations/it/LC_MESSAGES/messages.po
- superset/translations/ja/LC_MESSAGES/messages.po
- superset/translations/ko/LC_MESSAGES/messages.po
- superset/translations/messages.pot
- superset/translations/nl/LC_MESSAGES/messages.po
- superset/translations/pl/LC_MESSAGES/messages.po
- superset/translations/pt/LC_MESSAGES/messages.po
- superset/translations/pt_BR/LC_MESSAGES/messages.po
- superset/translations/ru/LC_MESSAGES/messages.po
- superset/translations/sk/LC_MESSAGES/messages.po
- superset/translations/sl/LC_MESSAGES/messages.po
- superset/translations/tr/LC_MESSAGES/messages.po
- superset/translations/uk/LC_MESSAGES/messages.po
- superset/translations/zh/LC_MESSAGES/messages.po
- superset/translations/zh_TW/LC_MESSAGES/messages.po
- tests/unit_tests/charts/test_client_processing.py
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| msgstr "氣泡尺寸" | ||
|
|
||
| msgid "Subtotal" | ||
| msgid "Subvalue" |
There was a problem hiding this comment.
The msgid has been changed from 'Subtotal' to 'Subvalue', but the msgstr remains empty. This will display 'Subvalue' to zh_TW users instead of a translated string. Please provide an appropriate translation.
Code Review Run #6b3610
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
|
|
||
| msgid "Subtotal" | ||
| msgid "Subvalue" | ||
| msgstr "" |
There was a problem hiding this comment.
The msgid was updated to "Subvalue", but the msgstr remains empty, causing pivot table subtotals to display as "Subvalue" instead of the correct "Subtotal". This affects user-facing labels in superset/charts/client_processing.py and superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.jsx.
Code suggestion
Check the AI-generated fix before applying
| msgstr "" | |
| msgstr "Subtotal" |
Code Review Run #6b3610
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| msgstr "Grandezza della bolla" | ||
|
|
||
| msgid "Subtotal" | ||
| msgid "Subvalue" |
There was a problem hiding this comment.
The msgid has been updated from 'Subtotal' to 'Subvalue' to match the string used in the code (e.g., in superset/charts/client_processing.py), but the msgstr remains empty. This could result in untranslated 'Subvalue' appearing in the UI for Italian users. Please add the appropriate Italian translation.
Code Review Run #6b3610
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| msgid "Subvalue" | ||
| msgstr "Суттєвий" |
There was a problem hiding this comment.
The msgid changed from "Subtotal" to "Subvalue", but the msgstr remained "Суттєвий" (meaning "significant"). In pivot table context, "Subvalue" refers to subtotal labels, which should be "підсумок" in Ukrainian, as confirmed by standard translations and other languages like French ("Sous-total").
Code suggestion
Check the AI-generated fix before applying
| msgid "Subvalue" | |
| msgstr "Суттєвий" | |
| msgid "Subvalue" | |
| msgstr "підсумок" |
Code Review Run #6b3610
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #37458 +/- ##
=======================================
Coverage 64.69% 64.69%
=======================================
Files 2686 2686
Lines 148607 148607
Branches 34291 34291
=======================================
Hits 96147 96147
Misses 50695 50695
Partials 1765 1765
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
costajohnt
force-pushed
the
fix/rename-subtotal-subvalue
branch
from
3f6c695 to
6919234
Compare
There was a problem hiding this comment.
Code Review Agent Run #cc19f2
Actionable Suggestions - 1
-
superset/translations/zh/LC_MESSAGES/messages.po - 1
- Missing Translation · Line 10498-10498
Additional Suggestions - 1
-
superset/translations/nl/LC_MESSAGES/messages.po - 1
-
Incorrect Dutch Translation · Line 10542-10542The msgid changed from "Subtotal" to "Subvalue", but the Dutch translation remained "Subtotaal" (subtotal). Since "Subvalue" means a subordinate value, it should be translated as "Subwaarde" in Dutch.
Code suggestion
@@ -10543,1 +10543,1 @@ -msgstr "Subtotaal" +msgstr "Subwaarde"
-
Review Details
-
Files reviewed - 25 · Commit Range:
6919234..6919234- superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.tsx
- superset/charts/client_processing.py
- superset/translations/ar/LC_MESSAGES/messages.po
- superset/translations/ca/LC_MESSAGES/messages.po
- superset/translations/de/LC_MESSAGES/messages.po
- superset/translations/en/LC_MESSAGES/messages.po
- superset/translations/es/LC_MESSAGES/messages.po
- superset/translations/fa/LC_MESSAGES/messages.po
- superset/translations/fr/LC_MESSAGES/messages.po
- superset/translations/it/LC_MESSAGES/messages.po
- superset/translations/ja/LC_MESSAGES/messages.po
- superset/translations/ko/LC_MESSAGES/messages.po
- superset/translations/messages.pot
- superset/translations/nl/LC_MESSAGES/messages.po
- superset/translations/pl/LC_MESSAGES/messages.po
- superset/translations/pt/LC_MESSAGES/messages.po
- superset/translations/pt_BR/LC_MESSAGES/messages.po
- superset/translations/ru/LC_MESSAGES/messages.po
- superset/translations/sk/LC_MESSAGES/messages.po
- superset/translations/sl/LC_MESSAGES/messages.po
- superset/translations/tr/LC_MESSAGES/messages.po
- superset/translations/uk/LC_MESSAGES/messages.po
- superset/translations/zh/LC_MESSAGES/messages.po
- superset/translations/zh_TW/LC_MESSAGES/messages.po
- tests/unit_tests/charts/test_client_processing.py
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| msgstr "气泡尺寸" | ||
|
|
||
| msgid "Subtotal" | ||
| msgid "Subvalue" |
There was a problem hiding this comment.
The msgid change from 'Subtotal' to 'Subvalue' aligns with the pot file and code usage in client_processing.py, but the msgstr remains empty. This will display untranslated 'Subvalue' to Chinese users, unlike other languages (e.g., French uses 'Sous-total'). A translation is needed to maintain consistency.
Code Review Run #cc19f2
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
costajohnt
force-pushed
the
fix/rename-subtotal-subvalue
branch
from
6919234 to
adccf26
Compare
costajohnt
force-pushed
the
fix/rename-subtotal-subvalue
branch
from
adccf26 to
c3d280e
Compare
rusackas
left a comment
rusackas
left a comment
There was a problem hiding this comment.
@costajohnt the rename itself reads fine, but test_pivot_df_single_row_null_values looks broken in this diff — both expected tables got replaced with the values (and index order) from the two-metrics test, so the nan/0 rows that test was actually checking are gone. Was that intentional, or a copy-paste while updating the markdown?
Also only labeler has run here; the Python unit tests haven't fired yet, so I'd want to see those green before merge. Last thing: "Subvalue (Sum)" is a user-facing string — happy to go with it, but want to make sure that's the agreed term and not just "Subtotal" minus the word "total." Thoughts? @yousoph might be able to weigh in here with the latest thinking.
|
| Language | Fuzzy before | Fuzzy after | New |
|---|---|---|---|
fr |
1004 | 1014 | +10 |
How to fix
1. Install dependencies (if not already set up):
pip install -r superset/translations/requirements.txt
sudo apt-get install gettext # or: brew install gettext2. Re-extract strings and sync .po files:
./scripts/translations/babel_update.shThis rewrites superset/translations/messages.pot from the current source files and merges the changes into every .po file. Strings whose msgid changed will be marked #, fuzzy.
3. Resolve the fuzzy entries in the affected language files (fr):
grep -n '#, fuzzy' superset/translations/<lang>/LC_MESSAGES/messages.poFor each fuzzy entry, either rewrite the msgstr to match the new string and remove the #, fuzzy line, or clear the msgstr to "" if you cannot provide a translation.
4. Commit your changes to the .po files.
…name Several expected-output tables in test_client_processing.py were overwritten with values from sibling test cases while updating the Subtotal -> Subvalue label, breaking the pivot unit tests. Restore each affected fixture to its correct values and index order, applying only the label rename and column re-padding. Add one noqa: E501 where the longer label pushes a table line past the limit.
Code Review Agent Run #a14c7eActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
After merging the upstream default (the branch was far behind), ruff format wraps the now-longer Subvalue ternary in client_processing.py. Translation catalogs are reverted to master so this PR stays code-only. CI regenerates the .pot/.po itself with --no-fuzzy-matching, so the rename adds no fuzzy entries and the regression check passes without committing catalog churn.
github-actions
Bot
removed
i18n
|
Good catch, that was a copy-paste slip while I updated the markdown for the longer label, not intentional. I restored test_pivot_df_single_row_null_values so its expected tables use that case's own data again. The boy row is null, so it shows nan across the metrics and the totals, and girl keeps 118065 and 2588, so the null handling is actually being exercised again instead of the numbers from test_pivot_df_single_row_two_metrics. While I was in here I caught the branch up with master, since it had fallen a long way behind. That clears the earlier Playwright failures, which were coming from build-embedded-sdk not existing on the stale base rather than anything in this PR. I also kept the translation catalogs matching master so the regression bot stays quiet, since the rename adds no new fuzzy entries once CI re-extracts. Ran the pivot unit tests locally against the pinned toolchain and all 30 pass. Whenever you have a moment, would you mind approving the workflows so the full suite can run. They are sitting in the queue. |
Code Review Agent Run #993dbbActionable Suggestions - 0Additional Suggestions - 2
Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
Thanks @costajohnt, the test restoration looks right, and the rebase helps. babel-extract is failing now though... probably the The last blocker is the name itself. @yousoph wasn't sold on "Subvalue" over on the issue and floated "Summary (Sum)", and @noam-k wasn't thrilled with either. I'd love a final call before we land a user-facing string. There are three other PRs renaming this same label, so once we agree on a term we may want to close the others in favor of this one. Thoughts? |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| total = ( | ||
| metric_name | ||
| if level == 0 | ||
| else __("Subvalue (%(aggfunc)s)", aggfunc=aggfunc) | ||
| ) |
There was a problem hiding this comment.
Suggestion: Add an explicit type annotation to the newly introduced local variable so the added logic complies with the type-hint requirement for annotatable variables. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The added local variable total is introduced in modified Python code and is clearly annotatable as a string, but it has no explicit type hint. This matches the rule requiring type hints on annotatable variables in new or modified Python code.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/charts/client_processing.py
**Line:** 209:213
**Comment:**
*Custom Rule: Add an explicit type annotation to the newly introduced local variable so the added logic complies with the type-hint requirement for annotatable variables.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix2 participants
Summary
Includes the aggregation function name in pivot table subtotal labels, changing "Subtotal" to "Subvalue (Sum)", "Subvalue (Average)", etc. This matches the existing "Total (Sum)" pattern used for grand totals.
The old "Subtotal" label was misleading when using aggregation functions like AVG, MAX, or MIN since "subtotal" implies summation.
Changes
client_processing.py- subtotal label now uses__("Subvalue (%(aggfunc)s)", aggfunc=aggfunc)TableRenderers.tsx- subtotal label now usest('Subvalue (%(aggregatorName)s)', ...)messages.pottemplate and 22 language files with both backend and frontend i18n keysTest plan
Closes #35089