Grant Henke 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 5: (12 comments) http://gerrit.cloudera.org:8080/#/c/9368/3/be/src/exec/kudu-util.h File be/src/exec/kudu-util.h: http://gerrit.cloudera.org:8080/#/c/9368/3/be/src/exec/kudu-util.h@78 PS3, Line 78: l_type > that needs updating (param is 'col_type'). But also I think you should not Done http://gerrit.cloudera.org:8080/#/c/9368/3/be/src/exec/kudu-util.h@85 PS3, Line 85: l_type > same Done 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: For example, The `impalaType` is : * translated to a Kudu Type 'type' and if 'type' is 'INT8', the returned : * value is a J > Update the comment (input param is no longer a Kudu.Type). Done 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: For example, The `impalaType` is : * translated to a Kudu Type 'type' and if 'type' is 'INT8', the returned : * value is a J > Update comment ('type' is renamed and it's not a kudu type). Done 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 de Done http://gerrit.cloudera.org:8080/#/c/9368/4/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@2157 PS4, Line 2157: "SORT BY (d)", "SORT BY colu > I think we need to add some tests here as well since a new column type is a Done http://gerrit.cloudera.org:8080/#/c/9368/4/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@2498 PS4, Line 2498: } else { : AnalyzesOk(createTblStr); : } : } : } > nit: plz don't change the style wrt '+' (always at the end of the line) Done http://gerrit.cloudera.org:8080/#/c/9368/4/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@2531 PS4, Line 2531: "value 'INVALID_ENC'. Supported encoding values are: " + : Joiner.on(", ").join(Encoding.values())); : // Unsupported compression algorithm : AnalysisError("create table tab (x int primary key, y int compression " + : "invalid_comp) partition by hash (x) partitions 3 stored as kudu", : "Unsupported compres > same here Done 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 : ( : 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; : ---- RESULTS : ==== : ---- QUERY : # Create as select table with decimal columns and primary key : create table ctas_decimal primary key (d3) : stored as kudu : as select * from functional.decimal_tbl; : select * from ctas_decimal; : ---- RESULTS : 1234,2222,1.23456789,.1 > This looks more like an insert test (shouldn't it be in kudu_insert.test?). When I wrote this I hadn't fixed the workload data loading issue for decimal_tiny and decimal_tbl. Will change this to just the create statement and add a ctas version. 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: INT, > Can you add some tests in which the predicate is on a decimal column? Done 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_8 decimal(18, 2) not null default 100.00, > Add another column "decimal_default decimal null" Done 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 Done -- 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: 5 Gerrit-Owner: Grant Henke <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Taras Bobrovytsky <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-Comment-Date: Thu, 22 Feb 2018 03:56:14 +0000 Gerrit-HasComments: Yes
