[GitHub] [druid] ccaominh commented on a change in pull request #10056: Ensure that join filter pre-analysis operates on optimized filters

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

ccaominh commented on a change in pull request #10056:
URL: https://github.com/apache/druid/pull/10056#discussion_r443886344



##########
File path: processing/src/main/java/org/apache/druid/segment/join/Joinables.java
##########
@@ -107,8 +107,17 @@ public static boolean isPrefixedBy(final String columnName, final String prefix)
             );
 
             for (Query joinQuery : joinQueryLevels) {
+              // The pre-analysis needs to apply to the optimized form of filters, as this is what will be
+              // passed to HashJoinSegmentAdapter.makeCursors().
+              // The optimize() call here means that the filter optimization will happen twice,
+              // since the query toolchests will call optimize() later.
+              // We do this for simplicity as we cannot override what query will get run later from this context.
+              // A more complicated approach involving wrapping the query runner after the pre-merge decoration
+              // and moving the pre-analysis might be viable.
+              // The additional overhead of the simple approach should be low, as the additional optimize() call
+              // on each filter will only occur once per query.
               preAnalysisGroup.computeJoinFilterPreAnalysisIfAbsent(
-                  joinQuery.getFilter() == null ? null : joinQuery.getFilter().toFilter(),
+                  joinQuery.getFilter() == null ? null : joinQuery.getFilter().optimize().toFilter(),

Review comment:
       The code coverage checker is flagging this change as uncovered (by the druid-processing unit tests):
   https://travis-ci.org/github/apache/druid/jobs/699956397#L1415
   
   Note: The new `CalciteQueryTest` added in this PR to druid-sql does hit this code path. 

##########
File path: sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java
##########
@@ -11987,6 +11992,83 @@ public void testNestedGroupByOnInlineDataSourceWithFilter(Map<String, Object> qu
     );
   }
 
+  @Test
+  @Parameters(source = QueryContextForJoinProvider.class)
+  public void testGroupByJoinAsNativeQueryWithUnoptimizedFilter(Map<String, Object> queryContext) throws Exception

Review comment:
       Intellij inspection check is flagging this line since `Exception` is not thrown by the method body:
   https://travis-ci.org/github/apache/druid/jobs/699956394#L11326




----------------------------------------------------------------
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