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 12: Code-Review+1 (3 comments) Adding a comment we discussed offline. I'll let you decide what to do. If you would like to keep it as/is, I'm ok with that. http://gerrit.cloudera.org:8080/#/c/23562/10/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java File fe/src/main/java/org/apache/impala/analysis/QueryStmt.java: http://gerrit.cloudera.org:8080/#/c/23562/10/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java@505 PS10, Line 505: * Planner can still decide to disable result spooling if there are other factor nit: factors http://gerrit.cloudera.org:8080/#/c/23562/10/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/10/fe/src/main/java/org/apache/impala/planner/PlanRootSink.java@113 PS10, Line 113: * BufferedTupleStream used by the backend plan-root-sink. The maximum reservation is Can we just set resourceProfile_ to estProfile? http://gerrit.cloudera.org:8080/#/c/23562/12/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/12/fe/src/main/java/org/apache/impala/planner/PlanRootSink.java@168 PS12, Line 168: private static ResourceProfile sanitizeSpoolingOptions( Optional: Gonna +1 this, but we talked offline about this, but I figured I should still mention it here. If you don't want to change it, I'm ok with what we have, but imo, this would make it better. This method seems overloaded. Sanitize is both sanitizing the query options and returning a ResourceProfile. The downside of doing this (and granted, it's not much) is that we have this awkward intermediate "initialProfile_" member variable which doesn't serve much purpose. It just holds a state that will either be ignored by computeResourceProfile or added onto. I think it would be better to separate out the functionality of sanitize. If sanitize only set the query option, then the intermediate state wouldn't be needed and most of this logic can go directly into computeResourceProfile. There would be a couple of duplicate calculations, but, imo, I think that code would be cleaner. Another option to prevent duplicate calculations, actually. This is sort of an intermediate state, but it seems more natural to me, imo. Instead of an initial profile, we could have member variables for certain variables used by both sanitizing and initialProfile. For instance, in constructor: scratchLimit_ = queryOptions.getScratch_limit(); maxRowBufferSize_ = getMaxRowBufferSize(...); sanitizeSpoolingOptions(queryOptions, requireSpooling, scratchLimit, maxRowBufferSize) // or make method non-static and use member variables instead of passing in ... and then use these variables in computeResourceProfile(), if necessary -- 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: 12 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 14:17:53 +0000 Gerrit-HasComments: Yes
