You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2020/01/29 19:02:31 UTC
[GitHub] [druid] suneet-s opened a new pull request #9287: Add
getRightColumns to JoinConditionAnalysis
suneet-s opened a new pull request #9287: Add getRightColumns to JoinConditionAnalysis
URL: https://github.com/apache/druid/pull/9287
This change other implementations of JoinableFactory to ask the analysis
for the right key columns instead of having to calculate it themselves.
This PR has:
- [x] been self-reviewed.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/licenses.yaml)
- [x] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [x] added unit tests or modified existing tests to cover new code paths.
- [ ] added integration tests.
- [ ] been tested in a test Druid cluster.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on a change in pull request #9287: Add
getRightColumns to JoinConditionAnalysis
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #9287: Add getRightColumns to JoinConditionAnalysis
URL: https://github.com/apache/druid/pull/9287#discussion_r372653362
##########
File path: server/src/main/java/org/apache/druid/segment/join/InlineJoinableFactory.java
##########
@@ -39,8 +38,7 @@
{
if (condition.canHashJoin() && dataSource instanceof InlineDataSource) {
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();
Review 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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on a change in pull request #9287: Add
getRightColumns to JoinConditionAnalysis
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #9287: Add getRightColumns to JoinConditionAnalysis
URL: https://github.com/apache/druid/pull/9287#discussion_r372694314
##########
File path: processing/src/test/java/org/apache/druid/segment/join/JoinConditionAnalysisTest.java
##########
@@ -60,6 +61,7 @@ public void test_forExpression_simple()
ImmutableList.of(),
exprsToStrings(analysis.getNonEquiConditions())
);
+ Assert.assertThat(analysis.getRightKeyColumns(), CoreMatchers.is(ImmutableList.of("y")));
Review 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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on a change in pull request #9287: Add
getRightColumns to JoinConditionAnalysis
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #9287: Add getRightColumns to JoinConditionAnalysis
URL: https://github.com/apache/druid/pull/9287#discussion_r372598548
##########
File path: processing/src/test/java/org/apache/druid/segment/join/JoinConditionAnalysisTest.java
##########
@@ -271,7 +283,7 @@ public void test_equals()
// These fields are tightly coupled with originalExpression
"equiConditions", "nonEquiConditions",
// These fields are calculated from nonEquiConditions
- "isAlwaysTrue", "isAlwaysFalse", "canHashJoin")
+ "isAlwaysTrue", "isAlwaysFalse", "canHashJoin", "rightKeyColumns")
Review comment:
The comment above is incorrect now. Actually, it was incorrect before too, since some of these fields are based on equiConditions as well.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on a change in pull request #9287: Add
getRightEquiConditionKeys to JoinConditionAnalysis
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #9287: Add getRightEquiConditionKeys to JoinConditionAnalysis
URL: https://github.com/apache/druid/pull/9287#discussion_r372777563
##########
File path: processing/src/test/java/org/apache/druid/segment/join/JoinConditionAnalysisTest.java
##########
@@ -220,6 +229,7 @@ public void test_forExpression_onlyRight()
ImmutableList.of(),
exprsToStrings(analysis.getNonEquiConditions())
);
+ Assert.assertEquals(analysis.getRightEquiConditionKeys(), ImmutableSet.of("x"));
Review comment:
These are backwards. (I think putting actual before expected is more natural, but JUnit disagrees and who am I to judge?)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on a change in pull request #9287: Add
getRightColumns to JoinConditionAnalysis
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #9287: Add getRightColumns to JoinConditionAnalysis
URL: https://github.com/apache/druid/pull/9287#discussion_r372627563
##########
File path: processing/src/test/java/org/apache/druid/segment/join/JoinConditionAnalysisTest.java
##########
@@ -60,6 +61,7 @@ public void test_forExpression_simple()
ImmutableList.of(),
exprsToStrings(analysis.getNonEquiConditions())
);
+ Assert.assertThat(analysis.getRightKeyColumns(), CoreMatchers.is(ImmutableList.of("y")));
Review comment:
assertEquals fails because getRightKeyColumns doesn't return an ImmutableList
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on a change in pull request #9287: Add
getRightColumns to JoinConditionAnalysis
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #9287: Add getRightColumns to JoinConditionAnalysis
URL: https://github.com/apache/druid/pull/9287#discussion_r372692912
##########
File path: server/src/main/java/org/apache/druid/segment/join/InlineJoinableFactory.java
##########
@@ -39,8 +38,7 @@
{
if (condition.canHashJoin() && dataSource instanceof InlineDataSource) {
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();
Review comment:
Ah, I just meant replacing this:
```java
} else if (!condition.getEquiConditions()
.stream()
.allMatch(eq -> eq.getRightColumn().equals(LookupColumnSelectorFactory.KEY_COLUMN))) {
```
With this:
```java
} else if (!condition.getRightEquiConditionKeys().equals(Collections.singletonSet(LookupColumnSelectorFactory.KEY_COLUMN)) {
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm merged pull request #9287: Add
getRightEquiConditionKeys to JoinConditionAnalysis
Posted by GitBox <gi...@apache.org>.
gianm merged pull request #9287: Add getRightEquiConditionKeys to JoinConditionAnalysis
URL: https://github.com/apache/druid/pull/9287
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on a change in pull request #9287: Add
getRightColumns to JoinConditionAnalysis
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #9287: Add getRightColumns to JoinConditionAnalysis
URL: https://github.com/apache/druid/pull/9287#discussion_r372643401
##########
File path: processing/src/main/java/org/apache/druid/segment/join/JoinConditionAnalysis.java
##########
@@ -176,6 +179,14 @@ public boolean canHashJoin()
return canHashJoin;
}
+ /**
+ * Returns the distinct column keys from the RHS required to evaluate the equi conditions.
+ */
+ public List<String> getRightKeyColumns()
+ {
Review comment:
I like the suggestion. Done.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on a change in pull request #9287: Add
getRightColumns to JoinConditionAnalysis
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #9287: Add getRightColumns to JoinConditionAnalysis
URL: https://github.com/apache/druid/pull/9287#discussion_r372692289
##########
File path: processing/src/test/java/org/apache/druid/segment/join/JoinConditionAnalysisTest.java
##########
@@ -60,6 +61,7 @@ public void test_forExpression_simple()
ImmutableList.of(),
exprsToStrings(analysis.getNonEquiConditions())
);
+ Assert.assertThat(analysis.getRightKeyColumns(), CoreMatchers.is(ImmutableList.of("y")));
Review comment:
How can that be? `CoreMatchers.is` is calling `actual.equals(expected)`, with extra steps.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on a change in pull request #9287: Add
getRightColumns to JoinConditionAnalysis
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #9287: Add getRightColumns to JoinConditionAnalysis
URL: https://github.com/apache/druid/pull/9287#discussion_r372599360
##########
File path: processing/src/main/java/org/apache/druid/segment/join/JoinConditionAnalysis.java
##########
@@ -176,6 +179,14 @@ public boolean canHashJoin()
return canHashJoin;
}
+ /**
+ * Returns the distinct column keys from the RHS required to evaluate the equi conditions.
+ */
+ public List<String> getRightKeyColumns()
+ {
Review comment:
Btw, a Set may be more appropriate than a List here.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on a change in pull request #9287: Add
getRightEquiConditionKeys to JoinConditionAnalysis
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #9287: Add getRightEquiConditionKeys to JoinConditionAnalysis
URL: https://github.com/apache/druid/pull/9287#discussion_r372777621
##########
File path: processing/src/test/java/org/apache/druid/segment/join/JoinConditionAnalysisTest.java
##########
@@ -270,8 +282,8 @@ public void test_equals()
.withIgnoredFields(
// These fields are tightly coupled with originalExpression
"equiConditions", "nonEquiConditions",
- // These fields are calculated from nonEquiConditions
- "isAlwaysTrue", "isAlwaysFalse", "canHashJoin")
+ // These fields are calculated from other other fields in the class
Review comment:
Typo (other other)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on a change in pull request #9287: Add
getRightColumns to JoinConditionAnalysis
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #9287: Add getRightColumns to JoinConditionAnalysis
URL: https://github.com/apache/druid/pull/9287#discussion_r372599030
##########
File path: server/src/main/java/org/apache/druid/segment/join/InlineJoinableFactory.java
##########
@@ -39,8 +38,7 @@
{
if (condition.canHashJoin() && dataSource instanceof InlineDataSource) {
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();
Review comment:
You could probably do a similar simplification in LookupJoinMatcher (there's a part that checks that all the equikeys are the key column).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on a change in pull request #9287: Add
getRightColumns to JoinConditionAnalysis
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #9287: Add getRightColumns to JoinConditionAnalysis
URL: https://github.com/apache/druid/pull/9287#discussion_r372597870
##########
File path: processing/src/main/java/org/apache/druid/segment/join/JoinConditionAnalysis.java
##########
@@ -176,6 +179,14 @@ public boolean canHashJoin()
return canHashJoin;
}
+ /**
+ * Returns the distinct column keys from the RHS required to evaluate the equi conditions.
+ */
+ public List<String> getRightKeyColumns()
+ {
Review 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`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on a change in pull request #9287: Add
getRightColumns to JoinConditionAnalysis
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #9287: Add getRightColumns to JoinConditionAnalysis
URL: https://github.com/apache/druid/pull/9287#discussion_r372597981
##########
File path: processing/src/test/java/org/apache/druid/segment/join/JoinConditionAnalysisTest.java
##########
@@ -60,6 +61,7 @@ public void test_forExpression_simple()
ImmutableList.of(),
exprsToStrings(analysis.getNonEquiConditions())
);
+ Assert.assertThat(analysis.getRightKeyColumns(), CoreMatchers.is(ImmutableList.of("y")));
Review comment:
Is this better than `Assert.assertEquals`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org