SEPURI-SAI-KRISHNA opened a new pull request, #45018:
URL: https://github.com/apache/superset/pull/45018
### SUMMARY
`_append_columns` chose between overwriting and appending with `all()`, so a
mapping that did both fell entirely to the append path and re-appended the
column that should have been overwritten, leaving two columns under one label.
Details and provenance in #45017.
This PR splits the mapping and handles each half, and selects the mapped
columns before renaming them:
```python
overwritten = {key: value for key, value in columns.items() if key == value}
appended = {key: value for key, value in columns.items() if key != value}
```
| mapping | before | after |
|---|---|---|
| `{"y": "y", "z": "z"}` | `['y', 'z']` | `['y', 'z']` (unchanged) |
| `{"y": "y2", "z": "z2"}` | `['y', 'z', 'y2', 'z2']` | `['y', 'z', 'y2',
'z2']` (unchanged) |
| `{"y": "y", "z": "z2"}` | `['y', 'z', 'y', 'z2']` | `['y', 'z', 'z2']` |
### Two defects, one cause
**1. A mixed mapping duplicates a label.** `cum`, `rolling` and `diff`
restrict `append_df` to the mapped columns, so for them this is the only
symptom. On MultiIndex columns, cumulating one metric in place after a pivot
while adding another alongside it, it duplicates `('sum_metric', 'UK')` and
`('sum_metric', 'US')`, and `df[label]` then hands callers a DataFrame where
they expect a Series.
**2. The append path carried columns the mapping never asked for.** The
three geography operators build a working frame wider than their mapping, so
they hit this independently of a mixed mapping:
```python
# geodetic_parse always parses an altitude, but maps it only when asked
geodetic_parse(df=..., geodetic="geodetic", latitude="lat", longitude="lon")
# before: ['city', 'geodetic', 'lat', 'lon', 'altitude'] <- unrequested
# after: ['city', 'geodetic', 'lat', 'lon']
# geohash_encode works on hardcoded latitude/longitude labels and maps only
the hash
geohash_encode(df=..., latitude="latitude", longitude="longitude",
geohash="hash")
# before: ['city', 'latitude', 'longitude', 'latitude', 'longitude', 'hash']
# columns.duplicated().any() is True, out["latitude"] is a DataFrame
# after: ['city', 'latitude', 'longitude', 'hash']
```
So the duplicate-label failure is not confined to mixed mappings. An
all-rename mapping reaches it too whenever `append_df` carries a column
`base_df` already has. Both come from the same two lines.
### What changes and what does not
This is a shared helper, so the exact blast radius:
- **Unchanged:** every all-overwrite mapping, and every all-rename mapping
whose `append_df` carries only mapped columns. That covers `cum`, `rolling` and
`diff` in full, and geography wherever the target names match the working-frame
names. That is what the existing tests exercise, and why all of them still pass
untouched.
- **Deliberately changed:** an all-rename mapping whose `append_df` is wider
than the mapping. That is only the geography operators, and in every such case
the dropped columns were either unrequested or duplicates. This is the second
fix above, not a side effect.
- **Preserved explicitly:** an empty mapping still returns `base_df.copy()`
rather than `base_df` itself. `cum` normalizes a missing mapping to `{}`, so
that path is reachable, and my first draft of the split would have returned the
input object. There is a test for it.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A: no UI change.
### TESTING INSTRUCTIONS
```bash
pytest tests/unit_tests/pandas_postprocessing/
```
Six tests are added:
| test | covers | on master |
|---|---|---|
| `test_append_columns_with_mixed_mapping` | all three mapping shapes |
**fails** |
| `test_append_columns_ignores_unmapped_columns` | unmapped column dropped |
**fails** |
| `test_append_columns_with_empty_mapping_returns_a_copy` | empty mapping
still copies | passes |
| `test_cum_with_mixed_column_mapping` | defect end-to-end through `cum` on
MultiIndex columns | **fails** |
| `test_geodetic_parse_without_altitude_drops_unasked_column` | the
`altitude` leak, end-to-end | **fails** |
| `test_geohash_encode_drops_working_columns` | the duplicate-label leak,
end-to-end | **fails** |
Five fail on master and pass with this change. The sixth passes on master by
design: it guards the empty-mapping behaviour this refactor could otherwise
have regressed, so it is a preservation test rather than a bug repro.
The whole `tests/unit_tests/pandas_postprocessing/` suite goes from 180 to
186 passing with no existing test modified, and
`tests/unit_tests/{pandas_postprocessing,models,semantic_layers,common,charts}`
run clean at 2197 passed, 2 xfailed (both pre-existing). `ruff check`, `ruff
format --check` and `mypy` are clean, and `pylint` rates the changed source
file 10.00/10.
### ADDITIONAL INFORMATION
- [x] Has associated issue: Fixes #45017
- [ ] 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]
