impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dimitris Tsirogiannis (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5654: Disallow setting Kudu table name in CREATE TABLE
Date Tue, 02 Jan 2018 22:30:06 GMT
Dimitris Tsirogiannis has posted comments on this change. ( http://gerrit.cloudera.org:8080/8820
)

Change subject: IMPALA-5654: Disallow setting Kudu table name in CREATE TABLE
......................................................................


Patch Set 5:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/8820/5/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
File fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java:

http://gerrit.cloudera.org:8080/#/c/8820/5/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java@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.g. L264). So, the use of 'generatedTblProperties' map is kind of inconsistent.


http://gerrit.cloudera.org:8080/#/c/8820/5/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java@68
PS5, Line 68: generatedTblProperties
generatedTblProperties_ (private fields must end with a '_')


http://gerrit.cloudera.org:8080/#/c/8820/5/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java@68
PS5, Line 68: new HashMap<String, String>()
use Guava's (Maps.newHashMap()) here and everywhere you allocate a HashMap.


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


http://gerrit.cloudera.org:8080/#/c/8820/5/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java@313
PS5, Line 313: // If re-analyzing this statement then the Kudu table name had already been
             :     // populated, hence skipping this functionality.
Not following that comment. I don't see anything specific to re-analyzing statement or skipping
some functionality in the code that follows.


http://gerrit.cloudera.org:8080/#/c/8820/5/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java@315
PS5, Line 315: setManagedKuduTableName();
inline function (it only has 2 statements and it's not used anywhere else).



-- 
To view, visit http://gerrit.cloudera.org:8080/8820
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieca037498abf8f5fde67b77e824b720482cdbe6f
Gerrit-Change-Number: 8820
Gerrit-PatchSet: 5
Gerrit-Owner: Gabor Kaszab <gaborkaszab@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Attila Jeges <attilaj@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <csringhofer@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <gaborkaszab@cloudera.com>
Gerrit-Reviewer: Laszlo Gaal <laszlo.gaal@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <boroknagyz@cloudera.com>
Gerrit-Comment-Date: Tue, 02 Jan 2018 22:30:06 +0000
Gerrit-HasComments: Yes

Mime
  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message