gabotorresruiz commented on code in PR #45979:
URL: https://github.com/apache/superset/pull/45979#discussion_r4461183220


##########
superset/charts/client_processing.py:
##########
@@ -1448,13 +1725,80 @@ def label_of(name: str) -> str:
         if f"%{name}" in df.columns
     }
 
+    # The chart does not render a comparison query's metric and offset columns.
+    # It replaces each metric with the four columns the derivation produces, so
+    # a report that leaves the frame as the query returned it shows neither the
+    # same columns nor the same numbers.
+    derived_columns: dict[Any, tuple[str, Any, Any]] = {}
+    if is_using_time_comparison(form_data) and (suffix := 
comparison_suffix(form_data)):
+        # The derivation reads the offset off the column names, which are the
+        # query's own labels, while the frame has been renamed to the verbose
+        # ones. Only a rename that happened is inverted: a verbose label that
+        # is also some other column's query label is left alone, the same
+        # hazard ``apply_client_processing`` avoids when it renames.
+        raw_by_label = {
+            label: name
+            for name, label in verbose_map.items()
+            if label in df.columns and name not in df.columns
+        }
+        # ``isNumeric`` on a column is the generic type the backend sent with
+        # the results, and ``query_actions`` builds that from
+        # ``extract_dataframe_dtypes``, so asking the same function the same
+        # question answers it the way the browser was answered. It reads the
+        # datasource's own column and metric types for a column it can name,
+        # which is why it is asked about the query's labels rather than the
+        # verbose ones the frame carries.
+        numeric_coltypes = {
+            column
+            for column, coltype in zip(
+                df.columns,
+                extract_dataframe_dtypes(
+                    df.set_axis(
+                        [raw_by_label.get(column, column) for column in 
df.columns],
+                        axis=1,
+                    ),
+                    datasource,
+                ),
+                strict=True,
+            )
+            if coltype == GenericDataType.NUMERIC
+        }
+        df, derived_columns = derive_comparison_columns(
+            df,
+            suffix,
+            metric_labels,
+            percent_metric_labels,
+            raw_by_label,
+            numeric_coltypes,
+        )
+
     # Mirror the Table plugin's per-column formatter selection in
     # ``plugin-chart-table/src/transformProps.ts``: percent metrics default to
     # PERCENT_3_POINT, every (numeric) metric gets a formatter that defaults to
     # 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:
+        if column in derived_columns:
+            label, name, parent = derived_columns[column]
+            format_column(

Review Comment:
   Not a blocker, but since parity is the goal: the derived columns never get 
their small-number format. Customize offers "Small number format" on each of 
`Main`, `#`, `△` and `%` (the control types them as numeric), and 
`formatColumnValue` applies the derived column's own `d3SmallNumberFormat` 
whenever `Math.abs(value) < 1`. This branch calls `format_column` without 
`small_d3_format`, so with `{"% m": {"d3SmallNumberFormat": ".2f"}}` and `m` at 
110 against 100, the chart shows `0.10` and the report shows `10.000%`. The `%` 
column is where this shows up most, since a ratio is usually below one.
   
   Reusing the plain-column rule (blank means unset, AG Grid skipped) closes it:
   
   ```python
   label, name, parent = derived_columns[column]
   own_config = comparison_column_config(label, name, column_config)
   format_column(
       ...,
       fallback_to_detected=False,
       small_d3_format=(
           (own_config.get("d3SmallNumberFormat") or "").strip() or None
           if applies_small_number_format
           else None
       ),
       small_currency=own_config.get("currencyFormat"),
   )
   ```
   
   With that, `%`, `#` and a `Main` that also carries a currency all match the 
chart. A test with the `% m` example above (`0.10` for the 110 against 100 row) 
would lock it in.



-- 
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]

  • [PR] fix(re... SEPURI-SAI-KRISHNA (via GitHub) via notifications
    • Re: [P... netlify[bot] (via GitHub) via notifications
    • Re: [P... codeant-ai-for-open-source[bot] (via GitHub) via notifications
    • Re: [P... SEPURI-SAI-KRISHNA (via GitHub) via notifications
    • Re: [P... codecov[bot] (via GitHub) via notifications
    • Re: [P... gabotorresruiz (via GitHub) via notifications
    • Re: [P... codeant-ai-for-open-source[bot] (via GitHub) via notifications

Reply via email to