Re: [PR] fix(histogram): add NULL handling for histogram [superset]

["Copilot (via GitHub)" <gi...@apache.org>]

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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org