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 4: (20 comments) next patch addresses the comments, adds more tests, and fixes a case that I ran into. http://gerrit.cloudera.org:8080/#/c/8322/4/fe/src/main/java/org/apache/impala/analysis/SlotRef.java File fe/src/main/java/org/apache/impala/analysis/SlotRef.java: http://gerrit.cloudera.org:8080/#/c/8322/4/fe/src/main/java/org/apache/impala/analysis/SlotRef.java@96 PS4, Line 96: Preconditions.checkState(rawPath_ != null); > Just by looking at some of the constructors, this precondition doesn't seem analyzer.resolvePath is the one that imposes this precondition (on L99). the point is that if you use the "wrong" constructor, this analysis method will not work. I put the precondition here since it made it easier for me to figure out the root cause of the error. http://gerrit.cloudera.org:8080/#/c/8322/4/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/4/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@225 PS4, Line 225: if (conjunct instanceof InPredicate) { > I think it's better if you extract the check from L347 and merge it here. T Done http://gerrit.cloudera.org:8080/#/c/8322/4/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@288 PS4, Line 288: the resulting expression : * is not analyzed > Out of curiosity, why not analyzing the rewritten expression here? I'm a little unclear on the protocol for analyzers, but what I've tried here is to separate things so that if an analyzer is needed from the input or its children (the InPredicate), analysis is done in this method. For the expression that's returned, I let the caller analyze it with its own analyzer so that this method does not need to know about anything beyond its input. http://gerrit.cloudera.org:8080/#/c/8322/4/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@296 PS4, Line 296: 1) > For each case, you may want to mention if it works for correlated/uncorrela Done http://gerrit.cloudera.org:8080/#/c/8322/4/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@302 PS4, Line 302: Example: > Case with "limit 1"? Added-- note that its covered by the returnsSingleRow method on L362. http://gerrit.cloudera.org:8080/#/c/8322/4/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@313 PS4, Line 313: NOT EXISTS (RHS AND C = e OR e IS NULL) > Not sure I understand what this means. Is C = e OR e is NULL injected into rearranged this comment to first state what I actually do rather than the simpler, but unsupported equivalent rewrite. I thought that the case stmt was a bit surprising to see first. http://gerrit.cloudera.org:8080/#/c/8322/4/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@332 PS4, Line 332: NOT EXISTS (S ? F (RHS) t WHERE C = e or e IS NOT NULL) > :) I am not following this notation. Let's talk, maybe I am missing somethi rewrote it; included examples for each rewrite. http://gerrit.cloudera.org:8080/#/c/8322/4/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@340 PS4, Line 340: * @param inPred : * @return rewritten expression or null : * @throws AnalysisException > Remove, we don't use javadoc :) Done http://gerrit.cloudera.org:8080/#/c/8322/4/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@349 PS4, Line 349: Expr rhs = inPred.getChild(1); : Preconditions.checkArgument(rhs.contains(Subquery.class)); > I believe this check is redundant. Subqueries can only be in the rhs of the could be an in-list, but this class should only rewrite if the rhs is a subquery so I'm asserting that here. http://gerrit.cloudera.org:8080/#/c/8322/4/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@353 PS4, Line 353: > nit: empty lines (remove) Done http://gerrit.cloudera.org:8080/#/c/8322/4/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@381 PS4, Line 381: new ArrayList<String>() > nit: we use guava's static functions. i.e. Lists.newArrayList(); even if yo Done http://gerrit.cloudera.org:8080/#/c/8322/4/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@408 PS4, Line 408: Subquery newSubquery = new Subquery(rewriteQuery); : newSubquery.analyze(rhsQuery.getAnalyzer()); > Hm, rewriteQuery is already analyzed, so I am wondering if the analyze() ca I've added resets where analyzed. I think that this worked without the resets since a method up the stack calls reset as well. I think its preferable that I handle this eagerly unless I can convince myself that deferring it is guaranteed to be safe. http://gerrit.cloudera.org:8080/#/c/8322/4/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@414 PS4, Line 414: aggregation > "or analytic function". Done http://gerrit.cloudera.org:8080/#/c/8322/4/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@430 PS4, Line 430: new ArrayList<>(); > use guava's static functions Done http://gerrit.cloudera.org:8080/#/c/8322/4/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@545 PS4, Line 545: firstItem.getExpr() != null && Check what triggers this new case and whether unintended cases will take a different code path. http://gerrit.cloudera.org:8080/#/c/8322/4/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@679 PS4, Line 679: > remove empty lines Done http://gerrit.cloudera.org:8080/#/c/8322/4/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/4/fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java@341 PS4, Line 341: TODO for 2.3 > I know it's not your fault but can you remove the reference to 2.3 from all Done http://gerrit.cloudera.org:8080/#/c/8322/4/fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java@477 PS4, Line 477: on;y > only Done http://gerrit.cloudera.org:8080/#/c/8322/4/fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java@484 PS4, Line 484: > it's only for correlated subqueries, no? Done http://gerrit.cloudera.org:8080/#/c/8322/4/testdata/workloads/functional-query/queries/QueryTest/subquery.test File testdata/workloads/functional-query/queries/QueryTest/subquery.test: http://gerrit.cloudera.org:8080/#/c/8322/4/testdata/workloads/functional-query/queries/QueryTest/subquery.test@962 PS4, Line 962: # LHS not-null, Predicate IN, RHS not-empty with match. Correlated. Expect 4 results, where int_col = 1. > nit: fix long lines 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: 4 Gerrit-Owner: Vuk Ercegovac <vercego...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogian...@cloudera.com> Gerrit-Reviewer: Vuk Ercegovac <vercego...@cloudera.com> Gerrit-Comment-Date: Tue, 24 Oct 2017 21:07:02 +0000 Gerrit-HasComments: Yes
