jon-wei commented on a change in pull request #9760:
URL: https://github.com/apache/druid/pull/9760#discussion_r416975593
##########
File path:
processing/src/main/java/org/apache/druid/segment/join/JoinConditionAnalysis.java
##########
@@ -79,7 +79,7 @@ private JoinConditionAnalysis(
.allMatch(expr
-> expr.isLiteral() && expr.eval(
ExprUtils.nilBindings()).asBoolean());
canHashJoin = nonEquiConditions.stream().allMatch(Expr::isLiteral);
- rightKeyColumns =
getEquiConditions().stream().map(Equality::getRightColumn).distinct().collect(Collectors.toSet());
+ rightKeyColumns =
getEquiConditions().stream().map(Equality::getRightColumn).collect(Collectors.toSet());
Review comment:
What's the impact of removing the `distinct()` call (is it just
unnecessary since they're being collected to a set already?)
##########
File path:
processing/src/main/java/org/apache/druid/segment/join/PossiblyNullDimensionSelector.java
##########
@@ -138,7 +138,8 @@ public int lookupId(@Nullable String name)
// id 0 is always null for this selector impl.
return 0;
} else {
- return baseSelector.idLookup().lookupId(name) + nullAdjustment;
+ IdLookup idLookup = baseSelector.idLookup();
+ return (idLookup == null ? 0 : idLookup.lookupId(name)) + nullAdjustment;
Review comment:
A null `idLookup` there should be an error condition, since the caller
should check if `idLookup()` returned null before calling `lookupId(name)` on
the `PossiblyNullDimensionSelector`
##########
File path:
processing/src/main/java/org/apache/druid/segment/join/lookup/LookupJoinable.java
##########
@@ -95,18 +96,23 @@ public JoinMatcher makeJoinMatcher(
boolean allowNonKeyColumnSearch
)
{
+ if (!ALL_COLUMNS.contains(searchColumnName) ||
!ALL_COLUMNS.contains(retrievalColumnName)) {
+ return ImmutableSet.of();
+ }
Set<String> correlatedValues;
if (LookupColumnSelectorFactory.KEY_COLUMN.equals(searchColumnName)) {
if (LookupColumnSelectorFactory.KEY_COLUMN.equals(retrievalColumnName)) {
correlatedValues = ImmutableSet.of(searchColumnValue);
} else {
- correlatedValues = ImmutableSet.of(extractor.apply(searchColumnName));
+ // This should not happen in practice because the column to be joined
on must be a key.
+ correlatedValues =
Collections.singleton(extractor.apply(searchColumnValue));
Review comment:
:+1:
----------------------------------------------------------------
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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
