sadpandajoe commented on code in PR #45817: URL: https://github.com/apache/superset/pull/45817#discussion_r4449026167
########## tests/unit_tests/common/test_query_context_processor_export_format.py: ########## @@ -0,0 +1,549 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""Integration tests for number-formatted CSV/XLSX export in get_data().""" + +import io +from decimal import Decimal +from types import SimpleNamespace +from typing import Any, cast + +import pandas as pd + +from superset.common.chart_data import ChartDataResultFormat, ChartDataResultType +from superset.common.query_context import QueryContext +from superset.common.query_context_processor import QueryContextProcessor +from superset.utils.core import GenericDataType +from tests.unit_tests.conftest import with_feature_flags + + +def _make_processor( + result_format: ChartDataResultFormat, + form_data: dict[str, Any], + datasource_data: dict[str, Any] | None = None, + result_type: ChartDataResultType = ChartDataResultType.FULL, +) -> QueryContextProcessor: + datasource = SimpleNamespace(data=datasource_data or {}) + context = SimpleNamespace( + datasource=datasource, + result_format=result_format, + result_type=result_type, + form_data=form_data, + ) + return QueryContextProcessor(cast(QueryContext, context)) + + +def _csv_text(payload: str | bytes | list[dict[str, Any]]) -> str: + # get_data() is typed as a union over export formats; the CSV branch always + # returns bytes, so narrow before decoding. + assert isinstance(payload, bytes) + return payload.decode("utf-8-sig") + + +def _read_xlsx_column( + payload: str | bytes | list[dict[str, Any]], column: str +) -> list[Any]: + # The XLSX branch of get_data() always returns bytes; narrow the union. + assert isinstance(payload, bytes) + frame = pd.read_excel(io.BytesIO(payload)) + return frame[column].tolist() + + +def _read_xlsx_raw_cell( + payload: str | bytes | list[dict[str, Any]], row: int, col: int +) -> tuple[Any, str]: + # Read the stored value *and* its cell type straight from the workbook, so a + # formatted string that XLSX re-serialization turned back into a number is + # distinguishable from one kept as text (openpyxl data_type "s" vs "n"). + from openpyxl import load_workbook + + assert isinstance(payload, bytes) + worksheet = load_workbook(io.BytesIO(payload)).active + cell = worksheet.cell(row=row, column=col) + return cell.value, cell.data_type + + +# @AC-FR001-01 +@with_feature_flags(EXPORT_FORMATTED_NUMBERS=True) +def test_csv_export_applies_configured_number_format(app_context: None) -> None: + processor = _make_processor( + ChartDataResultFormat.CSV, + {"column_config": {"amount": {"d3NumberFormat": ",.1f"}}}, + ) + df = pd.DataFrame({"amount": [19.7999992]}) + + result = processor.get_data(df, [GenericDataType.NUMERIC]) + + text = _csv_text(result) + assert "19.8" in text + assert "19.7999992" not in text + + +# @AC-FR001-03 +@with_feature_flags(EXPORT_FORMATTED_NUMBERS=True) +def test_csv_export_unformattable_value_takes_fixed_point_fallback( + app_context: None, +) -> None: + processor = _make_processor(ChartDataResultFormat.CSV, {}) + df = pd.DataFrame({"amount": [Decimal("19.7999992")]}) + + result = processor.get_data(df, [GenericDataType.NUMERIC]) + + text = _csv_text(result) + # Exact fixed-point text, never a float repr / scientific notation. + assert "19.7999992" in text + assert "e" not in text.lower().split("amount")[-1] + + +# @AC-FR002-01 +@with_feature_flags(EXPORT_FORMATTED_NUMBERS=True) +def test_xlsx_export_matches_csv_for_shared_column(app_context: None) -> None: + form_data = {"column_config": {"amount": {"d3NumberFormat": ",.1f"}}} + + csv_processor = _make_processor(ChartDataResultFormat.CSV, form_data) + csv_result = csv_processor.get_data( + pd.DataFrame({"amount": [19.7999992]}), [GenericDataType.NUMERIC] + ) + csv_value = _csv_text(csv_result).splitlines()[1].strip() + + xlsx_processor = _make_processor(ChartDataResultFormat.XLSX, form_data) + xlsx_result = xlsx_processor.get_data( + pd.DataFrame({"amount": [19.7999992]}), [GenericDataType.NUMERIC] + ) + xlsx_value = _read_xlsx_column(xlsx_result, "amount")[0] + + # Both formats display 19.8 for the same source value; the formatted text is + # now kept as a string in XLSX too, so the configured decimals survive. + assert str(xlsx_value) == csv_value == "19.8" + + +# @AC-FR004-01 +@with_feature_flags(EXPORT_FORMATTED_NUMBERS=True) +def test_csv_escapes_formatted_formula_prefixed_value(app_context: None) -> None: + processor = _make_processor( + ChartDataResultFormat.CSV, + {"column_config": {"amount": {"d3NumberFormat": "+,.1f"}}}, + ) + df = pd.DataFrame({"amount": [19.7999992]}) + + result = processor.get_data(df, [GenericDataType.NUMERIC]) + + text = _csv_text(result) + # The formatter emits "+19.8"; escaping (which runs after formatting) + # must prefix the formula-like value with a single quote. + assert "'+19.8" in text + + +# @AC-FR004-02 +@with_feature_flags(EXPORT_FORMATTED_NUMBERS=True) +def test_xlsx_quotes_formatted_formula_prefixed_value(app_context: None) -> None: + processor = _make_processor( + ChartDataResultFormat.XLSX, + {"column_config": {"amount": {"d3NumberFormat": "+,.2f"}}}, + ) + df = pd.DataFrame({"amount": [1234567.891]}) + + result = processor.get_data(df, [GenericDataType.NUMERIC]) + + # The formatter emits "+1,234,567.89"; formula-quoting (which runs after + # formatting) must prefix the formula-like value with a single quote. + value = _read_xlsx_column(result, "amount")[0] + assert value == "'+1,234,567.89" + + +# @AC-FR006-02 +def test_flag_off_csv_and_xlsx_byte_identical(app_context: None) -> None: + form_data = {"column_config": {"amount": {"d3NumberFormat": ",.1f"}}} + + csv_processor = _make_processor(ChartDataResultFormat.CSV, form_data) + csv_result = csv_processor.get_data( + pd.DataFrame({"amount": [19.7999992]}), [GenericDataType.NUMERIC] + ) + # Flag defaults to False: the raw float survives unformatted. + assert b"19.7999992" in csv_result + assert b"19.8" not in csv_result + + xlsx_processor = _make_processor(ChartDataResultFormat.XLSX, form_data) + xlsx_result = xlsx_processor.get_data( + pd.DataFrame({"amount": [19.7999992]}), [GenericDataType.NUMERIC] + ) + assert _read_xlsx_column(xlsx_result, "amount")[0] == 19.7999992 + + +# @AC-FR005-01 +@with_feature_flags(EXPORT_FORMATTED_NUMBERS=True) +def test_json_result_unchanged_by_formatting_stage(app_context: None) -> None: + processor = _make_processor( + ChartDataResultFormat.JSON, + {"column_config": {"amount": {"d3NumberFormat": ",.1f"}}}, + ) + df = pd.DataFrame({"amount": [19.7999992]}) + + result = processor.get_data(df, [GenericDataType.NUMERIC]) + + # JSON path returns records before the table_like formatting stage. + assert result == [{"amount": 19.7999992}] + + +# @AC-FR001-02 +@with_feature_flags(EXPORT_FORMATTED_NUMBERS=True) +def test_csv_export_de_locale_formats_consistently(app_context: None, mocker) -> None: + # A ``de`` request locale drives the currency symbol position; the d3 number + # grammar itself is locale-independent, matching the frontend d3-format. + mocker.patch("superset.utils.number_format.get_locale", return_value="de_DE") + processor = _make_processor( + ChartDataResultFormat.CSV, + { + "column_config": { + "amount": { + "d3NumberFormat": ",.2f", + "currencyFormat": {"symbol": "EUR", "symbolPosition": None}, + } + } + }, + ) + df = pd.DataFrame({"amount": [1234.5]}) + + result = processor.get_data(df, [GenericDataType.NUMERIC]) + + text = _csv_text(result) + # Consistent localized currency text: no raw float survives, and the value + # is not coerced to a date. + assert "€" in text + assert "1234.5" not in text + assert "1,234.50" in text + assert "Mai" not in text + + +# @AC-FR002-02 +@with_feature_flags(EXPORT_FORMATTED_NUMBERS=True) +def test_xlsx_export_de_locale_value_not_date_coerced( + app_context: None, mocker +) -> None: + # A localized currency value stays a string through the XLSX text path; + # ``apply_column_types`` cannot re-coerce it to a number (or a date). + mocker.patch("superset.utils.number_format.get_locale", return_value="de_DE") + processor = _make_processor( + ChartDataResultFormat.XLSX, + { + "column_config": { + "amount": { + "d3NumberFormat": ",.2f", + "currencyFormat": {"symbol": "EUR", "symbolPosition": None}, + } + } + }, + ) + df = pd.DataFrame({"amount": [1234.5]}) + + result = processor.get_data(df, [GenericDataType.NUMERIC]) + + value = _read_xlsx_column(result, "amount")[0] + # The cell is the formatted string, never a datetime and never "03. Mai". + assert isinstance(value, str) + assert "€" in value + assert "1,234.50" in value + assert not isinstance(value, pd.Timestamp) + assert "Mai" not in value + + +# @AC-FR005-01 +@with_feature_flags(EXPORT_FORMATTED_NUMBERS=True) +def test_arrow_result_byte_identical_with_flag_on(app_context: None) -> None: + form_data = {"column_config": {"amount": {"d3NumberFormat": ",.1f"}}} + source = pd.DataFrame({"amount": [19.7999992]}) + + on_processor = _make_processor(ChartDataResultFormat.ARROW, form_data) + on_bytes = on_processor.get_data(source.copy(), [GenericDataType.NUMERIC]) + + # Re-serialize the untouched frame independently: the formatting stage runs + # only on the table_like branch, so Arrow must be byte-for-byte identical. + reference = QueryContextProcessor._to_arrow_ipc(source.copy()) + + assert isinstance(on_bytes, bytes) + assert on_bytes == reference + + +# @AC-FR005-01 +def test_arrow_result_identical_regardless_of_flag(app_context: None) -> None: + # The flag-off Arrow bytes equal the independently serialized reference too, + # confirming the stage never touches the Arrow branch in either state. + form_data = {"column_config": {"amount": {"d3NumberFormat": ",.1f"}}} + source = pd.DataFrame({"amount": [19.7999992]}) + + off_processor = _make_processor(ChartDataResultFormat.ARROW, form_data) + off_bytes = off_processor.get_data(source.copy(), [GenericDataType.NUMERIC]) + reference = QueryContextProcessor._to_arrow_ipc(source.copy()) + + assert off_bytes == reference + + +# @AC-FR005-02 +def test_sql_lab_decimal_formatting_path_is_untouched() -> None: + # Scope guard: the SQL Lab export helper keeps its own decimal handling and + # never routes through the new chart-export helper. + import inspect + + from superset.commands.sql_lab import export as sql_lab_export + + source = inspect.getsource(sql_lab_export) + assert "apply_export_number_formats" not in source + assert "EXPORT_FORMATTED_NUMBERS" not in source + + # And its behavior is unchanged: a finite decimal becomes fixed-point text + # while floats/ints/None keep their types, with no d3 formatting applied. + df = pd.DataFrame( + {"amount": [Decimal("19.7999992"), 3.0, None]}, + dtype=object, + ) + + sql_lab_export._format_decimal_columns(df) + + values = df["amount"].tolist() + assert values[0] == "19.7999992" + assert values[1] == 3.0 + assert values[2] is None + + +# @AC-FR003-01 +@with_feature_flags(EXPORT_FORMATTED_NUMBERS=True) +def test_csv_export_null_nan_inf_pass_through(app_context: None) -> None: + # Edge values through the full export path: no raise, nulls stay blank, and + # a configured column still renders its finite cells. + processor = _make_processor( + ChartDataResultFormat.CSV, + {"column_config": {"amount": {"d3NumberFormat": ",.1f"}}}, + ) + df = pd.DataFrame( + { + "label": ["ok", None, "z"], + "amount": [19.75, float("nan"), float("inf")], + } + ) + + result = processor.get_data(df, [GenericDataType.STRING, GenericDataType.NUMERIC]) + + text = _csv_text(result) + # The finite cell is formatted; the frame still has three data rows. + assert "19.8" in text + assert len(text.strip().splitlines()) == 1 + 3 + + +# @AC-FR003-02 +@with_feature_flags(EXPORT_FORMATTED_NUMBERS=True) +def test_csv_export_unformattable_decimal_edge_values(app_context: None) -> None: + # Fixed-point fallback over a column with no config: finite decimals become + # exact text, non-finite decimals and None pass through without raising. + processor = _make_processor(ChartDataResultFormat.CSV, {}) + df = pd.DataFrame( + {"amount": [Decimal("19.7999992"), Decimal("NaN"), None]}, + dtype=object, + ) + + result = processor.get_data(df, [GenericDataType.NUMERIC]) + + text = _csv_text(result) + assert "19.7999992" in text + # No scientific notation leaked into the serialized decimal. + assert "e+" not in text.lower() + + +# @AC-FR002-01 +@with_feature_flags(EXPORT_FORMATTED_NUMBERS=True) +def test_xlsx_export_keeps_configured_trailing_decimals(app_context: None) -> None: + # A ",.2f" format renders 3.5 as "3.50"; the XLSX cell must stay text so the + # trailing zero is not coerced back to the number 3.5 by apply_column_types. + # (pd.read_excel would re-infer a numeric-looking string, so read the raw + # cell via openpyxl to observe what was actually stored.) + from openpyxl import load_workbook + + processor = _make_processor( + ChartDataResultFormat.XLSX, + {"column_config": {"amount": {"d3NumberFormat": ",.2f"}}}, + ) + df = pd.DataFrame({"amount": [3.5]}) + + result = processor.get_data(df, [GenericDataType.NUMERIC]) + + assert isinstance(result, bytes) + cell = load_workbook(io.BytesIO(result)).active["A2"] + assert cell.data_type == "s" + assert cell.value == "3.50" + + +# @AC-FR001-01 +@with_feature_flags(EXPORT_FORMATTED_NUMBERS=True) +def test_chart_config_format_wins_over_saved_datasource_format( + app_context: None, +) -> None: + # A chart's column_config override must win over the datasource's saved + # per-metric format and currency, pinning the config.get(...) precedence in + # apply_export_number_formats (a saved ",.2f"/USD would otherwise leak). + processor = _make_processor( + ChartDataResultFormat.CSV, + { + "column_config": { + "amount": { + "d3NumberFormat": ",.1f", + "currencyFormat": {"symbol": "EUR", "symbolPosition": None}, + } + } + }, + datasource_data={ + "column_formats": {"amount": ",.2f"}, + "currency_formats": {"amount": {"symbol": "USD"}}, + }, + ) + df = pd.DataFrame({"amount": [1234.5]}) + + result = processor.get_data(df, [GenericDataType.NUMERIC]) + + text = _csv_text(result) + # The chart override (one decimal, EUR) wins over the saved format. + assert "€" in text + assert "1,234.5" in text + assert "1,234.50" not in text + assert "$" not in text + + +# @AC-FR001-01 +@with_feature_flags(EXPORT_FORMATTED_NUMBERS=True) +def test_small_number_format_forwarded_to_export(app_context: None) -> None: + # The column's d3SmallNumberFormat is applied per cell: a sub-one value takes + # it while larger values keep the column's normal format, matching the chart. + processor = _make_processor( + ChartDataResultFormat.CSV, + { + "column_config": { + "amount": { + "d3NumberFormat": ",.0f", + "d3SmallNumberFormat": ",.1%", + } + } + }, + ) + df = pd.DataFrame({"amount": [0.125, 5.0]}) + + result = processor.get_data(df, [GenericDataType.NUMERIC]) + + text = _csv_text(result) + # Sub-one value rendered by the small-number format, the other by the main. + assert "12.5%" in text + assert "5" in text Review Comment: `assert "5" in text` is already satisfied by the `12.5%` on the line above, so it does not show the `5.0` row takes the main `,.0f` format; if that cell stayed `5.0` or disappeared the test would still pass. Could you parse the CSV and assert the amount cells are exactly `["12.5%", "5"]`? ########## superset/charts/client_processing.py: ########## @@ -956,7 +962,7 @@ def is_small_number(value: Any) -> bool: def format_column( df: pd.DataFrame, - column: Any, + col_pos: int, Review Comment: Switching `format_column` to a positional `col_pos` leaves its existing callers in `tests/unit_tests/charts/test_client_processing.py` (around lines 3915-3942) passing `"amount"`, so `df.iloc[:, "amount"]` raises a `ValueError`. That is why `unit-tests (current)` fails on three `test_format_column_*` tests, with the flag off. Should those callers pass the column position (`0`)? ########## superset/charts/client_processing.py: ########## @@ -1222,6 +1228,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 column in df.columns: Review Comment: Positional writes fix the column lookup, but two same-label metrics still lose their own formats and values. `number_format_by_label`, `currency_by_label` and `config_by_label` are keyed by label, so with `a` saved as `.2f` and `b` as `.0%`, both shown as `Total`, `a=1.5` exports as `150%` instead of `1.50`. For CSV the formatted columns are now strings, and `df_to_escaped_csv` writes them back with `df.at[label, name]`, which assigns the first value to both same-named columns (`1.50,2.50` exports as `1.50,1.50`; I reproduced it on a two-column frame). Should format resolution key off the original column identity, and the string write-back avoid label-based `.at`? -- 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]
