EnxDev commented on code in PR #45083:
URL: https://github.com/apache/superset/pull/45083#discussion_r4276164960
##########
superset/utils/pandas_postprocessing/utils.py:
##########
@@ -226,10 +227,20 @@ def _append_columns(
assign method, which overwrites the original column in `base_df` if the
column
already exists, and appends the column if the name is not defined.
- A mapping may do both at once, so the two halves are handled separately: an
- entry whose source and target names match overwrites in place, and one that
- renames is appended. Treating a mixed mapping as a whole would append the
- column that was meant to be overwritten, leaving a duplicate label behind.
+ A mapping may do both at once, so the two halves are handled separately.
+ An entry is appended only when its target names a column the result does
not
+ already have, which means the target differs from the source *and* is not
+ already in `base_df`. Everything else is written in place. Treating a mixed
+ mapping as a whole would append the column that was meant to be
overwritten,
+ leaving a duplicate label behind.
+
+ Both parts of that condition are needed. `{'y': 'z'}` against a frame that
+ already has a `z` renames onto an existing label, so appending it
duplicates
+ `z`. `{'latitude': 'latitude'}` names a target that is not in `base_df` at
+ all, which `geodetic_parse` does whenever the caller keeps the default
+ column name, and that must keep being written in place: appending it would
+ concatenate, and `pd.concat` unions the index rather than aligning to
Review Comment:
#45086 made the geography helpers build their frames on `df`'s own index, so
after a rebase the concat path lines up for `geodetic_parse` and wouldn't add
rows anymore.
The `value != key` half is still harmless to keep. The reason given here
just won't hold once this rebases, so it's worth rewording, or dropping if
nothing else depends on it.
##########
tests/unit_tests/pandas_postprocessing/test_utils.py:
##########
@@ -87,6 +89,69 @@ def test_append_columns_with_mixed_mapping():
assert series_to_list(mixed["z2"]) == [30.0, 40.0]
+def test_append_columns_overwrites_a_target_already_in_base_df():
+ """
+ A rename whose target is already a column must overwrite, not append.
+
+ `{"y": "z"}` against a frame that already has a `z` renames onto an
existing
+ label, so appending it leaves two columns called `z` and `df["z"]` returns
a
+ DataFrame where callers expect a Series. The source and target names
differ,
+ so a split keyed on them matching sends this down the append path.
+ """
+ base_df = DataFrame({"y": [1.0, 2.0], "z": [3.0, 4.0]})
+ append_df = DataFrame({"y": [10.0, 20.0]})
+
+ post_df = _append_columns(base_df, append_df, {"y": "z"})
+
+ assert post_df.columns.tolist() == ["y", "z"]
+ assert not post_df.columns.duplicated().any()
+ assert series_to_list(post_df["y"]) == [1.0, 2.0]
+ assert series_to_list(post_df["z"]) == [10.0, 20.0]
+ # The overwrite must not reach the caller's frame.
+ assert series_to_list(base_df["z"]) == [3.0, 4.0]
+
+
+def test_append_columns_splits_a_mapping_by_whether_the_target_exists():
+ """
+ One mapping may overwrite an existing target and append a new one.
+
+ Both halves are renames, so neither is recognizable by its source and
target
+ matching; only the presence of the target separates them.
+ """
+ base_df = DataFrame({"y": [1.0, 2.0], "z": [3.0, 4.0]})
+ append_df = DataFrame({"y": [10.0, 20.0], "z": [30.0, 40.0]})
+
+ post_df = _append_columns(base_df, append_df, {"y": "z", "z": "w"})
+
+ assert post_df.columns.tolist() == ["y", "z", "w"]
+ assert not post_df.columns.duplicated().any()
+ assert series_to_list(post_df["y"]) == [1.0, 2.0]
+ assert series_to_list(post_df["z"]) == [10.0, 20.0]
+ assert series_to_list(post_df["w"]) == [30.0, 40.0]
+
+
+def test_append_columns_writes_in_place_when_the_target_names_the_source():
+ """
+ A target that names its own source is written in place, not appended.
+
+ The target need not be a column of `base_df` for this: `geodetic_parse`
+ passes `{"latitude": "latitude"}` whenever the caller keeps the default
+ column name, and `latitude` is a column of `append_df` only. Appending it
+ would go through `pd.concat`, which unions the index instead of aligning to
+ `base_df`, so a frame that does not have a 0-based index would gain a row
per
+ parsed row. Testing only whether the target is already present would miss
+ this, since it is not.
+ """
+ base_df = DataFrame({"city": ["New York City", "Sydney"]}, index=[5, 6])
+ append_df = DataFrame({"latitude": [40.7, -33.8]})
+
+ post_df = _append_columns(base_df, append_df, {"latitude": "latitude"})
+
+ assert post_df.columns.tolist() == ["city", "latitude"]
+ assert post_df.index.tolist() == [5, 6]
+ assert len(post_df) == len(base_df)
Review Comment:
`append_df` keeps a 0-based index here while `base_df` is on `[5, 6]`, so
the `.loc` write aligns on index and `latitude` comes out as `[nan, nan]`. I
ran it at this head to check. The test passes because it only looks at shape.
Could we give `append_df` `index=[5, 6]`, which is what `geodetic_parse`
passes on master since #45086, and assert the values `[40.7, -33.8]`?
--
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]
