fix(dataframe): handle pandas dimensionality reduction in .xs() for single-item matches - #39851
Conversation
…ingle-item matches Beam's .xs() implementation assumed static output shape (DataFrame/Series) across partitions, but pandas reduces dimensionality (DataFrame->Series, Series->scalar) when a key matches exactly one row and all index levels are selected. This caused TypeError/shape-mismatch failures during cross-partition concat. Fixes the key_size >= nlevels path to route matching partitions through a singleton unwrap stage that mirrors pandas' actual runtime behavior, while documenting the inherent proxy-time ambiguity for duplicate-match cases (proxy assumes single-match dimensionality; runtime produces whichever type pandas actually returns). Fixes apache#28559 Signed-off-by: ManvithPanyam <250704031+ManvithPanyam@users.noreply.github.com>
|
Assigning reviewers: R: @jrmccluskey for label python. Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
19b0c08 to
5c6b20a
Compare
|
@tvalentyn — opened a fix for this. Root cause: Beam's One thing flagged for review: the proxy can't distinguish single-match |
| isinstance(k_val, tuple) else pd.Index([k_val], | ||
| name=proxy_frame.index.name)) | ||
| dummy_data = { | ||
| col: [proxy_frame[col].dtype.type()] |
There was a problem hiding this comment.
dtype.type() will crash on some Pandas types like Categorical Type.
will smth like this work?
proxy_frame = reindexed._expr.proxy()
dummy_index = (
pd.MultiIndex.from_tuples([k_val], names=proxy_frame.index.names) if
isinstance(k_val, tuple) else pd.Index([k_val],
name=proxy_frame.index.name))
dummy_obj = proxy_frame.reindex(dummy_index)
xs_proxy = dummy_obj.xs(k_val, **kwargs)
if isinstance(xs_proxy, (pd.DataFrame, pd.Series)):
xs_proxy = xs_proxy.iloc[:0]
|
thanks for the contribution. are you a Beam Dataframes user? |
5c6b20a to
f7090d4
Compare
|
Not really an active Beam DataFrames user day-to-day — found this while Thanks for the catch on Applied your Also had to handle proxy indexes with duplicate labels — Added regression tests for Categorical, tz-aware datetime, and nullable |
|
thanks, please don't squash reviewed and unreviewed code in the same commits as it hides the diff between versions. would you be able to rebase so we have both commits? thanks! |
…extension dtypes Addresses review feedback: dtype.type() crashes on Categorical and timezone-aware datetime dtypes since they aren't callable as zero-arg scalar constructors. Switched the DataFrame branch to build the dummy proxy via reindex() + xs(), letting pandas handle type construction internally. Kept dtype.type() as the primary path for the Series (single-match scalar) branch since reindex().iloc[0] alone silently upcasts plain numeric types (e.g. int64 -> float64) by introducing NaN; falls back to reindex().iloc[0] only on TypeError for extension types. Also handles proxy indexes with duplicate labels, which reindex() otherwise rejects. Added regression tests for Categorical, tz-aware datetime, and nullable Int64 columns. Signed-off-by: ManvithPanyam <250704031+ManvithPanyam@users.noreply.github.com>
f7090d4 to
ac0525f
Compare
|
Done — split into two commits, rebased/pushed. First is the original as you reviewed it, second is just the reindex()-based fix and everything else from this thread. |
…plicate-proxy crash Further simplifies the reindex()-based proxy generation from the previous commit: unifies dummy_index construction (was duplicated per-branch), and hoists the is_unique/drop_duplicates() dedup check to run once instead of twice. The dedup check is retained, not removed — traced that pandas can cache an IndexEngine on an index once inspected (e.g. via .is_unique, .loc, .get_loc), and that cache can survive slicing to iloc[:0], leaving is_unique stale as False on an otherwise-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 fixes a real bug found while testing: a non-empty user-supplied proxy with duplicate index labels (via to_dataframe(pcoll, proxy=df_with_dup_index)) crashed the old code with 'ValueError: Length mismatch', since drop_duplicates() shrinks the index but not the frame before reassignment. Pre-slicing to iloc[:0] before the dedup check resolves this. Added regression test test_dataframe_xs_non_empty_duplicate_proxy covering both DataFrame and Series non-empty duplicate-proxy cases. Signed-off-by: ManvithPanyam <250704031+ManvithPanyam@users.noreply.github.com>
|
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 |
|
thanks! |
What does this PR do?
Fixes
.xs()onDeferredDataFrame/DeferredSerieswhen a key matchesexactly one row and all index levels are selected.
The bug: pandas reduces dimensionality on single-item
.xs()matches(
DataFrame→Series,Series→ scalar), but Beam's implementationassumed a static output shape across partitions. Non-matching partitions
return an empty container of the original type, so when the matching
partition returned a dimensionality-reduced result, cross-partition
pd.concateither raisedTypeError(scalar concat) or silently produceda corrupted schema (NaN columns from a shape mismatch).
The fix: when
key_size >= nlevels, matching partitions are routedthrough a wrapped singleton stage that mirrors pandas' actual runtime
output, then unwrapped to the real return type — instead of assuming the
proxy shape holds at execution time.
Known limitation (documented in code): the proxy schema (computed
at graph-construction time from a 0-row template) can't know whether a
key will match 1 row or several at runtime, so it always assumes the
dimensionality-reduced type. If a key has duplicate matches, pandas
returns the non-reduced container instead — this is fundamentally
undecidable at proxy time, same class of limitation as
sort_values(),describe(), and other data-dependent-shape operations already in thismodule. Tests exercising duplicate-match keys use
check_proxy=Falseaccordingly, with the reasoning documented inline.
Fixes
Fixes #28559
Tests
Added regression coverage in
frames_test.pyfor all reported failuremodes: single-level index single match, MultiIndex 0-levels-remaining
single match (both unique and duplicate-key datasets), and Series
single-item
.xs(). Fullframes_test.pysuite: 452 passed, 19 skipped,zero regressions.