Aleksandr Efimov has posted comments on this change. ( http://gerrit.cloudera.org:8080/24718 )
Change subject: IMPALA-15278: Fix incorrect cardinality with OFFSET ...................................................................... Patch Set 4: Code-Review+1 (1 comment) Went through PS4. The clamp settles the cardinality=0 question - the join in subquery-rewrite keeps its shape, and the new file covers offset above the estimate in both shapes. One note on coverage rather than on the patch: outside VALIDATE_CARDINALITY files, TestUtils.CARDINALITY_FILTER drops the numbers before comparison, so the OFFSET plans in union.test, topn.test, order.test and ddl.test do not check them. union.test:3885 is one that does move - TOP-N [LIMIT=20 OFFSET=10] over an 8-row scan goes from cardinality=8 to 1 - and the file stays green either way. Nothing this patch has to carry; turning the option on in one of those files would just make such moves visible. The other comment is on ExchangeNode. I read the plans and the estimate code, but did not run PlannerTest myself. http://gerrit.cloudera.org:8080/#/c/24718/4/fe/src/main/java/org/apache/impala/planner/ExchangeNode.java File fe/src/main/java/org/apache/impala/planner/ExchangeNode.java: http://gerrit.cloudera.org:8080/#/c/24718/4/fe/src/main/java/org/apache/impala/planner/ExchangeNode.java@155 PS4, Line 155: cardinality_ = capCardinalityAtLimit(children_.get(0).getCardinality(), offset_); The merging exchange mem-estimate moves with this: in tpcds_cpu_cost/ddl.test 372.09KB becomes 16.00KB and 48.01KB becomes 16.02KB. Both halves of the estimate are sized from this node's cardinality - estimateDeferredRPCQueueSize() caps the row batch at it (line 326) and estimateTotalQueueByteSize() derives the bytes from it (lines 344-351) - but the receiver reads limit+offset rows per sender and skips the first offset itself (exchange-node.cc:253), so its input is larger than its output by offset. The old 372KB came from the zero: it kept the `> 0` branch in estimateDeferredRPCQueueSize() from firing, leaving a full row batch in the estimate. Now the cap is the post-offset cardinality. In these two plans it lands on MIN_ESTIMATE_BYTES and the real traffic (4 senders, at most 6 rows each) is well under it, so I do not think anything is wrong here - the gap just widens with the offset. Would a TODO on estimateDeferredRPCQueueSize() be worth leaving, or is the estimate loose enough that it is not worth the line? -- To view, visit http://gerrit.cloudera.org:8080/24718 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3b628beabc5c7ec6c4fdda9dff6aaf7a4acae538 Gerrit-Change-Number: 24718 Gerrit-PatchSet: 4 Gerrit-Owner: Quanlong Huang <[email protected]> Gerrit-Reviewer: Aleksandr Efimov <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Comment-Date: Tue, 01 Sep 2026 06:32:57 +0000 Gerrit-HasComments: Yes
