Aleksandr Efimov has posted comments on this change. ( http://gerrit.cloudera.org:8080/24665 )
Change subject: IMPALA-15243: Fix costing for CTEs ...................................................................... Patch Set 9: (3 comments) PS9: the within-segment dedup from PS2 is in place and the new childCores == null branch reads fine to me. Two questions from PS3 are still open (the left-child cap and the maxThreadAllowed clamp) and the code there is unchanged, so I repeated them inline rather than leaving them buried in an old patch set. Goldens I checked structurally, not line by line. http://gerrit.cloudera.org:8080/#/c/24665/9/fe/src/main/java/org/apache/impala/planner/CostingSegment.java File fe/src/main/java/org/apache/impala/planner/CostingSegment.java: http://gerrit.cloudera.org:8080/#/c/24665/9/fe/src/main/java/org/apache/impala/planner/CostingSegment.java@177 PS9, Line 177: if (childCores == null) { The comment says the producer "will be visited later and its CoreCount will be added to the subtreeCoreBuilder", but the builder here belongs to the fragment being processed right now, and Planner.computeBlockingAwareCores() only reads the root fragment's entry at the end. What keeps the number right is that the producer reaches the root through the single consumer that owns the fragment link, so skipping the others counts it exactly once - which is how the commit message puts it. Could the comment say that instead? As written it reads like a promise that this segment will pick the count up later. http://gerrit.cloudera.org:8080/#/c/24665/9/fe/src/main/java/org/apache/impala/planner/PlanFragment.java File fe/src/main/java/org/apache/impala/planner/PlanFragment.java: http://gerrit.cloudera.org:8080/#/c/24665/9/fe/src/main/java/org/apache/impala/planner/PlanFragment.java@1144 PS9, Line 1144: && verdict != ScalingVerdict.FIXED_BY_PARTITIONED_JOIN_BUILD) { Coming back to this from PS3, since the code is unchanged: the cap still skips only FIXED_BY_PLAN_NODE and FIXED_BY_PARTITIONED_JOIN_BUILD, so a fragment with CTE_FRAGMENT_BOUNDED can still be lowered by setAdjustedInstanceCount(maxThreadPerNode * lc.getNumNodes()) right after being pinned. I couldn't turn it into a real violation - the cap lands on the same bound the producer was clamped to - so this is mostly a question: if the consumer count is meant to follow the producer, should CTE_FRAGMENT_BOUNDED be listed here as well? http://gerrit.cloudera.org:8080/#/c/24665/9/fe/src/main/java/org/apache/impala/planner/PlanFragment.java@1278 PS9, Line 1278: } else if (isCTEConsumerFragment()) { Also from PS3, same reason: this is the only non-fixed path that doesn't clamp selectedParallelism to maxThreadAllowed, and the checkState at the end of the method asserts exactly that. Is there a guarantee that a consumer fragment can't end up with more instances per host than maxThreadPerNode? And since this branch runs before isPartitionedJoinBuildFragment(), a fragment that is both would stop following its parent's parallelism - I couldn't construct such a plan, so a Preconditions.checkState here would make that assumption explicit. -- To view, visit http://gerrit.cloudera.org:8080/24665 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I21b60af313070029a1fa8bf835107bef88f54912 Gerrit-Change-Number: 24665 Gerrit-PatchSet: 9 Gerrit-Owner: Michael Smith <[email protected]> Gerrit-Reviewer: Aleksandr Efimov <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Kurt Deschler <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Stamatis Zampetakis <[email protected]> Gerrit-Comment-Date: Tue, 18 Aug 2026 10:38:35 +0000 Gerrit-HasComments: Yes
