Quanlong Huang 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) Thanks for fixing this! 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. 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@322 PS2, Line 322: The senders : // do not apply the offset: DistributedPlanner clears it on the sender-side sort and, : // when there is a limit, raises that sort's limit to 'limit + offset', and : // GetNextMerging() drops the first 'offset' rows here as it reads. So the queues hold : // the rows before the offset, which is the input cardinality rather than this node's. : // When the input estimate is unknown, fall back to this node's cardinality, which is : // 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 merging-exchange. If we add this, we need to explain other cases as well, e.g. other exchanges types, no-offset cases, etc. That would be too lengthy. Saying "as before" is also confusing. I think you mean the previous commit. But we don't need to explain current change in the comments which live long in the code base. It'd be better to explain these in the commit message if you want. http://gerrit.cloudera.org:8080/#/c/24784/2/fe/src/main/java/org/apache/impala/planner/ExchangeNode.java@330 PS2, Line 330: getInputCardinality Could you test getChild(0).getFilteredCardinality() and see if it's better? http://gerrit.cloudera.org:8080/#/c/24784/2/fe/src/main/java/org/apache/impala/planner/ExchangeNode.java@331 PS2, Line 331: getCardinality() If inputCardinality == -1, isn't getCardinality() returns -1 as well? Can we consider using LIMIT+OFFSET instead? 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: 6200 It'd have larger difference if using a larger offset (e.g. 7000 or 7200 or even 8000) comparing to the estimate before this patch. How about using 7299 or 8000 here? -- 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: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Comment-Date: Thu, 10 Sep 2026 09:21:50 +0000 Gerrit-HasComments: Yes
