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 <asher...@cloudera.com> Gerrit-Reviewer: Andrew Sherman <asher...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Thomas Marshall <thomasmarsh...@cmu.edu> Gerrit-Reviewer: Vuk Ercegovac <vercego...@cloudera.com> Gerrit-Comment-Date: Thu, 01 Nov 2018 20:32:36 +0000 Gerrit-HasComments: Yes