Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/21498 )
Change subject: IMPALA-12871: Added filtering capability for Calcite planner ...................................................................... Patch Set 1: Code-Review+1 (2 comments) This looks good to me I think we're close. Here are the fixes that I'd like to see: - Update the JIRA on the commit message to point to subtask rather than the epic - Add a test case for the filter including a column not referenced by the select list - Fix the long line http://gerrit.cloudera.org:8080/#/c/21498/1/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaFilterRel.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaFilterRel.java: http://gerrit.cloudera.org:8080/#/c/21498/1/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaFilterRel.java@79 PS1, Line 79: // need to set the inputRefs. The HdfsScan RelNode needs to know which columns are : // needed from the table in order to implement the filter condition. The input ref : // used here may nor may not be projected out. So a union needs to be done with : // potential existing projected input refs from a parent RelNode. : // Note that if the parent RelNode hasn't set any input refs, it is assumed that all : // input refs are needed (the default case when inputRefs_ is null). > Correct Ok, can we add that as a test case? http://gerrit.cloudera.org:8080/#/c/21498/1/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaFilterRel.java@98 PS1, Line 98: List<RexNode> conditions = ImmutableList.of(previousCondition, newCondition); : return RexUtil.composeConjunction(getCluster().getRexBuilder(), conditions); > Unfortunately, this will not work right now. This works in a future commit. Ok, no problem, we can add tests later. -- To view, visit http://gerrit.cloudera.org:8080/21498 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If104bf1cd801d5ee92dd7e43d398a21a18be5d97 Gerrit-Change-Number: 21498 Gerrit-PatchSet: 1 Gerrit-Owner: Steve Carlin <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Steve Carlin <[email protected]> Gerrit-Comment-Date: Tue, 18 Jun 2024 16:33:13 +0000 Gerrit-HasComments: Yes
