Aleksandr Efimov has posted comments on this change. ( http://gerrit.cloudera.org:8080/24784 )
Change subject: IMPALA-15324: Size exchange memory on input rows ...................................................................... Patch Set 2: (5 comments) Rebased on master. PlannerTest#testOffsetCardinality passes; with the fix reverted it fails on all three merging exchanges. http://gerrit.cloudera.org:8080/#/c/24784/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24784/2//COMMIT_MSG@42 PS2, Line 42: > Please add the "Assisted-by" line for your coding agents. Added in PS3. http://gerrit.cloudera.org:8080/#/c/24784/2/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/24784/2/fe/src/main/java/org/apache/impala/planner/ExchangeNode.java@328 PS2, Line 328: // capped at the limit and keeps the limit as the upper bound as before. > nit: I think we don't need to explain too much here. This just talks about Trimmed from seven lines to four in PS3, and what the previous commit did is in the commit message now. http://gerrit.cloudera.org:8080/#/c/24784/2/fe/src/main/java/org/apache/impala/planner/ExchangeNode.java@330 PS2, Line 330: long inputCardinality = getInputCardinality(); > Could you test getChild(0).getFilteredCardinality() and see if it's better? Tried it, and it breaks TPC-DS: q11's `50:EXCHANGE [HASH(ws_bill_customer_sk)]` goes 5.53MB to 1.17MB and `TpcdsPlannerTest` fails. The estimate is a ceiling on what the receiver can buffer, and the filter is a prediction: if it is late or not selective, the full stream still arrives. PS3 keeps `getChild(0).getCardinality()`. http://gerrit.cloudera.org:8080/#/c/24784/2/fe/src/main/java/org/apache/impala/planner/ExchangeNode.java@331 PS2, Line 331: return inputCardinality < 0 ? getCardinality() : inputCardinality; > If inputCardinality == -1, isn't getCardinality() returns -1 as well? Can w Yes, but only without a limit, and there `LIMIT+OFFSET` is not available either: the fallback returns -1 and the queue term keeps its default. With a limit, `capCardinalityAtLimit(-1)` returns `limit_`, which is short whenever there is an offset, since `DistributedPlanner` gives the sender-side sort `limit + offset`. PS3 uses `LIMIT+OFFSET` there. http://gerrit.cloudera.org:8080/#/c/24784/2/testdata/workloads/functional-planner/queries/PlannerTest/card-limit-offset.test File testdata/workloads/functional-planner/queries/PlannerTest/card-limit-offset.test: http://gerrit.cloudera.org:8080/#/c/24784/2/testdata/workloads/functional-planner/queries/PlannerTest/card-limit-offset.test@115 PS2, Line 115: select id, int_col from functional.alltypes order by id offset 6200 > It'd have larger difference if using a larger offset (e.g. 7000 or 7200 or Took 7299 in PS3. The exchange estimate is 16.00KB before this change and 55.01KB after, and the node returns 1 row. -- To view, visit http://gerrit.cloudera.org:8080/24784 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4ab880549e63267f97b2c193cf76aa92bbfc581c Gerrit-Change-Number: 24784 Gerrit-PatchSet: 2 Gerrit-Owner: Aleksandr Efimov <[email protected]> Gerrit-Reviewer: Aleksandr Efimov <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Comment-Date: Thu, 10 Sep 2026 12:35:52 +0000 Gerrit-HasComments: Yes
