Aleksandr Efimov has posted comments on this change. ( http://gerrit.cloudera.org:8080/24665 )
Change subject: IMPALA-15243: Fix costing for CTEs ...................................................................... Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/24665/2/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/2/fe/src/main/java/org/apache/impala/planner/CostingSegment.java@182 PS2, Line 182: segmentCore = CoreCount.sum(segmentCore, childCores.first); > I'm not sure it makes sense to avoid all duplicates here. We should avoid d Ok, that makes sense - if the point is pipeline parallelism then matching each consumer to the producer is a reasonable reading, I'll drop that part. On the dedup itself: I went looking for a plan that exercises it and couldn't find one. It needs two consumers of the same CTE in one segment, which is possible in principle - every CTEConsumerNode gets the producer node as child(0) (DistributedPlanner adds it for all dests, only the fragment-level link goes to the first one), so the ids do collide. But across all of tpcds_cte there's no fragment in DISTRIBUTEDPLAN/PARALLELPLANS with two consumers of the same CTE; the only cases are in the single-node PLAN section. Which is consistent with no cpu-comparison-result moving in PS3. So the guard looks right, just uncovered. Do you have a shape in mind that hits it - union over the same CTE twice? Would be good to have it in the planner tests so it doesn't quietly rot. http://gerrit.cloudera.org:8080/#/c/24665/2/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/2/fe/src/main/java/org/apache/impala/planner/PlanFragment.java@1346 PS2, Line 1346: verdict = ScalingVerdict.SCAN_FRAGMENT_BOUNDED; > Yeah, I think a more strict restriction would make sense for the moment. Thanks, CTE_FRAGMENT_BOUNDED looks like the right shape - pinning to getNumInstances() and skipping step 2 is what I was after. One thing I'm unsure about: the left-child cap further down in traverseEffectiveParallelism (~1142 in PS3) only skips FIXED_BY_PLAN_NODE and FIXED_BY_PARTITIONED_JOIN_BUILD, so CTE_FRAGMENT_BOUNDED falls through and can still setAdjustedInstanceCount(maxThreadPerNode * lc.getNumNodes()) on top of the count you just pinned. computeEffectiveParallelism passes parentFragment == null for every non-join-build fragment, so consumer fragments do get there, and for the one holding the producer link child(0) is the producer fragment. To be clear this isn't new in PS3 - the old SCAN_FRAGMENT_BOUNDED verdict fell through the same way. And I couldn't turn it into an actual violation: the cap lands on maxThreadPerNode * producer_nodes and the producer was itself clamped to that, so dest >= src still holds. The even-multiple DCHECK looks more fragile, but I don't have a repro. So mostly checking it's deliberate - if the consumer count is meant to follow the producer, should CTE_FRAGMENT_BOUNDED be excluded there too? -- 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: 2 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: Thu, 06 Aug 2026 19:08:35 +0000 Gerrit-HasComments: Yes
