EnxDev commented on code in PR #45083:
URL: https://github.com/apache/superset/pull/45083#discussion_r4431286124
##########
superset/utils/pandas_postprocessing/utils.py:
##########
@@ -248,14 +260,39 @@ 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}
+ # Two sources cannot both be written to one target. Rejecting that up front
+ # keeps the in-place write below from being handed a duplicate label, which
+ # pandas refuses, and keeps the append half from quietly returning a frame
+ # that carries the target label twice.
+ duplicate_targets = sorted(
+ {target for target, count in Counter(columns.values()).items() if
count > 1}
+ )
+ if duplicate_targets:
+ raise InvalidPostProcessingError(
+ _(
+ "Columns mapped to the same target: %(columns)s",
+ columns=", ".join(duplicate_targets),
+ )
+ )
+
+ 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:
This conflicts with master now. #45325 (the pandas 3 bump) rewrote this
block as a per-column loop with a `TypeError` fallback, because pandas 3 raises
instead of upcasting when `.loc` writes a NaN into an int column.
If the resolution keeps this bulk set, every overwrite loses that fallback,
plain `{"y": "y"}` included. On 3.0.6 this line throws as soon as it gets a
NaN-bearing column for an int one, which is what `diff` hands back for an int
metric. Running master's loop by target keeps both fixes:
```python
for source, target in overwritten.items():
try:
_base_df.loc[:, [target]] = append_df[[source]].rename(
columns={source: target}
)
except TypeError:
_base_df[target] = append_df[source]
```
That held up on 3.0.6 for a rename onto an int column, the same-name case, a
swap and a pivoted frame. Nothing in `test_utils.py` covers the fallback yet,
so a case with an int `z` and a NaN in the source would pin it.
--
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]
