SEPURI-SAI-KRISHNA opened a new pull request, #45979:
URL: https://github.com/apache/superset/pull/45979
### SUMMARY
Fixes #45672.
A table report of a time-comparison chart shows different columns from the
chart, with different numbers in them. The chart does not render the query's
`<metric>` and `<metric>__<offset>` pair at all. `transformProps.ts` replaces
each comparison metric with four derived columns, `Main`, `#`, `△` and `%`,
and formats each of them separately. None of that happens in
`client_processing.table()`, so the report goes out with the raw query
columns
and the offset one unformatted.
This ports the derivation. `derive_comparison_columns` reproduces
`processComparisonColumns` and `processComparisonDataRecords` taken together,
and `comparison_column_format` reproduces `getComparisonColFormatter`, so the
report builds the same columns in the same order and formats them the same
way.
A few things in that logic are easy to get wrong by reading it, so each was
measured against the real `transformProps` under jest rather than inferred:
- Every read in `processComparisonDataRecords` is `|| 0`. A missing
comparison
value is `NaN` in a DataFrame, and `NaN` is falsy in JavaScript while
`bool(float("nan"))` is `True` in Python, so the coercion cannot be left to
`or`.
- `calculateDifferences`' percent difference is a ratio, not a percentage,
and
it short-circuits on a zero operand instead of dividing. Its denominator is
an absolute value, so a positive metric against a negative comparison grows
the ratio rather than flipping its sign.
- `isNumeric` requires `typeof` to say number for every value, nulls apart,
so
a metric returning strings is demoted to a plain column while a percent
metric in the same shape is dropped. `pd.api.types.is_numeric_dtype`
answers
a different question and disagrees in both directions, being true of a
boolean column and false of an all-null object column.
- `col.isNumeric` is the generic type the backend sent with the results,
which
`query_actions` builds with `extract_dataframe_dtypes`. Calling the same
function with the same datasource answers it the way the browser was
answered, including for an all-null column it types from the datasource.
- A format saved in `column_config` against the parent metric reaches the `%`
column, while the same format saved on the datasource does not. That is
what
the chart does, so the report does it too.
Three deliberate decisions:
**The JSON path only.** `table()` returns before any of this when
`apply_number_format` is false, which is every result format but JSON, so CSV
and XLSX attachments are byte-identical to master. Those formats get no table
post-processing at all today, which is a separate gap.
**No change to `post_processors`.** #45456 registers `ag-grid-table` against
the same `table` post-processor, and the two plugins' `transformProps` are
equivalent in this logic, so AG Grid is covered as soon as that lands. Adding
the registration here would only conflict with it.
**Faithful to the chart, including where the chart is wrong.** The offset
test
is `includes`, a substring test, and it is applied to dimensions as well as
metrics. A metric whose own label contains the offset text therefore looks
like
an offset column. With `time_compare: ['venue']` and a metric named
`revenue`,
`transformProps` returns no columns at all and the chart is empty. The report
now empties the same way. That is a chart-side defect and papering over it
here
would put the report back out of step with the chart, which is the thing this
PR is fixing.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
One metric at `1234567`, a `1 week ago` shift at `1000000`, with `,.2f` set
on
the metric in Customize:
```
before SUM(amount) 1,234,567.00
SUM(amount)__1 week ago 1000000.0
after Main SUM(amount) 1,234,567.00
# SUM(amount) 1,000,000.00
△ SUM(amount) 234,567.00
% SUM(amount) 0.23
```
The same chart with no format saved, where the `%` column falls to its own
default and the other three to SMART_NUMBER:
```
after Main SUM(amount) 1.23M
# SUM(amount) 1M
△ SUM(amount) 235k
% SUM(amount) 23.457%
```
Both are what the chart renders. `0.23` in the first block is not a rounding
slip: the parent's `d3NumberFormat` is inherited before the `%` column's own
default, so a metric formatted as `,.2f` shows its percent column as a ratio.
### TESTING INSTRUCTIONS
20 tests, 32 cases, in `tests/unit_tests/charts/test_client_processing.py`:
```
pytest tests/unit_tests/charts/test_client_processing.py
292 passed
```
260 pass on master, so every added case is new coverage.
Manually:
1. Build a Table chart with a metric, a time comparison, and a number format
on the metric in Customize.
2. Schedule a report on it and open the email body.
3. The report shows `Main`, `#`, `△` and `%`, formatted as the chart shows
them, instead of the metric and its raw offset column.
The derived columns carry the verbose metric label, like every other column
in
a report, while the `column_config` lookup uses the raw query label, which is
what `getComparisonColConfig` is passed.
### ADDITIONAL INFORMATION
- [x] Has associated issue: #45672
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in
[SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
Related and separate: `d3TimeFormat` on the report path is #45410, and
`table_timestamp_format` is still not applied anywhere there.
--
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]
