Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11443 )
Change subject: IMPALA-6202. mod and % should be equivalent. ...................................................................... Patch Set 6: (2 comments) Looks good to me, just had a couple of suggestions http://gerrit.cloudera.org:8080/#/c/11443/6/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java File fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java: http://gerrit.cloudera.org:8080/#/c/11443/6/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java@87 PS6, Line 87: public static Expr createExpr(FunctionName fnName, FunctionParams params, There are only two callsites for createExpr() so I think we should just update the other to pass through TQueryOptions too rather than making it nullable. It would be easy for future coders to introduce an bug otherwise where they forget to pass through the query options and we don't apply the rewrite. That would be hard to catch. http://gerrit.cloudera.org:8080/#/c/11443/6/fe/src/main/java/org/apache/impala/service/Frontend.java File fe/src/main/java/org/apache/impala/service/Frontend.java: http://gerrit.cloudera.org:8080/#/c/11443/6/fe/src/main/java/org/apache/impala/service/Frontend.java@1035 PS6, Line 1035: public StatementBase parse(String stmt) throws AnalysisException { Instead of threading null all the way through, can we just pass in a dummy TQueryOptions object. Best to keep test code paths close to the real ones where possible. -- To view, visit http://gerrit.cloudera.org:8080/11443 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib0067da04083859ffbf662a29007154461bea2ba Gerrit-Change-Number: 11443 Gerrit-PatchSet: 6 Gerrit-Owner: Yongjun Zhang <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Reviewer: Yongjun Zhang <[email protected]> Gerrit-Comment-Date: Tue, 25 Sep 2018 23:28:15 +0000 Gerrit-HasComments: Yes
