Copilot commented on code in PR #43693:
URL: https://github.com/apache/superset/pull/43693#discussion_r3890291347
##########
tests/unit_tests/pandas_postprocessing/test_pivot.py:
##########
@@ -871,3 +872,268 @@ def test_pivot_show_values_as_preserves_structural_nan()
-> None:
assert pd.isna(result.loc["r1", ("v", "c2")])
# r1's row-total is just c1 (10.0), so c1 is 100%.
assert result.loc["r1", ("v", "c1")] == pytest.approx(1.0)
+
+
+# --- NULL index preservation tests (#43547) ----------------------------------
+#
+# pandas pivot_table() silently drops rows whose index columns contain NaN,
+# regardless of the dropna= setting (which only governs the column axis).
+# The fix fills index columns with NULL_STRING before calling pivot_table(),
+# mirroring the existing treatment of the columns= parameter.
+
+
+def test_pivot_preserves_null_index_value() -> None:
+ """A NULL value in a flat (no columns groupby) index column must appear
+ as a '<NULL>' row in the result rather than being silently dropped.
+
+ Regression for #43547: pivot_table() drops NaN index rows regardless of
+ dropna=; filling the index with NULL_STRING before the call preserves them.
+ """
+ df = DataFrame(
+ {
+ "row": ["r1", None, "r2"], # middle row has a NULL index value
+ "v": [10, 99, 30],
+ }
+ )
+ result = pivot(
+ df=df,
+ index=["row"],
+ aggregates={"v": {"operator": "sum"}},
+ )
+ # The NULL group must survive as a real index label, not be dropped.
+ assert NULL_STRING in result.index, (
+ f"Expected '{NULL_STRING}' in pivot index; got {result.index.tolist()}"
+ )
+ # The metric value for the NULL group must be correct.
+ assert result.loc[NULL_STRING, "v"] == 99
+
+
+def test_pivot_preserves_null_index_value_with_columns() -> None:
+ """A NULL value in an index column must survive as '<NULL>' even when a
+ columns= groupby is also active (MultiIndex column case).
+
+ Regression for #43547: the fix must work for both the flat pivot and the
+ MultiIndex pivot so that NULL row groups are never silently dropped.
+ """
+ df = DataFrame(
+ {
+ "row": ["r1", None, "r2"], # middle row has a NULL index value
+ "col": ["c1", "c1", "c1"],
+ "v": [10, 99, 30],
+ }
+ )
+ result = pivot(
+ df=df,
+ index=["row"],
+ columns=["col"],
+ aggregates={"v": {"operator": "sum"}},
+ )
+ # The NULL group must survive as a real index label in the MultiIndex
pivot.
+ assert NULL_STRING in result.index, (
+ f"Expected '{NULL_STRING}' in pivot index; got {result.index.tolist()}"
+ )
+ # The metric value for the NULL-indexed group must be correct.
+ assert result.loc[NULL_STRING, ("v", "c1")] == 99
+
+
+def test_pivot_preserves_null_index_value_categorical() -> None:
+ """A categorical index column with a NULL value must have NULL_STRING added
+ as a valid category first and be preserved as '<NULL>' in the pivot output.
+
+ Regression for #43547: ensures fillna() on CategoricalDtype does not raise
+ and preserves the NULL index group.
+ """
+ df = DataFrame(
+ {
+ "row": pd.Categorical(["r1", None, "r2"]),
+ "v": [10, 99, 30],
+ }
+ )
+ result = pivot(
+ df=df,
+ index=["row"],
+ aggregates={"v": {"operator": "sum"}},
+ )
+ assert NULL_STRING in result.index, (
+ f"Expected '{NULL_STRING}' in pivot index; got {result.index.tolist()}"
+ )
+ assert result.loc[NULL_STRING, "v"] == 99
+
+
+def test_pivot_preserves_null_index_value_categorical_with_columns() -> None:
+ """Both index and column dimensions as categorical dtypes with NULL values
+ must properly add NULL_STRING to categories and preserve '<NULL>' rows and
+ columns in the MultiIndex output.
+ """
+ df = DataFrame(
+ {
+ "row": pd.Categorical(["r1", None, "r2"]),
+ "col": pd.Categorical(["c1", None, "c2"]),
+ "v": [10, 99, 30],
+ }
+ )
+ result = pivot(
+ df=df,
+ index=["row"],
+ columns=["col"],
+ aggregates={"v": {"operator": "sum"}},
+ )
+ assert NULL_STRING in result.index, (
+ f"Expected '{NULL_STRING}' in pivot index; got {result.index.tolist()}"
+ )
+ assert result.loc[NULL_STRING, ("v", NULL_STRING)] == 99
+
+
+def test_pivot_preserves_null_index_value_datetime() -> None:
+ """A datetime index column containing NaT/NULL must not raise a TypeError
+ when filled and must be preserved as '<NULL>' in the pivot output.
+
+ Regression for #43547: datetime64 columns cannot store strings directly;
+ converting NaT-containing datetime columns to string ensures NaT keys
+ survive pivot_table() without dtype/sort errors.
+ """
+ df = DataFrame(
+ {
+ "dttm": to_datetime(["2019-01-01", None, "2019-01-03"]),
+ "v": [10, 99, 30],
+ }
+ )
+ result = pivot(
+ df=df,
+ index=["dttm"],
+ aggregates={"v": {"operator": "sum"}},
+ )
+ assert NULL_STRING in result.index, (
+ f"Expected '{NULL_STRING}' in pivot index; got {result.index.tolist()}"
+ )
+ assert result.loc[NULL_STRING, "v"] == 99
+
+
+def test_pivot_preserves_null_index_value_datetime_with_columns() -> None:
+ """A datetime index column containing NaT/NULL with a columns groupby
+ must preserve '<NULL>' in the MultiIndex output without type errors.
+ """
+ df = DataFrame(
+ {
+ "dttm": to_datetime(["2019-01-01", None, "2019-01-03"]),
+ "col": ["c1", "c1", "c2"],
+ "v": [10, 99, 30],
+ }
+ )
+ result = pivot(
+ df=df,
+ index=["dttm"],
+ columns=["col"],
+ aggregates={"v": {"operator": "sum"}},
+ )
+ assert NULL_STRING in result.index, (
+ f"Expected '{NULL_STRING}' in pivot index; got {result.index.tolist()}"
+ )
+ assert result.loc[NULL_STRING, ("v", "c1")] == 99
+
+
+def test_pivot_preserves_null_index_value_datetime_timezone_aware() -> None:
+ """A timezone-aware datetime index containing NaT must preserve '<NULL>'
+ without dtype or timezone conversion errors.
+ """
+ df = DataFrame(
+ {
+ "dttm": to_datetime(["2019-01-01", None, "2019-01-03"], utc=True),
+ "v": [10, 99, 30],
+ }
+ )
+ result = pivot(
+ df=df,
+ index=["dttm"],
+ aggregates={"v": {"operator": "sum"}},
+ )
+ assert NULL_STRING in result.index, (
+ f"Expected '{NULL_STRING}' in pivot index; got {result.index.tolist()}"
+ )
+ assert result.loc[NULL_STRING, "v"] == 99
+
+
+def test_pivot_preserves_null_index_value_categorical_already_in_categories()
-> None:
+ """A categorical index that already has NULL_STRING in its categories
+ must not fail or attempt duplicate category insertion and must fill NULLs.
+ """
+ df = DataFrame(
+ {
+ "row": pd.Categorical(["r1", None, "r2"], categories=["r1", "r2",
NULL_STRING]),
+ "v": [10, 99, 30],
+ }
Review Comment:
This categorical constructor call is long enough that it will likely be
auto-reformatted by black (and may fail lint/format checks in CI). Consider
wrapping it across multiple lines for readability and to match the rest of the
test file's formatting style.
##########
superset/utils/pandas_postprocessing/pivot.py:
##########
@@ -187,6 +187,31 @@ def _restore_dropped_metric_columns(
return df
+def _fill_dimension_column(df: DataFrame, col: str, fill_value: str) -> None:
+ """Fill missing values in a groupby dimension column before pivoting.
+
+ Handles categorical dtypes (adding fill_value to categories) and datetime
+ dtypes (converting to string representation with fill_value for NaT) to
prevent
+ dtype errors and preserve NULL/NaN/NaT keys through pivot_table().
+ """
+ s = df[col]
+ if (
+ isinstance(s.dtype, pd.CategoricalDtype)
+ and fill_value not in s.cat.categories
+ ):
+ df[col] = s.cat.add_categories([fill_value]).fillna(value=fill_value)
+ elif pd.api.types.is_datetime64_any_dtype(s.dtype) or getattr(s.dtype,
"kind", None) == "M":
+ if s.isna().any():
+ df[col] = s.astype(str).replace({
+ "NaT": fill_value,
+ "<NA>": fill_value,
+ "nan": fill_value,
+ "None": fill_value,
+ })
Review Comment:
The datetime branch converts NaT-containing datetime dimensions using
`astype(str).replace({...})`, which is more complex than needed and can be
brittle (it relies on specific string spellings like "NaT"/"<NA>"). You can
simplify this by masking on `s.isna()` after the cast, which also avoids the
long line that will likely be reformatted by black.
--
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]
