fix(pivot-table): Rename 'Subtotal' to 'Subvalue' with aggregation fu…#40513
Open
divyeshreddy02 wants to merge 4 commits into
Open
fix(pivot-table): Rename 'Subtotal' to 'Subvalue' with aggregation fu…#40513divyeshreddy02 wants to merge 4 commits into
divyeshreddy02 wants to merge 4 commits into
Conversation
…nction The Pivot Table visualization previously used 'Subtotal' as a label for subgroup aggregations regardless of which aggregation function was used (e.g., Sum, Average, Max). This was confusing because 'Subtotal' implies a specific 'Sum' operation. This change: - Renames 'Subtotal' to 'Subvalue (%(aggfunc)s)' - Includes the actual aggregation function name (Sum, Average, etc.) - Makes the label consistent with 'Total (Sum)' at the top level Fixes apache#35089
Contributor
Code Review Agent Run #878742Actionable Suggestions - 0Filtered 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 |
Contributor
|
Congrats on making your first PR and thank you for contributing to Superset! 🎉 ❤️ Please read our New Contributor Welcome & Expectations guide. We hope to see you in our Slack community too! Not signed up? Use our Slack App to self-register. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| subtotal = pivot_v2_aggfunc_map[aggfunc](df.iloc[:, slice_], axis=1) | ||
| depth = df.columns.nlevels - len(subgroup) - 1 | ||
| total = metric_name if level == 0 else __("Subtotal") | ||
| total = metric_name if level == 0 else __("Subvalue (%(aggfunc)s)", aggfunc=aggfunc) |
Contributor
There was a problem hiding this comment.
Suggestion: This new subtotal label diverges from the frontend pivot-table renderer, which still renders subgroup headers as Subtotal. Since this module is meant to reproduce client-side processing for reports, changing only the backend label creates a contract mismatch (Explore UI vs CSV/JSON/report output labels differ). Keep both sides aligned by updating the frontend subtotal label logic in the same change or using the same label here. [api mismatch]
Severity Level: Major ⚠️
- ⚠️ Pivot table CSV subtotals differ from Explore UI.
- ⚠️ Scheduled pivot_table_v2 reports show renamed subtotal labels.
- ⚠️ Downstream tooling relying on UI labels may misalign.Steps of Reproduction ✅
1. Note that `superset/charts/client_processing.py:18-25` documents this module as
reproducing client-side post-processing so that reports show the same data as Explore
(pivot table UI).
2. On the frontend, `pivot_table_v2` uses the pivot-table plugin wired via
`transformProps` at
`superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/transformProps.ts:98-120`
and `PivotTableChart` (see plugin wiring); it passes subtotal controls
(`rowSubtotalPosition`, `colSubtotalPosition`, etc.) but does not receive or override the
backend subtotal label, so the UI continues to use the pivot library's built‑in subtotal
labeling (currently "Subtotal").
3. For reports/exports, the backend entrypoint `apply_client_processing()` at
`superset/charts/client_processing.py:13-27` routes `viz_type == "pivot_table_v2"` through
`pivot_table_v2()`, which in turn calls `pivot_df()` with
`aggfunc=form_data["aggregateFunction"]` to recompute the pivot for CSV/JSON (see tests
`test_apply_client_processing_json_format` and `test_apply_client_processing_csv_format`
at `tests/unit_tests/charts/test_client_processing.py:1193-1379`).
4. Inside `pivot_df()`, subtotals are now labeled with `__("Subvalue (%(aggfunc)s)",
aggfunc=aggfunc)` for non-top levels (`superset/charts/client_processing.py:77` for
`metric_name` and lines `119` and `26` for `total`), so for `aggfunc="Sum"` the exported
CSV/JSON indices contain "Subvalue (Sum)" while the Explore UI subtotal headers remain
"Subtotal"; this breaks the intended parity between Explore and exported reports for
subtotal labels.Fix in Cursor | Fix in VSCode Claude
(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:** 178:178
**Comment:**
*Api Mismatch: This new subtotal label diverges from the frontend pivot-table renderer, which still renders subgroup headers as `Subtotal`. Since this module is meant to reproduce client-side processing for reports, changing only the backend label creates a contract mismatch (Explore UI vs CSV/JSON/report output labels differ). Keep both sides aligned by updating the frontend subtotal label logic in the same change or using the same label here.
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 fix
Comment on lines
+374
to
+377
| | ('SUM(num)', 'Subvalue') | 165188 | | ||
| | ('MAX(num)', 'boy') | 1280 | | ||
| | ('MAX(num)', 'girl') | 2588 | | ||
| | ('MAX(num)', 'Subtotal') | 3868 | | ||
| | ('MAX(num)', 'Subvalue') | 3868 | |
Contributor
There was a problem hiding this comment.
Suggestion: The updated expectations still assert plain Subvalue, but the new implementation now generates Subvalue (<aggfunc>) (for example, Subvalue (Sum)). These snapshots will fail against the new behavior and make the test suite unreliable; update expected markdown labels to include the aggregation suffix consistently. [logic error]
Severity Level: Critical 🚨
- ❌ Pivot table backend unit tests fail on subtotal expectations.
- ❌ CI for charts client_processing module will be red.
- ⚠️ Future regressions in subtotal labeling harder to detect.Steps of Reproduction ✅
1. Run the unit test `test_pivot_df_single_row_two_metrics` defined at
`tests/unit_tests/charts/test_client_processing.py:361-381`, which builds a small
DataFrame and calls `pivot_df(...)` with `aggfunc="Sum"`, `show_rows_total=True`,
`show_columns_total=True`, and `apply_metrics_on_rows=True`.
2. The test asserts on `pivoted.to_markdown()` including subtotal rows labeled
`('SUM(num)', 'Subvalue')` and `('MAX(num)', 'Subvalue')` (lines 373-378), i.e. without
any aggregation suffix.
3. At runtime, `pivot_df()` in `superset/charts/client_processing.py` now computes
`metric_name = __("Total (%(aggfunc)s)", aggfunc=aggfunc)` at line 77 and uses `total =
metric_name if level == 0 else __("Subvalue (%(aggfunc)s)", aggfunc=aggfunc)` when
inserting subtotal columns/rows at lines 119 and 26, so with `aggfunc="Sum"` the subtotal
label becomes `"Subvalue (Sum)"`, not `"Subvalue"`.
4. As a result, the MultiIndex entries emitted by `pivot_df()` for the subtotal rows
contain `"Subvalue (Sum)"`, making the `to_markdown()` output diverge from the hard-coded
expectations in `test_pivot_df_single_row_two_metrics`, causing this and other similar
tests (e.g., expectations at `tests/unit_tests/charts/test_client_processing.py:402-405`,
`543-546`, `571-574`, `1075-1085`, etc.) to fail consistently until the snapshots are
updated to include the aggregation suffix.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/charts/test_client_processing.py
**Line:** 374:377
**Comment:**
*Logic Error: The updated expectations still assert plain `Subvalue`, but the new implementation now generates `Subvalue (<aggfunc>)` (for example, `Subvalue (Sum)`). These snapshots will fail against the new behavior and make the test suite unreliable; update expected markdown labels to include the aggregation suffix consistently.
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 fixAlign the frontend pivot table subtotal label with the backend change. Previously the backend said 'Subvalue (Sum)' while the UI said 'Subtotal'. This change: - Updates TableRenderers.tsx to use 'Subvalue (%(aggregatorName)s)' format - Adds the python-format marker to the translation file - Makes frontend consistent with backend and 'Total' labels Fixes PR apache#40513 review feedback
github-actions
Bot
added
i18n
…query The streaming CSV export path was executing raw SQL without running it through mutate_sql_based_on_config(). This caused: 1. Trino exports to fail on SQL with trailing semicolons (Trino rejects 'LIMIT N;' but accepts 'LIMIT N') The fix calls merged_database.mutate_sql_based_on_config(sql) before executing the query, ensuring SQL transformations are applied. This fixes Bug 1 of issue apache#40465. Bug 2 (user impersonation) will be addressed separately.
The streaming CSV export generator runs in a new Flask app context, but g.user was not being properly restored. This caused user impersonation to fail - all exports ran as the service account instead of the actual user. Changes: 1. Modified preserve_g_context() to accept an optional user parameter 2. Capture g.user separately before creating the generator (g.user may not serialize properly via __dict__.copy()) 3. Explicitly set g.user when restoring context This ensures get_username() returns the correct user, enabling: - Proper X-Trino-User header forwarding - Correct resource group routing - Accurate audit trails This fixes Bug 2 of issue apache#40465.
Contributor
Code Review Agent Run #79cf6fActionable Suggestions - 0Additional Suggestions - 1
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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…nction
The Pivot Table visualization previously used 'Subtotal' as a label for subgroup aggregations regardless of which aggregation function was used (e.g., Sum, Average, Max). This was confusing because 'Subtotal' implies a specific 'Sum' operation.
This change:
Fixes #35089
SUMMARY
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION