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 8: (2 comments) http://gerrit.cloudera.org:8080/#/c/23562/8/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/8/fe/src/main/java/org/apache/impala/planner/PlanRootSink.java@119 PS8, Line 119: public void computeResourceProfile(TQueryOptions queryOptions) { > Please add precondition in the beginning. This one is actually not true. This is why it needs to be disabled http://gerrit.cloudera.org:8080/#/c/23562/8/fe/src/main/java/org/apache/impala/planner/PlanRootSink.java@220 PS8, Line 220: // Need to disableResultSpooling if spoolResults_ is set to false so that : // the query option 'spool_query_results' is set to false. : disableResultSpooling(queryOptions); > This patch make the decision of spooling vs not spooling early on at Calcit So I totally agree with you about the general issue. There is a design flaw here. But it's really a deeper problem. This class really shouldn't be setting the query option at all. I suppose the one thing I dislike about your suggestion is setting the query option in multiple places because that just leads to confusion while debugging. Did the spoolResults/query option get set at point a? At point b? Seems weird to have that logic in multiple places. That actually was the point of my original commit, if you go back to version 1: The original commit only set the query option one time, at the end of computeResourceProfile. Flawed as well, of course, but I figured spoolResults_ would only be mutable in this one method, and the queryOption would only be set one time. That changed when I added the "mutate" method, and this kinda snowballed in a way into something worse, so I do agree with your dislike of the current code. So I tried to punt this problem with my "hack" comment. I guess I can change it to what you like. I guess it is slightly better...not sure if I like my original way better, but I'm ok with this. Of course, there's still a problem that one isn't tied to changing the mutable spoolResults_, and it doesn't stop someone else from creating a split brain problem. Another potential way to fix this is to create a lazy creation of a "SpoolResults" class. At constructor time, instead of passing in a boolean, pass in a CheckInitialSpoolResults (or something better named, but named in a way that people don't think it contains the final SpoolResults) class. In createResourceProfile, it uses the results of CheckInitialSpoolResults and any calculation done in creationResourceProfile to lazily create the SpoolResults class which is used by other methods. This variable would then be set to null in the constructor and only set in createResourceProfile, and this class can set the query option. And anyone trying to use SpoolResults before computeResourceProfile would get an NPE. Heh, but maybe that's overengineering. Another option: Wait until the very end after everything is computed, and have something outside be responsible for the query option, like FrontEnd. That, to me, is the best place to change the query option. But I'm nervous about that because I don't know the logic well enough to know if the PlanRootSink here is buried a little with a CTAS statement. And it would still have the same problem of spoolResults_ being mutable (unless we did both!), though it would fix the problem you're suggesting. Ok, now that I wrote a novel here, I don't feel strongly about keeping the code as/is. I mentioned what I thought is a little better, but after presenting this, I'm willing to go with your final thoughts here on your next comment. Thanks! -- 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: 8 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: Wed, 12 Nov 2025 23:29:37 +0000 Gerrit-HasComments: Yes
