SEPURI-SAI-KRISHNA commented on code in PR #45083:
URL: https://github.com/apache/superset/pull/45083#discussion_r4278588679
##########
superset/utils/pandas_postprocessing/utils.py:
##########
@@ -226,10 +227,20 @@ def _append_columns(
assign method, which overwrites the original column in `base_df` if the
column
already exists, and appends the column if the name is not defined.
- A mapping may do both at once, so the two halves are handled separately: an
- entry whose source and target names match overwrites in place, and one that
- renames is appended. Treating a mixed mapping as a whole would append the
- column that was meant to be overwritten, leaving a duplicate label behind.
+ A mapping may do both at once, so the two halves are handled separately.
+ An entry is appended only when its target names a column the result does
not
+ already have, which means the target differs from the source *and* is not
+ already in `base_df`. Everything else is written in place. Treating a mixed
+ mapping as a whole would append the column that was meant to be
overwritten,
+ leaving a duplicate label behind.
+
+ Both parts of that condition are needed. `{'y': 'z'}` against a frame that
+ already has a `z` renames onto an existing label, so appending it
duplicates
+ `z`. `{'latitude': 'latitude'}` names a target that is not in `base_df` at
+ all, which `geodetic_parse` does whenever the caller keeps the default
+ column name, and that must keep being written in place: appending it would
+ concatenate, and `pd.concat` unions the index rather than aligning to
Review Comment:
Reworded. You were right that the old wording stops being true once #45086
is in: the geography helpers carry `df`'s own index now, so that path no longer
gains rows through those two callers.
Dropped the "would gain a row per parsed row" claim and kept the part that
still holds, that `pd.concat` unions the index rather than aligning, so an
`append_df` not sharing `base_df`'s index would change the row count.
`_append_columns` is general, so the invariant is the helper's own rather than
something the geography callers happen to provide.
I kept `value != key` rather than dropping it, but only because there is now
a test that depends on it. See the other thread: with the indexes aligned
nothing was guarding it, so I added one that pins the row-count invariant and
fails against the presence-only form.
--
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]
