Steve Carlin has posted comments on this change. ( http://gerrit.cloudera.org:8080/23562 )
Change subject: IMPALA-13902: Calcite planner: Implement is_spool_query_results ...................................................................... Patch Set 5: (4 comments) http://gerrit.cloudera.org:8080/#/c/23562/2/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java File fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java: http://gerrit.cloudera.org:8080/#/c/23562/2/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@a506 PS2, Line 506: : : : > I don't quite get the logic of why createDataSink makes sense, but it looks So basically, I guess I didn't like keeping the logic in here. This logic is shared logic by both the original planner and the Calcite planner, but this only applies to the original planner. By moving it to QueryStmt, it keeps AnalysisContext free of planner specific code. The problem also comes from the fact that it's too early for the Calcite planner to know if it "returns at most one row". The original planner has this information at Analysis time, but Calcite does not have this information at Analysis/validation time. http://gerrit.cloudera.org:8080/#/c/23562/5/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteSingleNodePlanner.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteSingleNodePlanner.java: http://gerrit.cloudera.org:8080/#/c/23562/5/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteSingleNodePlanner.java@130 PS5, Line 130: while (relNode instanceof Filter || relNode instanceof Project) { > Is there a more generalized version of this? Seems like it might be somethi Changed this to call a method that handles this. http://gerrit.cloudera.org:8080/#/c/23562/5/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/ExecRequestCreator.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/service/ExecRequestCreator.java: http://gerrit.cloudera.org:8080/#/c/23562/5/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/ExecRequestCreator.java@253 PS5, Line 253: // This is a deprecated class, so the spool option is not supported here and set > Is there somewhere else talking about why it's deprecated? I just filed IMPALA-14541 The IMPALA-14408 patch was made so that queries go through JniFrontend instead of CalciteJniFrontend. This class is only used by CalciteJniFrontend. http://gerrit.cloudera.org:8080/#/c/23562/5/testdata/workloads/functional-query/queries/QueryTest/calcite.test File testdata/workloads/functional-query/queries/QueryTest/calcite.test: http://gerrit.cloudera.org:8080/#/c/23562/5/testdata/workloads/functional-query/queries/QueryTest/calcite.test@1127 PS5, Line 1127: row_regex: .*SPOOL_QUERY_RESULTS=0.* > Should there be a SPOOL_QUERY_RESULTS=1 case? I'm ok with adding one, but I didn't add it for two reasons: 1) spool_query_results is defaulted to true, so this is testing that new behavior works 2) There are e2e tests that will eventually test this when the e2e tests are turned on. I always kinda struggle to find a balance of what to put in this file and what I should just wait for when e2e tests are eventually tested on a nightly basis. If you think it should be here though, I can add it, I have no issue with that. -- To view, visit http://gerrit.cloudera.org:8080/23562 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5b9bf49e2874ee12de212b892bd898c296774c6f Gerrit-Change-Number: 23562 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: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Reviewer: Steve Carlin <[email protected]> Gerrit-Comment-Date: Fri, 07 Nov 2025 22:11:03 +0000 Gerrit-HasComments: Yes
