Steve Carlin has posted comments on this change. ( http://gerrit.cloudera.org:8080/24652 )
Change subject: IMPALA-15237: Add Calcite regression harness ...................................................................... Patch Set 2: (5 comments) http://gerrit.cloudera.org:8080/#/c/24652/2/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/2/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteOptimizer.java@119 PS2, Line 119: @VisibleForTesting I think it's better to move this into CalciteOptimizerTest and just make createPreImpalaConvertPlan as "VisibleForTesting" (and package-private) http://gerrit.cloudera.org:8080/#/c/24652/2/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteOptimizer.java@198 PS2, Line 198: static final class TestOptimizationResult { Same comment as above, I think this can be moved into CalciteOptimizerTest http://gerrit.cloudera.org:8080/#/c/24652/2/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteOptimizer.java@225 PS2, Line 225: private static final class PreImpalaConvertState { I learned a neat Java 17 thing in a code review private record PreImpalaConvertState (RelNode plan, ImpalaRexSimplify simplifier) {} and change references to plan_ and simplifier_ to plan() and simplifier(); http://gerrit.cloudera.org:8080/#/c/24652/2/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/2/java/calcite-planner/src/test/java/org/apache/impala/calcite/service/CalciteOptimizerTest.java@138 PS2, Line 138: assertFalse(containsImpalaPlanRel(result.getPreImpalaConvertPlan())); I'm ok with checking there is no ImpalaPlanRel, but it does seem a bit overkill. If we did what I mentioned in the general comment, this wouldn't be necessary. Also, my other comment about having more robust testing would remove the need for this as well. So I think we should remove this, but if you feel strongly about it, I won't object. http://gerrit.cloudera.org:8080/#/c/24652/2/java/calcite-planner/src/test/java/org/apache/impala/calcite/service/CalciteOptimizerTest.java@236 PS2, Line 236: private static boolean containsRelNodeFamily( While this is better than what we currently have and I would approve this in a code review, this seems a bit too generic and would catch way too many false positives. If we can match the order of the RelNode hierarchy, that would be better. When I've thought about this in the past, I've wondered if we could borrow stuff from the Calcite Apache project. But perhaps that is too much overkill as well. But yeah, there are gonna be more things that we are probably gonna wanna match. But this is a great start! I think this would be ok for a first pass and we can file Jiras to make it better. -- 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: 2 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 15:46:24 +0000 Gerrit-HasComments: Yes
