From a9b39fc29ddb34b3ac32bc9198b52ba86d8de2b8 Mon Sep 17 00:00:00 2001
From: Rohan Garg <7731512+rohangarg@users.noreply.github.com>
Date: Mon, 7 Nov 2022 23:19:18 +0530
Subject: [PATCH] Try converting all inner joins to filters (#13201)

---
 .../segment/join/JoinableFactoryWrapper.java  | 58 ++++++++++---------
 .../join/JoinableFactoryWrapperTest.java      | 37 +++++++++++-
 2 files changed, 68 insertions(+), 27 deletions(-)

diff --git a/processing/src/main/java/org/apache/druid/segment/join/JoinableFactoryWrapper.java b/processing/src/main/java/org/apache/druid/segment/join/JoinableFactoryWrapper.java
index 020d23317f7..134c9da48d7 100644
--- a/processing/src/main/java/org/apache/druid/segment/join/JoinableFactoryWrapper.java
+++ b/processing/src/main/java/org/apache/druid/segment/join/JoinableFactoryWrapper.java
@@ -79,34 +79,40 @@ public class JoinableFactoryWrapper
     }
 
     Set<String> rightPrefixes = clauses.stream().map(JoinableClause::getPrefix).collect(Collectors.toSet());
-    // Walk through the list of clauses, picking off any from the start of the list that can be converted to filters.
-    boolean atStart = true;
+    boolean isRightyJoinSeen = false;
     for (JoinableClause clause : clauses) {
-      if (atStart) {
-        // Remove this clause from columnsRequiredByJoinClauses. It's ok if it relies on itself.
-        for (String column : clause.getCondition().getRequiredColumns()) {
-          columnsRequiredByJoinClauses.remove(column, 1);
-        }
-
-        final JoinClauseToFilterConversion joinClauseToFilterConversion =
-            convertJoinToFilter(
-                clause,
-                Sets.union(requiredColumns, columnsRequiredByJoinClauses.elementSet()),
-                maxNumFilterValues,
-                rightPrefixes
-            );
-
-        // add the converted filter to the filter list
-        if (joinClauseToFilterConversion.getConvertedFilter() != null) {
-          filterList.add(joinClauseToFilterConversion.getConvertedFilter());
-        }
-        // if the converted filter is partial, keep the join clause too
-        if (!joinClauseToFilterConversion.isJoinClauseFullyConverted()) {
-          clausesToUse.add(clause);
-          atStart = false;
-        }
-      } else {
+      // Incase we find a RIGHT/OUTER join, we shouldn't convert join conditions to left column filters for any join
+      // afterwards because the values of left colmun might change to NULL after the RIGHT/OUTER join. We should only
+      // consider cases where the values of the filter columns do not change after the join.
+      isRightyJoinSeen = isRightyJoinSeen || clause.getJoinType().isRighty();
+      if (isRightyJoinSeen) {
         clausesToUse.add(clause);
+        continue;
+      }
+      // Remove this clause from columnsRequiredByJoinClauses. It's ok if it relies on itself.
+      for (String column : clause.getCondition().getRequiredColumns()) {
+        columnsRequiredByJoinClauses.remove(column, 1);
+      }
+
+      final JoinClauseToFilterConversion joinClauseToFilterConversion =
+          convertJoinToFilter(
+              clause,
+              Sets.union(requiredColumns, columnsRequiredByJoinClauses.elementSet()),
+              maxNumFilterValues,
+              rightPrefixes
+          );
+
+      // add the converted filter to the filter list
+      if (joinClauseToFilterConversion.getConvertedFilter() != null) {
+        filterList.add(joinClauseToFilterConversion.getConvertedFilter());
+      }
+      // if the converted filter is partial, keep the join clause too
+      if (!joinClauseToFilterConversion.isJoinClauseFullyConverted()) {
+        clausesToUse.add(clause);
+        // add back the required columns by this join since it wasn't converted fully
+        for (String column : clause.getCondition().getRequiredColumns()) {
+          columnsRequiredByJoinClauses.add(column, 1);
+        }
       }
     }
 
diff --git a/processing/src/test/java/org/apache/druid/segment/join/JoinableFactoryWrapperTest.java b/processing/src/test/java/org/apache/druid/segment/join/JoinableFactoryWrapperTest.java
index 3d8a01785a4..1ff3664aba4 100644
--- a/processing/src/test/java/org/apache/druid/segment/join/JoinableFactoryWrapperTest.java
+++ b/processing/src/test/java/org/apache/druid/segment/join/JoinableFactoryWrapperTest.java
@@ -506,7 +506,7 @@ public class JoinableFactoryWrapperTest extends NullHandlingTest
 
     Assert.assertEquals(
         Pair.of(
-            ImmutableList.of(),
+            ImmutableList.of(new InDimFilter("x", TEST_LOOKUP_KEYS)),
             clauses
         ),
         conversion
@@ -590,4 +590,39 @@ public class JoinableFactoryWrapperTest extends NullHandlingTest
         conversion
     );
   }
+
+  @Test
+  public void test_convertJoinsToFilters_dontConvertJoinsDependedOnByPreviousJoins()
+  {
+    // upon discovering a RIGHT/OUTER join, all conversions for subsequent joins should stop since the output of left
+    // table columns might change to NULL after the RIGHT/OUTER join.
+    final ImmutableList<JoinableClause> clauses = ImmutableList.of(
+        new JoinableClause(
+            "j.",
+            LookupJoinable.wrap(new MapLookupExtractor(TEST_LOOKUP, false)),
+            JoinType.RIGHT,
+            JoinConditionAnalysis.forExpression("x == \"j.k\"", "j.", ExprMacroTable.nil())
+        ),
+        new JoinableClause(
+            "_j.",
+            LookupJoinable.wrap(new MapLookupExtractor(TEST_LOOKUP, false)),
+            JoinType.INNER,
+            JoinConditionAnalysis.forExpression("x == \"_j.k\"", "_j.", ExprMacroTable.nil())
+        )
+    );
+
+    final Pair<List<Filter>, List<JoinableClause>> conversion = JoinableFactoryWrapper.convertJoinsToFilters(
+        clauses,
+        ImmutableSet.of("x"),
+        Integer.MAX_VALUE
+    );
+
+    Assert.assertEquals(
+        Pair.of(
+            ImmutableList.of(),
+            clauses
+        ),
+        conversion
+    );
+  }
 }