ManvithPanyam commented on PR #39851: URL: https://github.com/apache/beam/pull/39851#issuecomment-5540237036
Verified — the reindex()-based approach itself is correct and I've adopted it, simplifying quite a bit (unified dummy_index construction, removed the duplicate per-branch dedup/copy logic). One nuance: kept the is_unique/drop_duplicates() dedup check, just hoisted once at the top instead of duplicated per-branch. Traced it — pandas caches an IndexEngine on an index once something inspects it (e.g. .is_unique, .loc, .get_loc), and that cache can survive slicing to iloc[:0], leaving is_unique stale as False even on an empty result. This path is reachable in practice (e.g. the existing test harness calls .xs() on the full arg before slicing to an empty proxy), so removing the check entirely would reintroduce a reindex() failure in that case. Also found and fixed a real bug while testing this: a non-empty user-supplied proxy with duplicate index labels (via to_dataframe(pcoll, proxy=df_with_dups)) crashed the old code with a length-mismatch error, since drop_duplicates() shrinks the index but not the frame before reassignment. Pre-slicing to iloc[:0] before the dedup check (as in your suggestion) fixes this too. Added a regression test for it. 453 passed, 19 skipped, zero regressions. Note: 4 CI checks are failing on unrelated Bigtable IO tests — a fresh google-cloud-bigtable 2.44.0 (published Sept 3, 22:30 UTC) broke DirectRow._get_mutations() handling. Confirmed master's own scheduled runs are failing identically before and after this PR's CI run, so it's unrelated to this change. -- 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]
