SEPURI-SAI-KRISHNA commented on code in PR #45083:
URL: https://github.com/apache/superset/pull/45083#discussion_r4275020002
##########
superset/utils/pandas_postprocessing/utils.py:
##########
@@ -248,14 +258,24 @@ def _append_columns(
# 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}
+ 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}
_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[:, overwritten.keys()] = append_df
+ # Select before renaming, as below, and because once the source name
may
+ # differ from the target, it is the target that says which column to
+ # write.
+ overwritten_df = append_df.loc[:, overwritten.keys()].rename(
+ columns=overwritten
+ )
+ _base_df.loc[:, overwritten_df.columns] = overwritten_df
Review Comment:
Good catch, and it is worse than non-blocking: this PR introduces the 500.
Confirmed by running both versions of `_append_columns` against the same frames:
```
master
OK {"y":"z","z":"z"} -> cols=['y', 'z', 'z']
OK {"y":"w","z":"w"} -> cols=['y', 'z', 'w', 'w']
this PR
RAISES {"y":"z","z":"z"} -> ValueError: Setting with non-unique columns is
not allowed.
OK {"y":"w","z":"w"} -> cols=['y', 'z', 'w', 'w']
```
So I took your suggestion rather than leaving it as a follow-up, and
rejected duplicate targets up front. It covers both rows, since no split of the
mapping can serve two sources writing to one target.
```
this PR + guard
RAISES {"y":"z","z":"z"} -> InvalidPostProcessingError: Columns mapped to
the same target: z
RAISES {"y":"w","z":"w"} -> InvalidPostProcessingError: Columns mapped to
the same target: w
OK {"y":"z","z":"y"} -> cols=['y', 'z']
OK {"y":"z"} -> cols=['y', 'z']
```
`InvalidPostProcessingError.status` is 400, and measured through the real
operations it is a 400 in each: `rolling`, `cum` and `diff` all return `Columns
mapped to the same target: z`.
One thing the guard fixes that is not obvious from the line you commented
on. `geohash_decode` and `geodetic_parse` already wrap `_append_columns` in
`except ValueError: raise InvalidPostProcessingError(_("Invalid geodetic
string"))`, so for those two callers the `ValueError` was arriving as a 400
with the wrong cause named. Raising inside `_append_columns` goes past that:
```
geodetic_parse(df, geodetic="p", longitude="x", latitude="x")
-> InvalidPostProcessingError: Columns mapped to the same target: x
status=400
```
Added as a parametrized test over both of your mappings. 193 passed in
`tests/unit_tests/pandas_postprocessing/`, and the new string is in
`messages.pot`.
--
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]
