Match pandas merge/join dtype, index and error semantics - #23060
Conversation
…dation, numeric-vs-string raise, extension-dtype key preservation
…empty-key dtype, ungate to cudf-classic
…oalescing, and numpy-int->float64 upcast on unmatched rows
… left dtype for inner/left same-name keys
github-actions
Bot
added
Python
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR aligns cuDF merge, join, and index behavior more closely with pandas by updating categorical handling, index alignment and reindexing, merge key typing, result construction, post-merge key dtype restoration, and related tests and plugin mappings. ChangesMerge and index semantics rework
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/cudf/cudf/core/join/_join_helpers.py`:
- Around line 207-215: Update the public DataFrame.merge documentation to match
the new categorical join semantics: when left and right category sets differ,
both sides are decategorized to the common dtype for every join type. Locate the
merge API docstring and any categorical-join behavior notes tied to
DataFrame.merge, and revise the return/behavior section so it no longer
describes the old join-type-dependent unordered-categorical handling.
In `@python/cudf/cudf/core/join/join.py`:
- Around line 306-324: The merge-type guard in join logic is only checking
lcol.dtype and rcol.dtype, so categorical keys can slip through before
_match_join_keys decategorizes them. Update the numeric-vs-string validation in
join.py to also inspect categorical key category dtypes (for example via the
join-key handling around the existing l_num/r_num and l_str/r_str checks) and
reject cases where a categorical with string categories is paired with a numeric
key. Keep the existing ValueError path and ensure the check runs before any
decategorization/casting so pandas-compatible rejection still happens.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: dc85c2b8-ba45-4868-86a8-efbcca5e2b62
📒 Files selected for processing (6)
python/cudf/cudf/core/dataframe.pypython/cudf/cudf/core/join/_join_helpers.pypython/cudf/cudf/core/join/join.pypython/cudf/cudf/pandas/scripts/pandas-testing-plugin.pypython/cudf/cudf/tests/reshape/test_join.pypython/cudf/cudf/tests/reshape/test_merge.py
💤 Files with no reviewable changes (1)
- python/cudf/cudf/pandas/scripts/pandas-testing-plugin.py
| # Different category sets: pandas decategorizes both sides to their common | ||
| # categories dtype (the result is not categorical), for every join type. | ||
| common_type = find_common_type( | ||
| (ltype.categories.dtype, rtype.categories.dtype) | ||
| ) | ||
| return ( | ||
| lcol._get_decategorized_column().astype(common_type), | ||
| rcol._get_decategorized_column().astype(common_type), | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
HIGH: Update the public merge docs for the new categorical behavior.
Issue: DataFrame.merge still documents the old unordered-categorical behavior by join type, but this path now decategorizes differing category sets for every join type.
Why: Public API docs will describe the wrong dtype semantics for categorical joins.
As per coding guidelines, “Ensure all public API methods have complete docstrings documenting parameters, return values, and behavior.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@python/cudf/cudf/core/join/_join_helpers.py` around lines 207 - 215, Update
the public DataFrame.merge documentation to match the new categorical join
semantics: when left and right category sets differ, both sides are
decategorized to the common dtype for every join type. Locate the merge API
docstring and any categorical-join behavior notes tied to DataFrame.merge, and
revise the return/behavior section so it no longer describes the old
join-type-dependent unordered-categorical handling.
Source: Coding guidelines
| if len(lcol) and len(rcol): | ||
| # pandas refuses to merge a numeric key against a string key | ||
| # (a numeric-looking string is NOT silently parsed). Empty | ||
| # keys are inferred as ``empty`` rather than ``string`` by | ||
| # pandas and so are exempt from this check. | ||
| l_num = is_dtype_obj_numeric( | ||
| lcol.dtype | ||
| ) and lcol.dtype.kind in ("iuf") | ||
| r_num = is_dtype_obj_numeric( | ||
| rcol.dtype | ||
| ) and rcol.dtype.kind in ("iuf") | ||
| l_str = is_dtype_obj_string(lcol.dtype) | ||
| r_str = is_dtype_obj_string(rcol.dtype) | ||
| if (l_str and r_num) or (r_str and l_num): | ||
| raise ValueError( | ||
| f"You are trying to merge on {lcol.dtype} and " | ||
| f"{rcol.dtype} columns for key '{left_key.name}'. " | ||
| "If you wish to proceed you should use pd.concat" | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
HIGH: Validate categorical key category dtypes too.
Issue: This numeric-vs-string guard checks lcol.dtype/rcol.dtype directly, so a categorical key with string categories merged against a numeric key can bypass the new pandas-compatible rejection before _match_join_keys decategorizes/casts it.
Why: This can still coerce or compare numeric-vs-string merge keys instead of raising.
Suggested direction
+ def _comparison_dtype(col):
+ dtype = col.dtype
+ return (
+ dtype.categories.dtype
+ if isinstance(dtype, CategoricalDtype)
+ else dtype
+ )
+
if len(lcol) and len(rcol):
+ l_dtype = _comparison_dtype(lcol)
+ r_dtype = _comparison_dtype(rcol)
l_num = is_dtype_obj_numeric(
- lcol.dtype
- ) and lcol.dtype.kind in ("iuf")
+ l_dtype
+ ) and l_dtype.kind in ("iuf")
r_num = is_dtype_obj_numeric(
- rcol.dtype
- ) and rcol.dtype.kind in ("iuf")
- l_str = is_dtype_obj_string(lcol.dtype)
- r_str = is_dtype_obj_string(rcol.dtype)
+ r_dtype
+ ) and r_dtype.kind in ("iuf")
+ l_str = is_dtype_obj_string(l_dtype)
+ r_str = is_dtype_obj_string(r_dtype)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if len(lcol) and len(rcol): | |
| # pandas refuses to merge a numeric key against a string key | |
| # (a numeric-looking string is NOT silently parsed). Empty | |
| # keys are inferred as ``empty`` rather than ``string`` by | |
| # pandas and so are exempt from this check. | |
| l_num = is_dtype_obj_numeric( | |
| lcol.dtype | |
| ) and lcol.dtype.kind in ("iuf") | |
| r_num = is_dtype_obj_numeric( | |
| rcol.dtype | |
| ) and rcol.dtype.kind in ("iuf") | |
| l_str = is_dtype_obj_string(lcol.dtype) | |
| r_str = is_dtype_obj_string(rcol.dtype) | |
| if (l_str and r_num) or (r_str and l_num): | |
| raise ValueError( | |
| f"You are trying to merge on {lcol.dtype} and " | |
| f"{rcol.dtype} columns for key '{left_key.name}'. " | |
| "If you wish to proceed you should use pd.concat" | |
| ) | |
| def _comparison_dtype(col): | |
| dtype = col.dtype | |
| return ( | |
| dtype.categories.dtype | |
| if isinstance(dtype, CategoricalDtype) | |
| else dtype | |
| ) | |
| if len(lcol) and len(rcol): | |
| # pandas refuses to merge a numeric key against a string key | |
| # (a numeric-looking string is NOT silently parsed). Empty | |
| # keys are inferred as ``empty`` rather than ``string`` by | |
| # pandas and so are exempt from this check. | |
| l_dtype = _comparison_dtype(lcol) | |
| r_dtype = _comparison_dtype(rcol) | |
| l_num = is_dtype_obj_numeric( | |
| l_dtype | |
| ) and l_dtype.kind in ("iuf") | |
| r_num = is_dtype_obj_numeric( | |
| r_dtype | |
| ) and r_dtype.kind in ("iuf") | |
| l_str = is_dtype_obj_string(l_dtype) | |
| r_str = is_dtype_obj_string(r_dtype) | |
| if (l_str and r_num) or (r_str and l_num): | |
| raise ValueError( | |
| f"You are trying to merge on {lcol.dtype} and " | |
| f"{rcol.dtype} columns for key '{left_key.name}'. " | |
| "If you wish to proceed you should use pd.concat" | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@python/cudf/cudf/core/join/join.py` around lines 306 - 324, The merge-type
guard in join logic is only checking lcol.dtype and rcol.dtype, so categorical
keys can slip through before _match_join_keys decategorizes them. Update the
numeric-vs-string validation in join.py to also inspect categorical key category
dtypes (for example via the join-key handling around the existing l_num/r_num
and l_str/r_str checks) and reject cases where a categorical with string
categories is paired with a numeric key. Keep the existing ValueError path and
ensure the check runs before any decategorization/casting so pandas-compatible
rejection still happens.
|
/okay to test 01fbacc |
|
/okay to test 304c1d6 |
|
/okay to test d08fdd0 |
|
/okay to test 66d1d18 |
| self._left_index_flag = bool(left_index) | ||
| self._right_index_flag = bool(right_index) |
There was a problem hiding this comment.
nit: These should already be bool correct?
There was a problem hiding this comment.
Done — dropped the casts (they come straight from the public API defaults as bool).
| right_names = set(rhs._data) | ||
| on_names = [name for name in lhs._data if name in right_names] |
There was a problem hiding this comment.
| right_names = set(rhs._data) | |
| on_names = [name for name in lhs._data if name in right_names] | |
| right_names = set(rhs._column_names) | |
| on_names = [name for name in lhs._column_names if name in right_names] |
| # the intersection of columns in both frames, in left-frame | ||
| # column order like pandas (a set here would make the key |
There was a problem hiding this comment.
question: Is is always in left-frame column order regardless of the merge how method?
There was a problem hiding this comment.
Yes — pandas infers the common keys from the left frame's column order for every how (pandas 3.0.3, left.columns = [b, a, v], right.columns = [a, b, w]: result columns are [b, a, v, w] for inner/left/right/outer alike).
Good catch though: checking this exposed that cuDF's how='right' operand swap made the inferred keys follow the right frame's order — both the key column order and the multi-key sort priority. Fixed in 474f7f9 by passing the inferred keys (in original-left order) explicitly through the swap and restoring pandas' column layout by label, with a parametrized test over all four how values.
| ) and lcol.dtype.kind in ("iuf") | ||
| r_num = is_dtype_obj_numeric( | ||
| rcol.dtype | ||
| ) and rcol.dtype.kind in ("iuf") |
There was a problem hiding this comment.
| ) and lcol.dtype.kind in ("iuf") | |
| r_num = is_dtype_obj_numeric( | |
| rcol.dtype | |
| ) and rcol.dtype.kind in ("iuf") | |
| ) and lcol.dtype.kind in "iuf" | |
| r_num = is_dtype_obj_numeric( | |
| rcol.dtype | |
| ) and rcol.dtype.kind in "iuf" |
| label for label in set(rlabels) if label in left_not_renamed | ||
| ) | ||
| if dups: | ||
| from pandas.errors import MergeError |
There was a problem hiding this comment.
Can we import this at the top?
There was a problem hiding this comment.
Done — moved to the module imports.
| if col.null_count == 0: | ||
| return index | ||
| col = Merge._promote_column_with_nulls(col) | ||
| return cudf.Index._from_column(col, name=None) |
There was a problem hiding this comment.
Should name=index.name to maintain the name?
There was a problem hiding this comment.
Intentional — pandas drops the name exactly when unmatched rows introduce missing values into the mapped index (and upcasts int64 to float64), which is the only case this helper rewrites; the null_count == 0 early return above keeps the name otherwise. On pandas 3.0.3:
left = pd.DataFrame({"k": [10, 20], "v": [1, 2]}, index=pd.Index([100, 200], name="lname"))
right = pd.DataFrame({"w": [4, 5, 6]}, index=pd.Index([10, 20, 30], name="ridx"))
left.merge(right, left_on="k", right_index=True, how="right").index
# Index([100.0, 200.0, nan], dtype='float64') <- name dropped, upcast
left.merge(right.iloc[:2], left_on="k", right_index=True, how="right").index
# Index([100, 200], dtype='int64', name='lname') <- fully matched: name kept* Drop redundant bool() casts on left_index/right_index. * Infer common key columns via _column_names. * Simplify dtype.kind membership checks; hoist the MergeError import. * Keep the original left frame's key order for inferred-key right merges: pandas infers the merge keys from the left frame's column order for every `how`, but the right-join operand swap made both the key column order and the multi-key sort priority follow the right frame. Pass the inferred keys explicitly through the swap and restore pandas' column layout by label (auto-detected keys are never suffixed, so labels are stable).
|
/okay to test 71278c2 |
…tion The unaligned-index binop test's pow xfail (added on main) strict-XPASSes with this branch: aligning the frames introduces missing rows, which now promote the integer operands to float64 like pandas, so pow is computed in float and no longer overflows.
…agar/cudf into merge-pandas-semantics
|
CI failure diagnosed: all Python test jobs were failing on a single strict XPASS — |
|
/okay to test 25a73ec |
|
/merge |
galipremsagar
added
the
5 - Ready to Merge
# Conflicts: # python/cudf/cudf/tests/reshape/test_merge.py
|
/okay to test c9a24c7 |
|
/merge |
1 similar comment
|
/merge |
5 participants
Description
Makes
merge/joinmatch pandas across the dtype/index/error semantics exercised bytests/reshape/merge/test_merge.pyundercudf.pandas. The changes are not gated onmode.pandas_compatible— cudf-classic now matches pandas too, and the affected cudf-classic tests are updated accordingly.Failures in
tests/reshape/merge/test_merge.pygo from 92 → 13 undercudf.pandas; the full cudf-classic suite passes (80k+ tests viapytest -n 12 --dist=worksteal, 0 regressions), along with the dask_cudf and custreamz suites.What changed
pandas.errors.MergeErrorwhen suffixing introduces a duplicate label not already present in the inputs (previously silently dropped a column).left_index/right_indexmust be bool →ValueError.ValueError("You are trying to merge on ... use pd.concat") instead of silently coercing.*_indexflag, or the same index level is used as key on both sides (on=). An index level used vialeft_on/right_on, or a plain column merge, yields a defaultRangeIndex.left_on+right_index), drop the index name and upcast a numpy integer index/column tofloat64(numpy has no integer NA sentinel). The upcast applies only when the merge itself introduces the missing values: each side's gather map is checked for unmatched entries, so a column that merely carried cudf-native nulls into the merge (which pandas cannot represent in a numpy int column) keeps its integer dtype — as does every column of a fully-matched join.Keeping merge semantics out of internal operations
Several cudf operations are implemented on top of
DataFrame.merge/join— binop index alignment,reindex,.locsetitem value alignment, DataFrame construction from Series,concat(axis=1)— but pandas' merge dtype rules don't apply to those operations in pandas itself (e.g.reindex(..., fill_value=0)keepsint64, and misaligned binops are documented in cudf to produce nullable ints, notfloat64). Rather than gating merge behavior, the internal helpers no longer route data columns through the merge machinery:_align_to_indexand_reindexnow join indexes only (with a positional iota column) and gather the data columns natively afterwards, so column dtypes are untouched by alignment;_reindex's own pandas dtype rules (Align DataFrame.reindex dtype and validation behavior with pandas #22914) remain the single source of reindex dtype behavior._indices_from_labelsrestores the positional column's integer dtype after its label join (row positions round-trip float64 losslessly).concat's combined-index computation no longer sorts categorical unions — pandas' union of categoricals decategorizes, so sorting would order lexically instead of by appearance likepd.concatdoes.dask_cudf'stest_merging_categorical_columnsexpectation is updated to the pandas-matching result (merging on categorical keys with different category sets decategorizes; verified identical to dask-with-pandas, including the dtype-mismatchUserWarning).Not addressed (inherent / out of scope, kept in the xfail list)
test_merge_nocopy(cudf can't share memory),test_merge_left_empty_right_notempty(None-vs-NaN),test_merge_incompat_dtypes_are_ok(cudf represents strings asStringDtype, not numpyobject), theIntervalIndexmapped-index case, outer-merge row ordering (intc/uintc), theSeries-with-tuple-name nlevels check,test_merge_right_left_index(an extra materialized key column), and a tz DST / timedelta case.Checklist