Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/11942 )
Change subject: IMPALA-7801: Remove toSql() from ParseNode interface. ...................................................................... Patch Set 2: (6 comments) Thanks Paul for the interesting review and discussion http://gerrit.cloudera.org:8080/#/c/11942/2/fe/src/main/java/org/apache/impala/analysis/Expr.java File fe/src/main/java/org/apache/impala/analysis/Expr.java: http://gerrit.cloudera.org:8080/#/c/11942/2/fe/src/main/java/org/apache/impala/analysis/Expr.java@395 PS2, Line 395: String sql = toSql(DEFAULT); > Let's think about the meaning of the options for expressions. Expr nodes wo I understand the point about Expr nodes but there is no simple solution. The ToSqlOption(s) does allow more complex behaviors by having different methods that can be called on a ToSqlOption object. Csaba also suggested having some sort of context object to be passed as well. Maybe that is closer to what you want. In this change I am just trying to remove the default toSql() methods. http://gerrit.cloudera.org:8080/#/c/11942/2/fe/src/main/java/org/apache/impala/analysis/Expr.java@1300 PS2, Line 1300: name, (printExpr) ? " '" + toSql(DEFAULT) + "'" : "", type_.toString())); > This comment applies in a zillion other places. Our general rule for error In this change, as requested by Vuk, I am removing the default toSql() method. He thought, and I think I agree, that forcing a ToSqlOption object parameter makes things more consistent. If we don't want that then maybe I should abandon this change. http://gerrit.cloudera.org:8080/#/c/11942/2/fe/src/main/java/org/apache/impala/analysis/ParseNode.java File fe/src/main/java/org/apache/impala/analysis/ParseNode.java: http://gerrit.cloudera.org:8080/#/c/11942/2/fe/src/main/java/org/apache/impala/analysis/ParseNode.java@33 PS2, Line 33: String toSql(ToSqlOptions options); > A common practice in Java is to include the enum definition in the interfac Nice suggestion, done. http://gerrit.cloudera.org:8080/#/c/11942/2/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java File fe/src/main/java/org/apache/impala/analysis/QueryStmt.java: http://gerrit.cloudera.org:8080/#/c/11942/2/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java@217 PS2, Line 217: "OFFSET requires an ORDER BY clause: " + limitElement_.toSql(DEFAULT).trim()); > I wonder if we can solve a mess here with exceptions also. Rather than buil Nice idea, but IMHO beyond the scope of this change. http://gerrit.cloudera.org:8080/#/c/11942/2/fe/src/main/java/org/apache/impala/planner/SortNode.java File fe/src/main/java/org/apache/impala/planner/SortNode.java: http://gerrit.cloudera.org:8080/#/c/11942/2/fe/src/main/java/org/apache/impala/planner/SortNode.java@44 PS2, Line 44: import static org.apache.impala.analysis.ToSqlOptions.DEFAULT; > Two suggestions. Rename is done in next patch. I am not sure I agree about importing the enum (i.e. not a static import of the needed enums). I think it is tidier to have just ORIGINAL in the code. As it is (now) a common idiom I believe people will get used to reading this correctly. http://gerrit.cloudera.org:8080/#/c/11942/2/fe/src/main/java/org/apache/impala/planner/SortNode.java@214 PS2, Line 214: output.append(info_.getSortExprs().get(i).toSql(DEFAULT) + " "); > This is an example of the point made elsewhere. It SEEMS we are asking the OK, thanks for explaining this. I will add another followup jira which will say something like: IMPALA-7801 changed the code so that all ParseNode trees can be asked to create sql by calling toSql(SqlOption.ORIGINAL) or toSql(SqlOption.REWRITTEN). Although an Expr node produces output when toSql(SqlOption.ORIGINAL) is called, it is a lie in that it is not the original sql. The current Expr may have replaced a tree of the original Expr objects during query rewrite. The replaced objects are gone, so we cannot reconstruct the "original" sql. A possible solution is to observe that the select-like ParseNodes and Expr ParseNodes have different semantics and to change the interface of toSql() to express this. -- To view, visit http://gerrit.cloudera.org:8080/11942 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I17025901838e9ffd753894a8087170123f9d8b33 Gerrit-Change-Number: 11942 Gerrit-PatchSet: 2 Gerrit-Owner: Andrew Sherman <asher...@cloudera.com> Gerrit-Reviewer: Andrew Sherman <asher...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Paul Rogers <par0...@yahoo.com> Gerrit-Reviewer: Thomas Marshall <thomasmarsh...@cmu.edu> Gerrit-Reviewer: Vuk Ercegovac <vercego...@cloudera.com> Gerrit-Comment-Date: Fri, 16 Nov 2018 23:46:37 +0000 Gerrit-HasComments: Yes
