fix(reports): apply chart number and currency formatting to tables sent as text - #42820
Open
sadpandajoe wants to merge 13 commits into
Open
fix(reports): apply chart number and currency formatting to tables sent as text#42820sadpandajoe wants to merge 13 commits into
sadpandajoe wants to merge 13 commits into
Conversation
…nt as text When an Alert/Report sends a Table or Pivot Table chart as text embedded in the email body, the number and currency formats configured on the chart were not applied, so the email showed raw values. superset/charts/client_processing.py reproduces the chart's client-side formatting on the server for reports. For Table it attempted only d3NumberFormat, pasting the d3 format string straight into Python's str.format mini-language -- a different grammar -- inside a bare except. So ",d" happened to work while the common cases (SMART_NUMBER, SI like ".1s", currency like "$,.2f") raised and silently fell back to the raw value, and currencyFormat was never read. Pivot Table applied no formatting at all. Adds superset/utils/number_format.py, a Python port of the frontend d3-format, createSmartNumberFormatter and CurrencyFormatter behavior, reusing Babel for currency symbols, and wires it into table() and pivot_table_v2(). Originally authored in #41028; rebuilt here on a maintained branch. Co-authored-by: Jean Massucatto <massucattoj@gmail.com>
Addresses review feedback left unresolved on #41028. Saved metric formats were dropped: both frontend plugins read per-metric formats from datasource column_formats, while the server read columnFormats from form data only, and table() ignored datasource entirely. The datasource formats are now merged with chart-level overrides, with the chart override winning, matching plugin-chart-table transformProps and PivotTableChart. Default formatting now follows d3's exponent thresholds instead of forcing fixed-point, so very small and very large values render as they do in the browser. Currency symbol placement is resolved from the request or configured locale rather than a hard-coded en_US, so reports agree with Explore on non-English deployments. Reports execute in a Celery worker with no request context, so resolution falls back through BABEL_DEFAULT_LOCALE to a safe default.
Parity gaps found by diffing the port against regenerated d3-format output: - whole-valued floats no longer gain a trailing ".0" under "," and "+,"; pandas metric columns are float64, so this affected the common case - f/%/e/s/r now use d3-compatible binary-float rounding, so money presets such as ",.2f" and "$,.2f" match the browser - "~g" keeps small values in fixed notation instead of switching to scientific - ".0s" clamps to one significant digit rather than rendering "0k" Extends the parity matrix with float-valued inputs and boundary cases.
The specifier parser accepted d3 grammar it did not implement, so valid user-entered formats were silently mis-rendered. Accounting parentheses and space-sign are now implemented. Fill, align, zero-pad and width flags are rejected explicitly instead of being parsed and ignored, and the specialized duration, memory, transfer-rate and length preset families are rejected explicitly rather than silently mis-formatting. Rejected specifiers preserve the raw value, which is visible and correctable, instead of returning a confidently wrong number.
format_column and apply_pivot_number_formats were only exercised indirectly through pivot_table_v2, and resolve_symbol_position had no coverage at all. Adds direct tests for the happy path, the error path and the documented fallback behavior of each, including currency symbol position for locales where placement differs.
Adds docstrings to the formatter functions naming the frontend behavior each one mirrors, so the correspondence survives future changes on either side, and adds the missing type annotations on new functions and constants.
AUTO currency resolved only to the query-wide detected currency, so a chart carrying per-row currency context rendered differently in a report than in Explore. The query already returns the currency column and the payload retains it; the association was being lost in pivot_df. A parallel pivot now carries the set of contributing currencies through cells, totals and subtotals. A cell backed by a single currency renders that symbol, a cell mixing currencies stays neutral, and an empty context follows the existing fallback.
A pivot with both `groupbyRows` and `groupbyColumns` has empty cross-product cells. `pivot_table` never runs the currency `union` aggregator for a missing combination, so it fills those cells with a scalar `NaN` rather than an empty tuple. That `NaN` reached `resolve_auto_currency`, whose `list(currency_context)` raised `TypeError: 'float' object is not iterable`. Because the call sits outside the formatting try/except, it propagated out of the post-processor and failed the whole report instead of degrading to the raw value like every other formatting path. Coerce a non-iterable or `NaN` context to an empty context inside `resolve_auto_currency` itself, where no caller can bypass it, so AUTO resolution follows the existing empty-context path (single currency -> that symbol, mixed -> neutral, empty -> detected fallback). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The Table plugin gives every metric column a formatter even when nothing is configured: `getNumberFormatter(undefined)` resolves through the registry's default key to SMART_NUMBER, and percent metrics default to PERCENT_3_POINT (see `plugin-chart-table/src/transformProps.ts`). The report port only iterated columns that had a saved format, saved currency, or `column_config` entry, so an aggregate metric with no explicit format rendered `1.2M` in Explore but the raw `1234567` in the email body -- the exact "raw numbers in the email" symptom this change set exists to fix, in its most common (default) form. Rewrite the `table()` column loop to mirror the plugin's per-column selection: percent metrics default to PERCENT_3_POINT, every numeric metric gets a formatter defaulting to SMART_NUMBER, and other numeric columns are formatted only when an explicit format or currency is set -- so numeric dimensions the browser leaves alone stay raw. `pivot_table_v2` had the same hole: the pivot component's `getNumberFormatter(valueFormat)` also defaults to SMART_NUMBER, but an empty `valueFormat` with no per-metric format left cells raw, so apply the same default in `apply_pivot_number_formats`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The pre-fix `table()` applied `d3NumberFormat` to every result format, so CSV and XLSX exports contained pre-formatted strings. The report path now returns early for CSV/XLSX before formatting, keeping numeric values and column types (better for downstream analysis) while the rendered email body still gets the chart's formatting. Document this user-visible change under the unreleased "Next" section. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #42820 +/- ##
==========================================
- Coverage 66.37% 66.26% -0.11%
==========================================
Files 2857 2858 +1
Lines 161048 161390 +342
Branches 37046 37113 +67
==========================================
+ Hits 106892 106947 +55
- Misses 52141 52427 +286
- Partials 2015 2016 +1
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:
|
sadpandajoe
marked this pull request as ready for review
August 6, 2026 00:50
dosubot
Bot
added
alert-reports
`resolve_auto_currency` declared `currency_context: Iterable[Any] | None`, but the sparse-pivot guard exists precisely because pandas hands the function a scalar `NaN` float for a missing cross-product cell. mypy correctly flagged the test that passes that NaN as an incompatible `float` argument -- the annotation described a contract the runtime no longer honours. Widen the parameter to `Iterable[Any] | float | None` (narrow, so it documents that the only non-iterable that actually occurs on the live path is the pandas NaN) and reshape the guard to test positively for an iterable (`list(context) if isinstance(context, Iterable) else []`). That keeps mypy happy -- it narrows the union to the iterable arm, which a bare try/except could not -- while restoring defense-in-depth: any non-iterable sentinel (`np.nan`, `pd.NA`, `pd.NaT`) falls to the empty-context path rather than raising and taking down the whole report. Document the behavior in the parameter docstring where a future maintainer would look before touching the guard. No behavior change on the live path; the failing test keeps calling the function the way pandas does, and now also pins the `pd.NA`/`pd.NaT` sentinels. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
sadpandajoe
marked this pull request as draft
August 6, 2026 01:50
sadpandajoe
marked this pull request as ready for review
August 6, 2026 06:13
Contributor
Code Review Agent Run #f42b66Actionable 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 |
Contributor
Code Review Agent Run #f42b66Actionable 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 |
…mber-formatting # Conflicts: # UPDATING.md
sadpandajoe
force-pushed
the
fix-reports-text-number-formatting
branch
from
August 7, 2026 16:02
2b49fe9 to
ec2b949
Compare
Contributor
Code Review Agent Run #ba02caActionable Suggestions - 0Additional Suggestions - 2
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.
SUMMARY
When an Alert/Report sends a Table or Pivot Table chart as text (embedded in the email body), the number and currency formatting configured on the chart was not applied, so the email showed raw values.
superset/charts/client_processing.pyreproduces the chart's client-side formatting on the server for reports. For Table it attempted onlyd3NumberFormat, by pasting the d3 format string straight into Python'sstr.formatmini-language — a different grammar — wrapped in a bareexcept: passwhose comment reads "if we can't format the column for any reason, send as is".The two grammars overlap only narrowly, so most specifiers raised and silently fell back to the raw value:
.1s,$,.2f,~gand the defaultSMART_NUMBERall fail outright. Even,dworks only forint— it raisesUnknown format code 'd'onfloat, and pandas metric columns arefloat64, so it failed for ordinary counts and sums too. Specs valid in both grammars, such as,.2f, were the only ones that came through. Nothing was logged, so the failure was invisible rather than merely wrong.currencyFormatwas never read on that path at all, andpivot_table_v2()applied no formatting whatsoever.This adds
superset/utils/number_format.py, a Python port of the frontend formatters — d3-format,createSmartNumberFormatter.tsandCurrencyFormatter.ts— reusing Babel (already a dependency) for currency symbols, and wires it intotable()andpivot_table_v2().This supersedes #41028, which was opened against a fork that is no longer maintained. The original work is carried here with attribution, plus the review feedback that PR had accumulated. Thanks to @massucattoj for the original implementation.
WHAT'S INCLUDED BEYOND THE ORIGINAL
The commits are grouped by intent and are best read in order.
Review feedback from #41028
column_formats, while the server read onlycolumnFormatsfrom form data — andtable()ignored the datasource entirely. Datasource formats are now merged with chart-level overrides, chart winning, matchingplugin-chart-table/src/transformProps.tsandPivotTableChart.tsx.en_US, so reports agree with Explore on non-English deployments. Reports run in a Celery worker with no request context, so resolution falls back throughBABEL_DEFAULT_LOCALEto a safe default.pivot_df. A parallel pivot now carries the set of contributing currencies through cells, totals and subtotals. A cell backed by a single currency renders that symbol, a cell mixing currencies stays neutral, and an empty context follows the existing fallback.Columns relying on the implicit default
The browser gives every numeric metric column a formatter even when nothing is configured —
getNumberFormatter(undefined)resolves to the registry default,SMART_NUMBER. Report text previously formatted only columns with an explicit format, so an aggregate metric with no configured format showed1.23Min Explore and1234567in the email. That is the same symptom this PR targets, in its most common form.Table now applies the same defaults as the plugin:
PERCENT_3_POINTfor percent metrics,SMART_NUMBERfor numeric metrics with no explicit format. Numeric dimension columns are deliberately left untouched, matchingtransformProps.ts— there is a test asserting a numeric dimension stays raw while its sibling metric formats.This gap is Table-specific in practice. The Pivot Table's
valueFormatcontrol carries aSMART_NUMBERdefault that is persisted into saved form data, so pivots created through the UI were already covered; Table's is a per-column formatter the browser synthesizes at render time, so nothing is ever persisted for it. Pivot still gets a small defensive fallback for charts that never persisted the key (API-created or older), which is a no-op wherevervalueFormatis present.A crash, not just a formatting miss
Carrying per-row currency through a pivot left the parallel currency structure with
NaNin the empty cells of a sparse cross-product, which raisedTypeErroroutside any error handling and failed report generation entirely. A missing or non-iterable currency context is now coerced to the empty-context path, which resolves the same way it always did (single currency → that symbol, mixed → neutral, empty → detected fallback).d3 parity fixes
These came from diffing the port against regenerated
d3-formatoutput rather than from reading the Python:.0under,and+,— pandas metric columns arefloat64, so this affected the common case of counts and sumsf/%/e/s/ruse d3-compatible rounding rather than Python's half-to-even, so money presets like,.2fand$,.2fmatch the browser,formatting follows d3's exponent thresholds instead of forcing fixed-point~gkeeps small values in fixed notation instead of switching to scientific.0sclamps to one significant digit rather than rendering0kFailing loudly instead of quietly
The specifier parser previously accepted d3 grammar it did not implement, which is the worst case: a valid user-entered format silently rendering a confidently wrong number. Accounting parentheses and space-sign are now implemented; fill, align, zero-pad and width are rejected explicitly. Rejected formats preserve the raw value, which is visible and correctable.
KNOWN LIMITATION
The specialized preset families —
DURATION,DURATION_SUB,DURATION_COL,MEMORY_DECIMAL,MEMORY_BINARY,MEMORY_TRANSFER_RATE_*and the length formatters — are not ported. They are rejected explicitly and fall back to the raw value, so a chart using one of them still shows an unformatted number in report text.This is deliberate: those are locale-aware formatter factories rather than d3 format strings, and porting them faithfully is a separate piece of work. They are called out here rather than left to be discovered, since this PR does not make report formatting complete — it makes the d3 and currency formats work and makes the rest fail visibly instead of silently.
TESTING INSTRUCTIONS
.1s,,.2f) and/or a currency on a column./api/v1/chart/<chart_id>/data/?type=post_processed&format=jsonNumeric columns with a configured format now render formatted (
8k,$ 1,234.50) instead of raw.d3-formatoutput rather than from this implementation.CSV/XLSX attachments keep raw numeric values and column types for downstream analysis — formatting applies to the JSON path only. This is a behavior change from before (the old code formatted regardless of result format) and is noted in
UPDATING.md.ADDITIONAL INFORMATION