fitzee commented on code in PR #45311:
URL: https://github.com/apache/superset/pull/45311#discussion_r4321778499
##########
superset/charts/client_processing.py:
##########
@@ -654,17 +667,24 @@ def collapse(block: pd.DataFrame) -> tuple[pd.DataFrame,
str]:
# to a number. The row totals below already coerce; do the
# same here so a total means the same thing on both axes.
block = block.apply(pd.to_numeric, errors="coerce")
- if percent_mode:
- source, reducer = collapse(block)
+ source, reducer = collapse(block)
+ mixed_metrics = (
+ source is None and aggfunc != CURRENCY_CONTEXT_AGGREGATION
+ )
+ if mixed_metrics:
+ subtotal = pd.Series(np.nan, index=block.index)
Review Comment:
The paired browser change, #44657, merged on October 20
(25e94d00d376adf9051890e267c6a3d7ad20433a), and this branch already contains
that commit. The browser now blanks these shared mixed-metric slots as well, so
the dependency is satisfied.
##########
superset/charts/client_processing.py:
##########
@@ -752,13 +776,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:
Fixed in 044e2a578ef505454802bbcb3ff865f8cff16c1f. Confirmed that both
pd.to_numeric and scalar mapping over nullable integers can coerce through
float64. Excel preparation now uses nullable numeric conversion followed by
object conversion before mapping oversized values to text.
test_pivot_mixed_totals_preserve_integer_xlsx exercises the full XLSX path with
metrics on rows across Combine metrics/Transpose combinations, and
test_numeric_column_with_blanks_preserves_large_integers checks the utility
directly. All five cases fail before the fix and pass after it; the workbook
retains exact large integers as text, ordinary values as numeric cells, and
blank cells as blank.
##########
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:
Clarified the helper docstring in 044e2a578ef505454802bbcb3ff865f8cff16c1f:
every all-empty reduction stays blank. I checked all four call sites: two are
inside _apply_show_values_as, and the other two are explicitly guarded by
percent_mode. There are no non-percentage callers today, so this does not
change Actual Values aggregation.
test_pivot_rollup_reducer_preserves_blank_and_zero covers the
blank-versus-measured-zero distinction for sum/min/max.
##########
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:
The object dtype is intentional here: transposing mixed blank columns with
integer columns otherwise rounds integers above 2**53. Pivot formatting uses
format_column per metric rather than skipping object columns. Your downstream
Excel concern was valid: fixed in 044e2a578ef505454802bbcb3ff865f8cff16c1f by
preserving nullable integer values through Excel type preparation.
test_pivot_mixed_totals_preserve_integer_xlsx checks all four Combine
metrics/Transpose combinations through the full export path, including exact
text for large integers and numeric workbook cells for small values. The chart
and Excel unit suites pass (475 passed, 2 xfailed).
##########
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:
I checked the lifecycle and added an invariant comment in
044e2a578ef505454802bbcb3ff865f8cff16c1f. mixed_metric_totals is populated only
after a successful df.insert, and no columns are removed before this drop; row
concatenation explicitly reindexes to df.columns. Subtotal labels include the
complete group prefix, so different groups do not share labels; df.insert also
rejects an actual duplicate before it can enter this list. Keeping strict drop
is useful here: silently ignoring a missing label would hide an invariant
violation. The combined/transposed subtotal tests exercise both metric layouts.
--
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]
