Jim Apple 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 t Done Line 555: // hard cap on the number of rows returned. > comment seem a little verbose, how about something like: Done Line 563: if (-1 == node.limit) continue; > also check node.isSetLimit() Done Line 564: if (node.estimated_stats == null) continue; > is this the same as checking the isSet() method? Near as I can tell, there is no official standard or ground truth answer to this question. I have changed the code to check both. Line 569: "Expected cardinality estimate less than or equal to: " > less than or equal to LIMIT: ... Done Line 575: "In node type " > Better to print the plan node id Done 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 alrea Yes; removed. 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 I tested and didn't find one 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? They were covered elsewhere; removed. 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 I tested and they are not -- 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
