Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/21239 )
Change subject: IMPALA-13043: Implement Join Capability to the Calcite Planner ...................................................................... Patch Set 15: (12 comments) http://gerrit.cloudera.org:8080/#/c/21239/15/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/AnalyzedFunctionCallExpr.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/AnalyzedFunctionCallExpr.java: http://gerrit.cloudera.org:8080/#/c/21239/15/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/AnalyzedFunctionCallExpr.java@54 PS15, Line 54: // c'tor which does not depend on Calcite's RexCall but is used when Impala's This description doesn't make as much sense to me when you just removed RexCall from the other constructor. http://gerrit.cloudera.org:8080/#/c/21239/15/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/RexCallConverter.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/RexCallConverter.java: http://gerrit.cloudera.org:8080/#/c/21239/15/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/RexCallConverter.java@89 PS15, Line 89: assert params.size() == 2; We usually use Preconditions.checkArgument. http://gerrit.cloudera.org:8080/#/c/21239/15/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/RexLiteralConverter.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/RexLiteralConverter.java: http://gerrit.cloudera.org:8080/#/c/21239/15/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/RexLiteralConverter.java@42 PS15, Line 42: import org.slf4j.Logger; Why are these split from the other "import org" statements? http://gerrit.cloudera.org:8080/#/c/21239/15/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaJoinRel.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaJoinRel.java: http://gerrit.cloudera.org:8080/#/c/21239/15/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaJoinRel.java@149 PS15, Line 149: analyzer.registerConjuncts(getJoinConjunctListToRegister(nonEquiJoinConjuncts)); Why do we skip registering nonEquiJoinConjuncts if equiJoinConjuncts is empty? http://gerrit.cloudera.org:8080/#/c/21239/15/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaJoinRel.java@206 PS15, Line 206: input.planNode_.getTupleIds()) nit: There's enough space for this to appear on the previous line. We wrap at 90 characters. http://gerrit.cloudera.org:8080/#/c/21239/15/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaJoinRel.java@261 PS15, Line 261: // Right Semi Join Does Calcite simply not create Right Anti Joins? It's possible to declare a RIGHT ANTI JOIN, so what does Calcite map that to? http://gerrit.cloudera.org:8080/#/c/21239/15/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaJoinRel.java@304 PS15, Line 304: * An example query where we the casting adjustment is needed within the Impala test nit: extra "we", can just say "where we" http://gerrit.cloudera.org:8080/#/c/21239/15/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaJoinRel.java@331 PS15, Line 331: Preconditions.checkState(joinConjunct.getChildren().size() > 0); Shouldn't we check that this is == 2? http://gerrit.cloudera.org:8080/#/c/21239/15/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaJoinRel.java@348 PS15, Line 348: return Lists.newArrayList(joinConjunct.getChild(0), joinConjunct.getChild(1)); I'm not clear how this is only returning the LHS. http://gerrit.cloudera.org:8080/#/c/21239/15/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaJoinRel.java@448 PS15, Line 448: // check gets bypassed for some tests run through the test framework which explicitly nit: extra whitespace at the end http://gerrit.cloudera.org:8080/#/c/21239/15/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/NodeCreationUtils.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/NodeCreationUtils.java: http://gerrit.cloudera.org:8080/#/c/21239/15/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/NodeCreationUtils.java@57 PS15, Line 57: return new NodeWithExprs(selectNode, nodeWithExprs.outputExprs_, Could this use the simpler call? NodeWithExprs(selectNode, nodeWithExprs) http://gerrit.cloudera.org:8080/#/c/21239/15/testdata/workloads/functional-query/queries/QueryTest/calcite.test File testdata/workloads/functional-query/queries/QueryTest/calcite.test: http://gerrit.cloudera.org:8080/#/c/21239/15/testdata/workloads/functional-query/queries/QueryTest/calcite.test@459 PS15, Line 459: # left outer join test Should we test every type of join? Or leaving that for when we can enable a larger set of tests? -- To view, visit http://gerrit.cloudera.org:8080/21239 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5db097577907d79877f52feff2922000af074ecd Gerrit-Change-Number: 21239 Gerrit-PatchSet: 15 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-Comment-Date: Wed, 28 Aug 2024 19:01:03 +0000 Gerrit-HasComments: Yes
