Copilot commented on code in PR #35693:
URL: https://github.com/apache/superset/pull/35693#discussion_r2520611058
##########
superset/utils/pandas_postprocessing/histogram.py:
##########
@@ -48,6 +48,11 @@ def histogram(
if groupby is None:
groupby = []
+ # drop empty values from the target column
+ df.dropna(subset=[column], inplace=True)
Review Comment:
Using `inplace=True` with `dropna()` mutates the input DataFrame, which can
cause unexpected side effects for the caller. This violates best practices as
functions should avoid modifying their inputs unless explicitly documented.
Consider using `df = df.dropna(subset=[column])` instead to work with a copy
and avoid modifying the original DataFrame passed to the function.
```suggestion
df = df.dropna(subset=[column])
```
##########
superset/utils/pandas_postprocessing/histogram.py:
##########
@@ -48,6 +48,11 @@ def histogram(
if groupby is None:
groupby = []
+ # drop empty values from the target column
Review Comment:
[nitpick] The comment says "drop empty values" but the code actually drops
NULL/NaN values. Consider using more precise terminology: "Drop NULL values
from the target column" to be clearer about what's being removed.
```suggestion
# drop NULL/NaN values from the target column
```
--
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]
