SEPURI-SAI-KRISHNA opened a new pull request, #45083:
URL: https://github.com/apache/superset/pull/45083
### SUMMARY
`_append_columns` decides whether a mapping entry overwrites a column or
appends one by asking whether its source and target names match. A rename can
land on a label the frame already has, and that entry is appended, so the
result carries two columns under the same name.
#45018 introduced that split to fix the mixed-mapping case in #45017.
@EnxDev flagged this residual in review there, and I recorded it on #45017,
which auto-closed when #45018 merged. This is it on its own.
An entry is now appended only when its target is a label the result does not
already have, meaning the target differs from the source **and** is not already
in `base_df`:
```python
appended = {
key: value
for key, value in columns.items()
if value != key and value not in base_df.columns
}
overwritten = {key: value for key, value in columns.items() if key not in
appended}
```
**Both halves of that condition are needed, and I had this wrong at first.**
On #45018 I suggested keying only on `value in base_df.columns`, saying it
subsumes the matching case "since a `{"y": "y"}` target is present by
definition". That holds for `cum`, `diff` and `rolling`, where
`@validate_column_args` guarantees the mapping's keys are columns of `base_df`.
It does not hold for the geography operations, where the keys are `append_df`'s
column names. `geodetic_parse` passes `{"latitude": "latitude"}` whenever the
caller keeps the default column name, and `latitude` is not in `base_df` at
all, so a presence-only test would send it down the append path. `pd.concat`
unions the index rather than aligning, so a frame without a 0-based index would
gain a row per parsed row. A test pins that case, and it fails against the
presence-only form.
The overwrite branch now selects and renames before assigning, the way the
append branch already does, because once the source name may differ from the
target it is the target that says which column to write.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A: backend only.
```python
base_df = DataFrame({"y": [1, 2], "z": [3, 4]})
append_df = DataFrame({"y": [10, 20]})
_append_columns(base_df, append_df, {"y": "z"})
```
| | columns | `z` |
|---|---|---|
| before | `['y', 'z', 'z']` | two columns, `df["z"]` is a DataFrame |
| after | `['y', 'z']` | `[10, 20]` |
| mapping | before | after |
|---|---|---|
| `{"y": "z"}`, `z` present | `['y', 'z', 'z']` | `['y', 'z']`, overwritten |
| `{"y": "z", "z": "w"}` | `['y', 'z', 'z', 'w']` | `['y', 'z', 'w']` |
| `{"y": "y"}` | `['y']`, overwritten | unchanged |
| `{"y": "y2"}`, no `y2` | `['y', 'y2']`, appended | unchanged |
| `{"latitude": "latitude"}`, source not in `base_df` | written in place |
unchanged |
`base_df` is still left untouched in every case, which the new tests assert.
To check the change is confined to the bug I ran both the old and the new
helper over 1152 mappings, built from frames of one to three columns against
four index shapes including a reversed and a string index. Every difference is
a case where a target already existed; there is no column reordering, and the
only row-count changes are the 118 where the old code was returning both a
duplicate label and extra rows from the index union, where the new code returns
`base_df`'s own row count.
### TESTING INSTRUCTIONS
```bash
pytest tests/unit_tests/pandas_postprocessing
```
Three tests are added to
`tests/unit_tests/pandas_postprocessing/test_utils.py`:
| test | covers | on master |
|---|---|---|
| `test_append_columns_overwrites_a_target_already_in_base_df` | `{"y":
"z"}` with `z` present, the case from the issue, plus that `base_df` is not
mutated | fails |
| `test_append_columns_splits_a_mapping_by_whether_the_target_exists` |
`{"y": "z", "z": "w"}`, where both halves are renames and the split cannot be
read off the names | fails |
| `test_append_columns_writes_in_place_when_the_target_names_the_source` |
`{"latitude": "latitude"}` on a non-0-based index keeps `base_df`'s row count |
passes, guards against the presence-only form |
The full post-processing suite is 189 passed, up from 186, with no existing
test changed. `tests/unit_tests/{pandas_postprocessing,charts,queries}` run
clean at 582 passed, 2 xfailed (both pre-existing). `ruff check`, `ruff format
--check` and `pre-commit run mypy` are clean, and `pylint --rcfile=.pylintrc`
rates the changed file 10.00/10.
### ADDITIONAL INFORMATION
- [x] Has associated issue: Fixes #45082
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in
[SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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]
