Alex Behm 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 112:       sb.append(" " + type_.toString());

Line 114:       sb.append(" " + typeDef_.toString());
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 not 
have quoted identifiers. Unfortunately, we also use toSql() for error 
reporting. For example, take a look at or any other Expr. If 
analyze() fails we typically we print the offending expr with toSql().

Error messages will now become unnecessarily weird.

I think the callers need to decide the quoting policy.

We should have two versions of toSql():
1. Only adds quotes if necessary for Impala (no need to consider Hive as before)
2. Always quote identifiers

Imo, the default toSql() should have the first behavior, and we may add another 
version that always quotes identifiers. For example, we could have a setup like 

private String toSql(boolean quoteAllIdents);
private String toSql() { return toSql(false); }

You can imagine other variants. Happy to discuss.

Line 76:     return 
I don't think splitting here is the right fix. An identifier itself could 
contain a dot, even if unlikely. Also, qualified table or column references 
consist of multiple identifiers and not a single identifier, so the premise of 
this fix is kind of wrong (based on your comment).

Line 88:     if (ident.equals("*")) {
This doesn't seem right or necessary. First, "*" is not an identifier (it is a 
terminal), and second those clauses that could contain "*" should handle it 
specially in toSql().

For my understanding, what use case is this change addressing?

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

Reply via email to