fix(pivot-table): blank ambiguous cross-metric totals and null values in fraction mode - #42810
Conversation
|
Bito Automatic Review Skipped - Branch Excluded |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
|
| Language | Invalidated translations |
|---|---|
es |
75 |
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 (es):
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.
… in fraction mode Two edge cases in the "Show values as" percent display (introduced in #42761) flagged in review, both in fractionOf's value(): - "Metric-collapse totals" (processRecord) mirrors each metric's grand-total record into a single shared allTotal/rowTotals/colTotals slot when no real dimension is left to key on. metricAxis locks onto the first metric pushed, but the underlying value is always the last metric pushed, so a multi-metric table's grand-total corner cell could divide one metric's value by another metric's total and render a wrong-but-plausible-looking percentage. Now detects the collision on push and blanks the cell instead. - A DB-computed value can be a genuine SQL NULL (e.g. AVG over an empty group). `null / acc` coerces to `0` in JS, turning a cell that's blank in "Actual values" mode into a measured "0.0%". Now checks for null before dividing. Follow-up to #42761.
…ricAxis The rebase onto feat/pivot-table-fraction-of picked up a follow-up fix that updates metricAxis on every push (not just the first) so it stays in sync with the last-pushed metric, same as `inner`. The multi-metric corner cell now divides the last metric by its own total (a self-consistent 100%, per the documented "totals dividing by themselves" design) instead of blanking. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
4b7ac54 to
641de27
Compare
A third issue flagged in review (codeant-ai): the denominator, not just the numerator, can itself be a genuine SQL NULL (e.g. a row total that's an AVG over an empty group). `numerator / null` coerces to `numerator / 0` in JS, returning Infinity/NaN from value() instead of the blank the missing/null contract already establishes for the other branches in this function. The shared number formatter already blanks non-finite values, so this had no visible effect on rendered text, but other code that reads .value() directly (e.g. value-based row/column sorting) relies on the null contract to treat it as "no value" -- fixed at the source, with both a DOM-level regression test and one asserting value() directly (since the two null vs. Infinity/NaN cases render identically and the DOM test alone can't tell them apart).
…to fix/pivot-table-fraction-edge-cases
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feat/pivot-table-fraction-of #42810 +/- ##
=============================================================
Coverage 65.76% 65.77%
=============================================================
Files 2842 2842
Lines 162314 162316 +2
Branches 37197 37198 +1
=============================================================
+ Hits 106752 106756 +4
+ Misses 53494 53492 -2
Partials 2068 2068
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:
|
2 participants
SUMMARY
Follow-up to #42761 (pivot table "Show values as" percent display), fixing two edge cases flagged in review that weren't small enough to fold into that PR without risking a rushed change to already-reviewed code:
processRecord's "Metric-collapse totals" mirrors each metric's grand-total record into a single sharedallTotal/rowTotals/colTotalsslot when no real dimension is left to key on (documented in-code as deliberately deferred "future work" for actual-values mode). In fraction mode this is worse than a stale display:metricAxislocks onto the first metric pushed into that slot, but the underlying value is always the last metric pushed, so a multi-metric table's grand-total corner cell could divide one metric's value by a different metric's total and render a wrong-but-plausible percentage (concretely: with metric A's grand total of 30 and metric B's of 300, the corner cell showed1,000.0%). This PR detects the collision on push and blanks the cell instead of showing a misleading number. Actual-values mode is unchanged and still has the pre-existing "future work" limitation — this only prevents fraction mode from making it worse.AVGover an empty group), which renders blank in "Actual values" mode. In fraction mode,null / acccoerces to0in JS, so the same cell rendered a measured "0.0%" instead of staying blank. Now checked explicitly.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — covered by the new regression tests below; both were confirmed to fail against the pre-fix code (corner cell showed
1,000.0%, null cell showed0.0%) before being fixed.TESTING INSTRUCTIONS
npx jest plugins/plugin-chart-pivot-table/test/react-pivottable/tableRenders.test.tsx --runInBand— two new tests:blanks the grand-total corner cell when it mixes multiple metrics in fraction modeandkeeps a null metric value blank in fraction mode instead of showing 0.0%.ADDITIONAL INFORMATION
Base is
feat/pivot-table-fraction-of(notmaster) since this fixes code introduced there and hasn't landed yet — will retarget tomasteronce #42761 merges, or this can be merged into that branch directly if easier.A third item from the same review round — CSV/XLSX exports and scheduled reports not respecting
showValuesAs— needs a real design decision in the export/report query path rather than a quick fix, so it's tracked separately as #42809 instead of bundled here.