Steve Carlin has posted comments on this change. ( http://gerrit.cloudera.org:8080/24652 )
Change subject: IMPALA-15237: Add Calcite regression harness ...................................................................... Patch Set 3: (2 comments) http://gerrit.cloudera.org:8080/#/c/24652/3/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteOptimizer.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteOptimizer.java: http://gerrit.cloudera.org:8080/#/c/24652/3/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteOptimizer.java@91 PS3, Line 91: PreImpalaConvertState preImpalaConvertState = Ok, sorry to make this correction now, but I just got to thinking... The simplifier is only needed for optimization. It isn't really needed for the final step of converting logical nodes into physical nodes. The runProgram method uses the simplifier to get the executor, but I don't think it will be necessary to set the Executor for the final step. We can put a check in there to say "if (simplifier != null) set the rexExecutor So I guess what I'm saying is that the PreImpalaConvertState class (now record) doesn't need to exist. The createPreImpalaConvertState can just return a RelNode logical plan. http://gerrit.cloudera.org:8080/#/c/24652/3/java/calcite-planner/src/test/java/org/apache/impala/calcite/service/CalciteOptimizerTest.java File java/calcite-planner/src/test/java/org/apache/impala/calcite/service/CalciteOptimizerTest.java: http://gerrit.cloudera.org:8080/#/c/24652/3/java/calcite-planner/src/test/java/org/apache/impala/calcite/service/CalciteOptimizerTest.java@140 PS3, Line 140: assertEquals(ImmutableList.of(LogicalProject.class, LogicalTableScan.class), I suppose this is fine for now, but this is gonna get ugly for longer hierarchies. I didn't check how the RelVisitor pattern works with Joins/Unions and ordinals. I'm guessing they all get visited and the order will be deterministic, but when we do want to have checks in for that, we may have to rethink this. But let's go forward with this for now. -- To view, visit http://gerrit.cloudera.org:8080/24652 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8b0a1aca63b494d9c64fa6b8e33328bedce9c6b4 Gerrit-Change-Number: 24652 Gerrit-PatchSet: 3 Gerrit-Owner: Aleksandr Efimov <[email protected]> Gerrit-Reviewer: Aleksandr Efimov <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Steve Carlin <[email protected]> Gerrit-Comment-Date: Mon, 03 Aug 2026 23:04:19 +0000 Gerrit-HasComments: Yes
