aminghadersohi commented on code in PR #40456:
URL: https://github.com/apache/superset/pull/40456#discussion_r3343395699
##########
superset/utils/pandas_postprocessing/histogram.py:
##########
@@ -49,7 +49,7 @@ def histogram(
groupby = []
# drop empty values from the target column
- df = df.dropna(subset=[column])
+ df = df.dropna(subset=[column]).copy()
if df.empty:
Review Comment:
The fix is correct, but the PR description says "Unit tests verify the
warning is no longer raised" — that claim is not accurate. `test_histogram.py`
has 10 existing behavioral tests, none of which assert on
`SettingWithCopyWarning` absence. The existing tests use freshly-constructed
DataFrames that won't trigger the warning regardless of this change, because
`_is_copy` is `None` on directly-constructed DataFrames.
The actual triggering path is passing a slice: `histogram(df[df["date"] ==
x], col, [], bins)`. Please add a test that covers that input shape, e.g.:
```python
def test_histogram_no_setting_with_copy_warning():
import warnings
from pandas.errors import SettingWithCopyWarning
source = DataFrame({"a": [1, 2, None, 4, 5], "b": list(range(5))})
with warnings.catch_warnings(record=True) as w:
warnings.simplefilter("always")
histogram(source.loc[source["b"] >= 0], "a", [], 3)
assert not any(issubclass(x.category, SettingWithCopyWarning) for x in w)
```
--
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]
