Alexey Serbin 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: (6 comments) 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( : scan_node_->tuple_desc()->collection_slots().size()); Is it possible to call KuduScanner::Open() on the same object again? If so, what happens with the contents of the 'kudu_array_element_byte_sizes_' member field, and is any unexpected behavior possible in the case when kudu_array_element_byte_sizes_ contains elements from prior incarnation of the object? http://gerrit.cloudera.org:8080/#/c/23493/15/be/src/exec/kudu/kudu-scanner.cc@401 PS15, Line 401: slot->is_nullable() && kudu_tuple->IsNull(slot->null_indicator_offset()) Is it possible this method is called on a slot which isn't nullable, but kudu_tuple->IsNull(...) still evaluates to 'true' (e.g., after this method set kudu_tuple to NULL after TIMESTAMP_OUT_OF_RANGE error)? Would the code in this method behave as expected then (e.g., would not crash)? http://gerrit.cloudera.org:8080/#/c/23493/15/be/src/exec/kudu/kudu-scanner.cc@433 PS15, Line 433: slot->is_nullable() && kudu_tuple->IsNull(slot->null_indicator_offset() Could it happen that slot->is_nullable() evaluates to 'false', but kudu_tuple->IsNull(slot->null_indicator_offset()) evaluates to true? Is there a guarantee that upper level enforces consistency and this cannot happen? If not, does it make sense to handle such a situation in the code below then? http://gerrit.cloudera.org:8080/#/c/23493/15/be/src/exec/kudu/kudu-scanner.cc@552 PS15, Line 552: !status.ok() nit: wrap this into UNLIKELY()? http://gerrit.cloudera.org:8080/#/c/23493/15/be/src/exec/kudu/kudu-scanner.cc@559 PS15, Line 559: !status.ok() nit: wrap this into UNLIKELY()? at least, per current implementation of ConvertVarcharFromKudu() this cannot return non-OK status 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 re +1 With this update, it seems non-uniform from the API consistency standpoint as well, especially when toImpalaScalarType() throws under semantically similar conditions instead of returning null. -- 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:16:21 +0000 Gerrit-HasComments: Yes
