Repository navigation
feat: apply chart number formatting to CSV/Excel chart-data exports #45817
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
e7a4e8a
bdaad51
c4bec93
ce06342
936fa65
17b7fe5
7fed744
0d0f14d
fcd943d
4d55733
e4c3bce
8faccdf
dc15753
a934729
78351a9
ecb3342
5180710
e944e0a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,6 +25,7 @@ | |
| """ | ||
|
|
||
| import logging | ||
| import math | ||
| from collections.abc import Callable | ||
| from decimal import Decimal | ||
| from functools import partial | ||
|
|
@@ -43,10 +44,15 @@ | |
| from superset.utils import csv, excel | ||
| from superset.utils.core import ( | ||
| extract_dataframe_dtypes, | ||
| GenericDataType, | ||
| get_column_names, | ||
| get_metric_name, | ||
| get_metric_names, | ||
| ) | ||
| from superset.utils.currency import ( | ||
| detect_currency_from_df, | ||
| has_auto_currency_in_column_config, | ||
| ) | ||
| from superset.utils.number_format import ( | ||
| AUTO_CURRENCY, | ||
| format_number_with_config, | ||
|
|
@@ -956,7 +962,7 @@ def is_small_number(value: Any) -> bool: | |
|
|
||
| def format_column( | ||
| df: pd.DataFrame, | ||
| column: Any, | ||
| col_pos: int, | ||
| d3_format: Optional[str], | ||
| currency: dict[str, Any], | ||
| detected_currency: Optional[str] = None, | ||
|
|
@@ -968,6 +974,12 @@ def format_column( | |
| """ | ||
| Format a column in place when a number or currency format is configured. | ||
|
|
||
| The column is addressed by position (``col_pos``), not by label: two metrics | ||
| can share a verbose name, so ``get_data`` may rename distinct columns to the | ||
| same label. A label lookup (``df[label]``) would then return a two-column | ||
| frame and either skip the format silently or raise on ``zip(strict=True)``, | ||
| aborting the whole export; positional access formats each column exactly once. | ||
|
|
||
| ``detected_currency`` represents the query-wide single currency. When a | ||
| parallel ``currency_context`` series is present, AUTO uses each row/cell's | ||
| contributing currencies first. Mixed context renders a neutral number; | ||
|
|
@@ -996,6 +1008,7 @@ def format_column( | |
| small_d3_format = small_d3_format.replace("$", "") or SMART_NUMBER | ||
|
|
||
| if d3_format or currency.get("symbol") or small_d3_format: | ||
| series = df.iloc[:, col_pos] | ||
| if currency_context is None: | ||
| resolved_currency = resolve_auto_currency(currency, detected_currency) | ||
| format_value = partial( | ||
|
|
@@ -1005,15 +1018,18 @@ def format_column( | |
| format_small = partial( | ||
| format_number_with_config, small_d3_format, small_currency or {} | ||
| ) | ||
| df[column] = df[column].apply( | ||
| lambda value: ( | ||
| format_small(value) | ||
| if is_small_number(value) | ||
| else format_value(value) | ||
| ) | ||
| df.isetitem( | ||
| col_pos, | ||
| series.apply( | ||
| lambda value: ( | ||
| format_small(value) | ||
| if is_small_number(value) | ||
| else format_value(value) | ||
| ) | ||
| ), | ||
| ) | ||
| return | ||
| df[column] = df[column].apply(format_value) | ||
| df.isetitem(col_pos, series.apply(format_value)) | ||
| return | ||
|
|
||
| contexts = currency_context.reindex(df.index) | ||
|
|
@@ -1045,10 +1061,13 @@ def format_with_context(value: Any, context: Any) -> Any: | |
| value, | ||
| ) | ||
|
|
||
| df[column] = [ | ||
| format_with_context(value, context) | ||
| for value, context in zip(df[column], contexts, strict=True) | ||
| ] | ||
| df.isetitem( | ||
| col_pos, | ||
| [ | ||
| format_with_context(value, context) | ||
| for value, context in zip(series, contexts, strict=True) | ||
| ], | ||
| ) | ||
|
|
||
|
|
||
| def get_datasource_column_formats( | ||
|
|
@@ -1224,6 +1243,171 @@ def merge_currency_formats( | |
| return currency_formats | ||
|
|
||
|
|
||
| def _fallback_export_value(value: Any) -> Any: | ||
| """Render a finite numeric cell as exact fixed-point text. | ||
|
|
||
| An unconfigured numeric column must never serialize as a raw float | ||
| (``1e-07``/``19.7999992``) or lose precision to float64 re-inference, so | ||
| finite ``float`` and ``int`` cells become exact text here while every other | ||
| value -- ``Decimal`` (handled by ``csv.format_decimal``), ``None``, ``NaN``, | ||
| infinity and non-numeric cells -- passes through unchanged. | ||
| """ | ||
| if isinstance(value, bool): | ||
| return value | ||
| if isinstance(value, float) and math.isfinite(value): | ||
| # Route through ``Decimal(str())`` so the exact value the user sees is | ||
| # emitted as fixed-point text rather than a float repr or scientific | ||
| # notation, matching the Decimal branch below. | ||
| return format(Decimal(str(value)), "f") | ||
| if isinstance(value, int): | ||
| # Keep large integers exact: ``.map`` would otherwise re-infer an object | ||
| # column of ints with nulls as float64 and round them. | ||
| return str(value) | ||
| return csv.format_decimal(value) | ||
|
|
||
|
|
||
| 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``. Percent metric columns default to ``PERCENT_3_POINT`` and | ||
| 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()} | ||
|
|
||
| # Percent metric columns are emitted with a leading ``%`` and are not | ||
| # verbose-renamed, so match them by that prefixed label; they default to | ||
| # PERCENT_3_POINT the way the Table plugin's percent formatter does. | ||
| percent_metric_labels = { | ||
| f"%{name}" | ||
| for name in get_metric_names(form_data.get("percent_metrics")) | ||
| if f"%{name}" in df.columns | ||
| } | ||
|
|
||
| # The AG Grid table paints its cells through ``valueFormatter``, which never | ||
| # builds the small-number formatter, so an export leaves that format alone | ||
| # for it -- mirroring ``table()``'s own ``applies_small_number_format``. | ||
| applies_small_number_format = form_data.get("viz_type") != AG_GRID_TABLE | ||
|
|
||
| # Resolve an AUTO currency the way ``table()`` does: a per-row currency | ||
| # context drawn from the datasource's currency-code column drives each | ||
| # cell's symbol, with the query-wide single currency as the fallback. | ||
| # Without these an AUTO currency would export no symbol while the live | ||
| # chart shows one (e.g. ``1,234.50`` instead of ``$1,234.50``). | ||
| currency_column = get_datasource_currency_column(datasource, df) | ||
| row_currency_context = ( | ||
| df[currency_column].map(currency_context_value) if currency_column else None | ||
| ) | ||
| # The query-wide single currency backs an AUTO format whose per-row context | ||
| # is empty. It is detected from the frame already in hand (no extra query is | ||
| # issued here), and only when an AUTO currency is configured, mirroring | ||
| # ``_detect_currency``'s own gating. | ||
| top_level_currency = form_data.get("currency_format") or {} | ||
| top_level_auto = ( | ||
| isinstance(top_level_currency, dict) | ||
| and top_level_currency.get("symbol") == AUTO_CURRENCY | ||
| ) | ||
| auto_currency_configured = top_level_auto or has_auto_currency_in_column_config( | ||
| form_data | ||
| ) | ||
| detected_currency = ( | ||
| detect_currency_from_df(df, currency_column) | ||
| if auto_currency_configured and currency_column | ||
| else None | ||
| ) | ||
|
|
||
| numeric_columns = { | ||
| column | ||
| for column, coltype in zip(df.columns, coltypes, strict=True) | ||
| if coltype == GenericDataType.NUMERIC | ||
| } | ||
|
|
||
| for col_pos, column in enumerate(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; the AG Grid table ignores the small-number format. | ||
| small_number_format = ( | ||
| (config.get("d3SmallNumberFormat") or "").strip() or None | ||
| if applies_small_number_format | ||
| else None | ||
| ) | ||
|
|
||
| if column in percent_metric_labels: | ||
| # A percent metric defaults to PERCENT_3_POINT and its own formatter | ||
| # never carries a currency, matching the Table plugin. | ||
| format_column( | ||
| df, | ||
| col_pos, | ||
| number_format or PERCENT_3_POINT, | ||
| {}, | ||
| currency_context=row_currency_context, | ||
| small_d3_format=small_number_format, | ||
| small_currency=small_currency, | ||
| ) | ||
| elif number_format or currency.get("symbol") or small_number_format: | ||
| format_column( | ||
| df, | ||
| col_pos, | ||
| number_format, | ||
| currency, | ||
| detected_currency, | ||
| row_currency_context, | ||
| fallback_to_detected=False, | ||
| small_d3_format=small_number_format, | ||
| small_currency=small_currency, | ||
| ) | ||
|
Comment on lines
+1392
to
+1402
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: Assessment: 🟠 Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** superset/charts/client_processing.py
**Line:** 1291:1298
**Comment:**
*Api Mismatch: `AUTO` is passed without detected currency or row currency context, so it remains unresolved and exports amounts without the configured currency symbol.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed—this
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. This is a real chart/export mismatch: these calls pass the currency configuration but omit the detected currency and row-currency context that The export helper should receive and forward the same context as
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in 8faccdf.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of The formatting call now receives both If that's not right, unresolve this thread and CodeAnt will leave it open.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of
If that's not right, unresolve this thread and CodeAnt will leave it open.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of
If that's not right, unresolve this thread and CodeAnt will leave it open.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of
If that's not right, unresolve this thread and CodeAnt will leave it open.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Handled in the current revision.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of The formatting call now receives both If that's not right, unresolve this thread and CodeAnt will leave it open. |
||
| else: | ||
| # Nothing configured: keep exact fixed-point text rather than a raw | ||
| # float. Non-finite and non-numeric cells pass through untouched. | ||
| df.isetitem(col_pos, df.iloc[:, col_pos].map(_fallback_export_value)) | ||
|
|
||
| return df | ||
|
|
||
|
|
||
| def effective_show_values_as(form_data: dict[str, Any]) -> Optional[str]: | ||
| """Return the ``showValuesAs`` percent mode the live chart actually renders. | ||
|
|
||
|
|
@@ -1372,7 +1556,7 @@ def apply_pivot_number_formats( | |
| if currency_context is not None: | ||
| currency_context = currency_context.T | ||
|
|
||
| for column in df.columns: | ||
| for col_pos, column in enumerate(df.columns): | ||
| metric = column[metric_level] if isinstance(column, tuple) else column | ||
| column_currency_context = ( | ||
| currency_context[column] | ||
|
|
@@ -1389,7 +1573,7 @@ def apply_pivot_number_formats( | |
| column_number_format = SMART_NUMBER | ||
| format_column( | ||
| df, | ||
| column, | ||
| col_pos, | ||
| column_number_format, | ||
| column_currency, | ||
| detected_currency, | ||
|
|
@@ -1456,7 +1640,7 @@ def label_of(name: str) -> str: | |
| # SMART_NUMBER, and other numeric columns are only formatted when an explicit | ||
| # format or currency is configured. Dimension and non-numeric columns are | ||
| # left untouched, matching the browser. | ||
| for column in df.columns: | ||
| for col_pos, column in enumerate(df.columns): | ||
| 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 | ||
|
|
@@ -1482,15 +1666,15 @@ def label_of(name: str) -> str: | |
| else None | ||
| ) | ||
|
|
||
| is_number = pd.api.types.is_numeric_dtype(df[column]) | ||
| is_number = pd.api.types.is_numeric_dtype(df.iloc[:, col_pos]) | ||
|
|
||
| if column in percent_metric_labels: | ||
| # A percent metric's own formatter never carries a currency, but | ||
| # ``getSmallNumberFormatter`` still builds one from the column | ||
| # config, so the small-number cells need the row's context too. | ||
| format_column( | ||
| df, | ||
| column, | ||
| col_pos, | ||
| number_format or PERCENT_3_POINT, | ||
| {}, | ||
| currency_context=row_currency_context, | ||
|
|
@@ -1513,7 +1697,7 @@ def label_of(name: str) -> str: | |
| number_format = SMART_NUMBER | ||
| format_column( | ||
| df, | ||
| column, | ||
| col_pos, | ||
| number_format, | ||
| currency, | ||
| detected_currency, | ||
|
|
@@ -1641,7 +1825,16 @@ def apply_client_processing( # noqa: C901 | |
| if datasource and query["result_format"] == ChartDataResultFormat.JSON: | ||
| df.rename(columns=datasource.data["verbose_map"], inplace=True) | ||
|
|
||
| apply_number_format = query["result_format"] == ChartDataResultFormat.JSON | ||
| # With EXPORT_FORMATTED_NUMBERS the tabular export formats are formatted | ||
| # here, after the pivot/table post-processing arithmetic has run on the | ||
| # raw numbers, so the formatter never sees (and cannot coerce to NaN) | ||
| # the intermediate values the way pre-serialization formatting would. | ||
| from superset import is_feature_enabled # noqa: PLC0415 | ||
|
|
||
| apply_number_format = query["result_format"] == ChartDataResultFormat.JSON or ( | ||
| is_feature_enabled("EXPORT_FORMATTED_NUMBERS") | ||
| and query["result_format"] in ChartDataResultFormat.table_like() | ||
| ) | ||
| processed_df = post_processor( | ||
| df, | ||
| form_data, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switching
format_columnto a positionalcol_posleaves its existing callers intests/unit_tests/charts/test_client_processing.py(around lines 3915-3942) passing"amount", sodf.iloc[:, "amount"]raises aValueError. That is whyunit-tests (current)fails on threetest_format_column_*tests, with the flag off. Should those callers pass the column position (0)?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. The direct
format_columncallers intest_client_processing.py(~lines 3945-3972) now pass the column position0instead of"amount", so the threetest_format_column_*tests pass with the flag off. Verified locally:pytest tests/unit_tests/charts/test_client_processing.py -k format_column-> 4 passed.