fitzee commented on code in PR #44679:
URL: https://github.com/apache/superset/pull/44679#discussion_r4300439315
##########
superset/charts/client_processing.py:
##########
@@ -479,6 +479,133 @@ def _apply_show_values_as( # pylint:
disable=too-many-arguments
return numeric / denominator.replace(0, np.nan)
+def _reduce_result_values(values: pd.Series, aggregate: str) -> Any:
+ """Reduce original metric results without aggregating intermediate
summaries."""
+ if values.empty:
+ return None
+ name = aggregate.split(" as Fraction of ")[0]
+ if name == "Count":
+ return len(values)
+ if name == "Count Unique Values":
+ return values.nunique(dropna=False)
+ if name == "List Unique Values":
+ return ", ".join(sorted({str(value) for value in values}))
+ if name in {"First", "Last"}:
+ present = values.dropna()
+ return present.iloc[0 if name == "First" else -1] if len(present) else
None
Review Comment:
Fixed in 71d3b975becafbea80ac302955712879d26b4c7a. Last now retains a
trailing NULL. First continues to skip leading NULLs, matching the merged
browser implementation.
Regression test:
`tests/unit_tests/charts/test_client_processing.py::test_result_ordered_reducers_null_endpoint`.
The new regression fails against the previous implementation and passes with
this commit. Full chart suite: 434 passed, 2 expected failures.
Siblings checked:
- `superset/charts/client_processing.py` — fixed in
71d3b975becafbea80ac302955712879d26b4c7a: First and Last.
##########
superset/charts/client_processing.py:
##########
@@ -479,6 +479,133 @@ def _apply_show_values_as( # pylint:
disable=too-many-arguments
return numeric / denominator.replace(0, np.nan)
+def _reduce_result_values(values: pd.Series, aggregate: str) -> Any:
+ """Reduce original metric results without aggregating intermediate
summaries."""
+ if values.empty:
+ return None
+ name = aggregate.split(" as Fraction of ")[0]
+ if name == "Count":
+ return len(values)
+ if name == "Count Unique Values":
+ return values.nunique(dropna=False)
+ if name == "List Unique Values":
+ return ", ".join(sorted({str(value) for value in values}))
+ if name in {"First", "Last"}:
+ present = values.dropna()
+ return present.iloc[0 if name == "First" else -1] if len(present) else
None
+ numeric = pd.to_numeric(values, errors="coerce").dropna()
Review Comment:
This finding describes the older frontend revision. The merged
https://github.com/apache/superset/pull/44660 at
7505e2660b99b92e15f923073cfca243151da259 skips null/undefined in Sum, extrema,
quantile and runningStat. The backend's NULL-skipping numeric behavior already
agrees with that implementation; changing it to zero would reintroduce the
mismatch.
Expanded `test_result_numeric_export_ignores_nulls` in
71d3b975becafbea80ac302955712879d26b4c7a to cover Sum, Average, Median,
Minimum, Maximum, Sample Variance and Sample Standard Deviation. All pass.
Text-valued reducers remain a separate integration-contract question,
documented in the PR description.
##########
superset/charts/client_processing.py:
##########
@@ -479,6 +479,133 @@ def _apply_show_values_as( # pylint:
disable=too-many-arguments
return numeric / denominator.replace(0, np.nan)
+def _reduce_result_values(values: pd.Series, aggregate: str) -> Any:
+ """Reduce original metric results without aggregating intermediate
summaries."""
+ if values.empty:
+ return None
+ name = aggregate.split(" as Fraction of ")[0]
+ if name == "Count":
+ return len(values)
+ if name == "Count Unique Values":
+ return values.nunique(dropna=False)
+ if name == "List Unique Values":
+ return ", ".join(sorted({str(value) for value in values}))
+ if name in {"First", "Last"}:
+ present = values.dropna()
+ return present.iloc[0 if name == "First" else -1] if len(present) else
None
+ numeric = pd.to_numeric(values, errors="coerce").dropna()
+ if name == "Sum":
+ text = values.dropna()
+ return text.iloc[-1] if numeric.empty and len(text) else numeric.sum()
+ if numeric.empty:
+ return None
+ if name == "Sample Variance":
+ return numeric.var(ddof=1) if len(numeric) > 1 else 0
+ if name == "Sample Standard Deviation":
+ return numeric.std(ddof=1) if len(numeric) > 1 else 0
+ return {
+ "Average": numeric.mean,
+ "Median": numeric.median,
+ "Minimum": numeric.min,
+ "Maximum": numeric.max,
+ }[name]()
+
+
+def _group_result_records(
+ source: pd.DataFrame, dimensions: list[str]
+) -> dict[tuple[str, ...], pd.DataFrame]:
+ """Index contributing records by the dimension prefix of a summary
scope."""
+ if not dimensions:
+ return {(): source}
+ keys = [source[dim].fillna("SUPERSET_PANDAS_NAN").astype(str) for dim in
dimensions]
+ return {
+ key if isinstance(key, tuple) else (key,): group
+ for key, group in source.groupby(keys, sort=False, dropna=False)
+ }
Review Comment:
Fixed in 71d3b975becafbea80ac302955712879d26b4c7a. Result grouping and
lookup now retain scalar dimension types, keeping numeric 1 separate from text
"1".
Regression test:
`tests/unit_tests/charts/test_client_processing.py::test_result_dimension_keys_preserve_types`.
The new regression fails against the previous implementation and passes with
this commit. Full chart suite: 434 passed, 2 expected failures.
Siblings checked:
- `superset/charts/client_processing.py` — fixed in
71d3b975becafbea80ac302955712879d26b4c7a: row and column dimensions, including
midnight timestamps.
##########
superset/charts/client_processing.py:
##########
@@ -479,6 +479,133 @@ def _apply_show_values_as( # pylint:
disable=too-many-arguments
return numeric / denominator.replace(0, np.nan)
+def _reduce_result_values(values: pd.Series, aggregate: str) -> Any:
+ """Reduce original metric results without aggregating intermediate
summaries."""
+ if values.empty:
+ return None
+ name = aggregate.split(" as Fraction of ")[0]
+ if name == "Count":
+ return len(values)
+ if name == "Count Unique Values":
+ return values.nunique(dropna=False)
+ if name == "List Unique Values":
+ return ", ".join(sorted({str(value) for value in values}))
+ if name in {"First", "Last"}:
+ present = values.dropna()
+ return present.iloc[0 if name == "First" else -1] if len(present) else
None
+ numeric = pd.to_numeric(values, errors="coerce").dropna()
+ if name == "Sum":
+ text = values.dropna()
+ return text.iloc[-1] if numeric.empty and len(text) else numeric.sum()
+ if numeric.empty:
+ return None
+ if name == "Sample Variance":
+ return numeric.var(ddof=1) if len(numeric) > 1 else 0
+ if name == "Sample Standard Deviation":
+ return numeric.std(ddof=1) if len(numeric) > 1 else 0
+ return {
+ "Average": numeric.mean,
+ "Median": numeric.median,
+ "Minimum": numeric.min,
+ "Maximum": numeric.max,
+ }[name]()
+
+
+def _group_result_records(
+ source: pd.DataFrame, dimensions: list[str]
+) -> dict[tuple[str, ...], pd.DataFrame]:
+ """Index contributing records by the dimension prefix of a summary
scope."""
+ if not dimensions:
+ return {(): source}
+ keys = [source[dim].fillna("SUPERSET_PANDAS_NAN").astype(str) for dim in
dimensions]
+ return {
+ key if isinstance(key, tuple) else (key,): group
+ for key, group in source.groupby(keys, sort=False, dropna=False)
+ }
+
+
+def _apply_result_aggregation(
+ frame: pd.DataFrame,
+ source: pd.DataFrame,
+ rows: list[str],
+ columns: list[str],
+ metrics: list[str],
+ metric_level: int,
+ row_depths: dict[Any, int],
+ column_depths: dict[Any, int],
+ aggregate: str,
+ show_values_as: Optional[str],
+ metrics_on_rows: bool,
+) -> pd.DataFrame:
+ """Calculate every scope from its original contributing query results."""
+
+ if not set(metrics).issubset(source.columns):
+ return frame
+
+ grouped_sources: dict[tuple[int, int], dict[tuple[str, ...],
pd.DataFrame]] = {}
+
+ def contributing(
+ row: Any,
+ column: Any,
+ row_depth: int,
+ column_depth: int,
+ selected_metrics: list[str],
+ ) -> pd.Series:
+ level = (row_depth, column_depth)
+ if level not in grouped_sources:
+ grouped_sources[level] = _group_result_records(
+ source, rows[:row_depth] + columns[:column_depth]
+ )
+ key = _rollup_key(row, row_depth, metric_level, is_column=False) +
_rollup_key(
+ column,
+ column_depth,
+ metric_level,
+ is_column=True,
+ )
+ selected = grouped_sources[level].get(key, source.iloc[:0])
+ # Row-major order matches the browser's metric unpivoting.
+ return pd.Series(selected[selected_metrics].to_numpy().ravel(),
dtype=object)
+
+ old_fraction = aggregate.partition(" as Fraction of ")[2]
+ fraction = {
+ "Total": "percent_total",
+ "Rows": "percent_row",
+ "Columns": "percent_col",
+ }.get(old_fraction) or show_values_as
+ for ri, row in enumerate(frame.index):
+ for ci, column in enumerate(frame.columns):
+ rd = row_depths.get(row, len(rows))
+ cd = column_depths.get(column, len(columns))
+ metric = _metric_of_column(column, metric_level)
+ selected_metrics = (
+ [metric] if metric in metrics else list(dict.fromkeys(metrics))
+ )
+ # Match the browser's mixed-metric guard: an axis/corner that
+ # combines different metrics has no meaningful single summary.
+ if len(selected_metrics) != 1:
+ frame.iloc[ri, ci] = None
+ continue
+ value = _reduce_result_values(
+ contributing(row, column, rd, cd, selected_metrics), aggregate
+ )
+ if fraction in SHOW_VALUES_AS_PERCENT_MODES:
+ dr, dc = rd, cd
+ if fraction == "percent_total":
+ dr, dc = 0, 0
+ elif (fraction == "percent_row") != metrics_on_rows:
+ dc = 0
+ else:
+ dr = 0
+ denominator = _reduce_result_values(
+ contributing(row, column, dr, dc, selected_metrics),
aggregate
+ )
+ value = (
+ value / denominator if value is not None and denominator
else None
+ )
Review Comment:
Fixed in 71d3b975becafbea80ac302955712879d26b4c7a. Explicit result
aggregation now takes precedence over showValuesAs in value computation,
formatted output and XLSX number formats.
Regression test:
`tests/unit_tests/charts/test_client_processing.py::test_result_reducer_precedes_show_values_as`.
The new regression fails against the previous implementation and passes with
this commit. Full chart suite: 434 passed, 2 expected failures.
Siblings checked:
- `superset/charts/client_processing.py` — fixed in
71d3b975becafbea80ac302955712879d26b4c7a: formatted results and XLSX via
_pivot_percent_output.
##########
superset/charts/client_processing.py:
##########
@@ -978,6 +1162,7 @@ def build_pivot_currency_context(
currency_pivot_options = {
**pivot_options,
"aggfunc": CURRENCY_CONTEXT_AGGREGATION,
+ "result_aggregation": False,
Review Comment:
Fixed in 71d3b975becafbea80ac302955712879d26b4c7a. Currency context now
generates and filters the same subtotal-only layout as result values.
Regression test:
`tests/unit_tests/charts/test_client_processing.py::test_result_subtotal_only_auto_currency`.
The new regression fails against the previous implementation and passes with
this commit. Full chart suite: 434 passed, 2 expected failures.
Siblings checked:
- `superset/charts/client_processing.py` — fixed in
71d3b975becafbea80ac302955712879d26b4c7a: row and column subtotal-only layouts.
##########
superset/charts/client_processing.py:
##########
@@ -479,6 +479,133 @@ def _apply_show_values_as( # pylint:
disable=too-many-arguments
return numeric / denominator.replace(0, np.nan)
+def _reduce_result_values(values: pd.Series, aggregate: str) -> Any:
+ """Reduce original metric results without aggregating intermediate
summaries."""
+ if values.empty:
+ return None
+ name = aggregate.split(" as Fraction of ")[0]
+ if name == "Count":
+ return len(values)
+ if name == "Count Unique Values":
+ return values.nunique(dropna=False)
+ if name == "List Unique Values":
+ return ", ".join(sorted({str(value) for value in values}))
+ if name in {"First", "Last"}:
+ present = values.dropna()
+ return present.iloc[0 if name == "First" else -1] if len(present) else
None
+ numeric = pd.to_numeric(values, errors="coerce").dropna()
+ if name == "Sum":
+ text = values.dropna()
+ return text.iloc[-1] if numeric.empty and len(text) else numeric.sum()
+ if numeric.empty:
+ return None
+ if name == "Sample Variance":
+ return numeric.var(ddof=1) if len(numeric) > 1 else 0
+ if name == "Sample Standard Deviation":
+ return numeric.std(ddof=1) if len(numeric) > 1 else 0
+ return {
+ "Average": numeric.mean,
+ "Median": numeric.median,
+ "Minimum": numeric.min,
+ "Maximum": numeric.max,
+ }[name]()
+
+
+def _group_result_records(
+ source: pd.DataFrame, dimensions: list[str]
+) -> dict[tuple[str, ...], pd.DataFrame]:
+ """Index contributing records by the dimension prefix of a summary
scope."""
+ if not dimensions:
+ return {(): source}
+ keys = [source[dim].fillna("SUPERSET_PANDAS_NAN").astype(str) for dim in
dimensions]
Review Comment:
Fixed in 71d3b975becafbea80ac302955712879d26b4c7a. Both source grouping and
result lookup now use typed scalar keys. Midnight dates retain their leaf
values, and numeric/text labels no longer collide.
Regression test:
`tests/unit_tests/charts/test_client_processing.py::test_result_dimension_keys_preserve_types`.
The new regression fails against the previous implementation and passes with
this commit. Full chart suite: 434 passed, 2 expected failures.
Siblings checked:
- `superset/charts/client_processing.py` — fixed in
71d3b975becafbea80ac302955712879d26b4c7a: row and column dimensions.
--
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]
