sadpandajoe commented on code in PR #45817:
URL: https://github.com/apache/superset/pull/45817#discussion_r4406394937
##########
superset/common/query_context_processor.py:
##########
@@ -644,6 +648,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:
This runs for every table-like chart, but the helper only reads
`column_config` and the datasource's saved formats. A Timeseries chart with
`y_axis_format=",.1f"` and no saved metric format still goes through the
fixed-point fallback, so its CSV shows `19.7999992` while the chart axis shows
`19.8`. Is this intended to cover only Table charts, and if so should the flag
description say that (or should other chart types resolve their own format keys
here)?
##########
tests/unit_tests/charts/test_export_number_format.py:
##########
@@ -0,0 +1,196 @@
+# 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.
+"""Unit tests for the export number-format helper and its feature flag."""
+
+from decimal import Decimal
+from unittest.mock import MagicMock
+
+import numpy as np
+import pandas as pd
+
+from superset.charts.client_processing import apply_export_number_formats
+from superset.utils import csv
+from superset.utils.core import GenericDataType
+
+
+# @AC-FR001-01
+def test_applies_configured_d3_format_to_numeric_column() -> None:
+ df = pd.DataFrame({"amount": [19.7999992]})
+ form_data = {"column_config": {"amount": {"d3NumberFormat": ",.1f"}}}
+
+ result = apply_export_number_formats(df, [GenericDataType.NUMERIC],
form_data, None)
+
+ assert result["amount"].tolist() == ["19.8"]
+
+
+# @AC-FR001-02
+def test_configured_format_respects_de_locale(mocker) -> None:
+ mocker.patch("superset.utils.number_format.get_locale",
return_value="de_DE")
+ df = pd.DataFrame({"amount": [1234.5]})
+ form_data = {
+ "column_config": {
+ "amount": {
+ "d3NumberFormat": ",.2f",
+ "currencyFormat": {"symbol": "EUR", "symbolPosition": None},
+ }
+ }
+ }
+
+ result = apply_export_number_formats(df, [GenericDataType.NUMERIC],
form_data, None)
+
+ value = result["amount"].tolist()[0]
+ # Locale-aware currency text, not a raw float and not a coerced date.
+ assert isinstance(value, str)
+ assert "€" in value
+ assert value != "1234.5"
+
+
+# @AC-FR003-01
+def test_unformatted_numeric_column_takes_fixed_point_fallback(mocker) -> None:
+ fallback = mocker.spy(csv, "format_decimal")
+ df = pd.DataFrame({"amount": [Decimal("19.7999992")]})
+
+ result = apply_export_number_formats(df, [GenericDataType.NUMERIC], {},
None)
+
+ fallback.assert_called()
+ value = result["amount"].tolist()[0]
+ assert value == "19.7999992"
+ assert not isinstance(value, float)
+
+
+# @AC-FR003-02
+def test_non_numeric_and_null_cells_left_intact() -> None:
+ df = pd.DataFrame(
+ {
+ "label": ["a", None, "c"],
+ "amount": [1.0, None, 3.0],
+ }
+ )
+ form_data = {"column_config": {"amount": {"d3NumberFormat": ",.1f"}}}
+
+ result = apply_export_number_formats(
+ df, [GenericDataType.STRING, GenericDataType.NUMERIC], form_data, None
+ )
+
+ # Non-numeric column is untouched.
+ assert result["label"].tolist() == ["a", None, "c"]
+ # Null numeric cell is not turned into a raw float string.
+ assert result["amount"].tolist()[0] == "1.0"
+ assert result["amount"].tolist()[2] == "3.0"
+
+
+# @AC-FR006-01
+def test_export_formatted_numbers_flag_defaults_to_false() -> None:
+ from superset.config import DEFAULT_FEATURE_FLAGS
+
+ assert DEFAULT_FEATURE_FLAGS["EXPORT_FORMATTED_NUMBERS"] is False
+
+
+# NFR-REL-01
+def test_edge_values_do_not_raise() -> None:
+ df = pd.DataFrame(
+ {
+ "configured": [np.nan, np.inf, -np.inf, 10**18],
+ "fallback": [
+ Decimal("NaN"),
+ Decimal("Infinity"),
+ None,
+ Decimal(10**18),
+ ],
+ }
+ )
+ form_data = {"column_config": {"configured": {"d3NumberFormat": ",.2f"}}}
+
+ result = apply_export_number_formats(
+ df,
+ [GenericDataType.NUMERIC, GenericDataType.NUMERIC],
+ form_data,
+ None,
+ )
+
+ # The call completes without raising and returns a frame of the same shape.
+ assert list(result.columns) == ["configured", "fallback"]
+ assert len(result) == 4
+
+
+# @AC-FR002-02
+def test_configured_format_output_is_plain_text_not_date(mocker) -> None:
+ # A localized currency value is emitted as plain display text; nothing in
+ # the helper turns a formatted number into a datetime/date object.
+ mocker.patch("superset.utils.number_format.get_locale",
return_value="de_DE")
+ df = pd.DataFrame({"amount": [3.5]})
+ form_data = {
+ "column_config": {
+ "amount": {
+ "d3NumberFormat": ",.1f",
+ "currencyFormat": {"symbol": "EUR", "symbolPosition": None},
+ }
+ }
+ }
+
+ result = apply_export_number_formats(df, [GenericDataType.NUMERIC],
form_data, None)
+
+ value = result["amount"].tolist()[0]
+ import datetime
+
+ assert isinstance(value, str)
+ assert not isinstance(value, (datetime.date, datetime.datetime,
pd.Timestamp))
+ assert "€" in value
+
+
+# NFR-PERF-01
+def test_each_numeric_column_formatted_in_a_single_pass(mocker) -> None:
+ # Guard against an asymptotic regression: the fixed-point fallback maps
each
+ # cell of a numeric column exactly once (one linear pass per column), never
+ # re-scanning the frame per cell.
+ fallback = mocker.spy(csv, "format_decimal")
+ rows = 50
+ df = pd.DataFrame({"amount": [Decimal(str(i)) for i in range(rows)]})
+
+ apply_export_number_formats(df, [GenericDataType.NUMERIC], {}, None)
+
+ # Exactly one call per cell -- linear, not quadratic.
+ assert fallback.call_count == rows
+
+
+# @AC-FR005-02
+def test_helper_does_not_reference_sql_lab_export_path() -> None:
+ # Scope isolation: the chart-export helper does not import or invoke the
SQL
+ # Lab decimal formatter, keeping the two export paths independent.
+ import inspect
+
+ from superset.charts import client_processing
+
+ source = inspect.getsource(client_processing.apply_export_number_formats)
+ assert "_format_decimal_columns" not in source
+ assert "sql_lab" not in source
+
+
+# RISK-T06
+def test_verbose_map_renamed_column_gets_configured_format() -> None:
Review Comment:
The datasource-format test passes an empty `form_data`, and the
`column_config` tests pass no datasource formats, so nothing pins which one
wins. With a saved `amount` format of `,.2f` plus USD and a chart override of
`,.1f` plus EUR, flipping the `config.get(...) or
number_format_by_label.get(...)` order in `apply_export_number_formats` would
leave every test green while exports use the saved format. Could this case be
added (via `get_data()` with the flag on, asserting one decimal and the EUR
symbol)? The `d3SmallNumberFormat` forwarding has the same gap: no export test
supplies it.
--
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]
