msyavuz commented on code in PR #45311:
URL: https://github.com/apache/superset/pull/45311#discussion_r4321612509
##########
superset/charts/client_processing.py:
##########
@@ -752,13 +775,13 @@ def collapse(block: pd.DataFrame) -> tuple[pd.DataFrame,
str]:
else None,
)
- # if we want to apply the metrics on the rows we need to pivot the
- # dataframe back
+ # Replace placeholders before transposing, while column dtypes are intact.
+ df.replace("SUPERSET_PANDAS_NAN", np.nan, inplace=True)
if apply_metrics_on_rows:
- df = df.T
+ # Mixed blank columns must not coerce integer cells through float64
+ # during transposition (which would round integers above 2**53).
+ df = df.astype(object).T if mixed_metric_totals else df.T
Review Comment:
`df.astype(object).T` leaves object-dtype columns after the transpose when
mixed totals exist, so numeric formatting or `to_excel` downstream may treat
them differently than on the non-mixed path.
##########
superset/charts/client_processing.py:
##########
@@ -686,7 +705,7 @@ def collapse(block: pd.DataFrame) -> tuple[pd.DataFrame,
str]:
)
raise
- subtotal_values = df.iloc[slice_, :]
+ subtotal_values = df.iloc[slice_,
:].drop(columns=mixed_metric_totals)
Review Comment:
`drop(columns=mixed_metric_totals)` raises KeyError if a name in the list is
not in `df.columns`, and duplicate subtotal names across groups are not guarded.
##########
superset/charts/client_processing.py:
##########
@@ -194,14 +191,30 @@ def _metric_of_column(column: Any, metric_level: int) ->
Any:
return column[metric_level] if isinstance(column, tuple) else column
+def _rollup_metric(column: Any, metric_level: int, metrics: set[str]) -> Any:
+ """The metric to read from a rollup record for a cell in ``column``.
+
+ A column carrying a total label instead of a metric name collapses the
metric
+ axis; see ``_collapsed_metric``.
+ """
+ name = _metric_of_column(column, metric_level)
+ return name if name in metrics else _collapsed_metric(metrics)
+
+
def _reduce(
data: Union[pd.DataFrame, pd.Series],
reducer: str,
axis: Optional[int] = None,
) -> Any:
- """Apply a rollup reducer (``sum``/``min``/``max``), skipping empty
cells."""
+ """Apply a rollup reducer (``sum``/``min``/``max``), skipping empty cells.
+
+ A percentage-mode rollup with no non-null observations stays blank:
+ ``sum`` otherwise defaults an all-NaN input to 0, treating missing values
+ as a measured zero.
+ """
method = getattr(data, reducer)
- return method(axis=axis) if axis is not None else method()
+ kwargs = {"min_count": 1} if reducer == "sum" else {}
Review Comment:
`min_count=1` applies to every `_reduce` sum, but the docstring says it is
for percentage-mode rollups only, so any caller using `_reduce` outside percent
mode now returns NaN instead of 0 for all-empty input.
--
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]
