Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/8322 )
Change subject: IMPALA-1422: support a constant on LHS of IN predicates. ...................................................................... Patch Set 8: (24 comments) http://gerrit.cloudera.org:8080/#/c/8322/8/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java File fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java: http://gerrit.cloudera.org:8080/#/c/8322/8/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@326 PS8, Line 326: * condition using the LHS constant. The rewrite handles group by, > Not clear what "handles" here means. It doesn't really do anything special thanks for the suggestion.. re-worded. http://gerrit.cloudera.org:8080/#/c/8322/8/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@327 PS8, Line 327: * analytic functions, limit, and uncorrelated subqueries. Correlated subqueries > Move last sentence into a TODO like this: Done http://gerrit.cloudera.org:8080/#/c/8322/8/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@330 PS8, Line 330: * TODO: handle correlated NOT IN case > remove (see above comment) Done http://gerrit.cloudera.org:8080/#/c/8322/8/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@340 PS8, Line 340: * So, if C is equal to any x_i, the expression is false. Similarly, if any > Nice description. I'm wondering if we directly translate this insight into yup, simplified. http://gerrit.cloudera.org:8080/#/c/8322/8/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@355 PS8, Line 355: Preconditions.checkArgument(rhs.contains(Subquery.class)); > not needed because L356-357 performs the same check Done http://gerrit.cloudera.org:8080/#/c/8322/8/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@398 PS8, Line 398: newSubquery.analyze(rhsQuery.getAnalyzer()); > This seems wrong. It might happen to work but pretty sure this will eventua Done http://gerrit.cloudera.org:8080/#/c/8322/8/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@714 PS8, Line 714: if (isCorrelatedPredicate(subqueryStmt.getWhereClause(), subqueryTupleIds)) { > containsCorrelatedPredicate()? yes, good catch. further simplified. http://gerrit.cloudera.org:8080/#/c/8322/8/fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java: http://gerrit.cloudera.org:8080/#/c/8322/8/fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java@277 PS8, Line 277: // NOT IN subquery with a correlated non-equi predicate is ok if the subquery only > [NOT] IN subquery Done http://gerrit.cloudera.org:8080/#/c/8322/8/fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java@292 PS8, Line 292: AnalyzesOk(String.format("select 1 from functional.allcomplextypes t " + > What about queries without complex types and non-constant LHS in the IN pre there's several above, for example L220 has a non-constant LHS and the types are not complex. L92 loops over a bunch of scalar types (and non-constant LHS). http://gerrit.cloudera.org:8080/#/c/8322/8/fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java@299 PS8, Line 299: AnalysisError(String.format("select * from functional.alltypestiny t where id %s " + > These tests are pretty rough to follow, perhaps we should spent some time b makes sense. the cut in this patch is just to get the tests to exercise the various branches more consistently where applicable. http://gerrit.cloudera.org:8080/#/c/8322/8/fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java@437 PS8, Line 437: // Tests for constant on the left hand side of [NOT] IN. > Since we are doing a double rewrite here, it's interesting to test nested s separated these out. I looked at making constant LHS just another dimension of the existing tests but I think a more comprehensive clean up here is better as a separate change. for now, I've added several more tests. http://gerrit.cloudera.org:8080/#/c/8322/8/fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java@476 PS8, Line 476: > You can omit this part if you like. Error message matching is prefix based. Done http://gerrit.cloudera.org:8080/#/c/8322/8/fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java@482 PS8, Line 482: "SELECT int_col FROM functional.alltypestiny b WHERE b.id = a.id ORDER BY int_col ASC LIMIT 5"); > long line, you can omit this line in the expected error message Done http://gerrit.cloudera.org:8080/#/c/8322/8/fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java@520 PS8, Line 520: + "(select * from functional.tinyinttable x where t.id = x.int_col)"); > please move "+" to previous line (easier to read and more consistent with e Done http://gerrit.cloudera.org:8080/#/c/8322/8/testdata/workloads/functional-planner/queries/PlannerTest/subquery-rewrite.test File testdata/workloads/functional-planner/queries/PlannerTest/subquery-rewrite.test: http://gerrit.cloudera.org:8080/#/c/8322/8/testdata/workloads/functional-planner/queries/PlannerTest/subquery-rewrite.test@2117 PS8, Line 2117: # IMPALA-1422: Constant on LHS of IN, uncorrelated subquery > Let's not add the JIRA number. We typically only do that for bugfixes, this Done http://gerrit.cloudera.org:8080/#/c/8322/8/testdata/workloads/functional-planner/queries/PlannerTest/subquery-rewrite.test@2118 PS8, Line 2118: select * from functional.alltypessmall where 1 in (select int_col from functional.alltypestiny); > no need to trailing semicolon Done http://gerrit.cloudera.org:8080/#/c/8322/8/testdata/workloads/functional-planner/queries/PlannerTest/subquery-rewrite.test@2140 PS8, Line 2140: | predicates: 1 IS NULL OR CASE WHEN int_col IS NULL THEN 1 ELSE int_col END = 1 > Need to update this based on the new WHERE predicate Done http://gerrit.cloudera.org:8080/#/c/8322/8/testdata/workloads/functional-planner/queries/PlannerTest/subquery-rewrite.test@2147 PS8, Line 2147: select * from functional.alltypessmall a where 1 in (select int_col from functional.alltypestiny b where b.id = a.id); > long lines (here and elsewhere) Done http://gerrit.cloudera.org:8080/#/c/8322/8/testdata/workloads/functional-planner/queries/PlannerTest/subquery-rewrite.test@2219 PS8, Line 2219: | > whitespace Done http://gerrit.cloudera.org:8080/#/c/8322/8/testdata/workloads/functional-planner/queries/PlannerTest/subquery-rewrite.test@2281 PS8, Line 2281: ==== > Add a case with nested subqueries to make sure the generated plan is sane Done http://gerrit.cloudera.org:8080/#/c/8322/8/testdata/workloads/functional-query/queries/QueryTest/subquery.test File testdata/workloads/functional-query/queries/QueryTest/subquery.test: http://gerrit.cloudera.org:8080/#/c/8322/8/testdata/workloads/functional-query/queries/QueryTest/subquery.test@839 PS8, Line 839: ---- QUERY > Nice tests! Done http://gerrit.cloudera.org:8080/#/c/8322/8/testdata/workloads/functional-query/queries/QueryTest/subquery.test@872 PS8, Line 872: # Constant on LHS of [NOT] IN. Basics to test: > Move to the top? fit the queries above into the pattern and moved them below. http://gerrit.cloudera.org:8080/#/c/8322/8/testdata/workloads/functional-query/queries/QueryTest/subquery.test@901 PS8, Line 901: # LHS null, Predicate NOT IN, RHS NULL. Expect no results. > Comment does not match SQL Done http://gerrit.cloudera.org:8080/#/c/8322/8/testdata/workloads/functional-query/queries/QueryTest/subquery.test@1126 PS8, Line 1126: ==== > Add 1-2 tests with nested subqueries. Done -- To view, visit http://gerrit.cloudera.org:8080/8322 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0d69889a3c72e90be9d4ccf47d2816819ae32acb Gerrit-Change-Number: 8322 Gerrit-PatchSet: 8 Gerrit-Owner: Vuk Ercegovac <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Comment-Date: Thu, 30 Nov 2017 09:22:52 +0000 Gerrit-HasComments: Yes
