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-3726: Add support for Kudu-specific column options
Date Thu, 17 Nov 2016 19:12:25 GMT
Dimitris Tsirogiannis has posted comments on this change.

Change subject: IMPALA-3726: Add support for Kudu-specific column options
......................................................................


Patch Set 7: Code-Review+2

(2 comments)

Rebase and fixing tests, carry Alex's +2

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

Line 129:   public ColumnDef(String colName, TypeDef typeDef) {
> Seems like we'd often need a C'tor that also takes a comment string. You ca
Not as simple as it sounds, we need to make sure we don't insert nulls and then populate the
map. But because the call to the delegating constructor needs to be the first statement that
needs to happen outside the constructor.


http://gerrit.cloudera.org:8080/#/c/5026/5/tests/query_test/test_kudu.py
File tests/query_test/test_kudu.py:

Line 250:   def test_primary_key_and_distribution(self, cursor):
> AUTO and DEFAULT still seem uninformative. Let's revisit later like you sug
I agree for encoding, not so much for compression since it can have no_compression. Anyways,
as you said, let's revisit this later.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I727b9ae1b7b2387db752b58081398dd3f3449c02
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message