Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/12814 )
Change subject: IMPALA-4865: Reject Expr Rewrite When Appropriate ...................................................................... Patch Set 6: (6 comments) http://gerrit.cloudera.org:8080/#/c/12814/6/fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java File fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java: http://gerrit.cloudera.org:8080/#/c/12814/6/fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java@406 PS6, Line 406: analyzer.getQueryCtx(), 0); Not sure we want to do it this way. I'd suggest creating another version of EvalExprWithoutRow that takes two parameters, and have that turn around and call the three-parameter version. http://gerrit.cloudera.org:8080/#/c/12814/6/fe/src/main/java/org/apache/impala/analysis/ColumnDef.java File fe/src/main/java/org/apache/impala/analysis/ColumnDef.java: http://gerrit.cloudera.org:8080/#/c/12814/6/fe/src/main/java/org/apache/impala/analysis/ColumnDef.java@273 PS6, Line 273: FeSupport.MAX_STRING_LEN); Here, rather than decode the meaning as a parameter, just have another create() function, maybe createBounded, that takes a limit. Note that, here, we are creating a literal. In fact, we are creating a date time literals. The date time can never overflow. If we return an expression, rather than a literal, "bad things" will happen. http://gerrit.cloudera.org:8080/#/c/12814/6/fe/src/main/java/org/apache/impala/analysis/LiteralExpr.java File fe/src/main/java/org/apache/impala/analysis/LiteralExpr.java: http://gerrit.cloudera.org:8080/#/c/12814/6/fe/src/main/java/org/apache/impala/analysis/LiteralExpr.java@182 PS6, Line 182: public static LiteralExpr create(Expr constExpr, TQueryCtx queryCtx, int maxResultSize) There are only two cases: limited or unlimited. Best to have two functions: create and createBounded (say) that express those two cases, and encode the limits in those new functions. http://gerrit.cloudera.org:8080/#/c/12814/6/fe/src/main/java/org/apache/impala/analysis/RangePartition.java File fe/src/main/java/org/apache/impala/analysis/RangePartition.java: http://gerrit.cloudera.org:8080/#/c/12814/6/fe/src/main/java/org/apache/impala/analysis/RangePartition.java@186 PS6, Line 186: literal = LiteralExpr.create(e, analyzer.getQueryCtx(), 65_536); Magic constants are generally frowned upon. Make this a declared constant? http://gerrit.cloudera.org:8080/#/c/12814/6/fe/src/main/java/org/apache/impala/authorization/AuthorizationFactory.java File fe/src/main/java/org/apache/impala/authorization/AuthorizationFactory.java: http://gerrit.cloudera.org:8080/#/c/12814/6/fe/src/main/java/org/apache/impala/authorization/AuthorizationFactory.java@23 PS6, Line 23: import org.apache.impala.authorization.*; ? http://gerrit.cloudera.org:8080/#/c/12814/6/fe/src/main/java/org/apache/impala/rewrite/FoldConstantsRule.java File fe/src/main/java/org/apache/impala/rewrite/FoldConstantsRule.java: http://gerrit.cloudera.org:8080/#/c/12814/6/fe/src/main/java/org/apache/impala/rewrite/FoldConstantsRule.java@43 PS6, Line 43: public static final int MAX_STRING_LITERAL_SIZE = 65_536; This guy should be moved to LiteralExpr: it is a property of the rewrite operation, not of this rule. -- To view, visit http://gerrit.cloudera.org:8080/12814 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8b078113ccc1aa49b0cea0c86dff2e02e1dd0e23 Gerrit-Change-Number: 12814 Gerrit-PatchSet: 6 Gerrit-Owner: Fang-Yu Rao <fangyu....@cloudera.com> Gerrit-Reviewer: Anurag Mantripragada <anu...@cloudera.com> Gerrit-Reviewer: Fang-Yu Rao <fangyu....@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Paul Rogers <prog...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Thu, 04 Apr 2019 23:23:19 +0000 Gerrit-HasComments: Yes
