-
Notifications
You must be signed in to change notification settings - Fork 43
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
fix: only do row identity based joins when joining by index #356
Conversation
🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use -- conventional-commit-lint bot |
@TrevorBergeron can you take a look? |
@@ -64,6 +64,8 @@ def join_by_column_ordered( | |||
.equals(right._get_ibis_column(rcol).name("index")) | |||
for lcol, rcol in join.conditions | |||
) | |||
and not left._predicates |
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.
This would likely be a regression with regards to our SQL size. We want to simulate a join in this case. I'll take a look at join_by_row_identity_ordered
and join_by_row_identity_unordered
to see if there has been a regression there.
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.
any update on this?
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.
Hi @DLT1412 Sorry for the wait. I was OOO for a couple weeks to get married. I hope to look at this more deeply later this week. Thanks again for raising this issue.
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.
I'm able to reproduce this issue locally with inner, outer, and left join. Will look more tomorrow.
76b252f
into
googleapis:main
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #355 🦕