impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Zoltan Ivanfi (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-784: Use `-s in SHOW CREATE TABLE output
Date Wed, 28 Sep 2016 20:48:06 GMT
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/

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

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
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I8e66439daa35d2e3725e7277c16f3cc5fdb6f128
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zoltan Ivanfi <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-Reviewer: Zoltan Ivanfi <>
Gerrit-HasComments: Yes

View raw message