impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Matthew Jacobs (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-3724: Support Kudu non-covering range partitions
Date Wed, 02 Nov 2016 17:52:39 GMT
Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-3724: Support Kudu non-covering range partitions
......................................................................


Patch Set 3:

(5 comments)

nice!

http://gerrit.cloudera.org:8080/#/c/4856/3/fe/src/test/java/org/apache/impala/analysis/ParserTest.java
File fe/src/test/java/org/apache/impala/analysis/ParserTest.java:

Line 1632:     //TestUtils.assumeKuduIsSupported();
?


http://gerrit.cloudera.org:8080/#/c/4856/3/testdata/workloads/functional-query/queries/QueryTest/kudu_create.test
File testdata/workloads/functional-query/queries/QueryTest/kudu_create.test:

PS3, Line 82: create table tab (a int primary key) distributed by range (a) (partition value
= false)
            : stored as kudu
            : ---- CATCH
            : ImpalaRuntimeException: Expected 'int32' literal for column 'a' got 'BOOLEAN'
can this be an analysis FE test?


http://gerrit.cloudera.org:8080/#/c/4856/3/testdata/workloads/functional-query/queries/QueryTest/kudu_crud.test
File testdata/workloads/functional-query/queries/QueryTest/kudu_crud.test:

1. Can you add a case to exercise all supported key types (I believe that's all INTs and STRING)
in the partition syntax?

2. I think we should have a case where there's a gap too, so maybe this table for #1 could
do both.

E.g. 

create table allkeytypes
  (i1 tinyint, i2 smallint, i3 int, i4 bigint, name string, valf float, vali bigint, primary
key(i1,i2,i3,i4,name) )
  DISTRIBUTE BY RANGE (PARTITION VALUES < (1, 1, 1, 1, "1") , PARTITION (10, 10, 10, 10,
"10") <= VALUES < (100, 100, 100, 100, "100")) STORED AS KUDU

inserting 0 through 10 (all pk cols) should result in 2 rows being added.


3. Include a HASH partition first? Maybe that can just be added to the example above.


PS3, Line 21: varchar(20)
string


Line 348: INSERT IGNORE INTO kudu_test_tbl SELECT cast(id + 10000 as int), bool_col, tinyint_col,
> Let's take this up with MJ and see whether this is the behavior we want wit
Yeah, we'll eventually have to remove a few of these cases as we discussed. Likely this will
go in first so this is fine to get in as-is, and the tests can be fixed when merging.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6799c01a37003f0f4c068d911a13e3f060110a06
Gerrit-PatchSet: 3
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-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message