Alex Behm has posted comments on this change. Change subject: IMPALA-3450: LIMITs on plan nodes are reflected in cardinality estimates ......................................................................
Patch Set 5: (10 comments) http://gerrit.cloudera.org:8080/#/c/3127/5/fe/src/test/java/com/cloudera/impala/planner/PlannerTestBase.java File fe/src/test/java/com/cloudera/impala/planner/PlannerTestBase.java: Line 554: // Checks that the planner never overestimates cardinality by ignoring that LIMIT puts a nit: we use the /* *. style for method comments (see existing examples in this file) Line 555: // hard cap on the number of rows returned. comment seem a little verbose, how about something like: Checks that limits are accounted for in the cardinality of plan nodes. Line 563: if (-1 == node.limit) continue; also check node.isSetLimit() Line 564: if (node.estimated_stats == null) continue; is this the same as checking the isSet() method? Line 569: "Expected cardinality estimate less than or equal to: " less than or equal to LIMIT: ... Line 575: "In node type " Better to print the plan node id http://gerrit.cloudera.org:8080/#/c/3127/5/testdata/workloads/functional-planner/queries/PlannerTest/aggregation.test File testdata/workloads/functional-planner/queries/PlannerTest/aggregation.test: Line 1069: select sum(l_partkey) as sum_part from tpch.lineitem group by l_orderkey LIMIT 1 doesn't the above test or any other existing test with an agg + limit already cover this? http://gerrit.cloudera.org:8080/#/c/3127/5/testdata/workloads/functional-planner/queries/PlannerTest/inline-view-limit.test File testdata/workloads/functional-planner/queries/PlannerTest/inline-view-limit.test: Line 616: # IMPALA-3450: limits on select nodes are reflected in cardinality estimates. The test for pretty sure this is already covered in inline-view-limit.test http://gerrit.cloudera.org:8080/#/c/3127/5/testdata/workloads/functional-planner/queries/PlannerTest/nested-collections.test File testdata/workloads/functional-planner/queries/PlannerTest/nested-collections.test: Line 1779: select o_orderkey from tpch_nested_parquet.customer c, c.c_orders o limit 1; sure these are not already covered elsewhere in this file? http://gerrit.cloudera.org:8080/#/c/3127/5/testdata/workloads/functional-planner/queries/PlannerTest/union.test File testdata/workloads/functional-planner/queries/PlannerTest/union.test: Line 2664: # IMPALA-3450: limits on union nodes are reflected in cardinality estimates. The test for I think these are also covered elsewhere in this file (explicitly test for placement of LIMIT with union and union distinct) -- To view, visit http://gerrit.cloudera.org:8080/3127 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic06dcb93bbb2510c0d40151302bd817ef340b825 Gerrit-PatchSet: 5 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Jim Apple <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Jim Apple <[email protected]> Gerrit-HasComments: Yes
