Zoltan Ivanfi has posted comments on this change.

Change subject: IMPALA-784: Use `-s in SHOW CREATE TABLE output

Patch Set 2:


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 
> 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

Reply via email to