codeant-ai-for-open-source[bot] commented on code in PR #40513:
URL: https://github.com/apache/superset/pull/40513#discussion_r3322529455
##########
superset/charts/client_processing.py:
##########
@@ -175,7 +175,7 @@ def pivot_df( # pylint: disable=too-many-locals,
too-many-arguments, too-many-s
slice_ = df.columns.get_loc(subgroup)
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)
Review 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]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ 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.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
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.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d7f32be7a2234d84943a4184b7a56c55&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=d7f32be7a2234d84943a4184b7a56c55&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
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
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40513&comment_hash=47cefaf83167a7455617770da1586f45dda8d93bc88b87add33419e583284422&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40513&comment_hash=47cefaf83167a7455617770da1586f45dda8d93bc88b87add33419e583284422&reaction=dislike'>👎</a>
##########
tests/unit_tests/charts/test_client_processing.py:
##########
@@ -371,10 +371,10 @@ def test_pivot_df_single_row_two_metrics():
|:-------------------------|-------------------:|
| ('SUM(num)', 'boy') | 47123 |
| ('SUM(num)', 'girl') | 118065 |
-| ('SUM(num)', 'Subtotal') | 165188 |
+| ('SUM(num)', 'Subvalue') | 165188 |
| ('MAX(num)', 'boy') | 1280 |
| ('MAX(num)', 'girl') | 2588 |
-| ('MAX(num)', 'Subtotal') | 3868 |
+| ('MAX(num)', 'Subvalue') | 3868 |
Review 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]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ 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.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
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.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=8c457f0143b146adb29b0e24a3cac58e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=8c457f0143b146adb29b0e24a3cac58e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
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 fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40513&comment_hash=9126f7c4693d14fd64ea1daa63bdeee42f2745552fe4e2d67779050a71ee339c&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40513&comment_hash=9126f7c4693d14fd64ea1daa63bdeee42f2745552fe4e2d67779050a71ee339c&reaction=dislike'>👎</a>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
