Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/10528 )
Change subject: IMPALA-7090: Limit the size of expr created by EqualityDisjunctsToInRule ...................................................................... Patch Set 1: (6 comments) http://gerrit.cloudera.org:8080/#/c/10528/1/fe/src/main/java/org/apache/impala/rewrite/EqualityDisjunctsToInRule.java File fe/src/main/java/org/apache/impala/rewrite/EqualityDisjunctsToInRule.java: http://gerrit.cloudera.org:8080/#/c/10528/1/fe/src/main/java/org/apache/impala/rewrite/EqualityDisjunctsToInRule.java@67 PS1, Line 67: : el while here, pls fix this formatting. http://gerrit.cloudera.org:8080/#/c/10528/1/fe/src/main/java/org/apache/impala/rewrite/EqualityDisjunctsToInRule.java@79 PS1, Line 79: newInList.add(otherPred.getChild(1)); if newInList.size() == Expr.EXPR_CHILDREN_LIMIT, this operation will exceed it, and we'll have the same error, right? pls add a test for this edge case. http://gerrit.cloudera.org:8080/#/c/10528/1/fe/src/main/java/org/apache/impala/rewrite/EqualityDisjunctsToInRule.java@82 PS1, Line 82: if (newInList.size() + otherPred.getChildren().size() > Expr.EXPR_CHILDREN_LIMIT) pls add a jira, if we don't already have one, to back out of rewrites that produce an expression that fails analysis. ideally, we should be able to remove this specific guard (which is tested in analysis) with that support. it would be good to avoid duplicating checks that happen in analysis. http://gerrit.cloudera.org:8080/#/c/10528/1/fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java File fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java: http://gerrit.cloudera.org:8080/#/c/10528/1/fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java@174 PS1, Line 174: a nit: an http://gerrit.cloudera.org:8080/#/c/10528/1/fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java@178 PS1, Line 178: disjuct nit: disjunct http://gerrit.cloudera.org:8080/#/c/10528/1/fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java@194 PS1, Line 194: } add a test to check a partial rewrite. for example, a where clause with an in-list of size EXPR_CHILDREN_LIMIT - 2 or'd with 10 eq predicates. if I understand it correctly, 2 of the or'd predicates will be merged into the in-list and 8 will be merged into another in-list, leaving a disjunction of two in-lists. -- To view, visit http://gerrit.cloudera.org:8080/10528 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie40c3210271a9e3c7f1b2b869a8c2ec8bacaa72a Gerrit-Change-Number: 10528 Gerrit-PatchSet: 1 Gerrit-Owner: Tianyi Wang <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Comment-Date: Tue, 29 May 2018 21:18:56 +0000 Gerrit-HasComments: Yes
