[AI draft] Fix merge index for rows without a match - needs human review - #12595
Draft
thatssoheil wants to merge 2 commits into
Draft
thatssoheil wants to merge 2 commits into
thatssoheil wants to merge 2 commits into
Conversation
A merge that keys off one frame's index (left_index with right_on, or right_index with left_on) takes the result index from the other frame's index, and reports a missing value for rows that found no match. When that other frame's partition is empty, pandas keeps the index of the frame it does have instead, so dask - which merges partition by partition - reported left-frame index values for those rows, and the result changed with the partitioning of the inputs (dask#12564). merge_chunk now reports a missing index for every row whose merge side is empty, which is the value pandas reports whenever a non-empty frame has no match for the row. The index dtype follows the dtype the merge declares when it can hold a missing value, and falls back to float64 when it cannot (bool would otherwise become True). A frame that is empty as a whole is not partitioning-dependent, so pandas' behaviour there is deliberately not kept, to leave one rule; the test for it pins that deviation instead of asserting pandas equality. Tests: both merge directions, each asserting the index reported for every row against pandas; a right frame whose index is the merge column; a right frame laid out in a single partition, to show the result no longer depends on partitioning; and the dtype fallback. Assisted-by: Hermes Agent (deepseek-v4-flash-vision-exp)
Contributor
Unit Test ResultsSee test report for an extended history of previous test failures. This is useful for diagnosing flaky tests. 25 files ± 0 25 suites ±0 6h 50m 32s ⏱️ - 26m 19s Results for commit 22f75a5. ± Comparison against base commit 9dc535d. ♻️ This comment has been updated with latest results. |
The dtype test asserted that _missing_index(2, "str").dtype == "str", which only holds on pandas 3: before that "str" resolves to object, so all twelve CI test jobs failed on this one assertion while the other three new tests passed. Ask pandas which dtype it resolves to instead of pinning either convention, so the test states the contract (a missing index keeps the dtype a non-missing index of that dtype would have) on both. Verified on pandas 2.1.4 and pandas 3.0.5: the focused test is RED with the old assertion and GREEN with this one, and test_multi.py is 177 passed / 31 skipped / 6 xfailed / 2 xpassed on pandas 2.1.4 and 177 passed / 31 skipped / 8 xfailed on 3.0.5. Assisted-by: Hermes Agent (deepseek-v4-flash-vision-exp)
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Unsupervised AI draft. This PR was opened by an unattended AI agent (Hermes Agent, model deepseek-v4-flash-vision-exp) and its prose has not been reviewed by a human yet, per dask's AI contribution policy. Treat it as a draft for discussion, not as finished work. Do not merge before a human has reviewed and rewritten it.
Fixes #12564.
What the issue reports
A left merge that keys off the left frame's index (
left_index=True, right_on="id") returns matched rows indexed by the right frame and the unmatched row indexed by the left frame, and the result changes with the right frame's partitioning.Cause
Pandas keeps the index of the frame it still has when the frame merged against a row is empty:
lhs.merge(rhs_empty, how="left", left_index=True, right_on="id")returns the LEFT index, while a non-empty right frame that has no match for a row reports a missing value for that row. Dask merges partition by partition, so a row whose right partition happened to be empty kept the left index, while the same row opposite a non-empty right partition took the right frame's index. Which index a row got therefore depended on how the right frame was partitioned.Change
merge_chunknow reports a missing index for every row whose merge side is empty, in either direction (left_indexwithright_on, orright_indexwithleft_on), using the dtype the merge declares when that dtype can hold a missing value and falling back tofloat64where it cannot hold one (boolwould otherwise come back asTrue). One rule: a row without a match has no index.One deliberate deviation, for maintainers to call
When the other frame is empty as a whole rather than in a single partition, pandas keeps the other frame's index and dask did too. That case on its own is not partitioning-dependent, so this PR changes it as well, to leave a single rule.
test_merge_with_empty_other_side_reports_missing_indexpins that deviation rather than asserting pandas equality. The same choice shows up forhow="outer"with a wholly empty side. If you would rather keep pandas' behaviour for a wholly empty frame, this shrinks to restricting the condition, and I am happy to rework it.Tests
python -m pytest dask/dataframe/tests/test_multi.py -qgives177 passed, 31 skipped, 8 xfailed; with the source change reverted the four new tests fail (4 failed, 173 passed, 31 skipped, 8 xfailed). Coverage: the issue's repro compared row by row against pandas (plus the same right frame laid out as a single partition), the mirrored right-index direction, a right frame whose index is the merge column, and the dtype fallback.Assisted-by: Hermes Agent (deepseek-v4-flash-vision-exp)