EnxDev commented on PR #41028:
URL: https://github.com/apache/superset/pull/41028#issuecomment-4810171650
## EnxDev's Review Agent โ apache/superset#41028 ยท HEAD ab9ba0f
**comment** โ Solid, well-tested port that's a clear improvement over the
broken status quo, but it diverges from the browser (d3) on a common preset
with float data, plus a few rounding/scope items worth settling before merge. I
generated d3-format v3 output for the full preset set and diffed it against the
port; findings below are from that comparison.
### ๐ด Functional
- **`superset/utils/number_format.py:115`** (`format_d3`, no-precision path)
โ the `+,` preset renders a spurious trailing `.0` on whole-valued floats: `+,`
of `4725.0` โ `"+4,725.0"` vs the browser's `"+4,725"`. pandas metric columns
are float64, so this is the *common* case for counts/sums, not an edge. Root:
with no explicit precision/type the port defers to Python `format(float,
"+,")`. The parity matrix misses it because `VALUES`
(`number_format_test.py:94`) holds int literals (`4725`), not floats โ add
float-valued inputs (`4725.0`, `1000.0`) and `+,`/`,` will fail red. (test:
`+,` of `4725.0` should equal `"+4,725"`)
### ๐ก Should-fix
- **`superset/utils/number_format.py:115`** โ rounding mode doesn't match
d3. `f`/`%`/`e` types use Python's round-half-to-even: `$,.2f`/`,.2f` of
`0.125` โ `"0.12"` vs the browser's `"0.13"`; `.0f` of `2.5` โ `"2"` vs `"3"`.
The `s`/`r` path uses `Decimal(str(x))` half-up, which diverges the other way
(`.3s` of `2.675` โ `"2.68"` vs d3 `"2.67"`). Exact parity needs d3's
binary-float rounding; at minimum the money presets (`,.2f`/`$,.2f`/`,.1%`)
should match. (test: `$,.2f` of `0.125`)
- **`superset/utils/number_format.py:109`** (`format_numeric`) โ `DURATION`,
`DURATION_SUB`, `DURATION_COL`, `MEMORY_DECIMAL`, `MEMORY_BINARY`,
`MEMORY_TRANSFER_RATE_*` are all selectable number-format presets, but they
aren't ported: they fail the regex and silently fall back to raw (`DURATION` of
`66000` โ `"66000"`, browser shows `"1m 6s"`). That's the same silent-raw
failure this PR sets out to fix. Guard them or note the limitation explicitly.
- **`superset/charts/client_processing.py`** (`apply_pivot_number_formats`)
โ `SMART_NUMBER` is the pivot-table default `valueFormat`, so this formats
*every* pivot report, including CSV attachments (coltype flips
`NUMERIC`โ`STRING`, `4725`โ`"4.73k"`), not just text-embedded tables. The
changed `test_apply_client_processing_csv_format` confirms it. Intended? CSV
consumers often want raw numbers for downstream analysis.
### ๐ต Nits
- `superset/utils/number_format.py` โ `~g` ("Original value" preset) renders
very small values in scientific: `0.00005` โ `"5e-5"` vs the browser's
`"0.00005"`.
- `superset/utils/number_format.py:120` โ the regex parses `(` (accounting
parens), space-sign, width, and fill/align, but `format_d3` ignores them:
`($,.2f` of `-1234.5` โ `"-$1,234.50"` vs the browser's `"($1,234.50)"`.
Free-form custom formats only.
- `superset/utils/number_format.py` โ `.0s` (precision 0) is wrong: `4725` โ
`"0k"` vs `"5k"`. Non-preset.
- `format_column` / `apply_pivot_number_formats` have no direct unit tests
(only exercised via `pivot_table_v2`) โ also flagged by a bot.
### ๐ Praise
- The parity matrix regenerated from real `d3-format` output is the right
approach for a port like this โ it's why the gaps above are narrow rather than
systemic.
- Clean defensive handling: `None` / `NaN` / `inf` / non-numeric all return
safely instead of crashing a whole report table.
<!-- enxdev-review-agent:ab9ba0f -->
_Reviewed by EnxDev's Review Agent โ @EnxDev ยท HEAD ab9ba0f._
--
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]
