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

Reply via email to