Andrew Sherman 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 4:
(1 comment)
I agree that maintenance is a concern.
1) for ParseNode I could remove toSql() from the interface and force all
callers to use toSql(DEFAULT). This is easy in terms of effort as IntelliJ will
do it for me, but results in a lot of code changes, especially as many
exception messages call toSql(). I did consider doing this originally but
wanted to make this change as clear as possible. I can do this quickly in a
follow-up jira. There are few more associated classes in analysis which would
also be changed.
2) Do you want me to look at changes to toSql() that are not part of the
ParseNode interface (or associated classes)?
There is org.apache.impala.catalog.Type which has toSql() {return toSql(1)}
where the argument controls how deeply we show nested Types.
There is org.apache.impala.analysis.TableName.toSql() and
org.apache.impala.analysis.PlanHint.toSql()
Right now I would propose leaving these as is, but they could also be changed
if we wanted to never have a naked toSql() call.
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
> ok, just mention it in a brief comment. I prefer such inline constants to h
I removed it.
--
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: 4
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 20:32:36 +0000
Gerrit-HasComments: Yes
