Add getRightEquiConditionKeys to JoinConditionAnalysis#9287
Add getRightEquiConditionKeys to JoinConditionAnalysis#9287gianm merged 3 commits intoapache:masterfrom
Conversation
This change other implementations of JoinableFactory to ask the analysis for the right key columns instead of having to calculate it themselves.
| * Returns the distinct column keys from the RHS required to evaluate the equi conditions. | ||
| */ | ||
| public List<String> getRightKeyColumns() | ||
| { |
There was a problem hiding this comment.
I'm not sure if it's clear that this method only applies to the equi-conditions. The javadoc explains it but the method name might be misleading? What do you think?
I don't know if I have a good solution, btw. getRightKeyColumnsForEquiConditions is clear but quite a mouthful. Maybe getRightEquiConditionKeys.
There was a problem hiding this comment.
Btw, a Set may be more appropriate than a List here.
There was a problem hiding this comment.
I like the suggestion. Done.
| ImmutableList.of(), | ||
| exprsToStrings(analysis.getNonEquiConditions()) | ||
| ); | ||
| Assert.assertThat(analysis.getRightKeyColumns(), CoreMatchers.is(ImmutableList.of("y"))); |
There was a problem hiding this comment.
Is this better than Assert.assertEquals?
There was a problem hiding this comment.
assertEquals fails because getRightKeyColumns doesn't return an ImmutableList
There was a problem hiding this comment.
How can that be? CoreMatchers.is is calling actual.equals(expected), with extra steps.
There was a problem hiding this comment.
I definitely saw the test fail which is why I had to google how to fix this - and I found CoreMatchers.
hahaha I think I know why now -
Assert.assertEquals checks expected.equals(actual)
CoreMatchers.is checks actual.equals(expected)
That probably means the equals implementation for one (or both?) of those classes isn't correct according to the javadocs
| final InlineDataSource inlineDataSource = (InlineDataSource) dataSource; | ||
| final List<String> rightKeyColumns = | ||
| condition.getEquiConditions().stream().map(Equality::getRightColumn).distinct().collect(Collectors.toList()); | ||
| final List<String> rightKeyColumns = condition.getRightKeyColumns(); |
There was a problem hiding this comment.
You could probably do a similar simplification in LookupJoinMatcher (there's a part that checks that all the equikeys are the key column).
There was a problem hiding this comment.
It's not immediately obvious to me how other implementations would use what LookupJoinMatcher is doing with the equiConditions. If it's ok with you, I'd like to do that refactoring at a later time
There was a problem hiding this comment.
Ah, I just meant replacing this:
} else if (!condition.getEquiConditions()
.stream()
.allMatch(eq -> eq.getRightColumn().equals(LookupColumnSelectorFactory.KEY_COLUMN))) {With this:
} else if (!condition.getRightEquiConditionKeys().equals(Collections.singletonSet(LookupColumnSelectorFactory.KEY_COLUMN)) {
suneet-s
changed the title
gianm
left a comment
gianm
left a comment
There was a problem hiding this comment.
LGTM. I made two further comments but they are cosmetic. If you are doing further work in this area please consider addressing them in a later patch.
| ImmutableList.of(), | ||
| exprsToStrings(analysis.getNonEquiConditions()) | ||
| ); | ||
| Assert.assertEquals(analysis.getRightEquiConditionKeys(), ImmutableSet.of("x")); |
There was a problem hiding this comment.
These are backwards. (I think putting actual before expected is more natural, but JUnit disagrees and who am I to judge?)
| "equiConditions", "nonEquiConditions", | ||
| // These fields are calculated from nonEquiConditions | ||
| "isAlwaysTrue", "isAlwaysFalse", "canHashJoin") | ||
| // These fields are calculated from other other fields in the class |
3 participants
This change allows other implementations of JoinableFactory to ask the analysis
for the right key columns instead of having to calculate it themselves.
This PR has: