impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Behm (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-784: Use `-s in SHOW CREATE TABLE output
Date Wed, 28 Sep 2016 16:21:43 GMT
Alex Behm has posted comments on this change.

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


Patch Set 2:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/4527/2/fe/src/main/java/com/cloudera/impala/analysis/ColumnDef.java
File fe/src/main/java/com/cloudera/impala/analysis/ColumnDef.java:

Line 112:       sb.append(" " + type_.toString());
toSql()


Line 114:       sb.append(" " + typeDef_.toString());
toSql()


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 not have quoted
identifiers. Unfortunately, we also use toSql() for error reporting. For example, take a look
at AnalyticExpr.java 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 this:

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

You can imagine other variants. Happy to discuss.


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 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 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 <zi@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Zoltan Ivanfi <zi@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message