fix(pivot-table): include aggregation type in subtotal labels#41957
Open
RehanIslam09 wants to merge 1 commit into
Open
fix(pivot-table): include aggregation type in subtotal labels#41957RehanIslam09 wants to merge 1 commit into
RehanIslam09 wants to merge 1 commit into
Conversation
dosubot
Bot
added
explore:design
Contributor
Code Review Agent Run #58a382Actionable 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 |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
Comment on lines
+1076
to
+1078
| {t('Subvalue (%(aggregatorName)s)', { | ||
| aggregatorName: t(aggregatorName), | ||
| })} |
Contributor
There was a problem hiding this comment.
Suggestion: The new label text uses Subvalue, which contradicts the subtotal terminology used by the surrounding code (pvtSubtotalLabel) and the feature intent (subtotal labels). This introduces incorrect user-facing wording; use Subtotal (%(aggregatorName)s) instead. [inconsistent naming]
Severity Level: Minor 🧹
- ⚠️ Suggestion targets wording choice, not functional bug.
- ⚠️ Pivot table behavior unaffected; labels already consistent.
- ⚠️ CSS class name needn’t match human-readable text.Steps of Reproduction ✅
1. The column subtotal header cell is rendered in `renderColHeaderRow` at
`TableRenderers.tsx:860-1112`; when `attrIdx === colKey.length`, the code at `1060-1080`
creates a `<th>` with class `pvtSubtotalLabel` and the label `{t('Subvalue
(%(aggregatorName)s)', { aggregatorName: t(aggregatorName) })}` (lines `1076-1078`).
2. The same `Subvalue` label pattern is used for row subtotals in `renderTableRow` at
`TableRenderers.tsx:1322-1342`, so subtotal labels are consistent between row and column
headers within this component.
3. A repository-wide search for `"Subtotal"` in the frontend (`Grep` over
`superset-frontend/**/*.ts*`) shows no other user-facing strings like `"Subtotal
(%(aggregatorName)s)"`; the only uses of “subtotal” are internal types and flags such as
`SubtotalOptions` and style selectors `.pvtSubtotalLabel` in `Styles.ts:69-107`, which
just control visual formatting, not text.
4. Given there is no conflicting `"Subtotal"` label elsewhere in this visualization and
the PR description’s examples explicitly use `Subvalue (Sum)` / `Subvalue (Average)` as
the intended copy, this wording appears deliberate rather than a bug; changing it to
`"Subtotal"` is a product copy decision, not a functional defect.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.tsx
**Line:** 1076:1078
**Comment:**
*Inconsistent Naming: The new label text uses `Subvalue`, which contradicts the subtotal terminology used by the surrounding code (`pvtSubtotalLabel`) and the feature intent (subtotal labels). This introduces incorrect user-facing wording; use `Subtotal (%(aggregatorName)s)` instead.
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
+1338
to
+1340
| {t('Subvalue (%(aggregatorName)s)', { | ||
| aggregatorName: t(aggregatorName), | ||
| })} |
Contributor
There was a problem hiding this comment.
Suggestion: This new use of aggregatorName is inside a memoized row-render callback, but that callback’s dependency list does not include aggregatorName. When users switch aggregation (Sum/Average/Count), this label can keep showing the previous aggregator due to a stale closure. Add aggregatorName to that useCallback dependency array so subtotal labels update correctly. [stale reference]
Severity Level: Major ⚠️
- ⚠️ Row subtotal headers show outdated aggregation name after change.
- ⚠️ Users can misread subtotals when aggregator changes.
- ⚠️ Any UI tests on subtotal labels may become flaky.Steps of Reproduction ✅
1. In `TableRenderers.tsx:327-341`, `TableRenderer` destructures `aggregatorName` from
props and passes it into several memoized callbacks; row rendering is handled by
`renderTableRow` defined at `TableRenderers.tsx:1207-1410`.
2. Inside `renderTableRow`, the row subtotal header cell is rendered when `rowKey.length <
settingsRowAttrs.length` at `TableRenderers.tsx:1322-1342`; the label uses the new code at
`1338-1340`: `t('Subvalue (%(aggregatorName)s)', { aggregatorName: t(aggregatorName) })`,
so this callback closes over `aggregatorName`.
3. The `renderTableRow` function itself is memoized with `useCallback` at
`TableRenderers.tsx:1207-1418`, but its dependency array at `1410-1417` is `[tableOptions,
onContextMenu, toggleRowKey, clickHeaderHandler, rows, collapsedRows]` and does not
include `aggregatorName`, meaning the callback instance (and its closed-over
`aggregatorName` value) is reused across renders as long as those dependencies do not
change.
4. When the aggregator is changed from, for example, `"Count"` to `"Sum"` (as exercised in
tests by `aggregatorName: 'Count'` and `aggregatorName: 'Sum'` in `buildDefaultProps` at
`test/react-pivottable/tableRenders.test.tsx:62-73` and `211-215`, or via the
`aggregateFunction` control in Storybook at `src/stories/PivotTable.stories.tsx:31-51`),
`TableRenderer` re-renders with a new `aggregatorName` prop but `renderTableRow` is not
recreated (its deps are unchanged), so row subtotal header cells still display the stale
label like `Subvalue (Count)` instead of the updated `Subvalue (Sum)`.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.tsx
**Line:** 1338:1340
**Comment:**
*Stale Reference: This new use of `aggregatorName` is inside a memoized row-render callback, but that callback’s dependency list does not include `aggregatorName`. When users switch aggregation (Sum/Average/Count), this label can keep showing the previous aggregator due to a stale closure. Add `aggregatorName` to that `useCallback` dependency array so subtotal labels update correctly.
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
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #41957 +/- ##
==========================================
- Coverage 64.78% 64.77% -0.01%
==========================================
Files 2739 2739
Lines 152981 152980 -1
Branches 35058 35058
==========================================
- Hits 99107 99100 -7
- Misses 51974 51980 +6
Partials 1900 1900
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:
|
Comment on lines
88
to
94
| const { value, errorMsg } = calculateCellValue( | ||
| valueField, | ||
| columnConfig, | ||
| reversedEntries, | ||
| ); | ||
|
|
||
| if (columnConfig.colType === 'spark') { |
Contributor
There was a problem hiding this comment.
Suggestion: calculateCellValue is executed for every column before checking colType, so sparkline columns do unnecessary value computation that is immediately discarded; this adds avoidable per-row/per-column work in a hot render path. Move the calculation into the non-spark branch to avoid wasted processing. [performance]
Severity Level: Major ⚠️
- ⚠️ TimeTable sparkline columns perform redundant value calculations per render.
- ⚠️ Large tables with many spark columns render more slowly.Steps of Reproduction ✅
1. In the TimeTable component, inspect the memoizedRows computation at
superset-frontend/src/visualizations/TimeTable/TimeTable.tsx:81-126. For each row,
columnConfigs.reduce is used to build cellValues; within the reducer, calculateCellValue
is called unconditionally at lines 88-92 to derive { value, errorMsg } for the current
column.
2. Immediately after that call, the code branches on columnConfig.colType at lines 94-105.
If colType is 'spark', the reducer returns a Sparkline component: <Sparkline
valueField={valueField} column={columnConfig} entries={entries} /> (lines 98-102), and the
previously computed value and errorMsg are not referenced anywhere in the spark branch.
3. Review calculateCellValue in
superset-frontend/src/visualizations/TimeTable/utils/valueCalculations/valueCalculations.ts:142-160.
This helper performs non-trivial work per cell: it reads reversedEntries, derives the
“recent” value, and may call calculateTimeValue, calculateContribution, or
calculateAverage (lines 151-158), which themselves iterate over reversedEntries and
perform numeric computations.
4. Because calculateCellValue is invoked for every column, including spark columns whose
rendered cells never use its result, every TimeTable render with spark columns performs
unnecessary per-row/per-column value computation that is immediately discarded. This
wasted work sits on the core render path for TimeTable, so any dataset with many rows and
spark columns will pay extra CPU cost without functional benefit.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/visualizations/TimeTable/TimeTable.tsx
**Line:** 88:94
**Comment:**
*Performance: `calculateCellValue` is executed for every column before checking `colType`, so sparkline columns do unnecessary value computation that is immediately discarded; this adds avoidable per-row/per-column work in a hot render path. Move the calculation into the non-spark branch to avoid wasted processing.
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
110
to
114
| <ValueCell | ||
| valueField={valueField} | ||
| value={value} | ||
| errorMsg={errorMsg} | ||
| column={columnConfig} | ||
| reversedEntries={reversedEntries} | ||
| /> |
Contributor
There was a problem hiding this comment.
Suggestion: The new ValueCell props no longer include the metadata that the existing sortNumberWithMixedTypes comparator depends on (valueField/reversedEntries), so non-spark columns now sort as equal and user sorting/default sort behavior silently breaks. Preserve the previous sort contract (pass required props) or update the sorter to read the precomputed value prop. [api mismatch]
Severity Level: Critical 🚨
- ❌ TimeTable visualization columns cannot be sorted by value.
- ❌ Default descending sort by latest metric no longer applies.
- ⚠️ Users see inconsistent ordering across reloads and aggregations.Steps of Reproduction ✅
1. Open the Explore page and select the TimeTable visualization type, which is wired
through VizType.TimeTable (see
superset-frontend/src/explore/components/ControlPanelsContainer.tsx:13-20 where TimeTable
is listed in MATRIXIFY_INCOMPATIBLE_CHARTS, and
superset-frontend/src/explore/components/controls/VizTypeControl/VizTypeControl.test.tsx:72
where TimeTableChartPlugin is configured with key VizType.TimeTable).
2. Run a query that produces multiple rows and at least one non-spark metric column, so
TimeTable renders; the TimeTable component builds columns with sortType set to
sortNumberWithMixedTypes at
superset-frontend/src/visualizations/TimeTable/TimeTable.tsx:50-76, and memoizedRows at
lines 81-126, where each non-spark column cell is rendered as <ValueCell value={value}
errorMsg={errorMsg} column={columnConfig} /> (lines 107-115).
3. Note that ValueCell’s props, defined in
superset-frontend/src/visualizations/TimeTable/components/ValueCell/ValueCell.tsx:24-28,
only include value, errorMsg, and column; they do not include valueField or
reversedEntries. However, the shared comparator sortNumberWithMixedTypes in
superset-frontend/src/visualizations/TimeTable/utils/sortUtils/sortUtils.ts:57-106
directly inspects cellA.props and cellB.props and expects props with shape { valueField,
column, reversedEntries?, entries? } (see the comments at lines 65-68 and the typed casts
at lines 69-84).
4. When the user clicks a non-spark column header to sort, or when the initialSortBy
default sort is applied (TimeTable.tsx:128-135), react-table calls
sortNumberWithMixedTypes. Because the ValueCell elements in row.values[columnId] lack
valueField and reversedEntries, reversedEntriesA and reversedEntriesB resolve to undefined
(lines 86-89 in sortUtils.ts), triggering the guard at lines 91-93 that returns 0 for any
row pair, so all comparisons are “equal” and sorting for non-spark columns silently fails
(no change in order despite sort gestures or default sort configuration).
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/visualizations/TimeTable/TimeTable.tsx
**Line:** 110:114
**Comment:**
*Api Mismatch: The new `ValueCell` props no longer include the metadata that the existing `sortNumberWithMixedTypes` comparator depends on (`valueField`/`reversedEntries`), so non-spark columns now sort as equal and user sorting/default sort behavior silently breaks. Preserve the previous sort contract (pass required props) or update the sorter to read the precomputed `value` prop.
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
Contributor
Code Review Agent Run #a9723bActionable 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 |
RehanIslam09
force-pushed
the
fix-pivot-subvalue-label
branch
from
79c8ec6 to
b068321
Compare
Contributor
Code Review Agent Run #8fd9f2Actionable 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 |
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.
SUMMARY
This PR updates the Pivot Table subtotal labels to include the selected aggregation function.
Previously, subtotal rows and columns always displayed the generic label "Subtotal", regardless of the selected aggregation function.
This change updates the subtotal labels to display the selected aggregation function, making them consistent with the existing total labels.
Examples:
SubtotalSubvalue (Sum)Subvalue (Average)Subvalue (Count)Fixes #35089.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
After
The Pivot Table subtotal labels now include the aggregation type.
Example:
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION