Michael Smith 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 13: (5 comments) http://gerrit.cloudera.org:8080/#/c/23562/13/fe/src/main/java/org/apache/impala/planner/PlanRootSink.java File fe/src/main/java/org/apache/impala/planner/PlanRootSink.java: http://gerrit.cloudera.org:8080/#/c/23562/13/fe/src/main/java/org/apache/impala/planner/PlanRootSink.java@61 PS13, Line 61: List<Expr> outputExprs, boolean requireSpooling, TQueryOptions queryOptions) { I'm trying to make sense of why we pass queryOptions here and later at computeResourceProfile. Are they different query options? Is there some weird need for both in different circumstances? I'd like to see code comments clarifying how they're both used. I don't even see queryOptions preserved. Is this just mutating them? I'd rather see that done with a static helper method elsewhere rather than as a side-effect of creating a PlanRootSink. http://gerrit.cloudera.org:8080/#/c/23562/13/fe/src/main/java/org/apache/impala/planner/PlanRootSink.java@64 PS13, Line 64: sanitizeSpoolingOptions(queryOptions, requireSpooling); Can you add a comment why this is done here, but not later where we use just SpoolingMemoryBound(queryOptions)? http://gerrit.cloudera.org:8080/#/c/23562/13/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java File fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java: http://gerrit.cloudera.org:8080/#/c/23562/13/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@2380 PS13, Line 2380: QueryStmt queryStmt = ctx_.getAnalysisResult().getQueryStmt(); Maybe a comment why we need getAnalysisResult()? http://gerrit.cloudera.org:8080/#/c/23562/13/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@2384 PS13, Line 2384: ctx_.getRootAnalyzer().getQueryCtx().client_request.query_options); PlannerContext.getQueryOptions() exists for this. http://gerrit.cloudera.org:8080/#/c/23562/13/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaPlanRel.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaPlanRel.java: http://gerrit.cloudera.org:8080/#/c/23562/13/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaPlanRel.java@117 PS13, Line 117: return canPassThroughParentAggregate((RelNode) planRel); Should ImpalaPlanRel inherit from RelNode? -- 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: 13 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, 14 Nov 2025 20:47:11 +0000 Gerrit-HasComments: Yes
