SEPURI-SAI-KRISHNA commented on code in PR #45018:
URL: https://github.com/apache/superset/pull/45018#discussion_r4193174530
##########
superset/utils/pandas_postprocessing/utils.py:
##########
@@ -238,13 +243,25 @@ def _append_columns(
in `base_df` unchanged.
:return: new DataFrame with combined data from `base_df` and `append_df`
"""
- if all(key == value for key, value in columns.items()):
+ if not columns:
+ # Nothing to combine, but still hand back a new DataFrame so that a
+ # caller which mutates the result does not reach `base_df`.
+ return base_df.copy()
+
+ overwritten = {key: value for key, value in columns.items() if key ==
value}
+ appended = {key: value for key, value in columns.items() if key != value}
+
+ _base_df = base_df
+ if overwritten:
# make sure to return a new DataFrame instead of changing the
`base_df`.
_base_df = base_df.copy()
- _base_df.loc[:, columns.keys()] = append_df
+ _base_df.loc[:, overwritten.keys()] = append_df
+ if not appended:
return _base_df
- append_df = append_df.rename(columns=columns)
- return pd.concat([base_df, append_df], axis="columns")
+ # Select before renaming: `append_df` may carry columns the mapping does
not
+ # ask for, and those must not reach the result.
+ appended_df = append_df.loc[:, appended.keys()].rename(columns=appended)
+ return pd.concat([_base_df, appended_df], axis="columns")
Review Comment:
This is real, and it predates the PR. It reproduces on master with nothing
from this branch applied:
```python
df = pd.DataFrame({"city": ["A", "B"], "geodetic": ["41.1 -73.5", "40.7
-74.0"]}, index=[5, 6])
geodetic_parse(df=df, geodetic="geodetic", latitude="lat", longitude="lon")
# city geodetic lat lon altitude
# 5 A 41.1 -73.5 NaN NaN NaN
# 6 B 40.7 -74.0 NaN NaN NaN
# 0 NaN NaN 41.1 -73.5 0.0
# 1 NaN NaN 40.7 -74.0 0.0
```
Two rows in, four out. The same input with an identity mapping comes back
all NaN through the overwrite branch instead, so it is not specific to
`concat`. `geohash_decode` behaves the same way; `geohash_encode` does not.
The cause is in `geography.py`. `geodetic_parse` and `geohash_decode` build
their working frame as a bare `DataFrame()` and assign columns from `zip(...)`,
which gives it a fresh RangeIndex rather than `df.index`. `geohash_encode`
escapes it because it slices `df[[latitude, longitude]]` and keeps the index.
Setting `appended_df.index = _base_df.index` in the helper would hide it
rather than fix it. `cum`, `rolling` and `diff` pass an `append_df` that is
already index-aligned with `base_df`, and a forced positional reindex there
would paper over a genuine misalignment. The fix belongs in the two
constructors, as `DataFrame(index=df.index)`.
It is out of scope here: this PR changes which columns reach the result, not
how rows align. I will file it separately once this one lands, since its test
belongs in the same file this PR already touches.
--
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]
