-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Verify that table joins use the key in the join criterion #1718
Conversation
Should this target |
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.
LGTM. Left couple of comments.
@@ -481,6 +483,44 @@ public void shouldNotPerformJoinIfInputPartitionsMisMatch() { | |||
assertEquals(JoinNode.JoinType.OUTER, joinNode.getJoinType()); | |||
} | |||
|
|||
@SuppressWarnings("unchecked") | |||
@Test | |||
public void shouldFailJoinIfTableCriteriaColumnIsNotKey() { |
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.
Did you mean join criteria column
?
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.
No I mean the table's column in the join criteria.
String.format( | ||
"Source table key column (%s) is not the column used in the join criteria (%s).", | ||
schemaKStream.getKeyField().name(), | ||
keyFieldName |
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.
Can we also include the table alias in the message. It would make it less ambiguous if we have columns with the same name in both sides of the join.
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.
LGTM, assuming the comments are addressed.
Also, I think this should be merged to 5.0.x, it is a correctness bug with joins that should be fixed.
left, | ||
right, | ||
leftKeyFieldName, | ||
"COL0", |
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.
Probably add a comment in this test and the other that this should be COL1
.
Currently this targets the branch for #1697, since it depends on the function added there for field name comparison. Once that PR gets merged I'll update this to target 5.0.x |
f5ec518
to
15ab445
Compare
553363a
to
786d3e8
Compare
e1b81bf
to
5024527
Compare
Description
Verify that table joins use the key in the join criterion
Fixes #1712
Testing done
Added unit tests to JoinNodeTest
Reviewer checklist