You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "SEPURI-SAI-KRISHNA (via GitHub)" <gi...@apache.org> on 2026/10/06 07:18:37 UTC
[PR] fix(post-processing): stop duplicating columns in _append_columns [superset]
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(post-processing): stop duplicating columns in _append_columns [superset]
Posted by "SEPURI-SAI-KRISHNA (via GitHub)" <gi...@apache.org>.
SEPURI-SAI-KRISHNA commented on code in PR #45018:
URL: https://github.com/apache/superset/pull/45018#discussion_r4202763766
##########
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}
Review Comment:
Confirmed, `['y', 'z', 'z']` with `columns.duplicated().any()` true.
Recorded on #45017 with the repro, and I added a "Not covered" line to the PR body so the summary does not read as covering it.
On the fix itself: keying the split on `target in base_df.columns` rather than on `source == target` handles your case and subsumes the identity case, since an identity target is present by definition. That makes it a change to what the helper promises rather than another bug in the same lines, which is why I would rather do it as the follow-up than widen this PR.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(post-processing): stop duplicating columns in _append_columns [superset]
Posted by "SEPURI-SAI-KRISHNA (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(post-processing): stop duplicating columns in _append_columns [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev merged PR #45018:
URL: https://github.com/apache/superset/pull/45018
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(post-processing): stop duplicating columns in _append_columns [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #45018:
URL: https://github.com/apache/superset/pull/45018#issuecomment-6011486166
<!-- Bito Reply -->
The flagged issue is correct. When using `pd.concat` with `axis="columns"`, pandas aligns dataframes by their index. If the input dataframes have different indices (e.g., one has a `RangeIndex` and the other has a custom index), `pd.concat` will perform an outer join, potentially introducing unintended rows or `NaN` values where indices do not match.
To resolve this, you should ensure the indices are aligned before concatenation, for example by resetting the index or ensuring both dataframes share the same index:
```python
# Ensure indices match before concatenation
appended_df = append_df.loc[:, appended.keys()].rename(columns=appended)
appended_df.index = _base_df.index
return pd.concat([_base_df, appended_df], axis="columns")
```
Would you like me to fetch all other comments on this PR to validate and implement fixes for them as well?
**superset/utils/pandas_postprocessing/utils.py**
```
# Ensure indices match before concatenation
appended_df = append_df.loc[:, appended.keys()].rename(columns=appended)
appended_df.index = _base_df.index
return pd.concat([_base_df, appended_df], axis="columns")
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(post-processing): stop duplicating columns in _append_columns [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #45018:
URL: https://github.com/apache/superset/pull/45018#discussion_r4192623967
##########
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:
**Suggestion:** `pd.concat` aligns by index and takes the union, so parsing an input with a non-RangeIndex can add stray rows from the parsed frame's RangeIndex.
**Assessment:** ๐ `Major` ยท ๐ `Occurrence: Rarely` ยท ๐ท๏ธ `Logic error`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=96b0d1941daa432795694ab62f14d3c0&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=96b0d1941daa432795694ab62f14d3c0&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/utils/pandas_postprocessing/utils.py
**Line:** 264:264
**Comment:**
*Logic Error: `pd.concat` aligns by index and takes the union, so parsing an input with a non-RangeIndex can add stray rows from the parsed frame's RangeIndex.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45018&comment_hash=e23ce0ce57585945ed1ebd0fdba20d7066c9ac7a37df6fd98dabf2cb9a859abb&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45018&comment_hash=e23ce0ce57585945ed1ebd0fdba20d7066c9ac7a37df6fd98dabf2cb9a859abb&reaction=dislike'>๐</a>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(post-processing): stop duplicating columns in _append_columns [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #45018:
URL: https://github.com/apache/superset/pull/45018#issuecomment-6011493884
## CodeAnt PR Risk: Medium Risk
- The PR needs attention before merging because concatenating parsed columns may union mismatched indexes and add stray rows.
- Check geodetic parsing with a non-RangeIndex input and ensure appended values remain aligned to the original rows.
**Assessed commit:** `3a096c957d60`
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(post-processing): stop duplicating columns in _append_columns [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #45018:
URL: https://github.com/apache/superset/pull/45018#issuecomment-6011387166
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(post-processing): stop duplicating columns in _append_columns [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #45018:
URL: https://github.com/apache/superset/pull/45018#issuecomment-6011676318
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/45018?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `0%` with `10 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 66.09%. Comparing base ([`ce39d22`](https://app.codecov.io/gh/apache/superset/commit/ce39d221aacc6d6d1d9817d7fb9b42ac3fe5daae?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`3a096c9`](https://app.codecov.io/gh/apache/superset/commit/3a096c957d60c93f738f5e30e8e67e841f4b109b?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/45018?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/utils/pandas\_postprocessing/utils.py](https://app.codecov.io/gh/apache/superset/pull/45018?src=pr&el=tree&filepath=superset%2Futils%2Fpandas_postprocessing%2Futils.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvdXRpbHMvcGFuZGFzX3Bvc3Rwcm9jZXNzaW5nL3V0aWxzLnB5) | 0.00% | [10 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/45018?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (ce39d22) and HEAD (3a096c9). Click for more details.
>
> <details><summary>HEAD has 4 uploads less than BASE</summary>
>
>| Flag | BASE (ce39d22) | HEAD (3a096c9) |
>|------|------|------|
>|python|7|3|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #45018 +/- ##
===========================================
- Coverage 82.36% 66.09% -16.27%
===========================================
Files 2997 2997
Lines 185523 185529 +6
Branches 42938 42940 +2
===========================================
- Hits 152799 122634 -30165
- Misses 29970 60211 +30241
+ Partials 2754 2684 -70
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/45018/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage ฮ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/45018/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `36.30% <0.00%> (-0.01%)` | :arrow_down: |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/45018/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/45018/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/45018/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `38.17% <0.00%> (-0.01%)` | :arrow_down: |
| [python](https://app.codecov.io/gh/apache/superset/pull/45018/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.13% <0.00%> (-31.17%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/45018/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.01% <0.00%> (-0.01%)` | :arrow_down: |
| [unit](https://app.codecov.io/gh/apache/superset/pull/45018/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/45018?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(post-processing): stop duplicating columns in _append_columns [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev commented on code in PR #45018:
URL: https://github.com/apache/superset/pull/45018#discussion_r4210884333
##########
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}
Review Comment:
Looked into where the duplication came from. Before #19116 this was a plain `assign()`, which overwrote or appended column by column and never duplicated anything. The `all()` + `concat` version came in with the MultiIndex support, so this PR is really putting the old behavior back.
That makes the `{"y": "z"}` follow-up a restore too, since `assign` overwrote `z` as well. And I don't see anything relying on the duplicates: callers in the repo all pass identity maps, and the JSON serializer already drops duplicate labels.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org