Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11698 )
Change subject: IMPALA-5004: Switch to sorting node for large TopN queries ...................................................................... Patch Set 5: (3 comments) Looks good overall http://gerrit.cloudera.org:8080/#/c/11698/5/fe/src/main/java/org/apache/impala/analysis/SortInfo.java File fe/src/main/java/org/apache/impala/analysis/SortInfo.java: http://gerrit.cloudera.org:8080/#/c/11698/5/fe/src/main/java/org/apache/impala/analysis/SortInfo.java@244 PS5, Line 244: getSortTupleDescriptor().getAvgSerializedSize() * (cardinality + offset)); TODO: can cardinality + offset overflow? http://gerrit.cloudera.org:8080/#/c/11698/5/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java File fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java: http://gerrit.cloudera.org:8080/#/c/11698/5/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@324 PS5, Line 324: // SortNode#computeStats Can we factor out the formula into a static helper method in SortNode? http://gerrit.cloudera.org:8080/#/c/11698/5/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@337 PS5, Line 337: limit + offset Here and elsewhere: I think limit + offset could overflow. We don't have anything that limits the max values of these, e.g. the following is planned as normal. select * from tpch.lineitem order by l_orderkey limit 9223372036854775807 offset 9223372036854775807 We have a checkedAdd() helper for cases like this, but we probably should add a planner test to make sure that extreme cases like this are tested. -- To view, visit http://gerrit.cloudera.org:8080/11698 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I34c9db33c9302b55e9978f53f9c7061f2806c8a9 Gerrit-Change-Number: 11698 Gerrit-PatchSet: 5 Gerrit-Owner: Sahil Takiar <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Paul Rogers <[email protected]> Gerrit-Reviewer: Sahil Takiar <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Comment-Date: Thu, 25 Oct 2018 16:48:20 +0000 Gerrit-HasComments: Yes
