Fix merge with emtpy left DataFrame
#9578
Conversation
There was a problem hiding this comment.
Thanks @ian-r-rose! I left a few small comments / questions, but overall this looks good
| empty_index_dtype = result_meta.index.dtype | ||
| categorical_columns = result_meta.select_dtypes(include="category").columns |
There was a problem hiding this comment.
Just to clarify, this isn't a functional change -- it's just cleanup. Instead of computing and passing around empty_index_dtype and categorical_columns as kwargs, we're passing result_meta around and computing empty_index_dtype and categorical_columns once in the place it's used. Correct?
There was a problem hiding this comment.
That's right, it's just consolidation of that logic. Now that I'm passing in result_meta directly to the chunk function, these derived-from-meta parameters can all just be computed once.
|
|
||
| merged = dd_left.merge(dd_right, on="a", how=how) | ||
| expected = left.merge(right, on="a", how=how) | ||
| assert_eq(merged, expected, check_index=False) |
There was a problem hiding this comment.
Do we expect this to fail on main? Running this test locally on main this assert_eq passes
There was a problem hiding this comment.
That's correct, this doesn't fail on main, but the next line does.
There was a problem hiding this comment.
I see. In that case would you mind adding a comment that merge works, but subsequent operation may fail due to meta mismatches? This is a subtle enough point that I think a bit more context would be nice for future readers
There was a problem hiding this comment.
It may even be possible to construct a case where merged does not provide the correct answer (it probably depends on the order of concatenation of the result). I'll poke at it, and add a comment if I can't find anything
There was a problem hiding this comment.
Okay, I tweaked the test data, and now it is covered by this line as well
There was a problem hiding this comment.
Even better -- thanks @ian-r-rose
| # Workaround for pandas bug where if the left frame of a merge operation is | ||
| # empty, the resulting dataframe can have columns in the wrong order. | ||
| # https://github.com/pandas-dev/pandas/issues/9937 |
There was a problem hiding this comment.
"Thanks for writing this detailed comment"
- Future me
jrbourbeau
changed the title
merge with emtpy left DataFrame
Fixes #9577. The issue over there is that when the left df in a merge is empty, pandas can give back columns in the wrong order, resulting in a meta mismatch. This just identifies that case and re-orders the columns.
pre-commit run --all-files