Dimitris Tsirogiannis has posted comments on this change. ( http://gerrit.cloudera.org:8080/9368 )
Change subject: IMPALA-5752: Add support for DECIMAL on Kudu tables ...................................................................... Patch Set 4: (10 comments) http://gerrit.cloudera.org:8080/#/c/9368/3/fe/src/main/java/org/apache/impala/util/KuduUtil.java File fe/src/main/java/org/apache/impala/util/KuduUtil.java: http://gerrit.cloudera.org:8080/#/c/9368/3/fe/src/main/java/org/apache/impala/util/KuduUtil.java@190 PS3, Line 190: E.g. if 'type' is 'INT8', the returned : * value is a Java byte, and if 'type' is 'UNIXTIME_MICROS', the returned value is : * a Java long. Update the comment (input param is no longer a Kudu.Type). http://gerrit.cloudera.org:8080/#/c/9368/4/fe/src/main/java/org/apache/impala/util/KuduUtil.java File fe/src/main/java/org/apache/impala/util/KuduUtil.java: http://gerrit.cloudera.org:8080/#/c/9368/4/fe/src/main/java/org/apache/impala/util/KuduUtil.java@190 PS4, Line 190: E.g. if 'type' is 'INT8', the returned : * value is a Java byte, and if 'type' is 'UNIXTIME_MICROS', the returned value is : * a Java long. Update comment ('type' is renamed and it's not a kudu type). http://gerrit.cloudera.org:8080/#/c/9368/4/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java: http://gerrit.cloudera.org:8080/#/c/9368/4/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@a1809 PS4, Line 1809: : : Why not converting it into a positive test? Do you have a CTAS test with decimal somewhere else? http://gerrit.cloudera.org:8080/#/c/9368/4/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@2157 PS4, Line 2157: public void TestAlterKuduTable() { I think we need to add some tests here as well since a new column type is added. http://gerrit.cloudera.org:8080/#/c/9368/4/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@2498 PS4, Line 2498: AnalyzesOk("create table tab (x int primary key, i1 tinyint default null, " : + "i2 smallint default null, i3 int default null, i4 bigint default null, " : + "vals string default null, valf float default null, vald double default null, " : + "valb boolean default null, valdec decimal(10, 5) default null) " : + "partition by hash (x) partitions 3 stored as kudu"); nit: plz don't change the style wrt '+' (always at the end of the line) http://gerrit.cloudera.org:8080/#/c/9368/4/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@2531 PS4, Line 2531: + "i3 int default 100, i4 bigint default 1000, vals string default 'test', " : + "valf float default cast(1.2 as float), vald double default " : + "cast(3.1452 as double), valb boolean default true, " : + "valdec decimal(10, 5) default 3.14159, " : + "primary key (i1, i2, i3, i4, vals)) partition by hash (i1) partitions 3 " : + "stored as kudu"); same here http://gerrit.cloudera.org:8080/#/c/9368/4/testdata/workloads/functional-query/queries/QueryTest/kudu_create.test File testdata/workloads/functional-query/queries/QueryTest/kudu_create.test: http://gerrit.cloudera.org:8080/#/c/9368/4/testdata/workloads/functional-query/queries/QueryTest/kudu_create.test@293 PS4, Line 293: ---- QUERY : # Creates a Kudu table with decimal columns and primary key : create table create_decimal_test : ( : decimal_4 decimal(9, 9), : decimal_8 decimal(18, 2) not null default 100.00, : decimal_16 decimal(38, 0) null, : primary key (decimal_4)) : stored as kudu; : insert into create_decimal_test (decimal_4) values (0.111111111) : ---- RUNTIME_PROFILE : NumModifiedRows: 1 : NumRowErrors: 0 : ---- LABELS : DECIMAL_4, DECIMAL_8, DECIMAL_16 : ---- DML_RESULTS: create_decimal_test : 0.111111111,100.00,NULL : ---- TYPES : DECIMAL,DECIMAL,DECIMAL This looks more like an insert test (shouldn't it be in kudu_insert.test?). Also, you should add a CTAS test with decimal (couldn't find any). http://gerrit.cloudera.org:8080/#/c/9368/4/testdata/workloads/functional-query/queries/QueryTest/kudu_delete.test File testdata/workloads/functional-query/queries/QueryTest/kudu_delete.test: http://gerrit.cloudera.org:8080/#/c/9368/4/testdata/workloads/functional-query/queries/QueryTest/kudu_delete.test@299 PS4, Line 299: ==== Can you add some tests in which the predicate is on a decimal column? http://gerrit.cloudera.org:8080/#/c/9368/4/testdata/workloads/functional-query/queries/QueryTest/kudu_describe.test File testdata/workloads/functional-query/queries/QueryTest/kudu_describe.test: http://gerrit.cloudera.org:8080/#/c/9368/4/testdata/workloads/functional-query/queries/QueryTest/kudu_describe.test@54 PS4, Line 54: decimal_16 decimal(38, 0) null) Add another column "decimal_default decimal null" http://gerrit.cloudera.org:8080/#/c/9368/4/testdata/workloads/functional-query/queries/QueryTest/kudu_upsert.test File testdata/workloads/functional-query/queries/QueryTest/kudu_upsert.test: http://gerrit.cloudera.org:8080/#/c/9368/4/testdata/workloads/functional-query/queries/QueryTest/kudu_upsert.test@320 PS4, Line 320: ---- QUERY Add an upsert select with a predicate on a decimal column -- To view, visit http://gerrit.cloudera.org:8080/9368 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3a9fe5acadc53ec198585d765a8cfb0abe56e199 Gerrit-Change-Number: 9368 Gerrit-PatchSet: 4 Gerrit-Owner: Grant Henke <granthe...@gmail.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogian...@cloudera.com> Gerrit-Reviewer: Grant Henke <granthe...@gmail.com> Gerrit-Reviewer: Taras Bobrovytsky <tbobrovyt...@cloudera.com> Gerrit-Reviewer: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-Comment-Date: Wed, 21 Feb 2018 23:29:51 +0000 Gerrit-HasComments: Yes
