Repository navigation
fix(postprocessing): preserve NULL grouping index values in pivot() - #43694
Archita-kale wants to merge 5 commits into
Conversation
Code Review Agent Run #523df3Actionable Suggestions - 0Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
|
The flagged issue is correct. Filling a datetime index with a string like superset/utils/pandas_postprocessing/pivot.py |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #43694 +/- ##
=======================================
Coverage 81.51% 81.52%
=======================================
Files 2973 2973
Lines 180273 180319 +46
Branches 41734 41739 +5
=======================================
+ Hits 146949 146998 +49
+ Misses 30601 30599 -2
+ Partials 2723 2722 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
I’ve implemented the follow-up fix for the NULL grouping issue in the pivot() post-processing path. The fix preserves NULL/NaN/NaT grouping values in index columns while handling categorical and datetime dtypes appropriately. I’ve also added regression coverage for the affected pivot scenarios. The changes are now included in this PR and ready for review. Thanks for identifying the remaining aggregate → pivot issue! |
|
Thanks for the coverage feedback! I’ve added the missing regression test coverage for the newly introduced pivot handling, including the relevant categorical and NULL-value paths. The updated tests cover the previously uncovered lines, and the existing pivot test suite has been run successfully. The changes have been pushed to this PR for review. |
Code Review Agent Run #6f5c70Actionable Suggestions - 0Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
There was a problem hiding this comment.
Pull request overview
Preserves NULL grouping keys during pandas pivot post-processing.
Changes:
- Adds dtype-aware dimension NULL filling.
- Adds regression coverage for categorical, datetime, numeric, and MultiIndex pivots.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
superset/utils/pandas_postprocessing/pivot.py |
Fills missing pivot dimensions before pivoting. |
tests/unit_tests/pandas_postprocessing/test_pivot.py |
Adds NULL-preservation regression tests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
@Archita-kale nice catch on the root cause, but Also still open: the datetime path stringifies the whole column instead of just the missing values, which per copilot's thread breaks the epoch serializer downstream in charts/data/api.py, and timedelta64 columns aren't caught by that check so they'll still hit fillna() and raise. Those threads are still unresolved, want to take another pass before this is mergeable? |
|
No update since my last comment naming the three open threads (the datetime-NaT TypeError risk, the spurious |
8d13196 to
4c58a06
Compare
|
| Language | Invalidated translations |
|---|---|
zh |
7 |
How to fix
1. Install dependencies (if not already set up):
pip install -r superset/translations/requirements.txt
sudo apt-get install gettext # or: brew install gettext2. Re-extract strings and sync .po files:
./scripts/translations/babel_update.shThis rewrites superset/translations/messages.pot from the current source files and merges the changes into every .po file. Strings whose msgid changed will be marked #, fuzzy.
3. Resolve the fuzzy entries in the affected language files (zh):
grep -n '#, fuzzy' superset/translations/<lang>/LC_MESSAGES/messages.poFor each fuzzy entry, either rewrite the msgstr to match the new string and remove the #, fuzzy line, or clear the msgstr to "" if you cannot provide a translation.
4. Commit your changes to the .po files.
Code Review Agent Run #ec8993Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
@Archita-kale just echoing that @sadpandajoe caught one more issue on pivot.py:217. Nullable extension dtypes ( Might be worth handling this by upcasting to object dtype once in |
…categorical dtypes in pivot() (apache#43547) - Fill NULL/NaN values in index columns with NULL_STRING (<NULL>) prior to calling pivot_table() - For categorical index and column dtypes, add the fill value to cat.categories if not already present before calling fillna() - Preserve existing drop_missing_columns behavior without altering dropna=drop_missing_columns - Add regression tests for flat index, MultiIndex with columns, and categorical index/column dimensions with NULL values
…ors in pivot() (apache#43547) - Convert datetime dimensions containing NaT to string representation with NULL_STRING (<NULL>) - Prevent TypeError on datetime64 arrays and mixed-type MultiIndex sorting - Add regression tests for datetime index with NaT (flat, MultiIndex, and timezone-aware)
…ic pivot dimensions (apache#43547) - test_pivot_preserves_null_index_value_categorical_already_in_categories - test_pivot_categorical_column_with_null - test_pivot_categorical_column_already_in_categories - test_pivot_preserves_null_numeric_index_value
Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
4c58a06 to
bcafb71
Compare
Code Review Agent Run #723801Actionable Suggestions - 0Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
…pivot() NULL fill Addresses the three review threads still open on this PR: the categorical branch added NULL_STRING as a category unconditionally, creating a spurious all-zero '<NULL>' group in pivots that never had a missing value; the datetime special case stringified every value (not just the missing ones) to work around NaT, rewriting valid labels and breaking the epoch serializer downstream; and timedelta64 (kind "m") and pandas' nullable extension dtypes (Int64, Float64, boolean) weren't handled at all, so a NULL group on one of those raised TypeError instead of being preserved. Rewrites _fill_dimension_column with one rule: skip columns with no missing values entirely (fixes the categorical case and avoids unnecessary casts), and for dtypes that reject a string sentinel outright -- datetime64, timedelta64, and any pandas ExtensionDtype -- cast to `object` first so valid values keep their real type and only the missing slots become the sentinel, instead of stringifying the whole column. Adds five regression tests: categorical with no nulls (asserts no spurious group), datetime with a null (asserts valid entries stay real Timestamp objects), timedelta64 with a null, and Int64/boolean nullable dtypes with a null. Confirmed each fails on the prior code before this fix and passes after. Rebased onto current master to clear the stale-branch babel-extract/pot failures; full pandas_postprocessing suite (180 tests) and ruff both pass. pylint verified manually against the project's own venv (10/10) -- the pre-commit hook itself failed here on a broken local environment (system `pylint` on PATH resolves to an unrelated pyenv shim, not this project's venv), unrelated to this change. Co-Authored-By: Archita Kale <Archita-kale@users.noreply.github.com> Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #990cacActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
| # Mirrors the column fill above; pivot_table() drops NaN index rows | ||
| # regardless of the dropna= setting (dropna only governs the column axis). | ||
| for col in index: | ||
| _fill_dimension_column(df, col, NULL_STRING) |
There was a problem hiding this comment.
A null timestamp now makes the pivot index a mixed object Index, so Timeseries charts with resampling enabled fail with Resample operation requires DatetimeIndex instead of resampling the remaining dates. Could the null-preservation policy account for the temporal index required by the following resample operation?
github-actions
Bot
added
the
requires:rebase
Summary
Follow-up fix for #43547 to preserve
NULL/NaNgrouping values through the pandas post-processingpivot()operator.Problem
NULLvalues in thecolumnsparameter were already handled using Superset's existingNULL_STRINGbefore callingpivot_table(). However,indexcolumns did not have equivalent handling.As a result, when a
NULLgrouping value survived theaggregatestep, pandaspivot_table()could drop that row during pivoting because the grouping key containedNaN.This caused valid
NULLgroups to disappear from the pivot output.Solution
NULL/NaNvalues inindexcolumns with the existingNULL_STRINGbefore callingpivot_table().indexcolumns by addingNULL_STRINGto their categories before filling.columnsvalues when the configured fill value is not already present in the categories.drop_missing_columns/dropnabehavior.NULL_STRINGconstant.Tests
Added regression tests for:
Tested with:
@kokhlo I’ve submitted this follow-up PR for the NULL index handling issue identified in the discussion. The change preserves NULL grouping values through the pivot post-processing path and includes regression tests. Would appreciate your review!
Fixes #43547