Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/21269 )
Change subject: IMPALA-12973,IMPALA-11491,IMPALA-12651: Support BINARY nested in complex types in select list ...................................................................... Patch Set 2: (7 comments) http://gerrit.cloudera.org:8080/#/c/21269/1/be/src/exec/iceberg-metadata/iceberg-row-reader.cc File be/src/exec/iceberg-metadata/iceberg-row-reader.cc: http://gerrit.cloudera.org:8080/#/c/21269/1/be/src/exec/iceberg-metadata/iceberg-row-reader.cc@135 PS1, Line 135: DCHECK(false) << "Unsupported column type: " << slot_desc->type().type; > Are there still unsupported types that are intentionally nulled? How is thi There aren't any more types that we don't support but are present in metadata tables (TINYINT and SHORTINT are not Iceberg types). In our tests we have "select *" for all metadata tables so if Iceberg later adds e.g. a DATE field our tests will catch it. Added a DCHECK instead. http://gerrit.cloudera.org:8080/#/c/21269/1/be/src/exec/iceberg-metadata/iceberg-row-reader.cc@187 PS1, Line 187: const jobject &accessed_value, void* slot, MemPool* tuple_data_pool) { > line too long (92 > 90) Done http://gerrit.cloudera.org:8080/#/c/21269/1/be/src/exec/iceberg-metadata/iceberg-row-reader.cc@198 PS1, Line 198: RETURN_IF_ERROR(metadata_scanner_->ConvertJavaByteBufferToByteArray( > tl/dr: The binary handling seems good to me, but I have concerns about the I agree with what you say, let's see if we have capacity to do it. Please create a Jira for it. Thanks. http://gerrit.cloudera.org:8080/#/c/21269/1/be/src/exec/iceberg-metadata/iceberg-row-reader.cc@209 PS1, Line 209: > nit: extra ; Done http://gerrit.cloudera.org:8080/#/c/21269/1/be/src/rpc/jni-thrift-util.h File be/src/rpc/jni-thrift-util.h: http://gerrit.cloudera.org:8080/#/c/21269/1/be/src/rpc/jni-thrift-util.h@62 PS1, Line 62: } > With JniByteArrayGuard we could just return DeserializeThriftMsg directly Done http://gerrit.cloudera.org:8080/#/c/21269/1/be/src/util/jni-util.h File be/src/util/jni-util.h: http://gerrit.cloudera.org:8080/#/c/21269/1/be/src/util/jni-util.h@115 PS1, Line 115: /// is more restricted, see https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/functions.html#GetPrimitiveArrayCritical_ReleasePrimitiveArrayCritical > line too long (162 > 90) Done http://gerrit.cloudera.org:8080/#/c/21269/1/testdata/workloads/functional-query/queries/QueryTest/iceberg-metadata-tables.test File testdata/workloads/functional-query/queries/QueryTest/iceberg-metadata-tables.test: http://gerrit.cloudera.org:8080/#/c/21269/1/testdata/workloads/functional-query/queries/QueryTest/iceberg-metadata-tables.test@780 PS1, Line 780: > nit: extra lines? (line 791) We have these extra empty lines in this file, for example on L686 and L765 from before. I think they contribute to making the code easier to read and navigate. -- To view, visit http://gerrit.cloudera.org:8080/21269 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7b1d7fa332a901f05a46e0199e13fb841d2687c2 Gerrit-Change-Number: 21269 Gerrit-PatchSet: 2 Gerrit-Owner: Daniel Becker <daniel.bec...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Daniel Becker <daniel.bec...@cloudera.com> Gerrit-Reviewer: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Noemi Pap-Takacs <npaptak...@cloudera.com> Gerrit-Comment-Date: Wed, 10 Apr 2024 11:25:16 +0000 Gerrit-HasComments: Yes
