sadpandajoe commented on code in PR #45817:
URL: https://github.com/apache/superset/pull/45817#discussion_r4401336035
##########
superset/common/query_context_processor.py:
##########
@@ -644,6 +646,14 @@ def get_data(
if verbose_map:
df.columns = [verbose_map.get(column, column) for column in
columns]
+ if is_feature_enabled("EXPORT_FORMATTED_NUMBERS"):
Review Comment:
With the flag on, `POST_PROCESSED` exports get formatted strings here, and
`apply_client_processing()` then runs the pivot arithmetic on that CSV. Pivot
totals go through `pd.to_numeric(errors="coerce")`, so a saved `,.2f` metric
(`"1,000.00"` + `"2,000.00"`) turns into NaN and the total exports as 0 instead
of 3000. Should the formatting happen after the pivot/post-processing step
rather than before it?
##########
superset/charts/client_processing.py:
##########
@@ -1222,6 +1223,87 @@ def merge_currency_formats(
return currency_formats
+def apply_export_number_formats(
+ df: pd.DataFrame,
+ coltypes: list[GenericDataType],
+ form_data: dict[str, Any],
+ datasource: Optional[Union["BaseDatasource", "Query"]] = None,
+) -> pd.DataFrame:
+ """Format numeric columns as display text for a chart-data export.
+
+ Each numeric column (by ``coltypes``, aligned with ``df.columns``) is
+ resolved to its configured d3 format, currency and small-number format the
+ same way the live Table plugin does -- ``column_config`` overrides first,
+ then the datasource's saved per-metric formats -- and handed to
+ ``format_column``. A numeric column with nothing configured takes the
+ fixed-point fallback so its cells are never serialized as raw floats.
+ Non-numeric columns, nulls, NaN and infinity are left to the existing
+ helpers and pass through unchanged. The passed frame is mutated in place
and
+ returned; the helper runs no query and performs no I/O.
+ """
+ saved_formats, verbose_map = get_datasource_column_formats(datasource)
+ saved_currency_formats, _ = get_datasource_currency_formats(datasource)
+ column_config = form_data.get("column_config") or {}
+
+ def label_of(name: str) -> str:
+ """Return the column label as it appears in ``df``
(verbose-renamed)."""
+ return name if name in df.columns else verbose_map.get(name, name)
+
+ # Index the per-column overrides by the label present in ``df`` so the
+ # lookups line up with the (possibly verbose-renamed) frame columns.
+ number_format_by_label = {
+ label_of(name): fmt for name, fmt in saved_formats.items()
+ }
+ currency_by_label = {
+ label_of(name): currency for name, currency in
saved_currency_formats.items()
+ }
+ config_by_label = {label_of(name): config for name, config in
column_config.items()}
+
+ numeric_columns = {
+ column
+ for column, coltype in zip(df.columns, coltypes, strict=True)
+ if coltype == GenericDataType.NUMERIC
+ }
+
+ for column in df.columns:
+ if column not in numeric_columns:
+ continue
+
+ config = config_by_label.get(column) or {}
+ configured_currency = config.get("currencyFormat") or {}
+ # The small-number formatter is built from this object whether or not a
+ # symbol survived in it, so an absent one has to stay distinguishable
+ # from a cleared one.
+ small_currency = config.get("currencyFormat")
+ number_format = config.get("d3NumberFormat") or
number_format_by_label.get(
+ column
+ )
+ currency = (
+ configured_currency
+ if configured_currency.get("symbol")
+ else currency_by_label.get(column) or {}
+ )
+ # A cleared Select leaves a blank string behind, which the browser
+ # treats as unset.
+ small_number_format = (config.get("d3SmallNumberFormat") or
"").strip() or None
Review Comment:
This applies `d3SmallNumberFormat` unconditionally, but the existing
`table()` export helper skips it for `ag-grid-table`
(`applies_small_number_format`, ~line 1507) because AG Grid cells only use the
column's normal formatter. With `.2f` plus a `.1%` small-number format, `0.125`
displays as `0.13` in the chart but exports as `12.5%`. Should this honor the
same AG Grid exemption?
##########
superset/charts/client_processing.py:
##########
@@ -1222,6 +1223,87 @@ def merge_currency_formats(
return currency_formats
+def apply_export_number_formats(
+ df: pd.DataFrame,
+ coltypes: list[GenericDataType],
+ form_data: dict[str, Any],
+ datasource: Optional[Union["BaseDatasource", "Query"]] = None,
+) -> pd.DataFrame:
+ """Format numeric columns as display text for a chart-data export.
+
+ Each numeric column (by ``coltypes``, aligned with ``df.columns``) is
+ resolved to its configured d3 format, currency and small-number format the
+ same way the live Table plugin does -- ``column_config`` overrides first,
+ then the datasource's saved per-metric formats -- and handed to
+ ``format_column``. A numeric column with nothing configured takes the
+ fixed-point fallback so its cells are never serialized as raw floats.
+ Non-numeric columns, nulls, NaN and infinity are left to the existing
+ helpers and pass through unchanged. The passed frame is mutated in place
and
+ returned; the helper runs no query and performs no I/O.
+ """
+ saved_formats, verbose_map = get_datasource_column_formats(datasource)
+ saved_currency_formats, _ = get_datasource_currency_formats(datasource)
+ column_config = form_data.get("column_config") or {}
+
+ def label_of(name: str) -> str:
+ """Return the column label as it appears in ``df``
(verbose-renamed)."""
+ return name if name in df.columns else verbose_map.get(name, name)
+
+ # Index the per-column overrides by the label present in ``df`` so the
+ # lookups line up with the (possibly verbose-renamed) frame columns.
+ number_format_by_label = {
+ label_of(name): fmt for name, fmt in saved_formats.items()
+ }
+ currency_by_label = {
+ label_of(name): currency for name, currency in
saved_currency_formats.items()
+ }
+ config_by_label = {label_of(name): config for name, config in
column_config.items()}
+
+ numeric_columns = {
+ column
+ for column, coltype in zip(df.columns, coltypes, strict=True)
+ if coltype == GenericDataType.NUMERIC
+ }
+
+ for column in df.columns:
+ if column not in numeric_columns:
+ continue
+
+ config = config_by_label.get(column) or {}
+ configured_currency = config.get("currencyFormat") or {}
+ # The small-number formatter is built from this object whether or not a
+ # symbol survived in it, so an absent one has to stay distinguishable
+ # from a cleared one.
+ small_currency = config.get("currencyFormat")
+ number_format = config.get("d3NumberFormat") or
number_format_by_label.get(
+ column
+ )
+ currency = (
+ configured_currency
+ if configured_currency.get("symbol")
+ else currency_by_label.get(column) or {}
+ )
+ # A cleared Select leaves a blank string behind, which the browser
+ # treats as unset.
+ small_number_format = (config.get("d3SmallNumberFormat") or
"").strip() or None
+
+ if number_format or currency.get("symbol") or small_number_format:
+ format_column(
+ df,
+ column,
+ number_format,
+ currency,
+ small_d3_format=small_number_format,
+ small_currency=small_currency,
+ )
+ else:
+ # Nothing configured: keep exact fixed-point text rather than a raw
+ # float. Non-finite and non-decimal cells pass through untouched.
+ df[column] = df[column].map(csv.format_decimal)
Review Comment:
`csv.format_decimal` only converts `Decimal`, so an unconfigured numeric
column that holds floats is left as a float here (and `map` can re-infer an
object column of large ints with nulls as float64, e.g. `9007199254740993`
becomes `9007199254740992.0`). The CSV can still contain `1e-07` or
`19.7999992`, which is the symptom this PR is fixing. Should the fallback
format floats as fixed-point text too, with a float case added to the fallback
tests (they only use `Decimal` today)?
##########
superset/config.py:
##########
@@ -850,6 +850,10 @@ class D3TimeFormat(TypedDict, total=False):
# Warning: Could cause server memory/compute issues with large datasets.
# @lifecycle: testing
"ALLOW_FULL_CSV_EXPORT": False,
+ # Apply the chart's configured number formats to chart-data CSV/XLSX
exports
+ # instead of serializing raw values.
+ # @lifecycle: testing
+ "EXPORT_FORMATTED_NUMBERS": False,
Review Comment:
The flag is described as covering chart-data CSV exports, but formatting
only lives in `QueryContextProcessor.get_data`. Large CSV downloads go through
`StreamingCSVExportCommand` (above `CSV_STREAMING_ROW_THRESHOLD`, and sometimes
before the query runs), which writes raw values. The same `,.1f` column exports
`19.8` below the threshold and `19.7999992` above it with the flag on. Should
the streaming path apply the formats too, or should the flag description call
out the limitation?
--
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]
