-
Notifications
You must be signed in to change notification settings - Fork 16k
fix(histogram): add NULL handling for histogram #35693
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
fix(histogram): add NULL handling for histogram #35693
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
Category | Issue | Status |
---|---|---|
Inefficient DataFrame copy for null filtering ▹ view | ✅ Fix detected |
Files scanned
File Path | Reviewed |
---|---|
superset/utils/pandas_postprocessing/histogram.py | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
Category | Issue | Status |
---|---|---|
Input DataFrame modified in-place ▹ view |
Files scanned
File Path | Reviewed |
---|---|
superset/utils/pandas_postprocessing/histogram.py | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
df.dropna(subset=[column], inplace=True) | ||
if df.empty: | ||
return df |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Input DataFrame modified in-place 
Tell me more
What is the issue?
The function modifies the input DataFrame in-place by dropping rows with NULLs, which violates the principle of not mutating input parameters and can cause unexpected side effects for the caller.
Why this matters
Callers may not expect their original DataFrame to be modified, leading to data loss in the calling code and potential bugs in downstream processing that depends on the original DataFrame structure.
Suggested change ∙ Feature Preview
Create a copy of the DataFrame before dropping NULLs:
# drop empty values from the target column
df = df.dropna(subset=[column])
if df.empty:
return df
Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #35693 +/- ##
===========================================
+ Coverage 0 71.92% +71.92%
===========================================
Files 0 589 +589
Lines 0 43638 +43638
Branches 0 4726 +4726
===========================================
+ Hits 0 31388 +31388
- Misses 0 11006 +11006
- Partials 0 1244 +1244
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🎪 Showtime deployed environment on GHA for 55b453f • Environment: http://52.37.95.52:8080 (admin/admin) |
Are there cases where we should use zero-imputation for these values (counting them as zero)? If selecting zero imputation in Advanced Analytics solves that use case, we may be good. |
This viz doesn't have the Advanced Analytics feature, it seems that would be worth adding here, since it provides the zero-imputation feature. Advanced Analytics doesn't have an option to strip out null values however. It's probably better to use Advanced Analytics to "fix" null values OR use a Filter to remove null values right from the control panel. Stripping them out in post-processing doesn't give users the chance to set them as 0... they won't even know there ARE null values, which seems dangerous. |
SUMMARY
This issue is fixed by removing empty values from the input DataFrame before rendering. In cases where dropping these values results in an empty DataFrame, we now safely return the empty DataFrame instead of failing.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before

The chart failed to render when the selected column contained NULLs.
After

The chart now renders correctly, ignoring NULL values.
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION