Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11719 )
Change subject: IMPALA-5821: Add query with implicit casts to extended explain output. ...................................................................... Patch Set 5: (4 comments) While here, do you have thoughts on how to maintain this going forward? Main concern is that a "toSql()" (default options) will be used and we'll forget to look for casts, if expected. I see that "toSql()" is used in many places other than this use-case so I'd lean towards follow-up changes to require the option and see if we can replace the other "toSql()" calls with something more explicit for their use. I'm fine with a todo/jira. http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/main/java/org/apache/impala/common/PrintUtils.java File fe/src/main/java/org/apache/impala/common/PrintUtils.java: http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/main/java/org/apache/impala/common/PrintUtils.java@110 PS4, Line 110: 32 > Just some padding for if lines expand. Maybe this is too ugly. ok, just mention it in a brief comment. I prefer such inline constants to have some explanation. It seems to be an optimization so I'm also ok with removing it to simplify. http://gerrit.cloudera.org:8080/#/c/11719/4/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/11719/4/fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java@424 PS4, Line 424: col > I don't really understand what is wrong with param? nothing's wrong with it. my comments are motivated to keep things consistent. http://gerrit.cloudera.org:8080/#/c/11719/5/fe/src/test/java/org/apache/impala/util/PrintUtilsTest.java File fe/src/test/java/org/apache/impala/util/PrintUtilsTest.java: http://gerrit.cloudera.org:8080/#/c/11719/5/fe/src/test/java/org/apache/impala/util/PrintUtilsTest.java@89 PS5, Line 89: * nit: looks like there's still an extra '*' here. http://gerrit.cloudera.org:8080/#/c/11719/4/testdata/workloads/functional-planner/queries/PlannerTest/spillable-buffer-sizing.test File testdata/workloads/functional-planner/queries/PlannerTest/spillable-buffer-sizing.test: http://gerrit.cloudera.org:8080/#/c/11719/4/testdata/workloads/functional-planner/queries/PlannerTest/spillable-buffer-sizing.test@9 PS4, Line 9: -- +straight_join : * > Yes. Though I welcome your opinion. that's fine by me -- To view, visit http://gerrit.cloudera.org:8080/11719 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I55c3bdacc295137f66b2316a912fc347da30d6b0 Gerrit-Change-Number: 11719 Gerrit-PatchSet: 5 Gerrit-Owner: Andrew Sherman <[email protected]> Gerrit-Reviewer: Andrew Sherman <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Thomas Marshall <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Comment-Date: Thu, 01 Nov 2018 16:21:52 +0000 Gerrit-HasComments: Yes
