Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/12340 )
Change subject: IMPALA-8156: Add format options to the EXPLAIN statement ...................................................................... Patch Set 1: (7 comments) I think this is super helpful with debugging. Glad we will be able to pass explain opts inline with the EXPLAIN statement rather than as a session option. It helps to see the EXPANDED and REWRITTEN forms of the query, especially with complex queries containing lots of views. It probably helps to clarify what is the difference between these two formats with some examples. ** After going through the parser changes, I figured this is a preview patch. I still preserved my comments there for the record. http://gerrit.cloudera.org:8080/#/c/12340/1/fe/src/main/cup/sql-parser.cup File fe/src/main/cup/sql-parser.cup: http://gerrit.cloudera.org:8080/#/c/12340/1/fe/src/main/cup/sql-parser.cup@740 PS1, Line 740: setExplain call it setExplainOpts()? http://gerrit.cloudera.org:8080/#/c/12340/1/fe/src/main/cup/sql-parser.cup@743 PS1, Line 743: KW_EXPLAIN KW_FORMAT LPAREN RPAREN explainable_stmt:stmt : {: : stmt.setExplain(new ExplainOptions()); : RESULT = stmt; : :} Handle this in the explain_opts below, as an empty case? http://gerrit.cloudera.org:8080/#/c/12340/1/fe/src/main/cup/sql-parser.cup@773 PS1, Line 773: HashMap Use a LinkedHashMap to preserve the order, just incase we need to break the ties somewhere. http://gerrit.cloudera.org:8080/#/c/12340/1/fe/src/main/cup/sql-parser.cup@776 PS1, Line 776: explain_opts btw, we do have a properties_map below incase you didn't notice it already. It is for string:string mapping though. http://gerrit.cloudera.org:8080/#/c/12340/1/fe/src/main/cup/sql-parser.cup@790 PS1, Line 790: KW_FORMAT EQUAL STRING_LITERAL:value : {: RESULT = new Pair<String,String>(ExplainOptions.FORMAT_KEY, value); :} I don't understand this. FORMAT (FORMAT="foo")? http://gerrit.cloudera.org:8080/#/c/12340/1/fe/src/main/cup/sql-parser.cup@799 PS1, Line 799: STRING_LITERAL:value : {: RESULT = new Pair<String,String>(ExplainOptions.FORMAT_KEY, value); :} : ; nit: Multiple keys seem to be overwriting each other, especially if there are multiple STRING_LITERALs. http://gerrit.cloudera.org:8080/#/c/12340/1/fe/src/main/java/org/apache/impala/analysis/ToSqlOptions.java File fe/src/main/java/org/apache/impala/analysis/ToSqlOptions.java: http://gerrit.cloudera.org:8080/#/c/12340/1/fe/src/main/java/org/apache/impala/analysis/ToSqlOptions.java@41 PS1, Line 41: SHOW_IMPLICIT_CASTS(true, true), : EXPANDED(true, true); Could you clarify what is the difference between EXPANDED vs REWRITTEN? Probably give a couple of examples where EXPANDED is different from REWRITTEN (and when to use them) -- To view, visit http://gerrit.cloudera.org:8080/12340 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I84a8bc4fbff52b70a747f3a3c08abe6973e37fc1 Gerrit-Change-Number: 12340 Gerrit-PatchSet: 1 Gerrit-Owner: Paul Rogers <prog...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Paul Rogers <prog...@cloudera.com> Gerrit-Comment-Date: Thu, 07 Feb 2019 07:38:25 +0000 Gerrit-HasComments: Yes
