Mailing-List: contact reviews-help@impala.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201801022230.w02MU8qq000772@ip-10-146-233-104.ec2.internal>
Date: Tue, 2 Jan 2018 22:30:06 +0000
From: "Dimitris Tsirogiannis (Code Review)" <gerrit@cloudera.org>
To: Gabor Kaszab <gaborkaszab@cloudera.com>, impala-cr@cloudera.com,
        reviews@impala.incubator.apache.org
CC: Alex Behm <alex.behm@cloudera.com>,
        Tim Armstrong <tarmstrong@cloudera.com>,
        Attila Jeges <attilaj@cloudera.com>,
        Laszlo Gaal <laszlo.gaal@cloudera.com>,
        Csaba Ringhofer <csringhofer@cloudera.com>,
        Zoltan Borok-Nagy <boroknagyz@cloudera.com>
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-5654=3A_Disallow_setting_Kudu_table_name_in_CREATE_TABLE=0A?=
In-Reply-To: <gerrit.1513069043000.Ieca037498abf8f5fde67b77e824b720482cdbe6f@gerrit.cloudera.org>
References: <gerrit.1513069043000.Ieca037498abf8f5fde67b77e824b720482cdbe6f@gerrit.cloudera.org>
Reply-To: dtsirogiannis@cloudera.com, impala-cr@cloudera.com,
        boroknagyz@cloudera.com, gaborkaszab@cloudera.com, marcelk@gmail.com,
        tarmstrong@cloudera.com, laszlo.gaal@cloudera.com,
        csringhofer@cloudera.com, attilaj@cloudera.com, alex.behm@cloudera.com,
        reviews@impala.incubator.apache.org
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.14.2
Content-Type: multipart/alternative; boundary="doFsVJTJvwE="; charset=UTF-8
archived-at: Tue, 02 Jan 2018 22:30:16 -0000

--doFsVJTJvwE=
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

Dimitris Tsirogiannis has posted comments on this change=2E ( http://gerrit=
=2Ecloudera=2Eorg:8080/8820 )

Change subject: IMPALA-5654: Disallow settin=
g Kudu table name in CREATE TABLE
=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=
=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=
=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=
=2E=2E=2E=2E=2E=2E=2E


Patch Set 5:

(6 comments)

http://gerrit=2Eclouder=
a=2Eorg:8080/#/c/8820/5/fe/src/main/java/org/apache/impala/analysis/CreateT=
ableStmt=2Ejava
File fe/src/main/java/org/apache/impala/analysis/CreateTabl=
eStmt=2Ejava:

http://gerrit=2Ecloudera=2Eorg:8080/#/c/8820/5/fe/src/main/j=
ava/org/apache/impala/analysis/CreateTableStmt=2Ejava@67
PS5, Line 67:  // =
Table properties generated during analysis
There are other table properties=
 that get generated during the analysis that are not stored here (e=2Eg=2E =
L264)=2E So, the use of 'generatedTblProperties' map is kind of inconsisten=
t=2E


http://gerrit=2Ecloudera=2Eorg:8080/#/c/8820/5/fe/src/main/java/org/=
apache/impala/analysis/CreateTableStmt=2Ejava@68
PS5, Line 68: generatedTbl=
Properties
generatedTblProperties_ (private fields must end with a '_')


h=
ttp://gerrit=2Ecloudera=2Eorg:8080/#/c/8820/5/fe/src/main/java/org/apache/i=
mpala/analysis/CreateTableStmt=2Ejava@68
PS5, Line 68: new HashMap<String, =
String>()
use Guava's (Maps=2EnewHashMap()) here and everywhere you allocat=
e a HashMap=2E


http://gerrit=2Ecloudera=2Eorg:8080/#/c/8820/5/fe/src/main=
/java/org/apache/impala/analysis/CreateTableStmt=2Ejava@179
PS5, Line 179: =
The generated values overwrite the user provided values
Not sure about the =
validity of this statement=2E I'd expect user provided values (if allowed) =
to overwrite generated ones=2E


http://gerrit=2Ecloudera=2Eorg:8080/#/c/88=
20/5/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt=2Ejava@313=

PS5, Line 313: // If re-analyzing this statement then the Kudu table name =
had already been
             :     // populated, hence skipping this funct=
ionality=2E
Not following that comment=2E I don't see anything specific to =
re-analyzing statement or skipping some functionality in the code that foll=
ows=2E


http://gerrit=2Ecloudera=2Eorg:8080/#/c/8820/5/fe/src/main/java/or=
g/apache/impala/analysis/CreateTableStmt=2Ejava@315
PS5, Line 315: setManag=
edKuduTableName();
inline function (it only has 2 statements and it's not u=
sed anywhere else)=2E


-- 
To view, visit http://gerrit=2Ecloudera=2Eorg:=
8080/8820
To unsubscribe, visit http://gerrit=2Ecloudera=2Eorg:8080/setting=
s

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: com=
ment
Gerrit-Change-Id: Ieca037498abf8f5fde67b77e824b720482cdbe6f
Gerrit-Cha=
nge-Number: 8820
Gerrit-PatchSet: 5
Gerrit-Owner: Gabor Kaszab <gaborkaszab=
@cloudera=2Ecom>
Gerrit-Reviewer: Alex Behm <alex=2Ebehm@cloudera=2Ecom>
Ge=
rrit-Reviewer: Attila Jeges <attilaj@cloudera=2Ecom>
Gerrit-Reviewer: Csaba=
 Ringhofer <csringhofer@cloudera=2Ecom>
Gerrit-Reviewer: Dimitris Tsirogian=
nis <dtsirogiannis@cloudera=2Ecom>
Gerrit-Reviewer: Gabor Kaszab <gaborkasz=
ab@cloudera=2Ecom>
Gerrit-Reviewer: Laszlo Gaal <laszlo=2Egaal@cloudera=2Ec=
om>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera=2Ecom>
Gerrit-Revie=
wer: Zoltan Borok-Nagy <boroknagyz@cloudera=2Ecom>
Gerrit-Comment-Date: Tue=
, 02 Jan 2018 22:30:06 +0000
Gerrit-HasComments: Yes

--doFsVJTJvwE=--