Michael Smith 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 15: (7 comments) http://gerrit.cloudera.org:8080/#/c/23493/15/be/src/exec/kudu/kudu-array-inserter.cc File be/src/exec/kudu/kudu-array-inserter.cc: http://gerrit.cloudera.org:8080/#/c/23493/15/be/src/exec/kudu/kudu-array-inserter.cc@67 PS15, Line 67: const vector<Slice> UTF8_ARRAY = {u8"Σ", u8"π", u8"λ"}; Maybe a comment these are selected to test multi (2) byte characters? http://gerrit.cloudera.org:8080/#/c/23493/15/be/src/exec/kudu/kudu-array-inserter.cc@164 PS15, Line 164: // The array is longer than those in the previous rows. I think it'd also be useful to test a really big array. Are there limits to what Kudu will store? http://gerrit.cloudera.org:8080/#/c/23493/5/be/src/exec/kudu/kudu-scanner.cc File be/src/exec/kudu/kudu-scanner.cc: http://gerrit.cloudera.org:8080/#/c/23493/5/be/src/exec/kudu/kudu-scanner.cc@400 PS5, Line 400: DCHECK(slot->type().type == TYPE_TIMESTAMP); > Thanks! Changed. You expect that to help with branch prediction? http://gerrit.cloudera.org:8080/#/c/23493/15/be/src/exec/kudu/kudu-scanner.cc File be/src/exec/kudu/kudu-scanner.cc: http://gerrit.cloudera.org:8080/#/c/23493/15/be/src/exec/kudu/kudu-scanner.cc@95 PS15, Line 95: kudu_array_element_byte_sizes_.resize( Can you add a comment about pre-computing the element size here, since it's stable for the whole column? http://gerrit.cloudera.org:8080/#/c/23493/15/be/src/exec/kudu/kudu-scanner.cc@395 PS15, Line 395: // TODO: avoid mem copies with a Kudu mem 'release' mechanism, attaching mem to the Do we need a ticket to follow up on these? http://gerrit.cloudera.org:8080/#/c/23493/15/fe/src/main/java/org/apache/impala/analysis/FromClause.java File fe/src/main/java/org/apache/impala/analysis/FromClause.java: http://gerrit.cloudera.org:8080/#/c/23493/15/fe/src/main/java/org/apache/impala/analysis/FromClause.java@179 PS15, Line 179: // TODO: Support referencing a Kudu collection column as a table. Is there a ticket we should mention? http://gerrit.cloudera.org:8080/#/c/23493/15/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/15/fe/src/main/java/org/apache/impala/util/KuduUtil.java@429 PS15, Line 429: * Returns null if the type cannot be converted. I'm not sure why the function was switched from throwing an exception to returning null. We moved having to deal with throwing the same exception from a couple places in this function to having to check and throw a couple places in other classes. Was there some other reason to change it? -- 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: 15 Gerrit-Owner: Riza Suminto <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Kurt Deschler <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Reviewer: Xuebin Su <[email protected]> Gerrit-Comment-Date: Wed, 05 Nov 2025 22:02:34 +0000 Gerrit-HasComments: Yes
