SEPURI-SAI-KRISHNA commented on code in PR #45083:
URL: https://github.com/apache/superset/pull/45083#discussion_r4278566857
##########
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:
Good catch, and you were right that it was writing NaNs. Measured it:
```
append_df 0-based (as the test had it) -> [nan, nan]
append_df on index=[5, 6] -> [40.7, -33.8]
```
Merged master in and did what you suggested: `append_df` is on `index=[5,
6]` now, and the test asserts `[40.7, -33.8]`. Putting the 0-based frame back
fails it, so the assertion actually bites.
That turned up a second thing worth flagging. With the indexes aligned, the
whole suite passes against the presence-only form, so nothing was pinning the
`value != key` half any more. Rather than drop it I pinned the invariant it
really owns, which is that a mapping renaming nothing must not change
`base_df`'s row count:
```python
base_df = DataFrame({"city": ["New York City", "Sydney"]})
append_df = DataFrame({"latitude": [40.7, -33.8, 99.9]}) # one row more
in-place -> (2, 2) latitude [40.7, -33.8]
concat -> (3, 2) city ['New York City', 'Sydney', nan]
```
`test_append_columns_never_adds_rows_for_a_non_renaming_mapping` covers
that, asserting real values rather than a shape. It is now the one test that
fails against the presence-only form.
--
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]
