-
-
Notifications
You must be signed in to change notification settings - Fork 19.4k
BUG: Raise TypeError when joining with non-DataFrame using 'on=' (GH#61434) #61454
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
78fbc2c to
b604714
Compare
|
All checks passed except the Pyodide build, which failed due to a rate-limit (HTTP 429). The failure seems unrelated to this PR. A rerun should resolve it |
b604714 to
71eb2e7
Compare
| if isinstance(other, Iterable) and not isinstance( | ||
| other, (DataFrame, Series, str, bytes, bytearray) | ||
| ): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if isinstance(other, Iterable) and not isinstance( | |
| other, (DataFrame, Series, str, bytes, bytearray) | |
| ): | |
| if is_list_like(other): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, this probably should be done in merge
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mroeschke - is_list_like will return True on a DataFrame. We only want to enter this block on a potential sequence of DataFrame/Series.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah OK ignore my suggestion then. But I believe this check should still be done in merge probably
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah OK ignore my suggestion then. But I believe this check should still be done in
mergeprobably
ValueError is raised in join() before the call reaches merge when on is specified. Would you prefer that I let these inputs flow into merge and move the check there for consistency?
71eb2e7 to
6789fb6
Compare
6789fb6 to
c9ac591
Compare
|
This pull request is stale because it has been open for thirty days with no activity. Please update and respond to this comment if you're still interested in working on this. |
|
@mroeschke @rhshadrach can you please take a look at this? |
|
Thanks for the pull request, but it appears to have gone stale. If interested in continuing, please merge in the main branch, address any review comments and/or failing tests, and we can reopen. |
Closes GH#61434
What does this PR change?
When using
DataFrame.join()with theonparameter, passing an invalid object like adict,int, or third-party DataFrame previously resulted in unclear internal errors.This PR adds a minimal type check that raises a clear
TypeErrorwhenotheris not aDataFrame,Series, or a list of such objects. Valid list-based joins withoutonremain unaffected.Checklist
pre-commit run --all-filesdoc/source/whatsnew/v3.0.0.rst