Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/23929 )
Change subject: IMPALA-14541: Remove deprecated CalciteJniFrontend class ...................................................................... Patch Set 5: (2 comments) http://gerrit.cloudera.org:8080/#/c/23929/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/23929/5//COMMIT_MSG@19 PS5, Line 19: One minor change also in this commit: The Tpcds test was moved to the : planner directory to be more consistent with the other tests. Nit: As a curiosity: Why standardize on org.apache.impala.planner rather than org.apache.impala.calcite.planner? Does it make something easier? Typically, tests for the package org.foo.bar would also be in org.foo.bar. This allows them to avoid imports for things in that package and lets them access package private things. There's no such thing as org.apache.impala.calcite.planner, so this doesn't help here. Those are planner tests that don't really need deep access to the package. But if we wrote individual unit tests for specific pieces of code in org.apache.impala.calcite.foo, they would benefit from living in org.apache.impala.calcite.foo. Maybe there is something about our relationship to impala-frontend that changes the equation. http://gerrit.cloudera.org:8080/#/c/23929/5/java/calcite-planner/src/test/java/org/apache/impala/planner/TestReduceExprShuttle.java File java/calcite-planner/src/test/java/org/apache/impala/planner/TestReduceExprShuttle.java: http://gerrit.cloudera.org:8080/#/c/23929/5/java/calcite-planner/src/test/java/org/apache/impala/planner/TestReduceExprShuttle.java@375 PS5, Line 375: private static class ReduceShuttleObjects { This is not really related to this change and doesn't need to happen in this specific review. I think the code would benefit from a helper function. The collection of fields in ReduceShuttleObjects would be local variables in the helper function, and the tests wouldn't know about them at all. For example (I haven't compiled this, so it probably needs a couple fixes): void verifyExprReduction(String expr, Map<String, TColumn> reductionMap, List<String> expectedReducedExprs) { CalciteAnalysisResult analysisResult = (CalciteAnalysisResult) parseAndAnalyze("SELECT " + query, feFixture_.createAnalysisCtx(), new CalciteCompilerFactory()); CalciteRelNodeConverter relNodeConverter = new CalciteRelNodeConverter(analysisResult); RelNode rootNode = relNodeConverter.convert(analysisResult.getValidatedNode()); Preconditions.checkState(rootNode instanceof Project); Project project = (Project) rootNode; Analyzer analyzer = analysisResult.getAnalyzer(); TQueryCtx queryCtx = analyzer.getQueryCtx(); TestReducerTmp testReducer = new TestReducerTmp(reductionMap); RexExecutor executor = new ImpalaRexExecutor( analyzer, queryCtx, testReducer); RexBuilder rexBuilder = new RexBuilder( new JavaTypeFactoryImpl(new ImpalaTypeSystemImpl())); List<RexNode> reducedExprs = new ArrayList<>(); executor.reduce(rexBuilder, project.getProjects(), reducedExprs); assertEquals(expectedReducedExprs.size(), reducedExprs.size()); for (int i = 0; i < expectedReducedExprs.size(); i++) { assertEquals(exectedReducedExprs.get(i), reducedExprs.get(i).toString()); } } -- To view, visit http://gerrit.cloudera.org:8080/23929 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3797acf0f23da4f94098e2f8f5f7aa378c9ae91a Gerrit-Change-Number: 23929 Gerrit-PatchSet: 5 Gerrit-Owner: Steve Carlin <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Steve Carlin <[email protected]> Gerrit-Comment-Date: Wed, 04 Feb 2026 20:09:14 +0000 Gerrit-HasComments: Yes
