Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/23493 )
Change subject: IMPALA-14472: Add create/read support for ARRAY column of Kudu ...................................................................... Patch Set 3: (14 comments) http://gerrit.cloudera.org:8080/#/c/23493/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/23493/1//COMMIT_MSG@10 PS1, Line 10: initia > nit: initial Done http://gerrit.cloudera.org:8080/#/c/23493/1//COMMIT_MSG@10 PS1, Line 10: : support for working with Kudu tables having array type > nit: maybe, replace with Done http://gerrit.cloudera.org:8080/#/c/23493/1/be/src/exec/kudu/kudu-array-inserter.cc File be/src/exec/kudu/kudu-array-inserter.cc: http://gerrit.cloudera.org:8080/#/c/23493/1/be/src/exec/kudu/kudu-array-inserter.cc@45 PS1, Line 45: // id TINYINT PRIMARY KEY, : // array_INT ARRAY<INT>, > nit: these might be 'constexpr const char* const' Done http://gerrit.cloudera.org:8080/#/c/23493/1/be/src/exec/kudu/kudu-array-inserter.cc@79 PS1, Line 79: 'id' starts f > nit: is it meant to be KUDU_RETURN_NOT_OK or KUDU_CHECK_OK instead? Since Done http://gerrit.cloudera.org:8080/#/c/23493/1/be/src/exec/kudu/kudu-array-inserter.cc@124 PS1, Line 124: KUDU_RETURN_NOT_OK(KuduClientBuilder() : .add_master_server_addr(KUDU_MASTER_DEFAULT_ADDR) : .Build(&client)); > Since this interface was designed to be C++98-compatible (i.e. no std::uniq Done http://gerrit.cloudera.org:8080/#/c/23493/1/be/src/exec/kudu/kudu-array-inserter.cc@129 PS1, Line 129: > This doesn't make much sense: if there were errors, non of the statuses wou Changed to print to cerr. http://gerrit.cloudera.org:8080/#/c/23493/1/be/src/exec/kudu/kudu-array-inserter.cc@137 PS1, Line 137: KUDU_RETU > Does it make sense returning non-zero status if anything went wrong? Return 1 of status is not OK. http://gerrit.cloudera.org:8080/#/c/23493/1/be/src/exec/kudu/kudu-scanner.cc File be/src/exec/kudu/kudu-scanner.cc: http://gerrit.cloudera.org:8080/#/c/23493/1/be/src/exec/kudu/kudu-scanner.cc@402 PS1, Line 402: > readability nit: 'else' isn't necessary since 'if' above contains 'return' Done http://gerrit.cloudera.org:8080/#/c/23493/1/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/23493/1/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java@496 PS1, Line 496: String.format("Cannot create table '%s': Type %s is not supported in Kudu", : getTbl(), col.getType().toSql( > It seems 3rd parameter is missing (given the format string). It only need 2 parameter. Edit the code a bit for clarity. http://gerrit.cloudera.org:8080/#/c/23493/1/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/23493/1/fe/src/main/java/org/apache/impala/util/KuduUtil.java@441 PS1, Line 441: case SMA > readability nit: it's possible to omit the 'else' part of the clause becaus Done http://gerrit.cloudera.org:8080/#/c/23493/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java: http://gerrit.cloudera.org:8080/#/c/23493/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java@394 PS1, Line 394: AnalyzesOk("create table tab (x int primary key, y ARRAY<INT>) " : + "partition by hash(x) partitions 3 stored as kudu", : isExternalPurgeTbl > I think there is a TODO from Kudu on both: Left note about not throwing explicit error. http://gerrit.cloudera.org:8080/#/c/23493/1/testdata/cluster/node_templates/common/etc/kudu/master.conf.tmpl File testdata/cluster/node_templates/common/etc/kudu/master.conf.tmpl: http://gerrit.cloudera.org:8080/#/c/23493/1/testdata/cluster/node_templates/common/etc/kudu/master.conf.tmpl@26 PS1, Line 26: > This is not needed anymore as of commit here(doesn't hurt to have it): Done http://gerrit.cloudera.org:8080/#/c/23493/1/testdata/cluster/node_templates/common/etc/kudu/tserver.conf.tmpl File testdata/cluster/node_templates/common/etc/kudu/tserver.conf.tmpl: http://gerrit.cloudera.org:8080/#/c/23493/1/testdata/cluster/node_templates/common/etc/kudu/tserver.conf.tmpl@32 PS1, Line 32: > same as before Done http://gerrit.cloudera.org:8080/#/c/23493/1/testdata/datasets/functional/functional_schema_template.sql File testdata/datasets/functional/functional_schema_template.sql: http://gerrit.cloudera.org:8080/#/c/23493/1/testdata/datasets/functional/functional_schema_template.sql@4820 PS1, Line 4820: ---- BASE_TABLE_NAME > I'm curious: what drives the selection of array element types for this test Added DOUBLE, STRING, and BOOLEAN. I think those are quite representative. Moved the DDL query to TestKuduArray so that it can be combined with test vectors and run concurrently in separate unique database. -- To view, visit http://gerrit.cloudera.org:8080/23493 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9282aac821bd30668189f84b2ed8fff7047e7310 Gerrit-Change-Number: 23493 Gerrit-PatchSet: 3 Gerrit-Owner: Riza Suminto <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Reviewer: Xuebin Su <[email protected]> Gerrit-Comment-Date: Sat, 04 Oct 2025 04:21:46 +0000 Gerrit-HasComments: Yes
