codeant-ai-for-open-source[bot] commented on code in PR #41028: URL: https://github.com/apache/superset/pull/41028#discussion_r3481387437
########## superset/utils/number_format.py: ########## @@ -0,0 +1,250 @@ +# 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. +""" +Server-side port of the d3-format based number and currency formatters used by +the Table and Pivot Table chart plugins. + +Report notifications that embed a chart as text build the table in Python and +have no access to the frontend formatters, so chart number/currency format +configuration has to be reproduced here to render the same values an end user +sees in the browser. +""" + +from __future__ import annotations + +import math +import re +from decimal import Decimal, ROUND_HALF_UP +from typing import Any + +from babel.numbers import get_currency_symbol + +SMART_NUMBER = "SMART_NUMBER" +SMART_NUMBER_SIGNED = "SMART_NUMBER_SIGNED" +AUTO_CURRENCY = "AUTO" + +LOCALE = "en_US" + +# SI prefixes keyed by their power-of-1000 exponent, mirroring d3-format. +SI_PREFIXES = { + -8: "y", + -7: "z", + -6: "a", + -5: "f", + -4: "p", + -3: "n", + -2: "µ", + -1: "m", + 0: "", + 1: "k", + 2: "M", + 3: "G", + 4: "T", + 5: "P", + 6: "E", + 7: "Z", + 8: "Y", +} + +# d3-format specifier grammar: +# [[fill]align][sign][symbol][0][width][,][.precision][~][type] +D3_FORMAT_RE = re.compile( + r"^(?:(.)?([<>=^]))?([+\-( ])?([$#])?(0)?(\d+)?(,)?(?:\.(\d+))?(~)?([a-z%])?$", + re.IGNORECASE, +) + + +def format_number_with_config( + d3_format: str | None, + currency: dict[str, Any] | None, + value: Any, +) -> Any: + """ + Format ``value`` using a d3-format string and optional currency config. + + :param d3_format: a d3-format specifier (e.g. ``",.2f"``) or ``SMART_NUMBER`` + :param currency: ``{"symbol": <ISO 4217>, "symbolPosition": "prefix"|"suffix"}`` + :param value: the raw value to format + :return: the formatted string, or the value unchanged when it is not a + number that can be formatted + """ + if value is None: + return "" + if isinstance(value, bool) or not isinstance(value, (int, float)): + return value + if math.isnan(value) or math.isinf(value): + return "" + + try: + if currency and currency.get("symbol"): + # the frontend strips the currency symbol from the d3 format and + # falls back to SMART_NUMBER when no explicit format is set + number_format = (d3_format or SMART_NUMBER).replace("$", "") + formatted = format_numeric(number_format, value) + if currency["symbol"] == AUTO_CURRENCY: + return formatted + return apply_currency(formatted, currency) + if not d3_format: + return raw_string(value) + return format_numeric(d3_format, value) + except Exception: # pylint: disable=broad-except # noqa: BLE001 + # never let an unexpected value break a whole report table + return raw_string(value) + + +def format_numeric(d3_format: str, value: float) -> str: + if d3_format in (SMART_NUMBER, SMART_NUMBER_SIGNED): + return format_smart_number(value, signed=d3_format == SMART_NUMBER_SIGNED) + return format_d3(d3_format, value) + + +def format_d3(d3_format: str, value: float) -> str: + match = D3_FORMAT_RE.match(d3_format) + if not match: + raise ValueError(d3_format) + + sign = "+" if match.group(3) == "+" else "" Review Comment: **Suggestion:** The parser accepts all d3 sign modes (`+`, `-`, `(`, and space), but only `+` is implemented, so accounting formats like `(.2f` are rendered with a minus sign instead of parentheses. This breaks parity with d3 for valid format strings. Implement handling for `(` and space sign modes when building the formatted output. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Accounting-style formats show minus sign instead of parentheses. - ⚠️ Reports diverge from frontend display for custom d3 formats. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. A Table or Pivot Table v2 chart is configured in the frontend with a custom d3 number format that uses a non-default sign mode, such as `"(.2f"` for accounting-style negatives or `" .2f"` for leading-space positives. This format is stored in the chart's form data as `d3NumberFormat` (for Table columns) or `valueFormat`/`columnFormats` (for Pivot), which are passed unchanged into the server-side `form_data`. 2. When an Alert/Report or API client requests the chart in post-processed form, `ChartDataRestApi._send_chart_response` in `superset/charts/data/api.py:1-19` sees `result_type == ChartDataResultType.POST_PROCESSED` and calls `apply_client_processing(result, form_data, datasource)` at line 16. For `"table"` or `"pivot_table_v2"` viz types, `apply_client_processing` selects the corresponding post-processor from `post_processors` at `superset/charts/client_processing.py:129-131`. 3. In both `table()` and `apply_pivot_number_formats()` (`superset/charts/client_processing.py:106-126` and 77-103), numeric cells are formatted via `format_column`, which delegates to `format_number_with_config(d3_format, currency, value)` in `superset/utils/number_format.py:71-107`. That function parses the d3 specifier using `D3_FORMAT_RE` and then calls `format_d3(d3_format, value)` at line 112 for non-SMART_NUMBER formats. 4. Inside `format_d3` at `superset/utils/number_format.py:115-145`, the parser extracts the sign component to `match.group(3)`, which can legally be `"+"`, `"-"`, `"("`, or `" "` per the regex at lines 63-67. However, the implementation reduces this to `sign = "+" if match.group(3) == "+" else ""` at line 120 and never distinguishes `"("` or `" "` from the default. For negative values formatted with `"(.2f"`, Python's `format(value, f"{sign}{comma}{decimals}{type_}")` (lines 134-138) produces a leading `"-"` rather than surrounding parentheses, and for `" .2f"` it omits the leading space for positives. As a result, valid d3 sign specifiers are parsed but not honored, and server-side report output for such formats diverges from the d3 frontend behavior. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=1c09722eee5a426aa2beee9f32cf6943&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=1c09722eee5a426aa2beee9f32cf6943&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/utils/number_format.py **Line:** 120:120 **Comment:** *Logic Error: The parser accepts all d3 sign modes (`+`, `-`, `(`, and space), but only `+` is implemented, so accounting formats like `(.2f` are rendered with a minus sign instead of parentheses. This breaks parity with d3 for valid format strings. Implement handling for `(` and space sign modes when building the formatted output. 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 ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41028&comment_hash=5795c62533ddf2b9ecc2fc924fc89d55698d78153bb003282b73abb9d70363c6&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41028&comment_hash=5795c62533ddf2b9ecc2fc924fc89d55698d78153bb003282b73abb9d70363c6&reaction=dislike'>👎</a> ########## superset/utils/number_format.py: ########## @@ -0,0 +1,250 @@ +# 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. +""" +Server-side port of the d3-format based number and currency formatters used by +the Table and Pivot Table chart plugins. + +Report notifications that embed a chart as text build the table in Python and +have no access to the frontend formatters, so chart number/currency format +configuration has to be reproduced here to render the same values an end user +sees in the browser. +""" + +from __future__ import annotations + +import math +import re +from decimal import Decimal, ROUND_HALF_UP +from typing import Any + +from babel.numbers import get_currency_symbol + +SMART_NUMBER = "SMART_NUMBER" +SMART_NUMBER_SIGNED = "SMART_NUMBER_SIGNED" +AUTO_CURRENCY = "AUTO" + +LOCALE = "en_US" + +# SI prefixes keyed by their power-of-1000 exponent, mirroring d3-format. +SI_PREFIXES = { + -8: "y", + -7: "z", + -6: "a", + -5: "f", + -4: "p", + -3: "n", + -2: "µ", + -1: "m", + 0: "", + 1: "k", + 2: "M", + 3: "G", + 4: "T", + 5: "P", + 6: "E", + 7: "Z", + 8: "Y", +} + +# d3-format specifier grammar: +# [[fill]align][sign][symbol][0][width][,][.precision][~][type] +D3_FORMAT_RE = re.compile( + r"^(?:(.)?([<>=^]))?([+\-( ])?([$#])?(0)?(\d+)?(,)?(?:\.(\d+))?(~)?([a-z%])?$", + re.IGNORECASE, +) + + +def format_number_with_config( + d3_format: str | None, + currency: dict[str, Any] | None, + value: Any, +) -> Any: + """ + Format ``value`` using a d3-format string and optional currency config. + + :param d3_format: a d3-format specifier (e.g. ``",.2f"``) or ``SMART_NUMBER`` + :param currency: ``{"symbol": <ISO 4217>, "symbolPosition": "prefix"|"suffix"}`` + :param value: the raw value to format + :return: the formatted string, or the value unchanged when it is not a + number that can be formatted + """ + if value is None: + return "" + if isinstance(value, bool) or not isinstance(value, (int, float)): + return value + if math.isnan(value) or math.isinf(value): + return "" + + try: + if currency and currency.get("symbol"): + # the frontend strips the currency symbol from the d3 format and + # falls back to SMART_NUMBER when no explicit format is set + number_format = (d3_format or SMART_NUMBER).replace("$", "") + formatted = format_numeric(number_format, value) + if currency["symbol"] == AUTO_CURRENCY: + return formatted + return apply_currency(formatted, currency) + if not d3_format: + return raw_string(value) + return format_numeric(d3_format, value) + except Exception: # pylint: disable=broad-except # noqa: BLE001 + # never let an unexpected value break a whole report table + return raw_string(value) Review Comment: **Suggestion:** Invalid currency codes currently drop back to `raw_string(value)`, which removes the already-computed d3 formatting (for example returning `1234.5` instead of `1,234.50`). This diverges from the frontend `CurrencyFormatter`, which preserves formatted numeric output when symbol lookup fails. Catch currency-symbol lookup failures inside `apply_currency` (or handle them separately) and return the formatted number without a symbol. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Table reports with bad currency codes lose numeric formatting. - ⚠️ Pivot table reports likewise fall back to raw numeric strings. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. A text-based chart (Table or Pivot Table v2) is requested in post-processed form, e.g. by an Alert/Report hitting the chart data endpoint, which flows through `ChartDataRestApi._send_chart_response` in `superset/charts/data/api.py:1-19`. When `result_type == ChartDataResultType.POST_PROCESSED`, `_send_chart_response` calls `apply_client_processing(result, form_data, datasource)` at line 16. 2. For a Table viz, `apply_client_processing` in `superset/charts/client_processing.py:135-149` selects the `table` post-processor from the `post_processors` mapping at lines 129-131. `table()` (lines 106-126) iterates over `column_config` from `form_data` and, for each configured column, calls `format_column` with `config.get("d3NumberFormat")` and `config.get("currencyFormat") or {}` as arguments. 3. `format_column` at `superset/charts/client_processing.py:202-209` wraps `format_number_with_config` using `functools.partial`, so each cell in the column is formatted via `format_number_with_config(d3_format, currency, value)` defined in `superset/utils/number_format.py:71-107`. When a chart has a currency configuration whose `symbol` is not a valid ISO 4217 code (e.g. a typo or legacy value), `format_number_with_config` takes the currency branch at lines 93-100 and computes `formatted = format_numeric(number_format, value)` first. 4. `format_number_with_config` then calls `apply_currency(formatted, currency)` at `superset/utils/number_format.py:100`, which in turn calls `get_currency_symbol(code, locale=LOCALE)` at lines 230-233. For an invalid currency code, Babel's `get_currency_symbol` raises `UnknownCurrencyError` instead of returning a symbol; this propagates back into the `try` block at line 92. Because the `except Exception` handler at lines 104-106 catches all exceptions and returns `raw_string(value)`, the correctly computed `formatted` string (e.g. `"1,234.50"`) is discarded and the caller receives the unformatted raw value string (e.g. `"1234.5"`). Thus, invalid currency metadata causes d3-style numeric formatting to be lost, matching the suggestion's description. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=56adcb202e0840c6ab210937401a2566&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=56adcb202e0840c6ab210937401a2566&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/utils/number_format.py **Line:** 92:106 **Comment:** *Logic Error: Invalid currency codes currently drop back to `raw_string(value)`, which removes the already-computed d3 formatting (for example returning `1234.5` instead of `1,234.50`). This diverges from the frontend `CurrencyFormatter`, which preserves formatted numeric output when symbol lookup fails. Catch currency-symbol lookup failures inside `apply_currency` (or handle them separately) and return the formatted number without a 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 ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41028&comment_hash=cd72f0caa88335d32beca1b3d57fb2c0bd1916b4fc5de85903d7c1afb8b60ad7&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41028&comment_hash=cd72f0caa88335d32beca1b3d57fb2c0bd1916b4fc5de85903d7c1afb8b60ad7&reaction=dislike'>👎</a> -- 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]
