SEPURI-SAI-KRISHNA commented on code in PR #45979:
URL: https://github.com/apache/superset/pull/45979#discussion_r4456844249
##########
superset/charts/client_processing.py:
##########
@@ -1397,6 +1398,267 @@ def apply_pivot_number_formats(
return df.T if metrics_on_rows else df
+def time_comparison_shifts(form_data: dict[str, Any]) -> list[Any]:
+ """The saved time shifts, as ``ensureIsArray`` reads them."""
+ shifts = form_data.get("time_compare")
+ if shifts is None:
+ return []
+ if isinstance(shifts, (list, tuple)):
+ return list(shifts)
+ return [shifts]
+
+
+def is_using_time_comparison(form_data: dict[str, Any]) -> bool:
+ """
+ Whether the Table plugins derive time-comparison columns.
+
+ Mirrors ``isUsingTimeComparison`` in
+ ``plugin-chart-table/src/transformProps.ts``, which the AG Grid plugin
+ repeats verbatim. Both conditions after the shift are read straight off the
+ form data: ``transformProps`` does not infer ``query_mode`` the way
+ ``buildQuery`` does, and ``comparison_type``'s ``values`` default lives in
+ the control rather than in the chart code, so a chart that saved neither
+ derives nothing in the browser either.
+ """
+ return bool(
+ time_comparison_shifts(form_data)
+ and form_data.get("query_mode") == "aggregate"
+ and form_data.get("comparison_type") == "values"
+ )
+
+
+def comparison_suffix(form_data: dict[str, Any]) -> Optional[str]:
+ """
+ The offset whose columns the derivation reads, or ``None`` for no shift.
+
+ Mirrors ``timeOffsets[0]``: the shifts naming a period come first in the
+ order they were saved, a ``custom`` shift contributes its start date and an
+ ``inherit`` shift the literal ``inherit``, and only the first entry is
read.
+ """
+ shifts = time_comparison_shifts(form_data)
+ offsets: list[Any] = [
+ shift for shift in shifts if shift not in ("custom", "inherit")
+ ]
+ if "custom" in shifts:
+ offsets.append(form_data.get("start_date_offset"))
+ if "inherit" in shifts:
+ offsets.append("inherit")
+ if not offsets:
+ return None
+ # The offset is interpolated into a template string before
+ # ``String.prototype.includes`` ever sees it, so a ``custom`` shift saved
+ # without a start date is compared as the text ``undefined`` rather than
+ # suppressing the derivation. It matches no column, every comparison value
+ # reads as zero, and that is what the browser shows.
+ return "undefined" if offsets[0] is None else str(offsets[0])
+
+
+def comparison_value(value: Any) -> Any:
+ """
+ A comparison operand with the browser's ``|| 0`` applied.
+
+ Every read in ``processComparisonDataRecords`` is ``|| 0``, so a null, a
+ missing column and a NaN are all zero before any branch inspects the
+ operand. ``bool(float("nan"))`` is ``True`` in Python while ``NaN`` is
+ falsy in JavaScript, which is why the coercion cannot be left to ``or``.
+ """
+ return 0 if value is None or pd.isna(value) else value
+
+
+def percent_difference(original: Any, comparison: Any) -> float:
+ """
+ ``calculateDifferences``' percent difference, which is a ratio not a
percent.
+
+ A zero operand short-circuits to a whole step in one direction rather than
+ dividing, and the denominator is an absolute value, so a positive metric
+ against a negative comparison grows the ratio instead of flipping its sign.
+ """
+ if not original and not comparison:
+ return 0
+ if not original or not comparison:
+ return 1 if original else -1
+ return (original - comparison) / abs(comparison)
+
+
+def holds_only_numbers(series: pd.Series) -> bool:
+ """
+ Whether every value is a number or null, as ``isNumeric`` decides it.
+
+ ``transformProps`` treats a column as a metric only when ``typeof`` says
+ each of its values is a number, nulls apart, so a metric returning strings
+ is presented as a plain column instead. ``is_numeric_dtype`` answers a
+ different question and disagrees in both directions: it is true of a
+ boolean column, which ``typeof`` calls a boolean, and false of an all-null
+ object column, which has no value to disqualify it.
+ """
+ for value in series:
+ if value is None or pd.isna(value):
Review Comment:
Correct, with one narrowing: it is a list or array of more than one element.
`pd.isna` answers a tuple, dict or set with a scalar, and a one-element array
has a defined truth value, so only the multi-element case raises.
Fixed by ruling out a list-like before the null test, so an array metric is
demoted to a plain column. That is what the one-element case already did, and
what `typeof` says about it.
The same mistake was in a second place. The offset column is read with no
type
check, so it raised there too, on text as well as on arrays:
```
m numeric, m__P1D ['a', 'b'] -> TypeError: unsupported operand type(s) for
-: 'float' and 'str'
```
It goes through the same check now, and an offset column that is not all
numbers is read as absent, which is the zero `|| 0` already gives a
comparison
value that is not there. One behaviour change worth naming: a boolean offset
column used to reach the arithmetic and agree with JS, where `1 - true` is 0,
and reads as zero instead. Neither shape can come out of one query, since the
two columns are a single metric over two windows.
`test_table_keeps_a_metric_holding_arrays` and
`test_table_reads_an_unusable_comparison_column_as_absent` cover it, 296 pass
in the file, and reverting either condition fails a named test.
--
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]
