Riza Suminto 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 14: (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: sanitizeSpoolingOptions(ctx.getQueryOptions(), requireSpooling); > I'm trying to make sense of why we pass queryOptions here and later at comp This has been a contention point between me and Steve on how to do it. Before this patch, result spooling option can be disabled when Planner call computeResourceProfile(), which is very late in planning phase. There is another callsite in AnalysisContext.analyzeAndAuthorize() that can disable result spooling if planner determine that the query return at most one row. This patch unify that spooling option sanitation for both Classic and Calcite planner. But where/when to do it remain debateable. In patch set 14, I opt to created a factory method PlanRootSink.create() to handle both sanitizing options and creating PlanRootSink. I also decide to redundantly sanitize the queryOptions again in both computeProcessingCost() and computeResourceProfile() to guarantee correctness until we can make further adjustment downstream and make PlanRootSink constructor private. I hope this is OK. http://gerrit.cloudera.org:8080/#/c/23562/13/fe/src/main/java/org/apache/impala/planner/PlanRootSink.java@64 PS13, Line 64: > Can you add a comment why this is done here, but not later where we use jus Explained in PlanRootSink.create() method. 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: // Lookup the analyzed query statement to get the latest value of canSpoolResult() > Maybe a comment why we need getAnalysisResult()? I'm guessing canSpoolResult() return value is only valid (may turn from True to False) after statement analysis finish. Steve can correct me on that. http://gerrit.cloudera.org:8080/#/c/23562/13/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@2384 PS13, Line 2384: List<Expr> resultExprs = queryStmt.getResultExprs(); > PlannerContext.getQueryOptions() exists for this. Make sense. Done by passing the whole PlannerContext to PlanRootSink.create(). 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: > Should ImpalaPlanRel inherit from RelNode? This is only called by ParentPlanRelContext.Builder(). I'm guessing this is just a convenience method, so I added Preconditions to guard for it. Steve, please correct me if I'm wrong. -- 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: 14 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 23:02:24 +0000 Gerrit-HasComments: Yes
