Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201609281621.u8SGLhaO005231@ip-10-146-233-104.ec2.internal>
Date: Wed, 28 Sep 2016 16:21:43 +0000
From: "Alex Behm (Code Review)" <gerrit@cloudera.org>
To: Zoltan Ivanfi <zi@cloudera.com>, impala-cr@cloudera.com,
        reviews@impala.incubator.apache.org
CC: Tim Armstrong <tarmstrong@cloudera.com>
Reply-To: alex.behm@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-784=3A_Use_=60-s_in_SHOW_CREATE_TABLE_output=0A?=
In-Reply-To: <gerrit.1474653837000.I8e66439daa35d2e3725e7277c16f3cc5fdb6f128@gerrit.cloudera.org>
References: <gerrit.1474653837000.I8e66439daa35d2e3725e7277c16f3cc5fdb6f128@gerrit.cloudera.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.12.2
archived-at: Wed, 28 Sep 2016 16:21:56 -0000

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