[GitHub] [druid] jon-wei commented on a change in pull request #9760: Fix potential NPEs in joins

[GitBox <gi...@apache.org>]

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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org