Zoltan Ivanfi has posted comments on this change. Change subject: IMPALA-784: Use `-s in SHOW CREATE TABLE output ......................................................................
Patch Set 2: (3 comments) http://gerrit.cloudera.org:8080/#/c/4527/2/fe/src/main/java/com/cloudera/impala/analysis/ToSqlUtils.java File fe/src/main/java/com/cloudera/impala/analysis/ToSqlUtils.java: Line 69: public static String getIdentSql(String ident) { > The main issue with this approach is that all invocations of toSql() will n Initially I was also worried that identifiers become "ugly" and considered making this behavior optional. I was thinking of a query option that the user can control using the SET statement. Then I checked how Hive works and I found that it already quotes every identifier (I checked the table and view definitions if I remember correctly), so I came to the conclusion that quoting everything should be okay. Of course I expected some suggestions to how it should work, that's why I didn't fix the styling issues in the test. Line 76: return Joiner.on(".").join(getStandaloneIdentSqlList((Splitter.on('.').split(ident)))); > I don't think splitting here is the right fix. An identifier itself could c I checked that identifiers can't contain dots, or at least users can't create such identifiers. If you try specifying `a.b` as an identifier, Impala will complain that dots are not allowed in identifier names. Do you know of a code piece that gets around this mechanism and creates identifiers with dots in some other way? Qualified table or column references are exactly the reason why I only made this version public and the other one private, as only this is safe to use to quote table or column references. In case of complex types, dots can become a part of either the table reference or the column reference. In case of a struct, a column reference might look like column_name.field_name, which should be quoted as `column_name`.`field_name`. In case of a map or array, table references might look like table_name.map_or_array_name, which should be quoted `table_name`.`map_or_array_name`. This is my understanding based on the short amount of time I spent on Impala, so please correct me if I'm wrong. Line 88: if (ident.equals("*")) { > This doesn't seem right or necessary. First, "*" is not an identifier (it i Without this check a SELECT * became SELECT `*` which is invalid. -- To view, visit http://gerrit.cloudera.org:8080/4527 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8e66439daa35d2e3725e7277c16f3cc5fdb6f128 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Zoltan Ivanfi <z...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Zoltan Ivanfi <z...@cloudera.com> Gerrit-HasComments: Yes
