You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Tamas Mate (Code Review)" <ge...@cloudera.org> on 2024/02/23 09:53:47 UTC

[Impala-ASF-CR] IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables

Tamas Mate has uploaded this change for review. ( http://gerrit.cloudera.org:8080/21061


Change subject: IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables
......................................................................

IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables

This commit adds support for reading ARRAY columns inside Iceberg
Metadata tables.

The change starts with some refactoring, to consolidate accessing JVM
through JNI a new backend class was introduced IcebergMetadataScanner.
This class is the C++ part of the Java IcebergMetadataScanner, it is
responsible to manage the Java scanner object.

In Iceberg array types do not have accessors, so structs inside arrays
have to be accessed by position, for the the value obtaining logics have
been changed to allow access by position.

The IcebergRowReader needed an IcebergMetadataScanner, so that it can
iterate over the arrays returned by the scanner and add them to the
collection.

Testing:
 - Added E2E tests.

Change-Id: Ieb9bac1822a17bd3cd074b4b98e4d010703cecb1
---
M be/src/exec/iceberg-metadata/CMakeLists.txt
M be/src/exec/iceberg-metadata/iceberg-metadata-scan-node.cc
M be/src/exec/iceberg-metadata/iceberg-metadata-scan-node.h
A be/src/exec/iceberg-metadata/iceberg-metadata-scanner.cc
A be/src/exec/iceberg-metadata/iceberg-metadata-scanner.h
M be/src/exec/iceberg-metadata/iceberg-row-reader.cc
M be/src/exec/iceberg-metadata/iceberg-row-reader.h
M be/src/service/impalad-main.cc
M fe/src/main/java/org/apache/impala/analysis/FromClause.java
M fe/src/main/java/org/apache/impala/planner/IcebergScanPlanner.java
M fe/src/main/java/org/apache/impala/util/IcebergMetadataScanner.java
M testdata/workloads/functional-query/queries/QueryTest/iceberg-metadata-tables.test
12 files changed, 621 insertions(+), 238 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/61/21061/1
-- 
To view, visit http://gerrit.cloudera.org:8080/21061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ieb9bac1822a17bd3cd074b4b98e4d010703cecb1
Gerrit-Change-Number: 21061
Gerrit-PatchSet: 1
Gerrit-Owner: Tamas Mate <tm...@apache.org>

[Impala-ASF-CR] IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables

Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
Gabor Kaszab has posted comments on this change. ( http://gerrit.cloudera.org:8080/21061 )

Change subject: IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables
......................................................................


Patch Set 7:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/21061/7/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/21061/7/be/src/exec/iceberg-metadata/iceberg-row-reader.cc@250
PS7, Line 250:       VLOG(3) << "Skipping unsupported column type: " << type.type;
ahh, I just noticed that we just write a log line if there is an unsupported column type. Shouldn't we run into a DCHECK failure or such in this case?


http://gerrit.cloudera.org:8080/#/c/21061/6/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/21061/6/testdata/workloads/functional-query/queries/QueryTest/iceberg-metadata-tables.test@715
PS6, Line 715: # Describe all the metadata tables once
> Ok, I'm going to add support for MAPs probably soon anyway. But please ment
Expand complex types would come after we have MAP support anyway. So I still think that giving an error instead of a null would make sense if it's not that complex to implement.



-- 
To view, visit http://gerrit.cloudera.org:8080/21061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieb9bac1822a17bd3cd074b4b98e4d010703cecb1
Gerrit-Change-Number: 21061
Gerrit-PatchSet: 7
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 29 Feb 2024 14:41:16 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21061 )

Change subject: IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables
......................................................................


Patch Set 9:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/10327/ DRY_RUN=false


-- 
To view, visit http://gerrit.cloudera.org:8080/21061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieb9bac1822a17bd3cd074b4b98e4d010703cecb1
Gerrit-Change-Number: 21061
Gerrit-PatchSet: 9
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 29 Feb 2024 15:58:22 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables

Posted by "Tamas Mate (Code Review)" <ge...@cloudera.org>.
Tamas Mate has posted comments on this change. ( http://gerrit.cloudera.org:8080/21061 )

Change subject: IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/21061/3/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/21061/3/fe/src/main/java/org/apache/impala/analysis/FromClause.java@169
PS3, Line 169:         tblRef.getDesc().getType().isMapType()) {
> I just realised that this change won't cover collections inside from clause
Adding Jira:  IMPALA-12853



-- 
To view, visit http://gerrit.cloudera.org:8080/21061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieb9bac1822a17bd3cd074b4b98e4d010703cecb1
Gerrit-Change-Number: 21061
Gerrit-PatchSet: 3
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 28 Feb 2024 15:22:25 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables

Posted by "Daniel Becker (Code Review)" <ge...@cloudera.org>.
Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/21061 )

Change subject: IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables
......................................................................


Patch Set 6:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-metadata-scanner.cc
File be/src/exec/iceberg-metadata/iceberg-metadata-scanner.cc:

http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-metadata-scanner.cc@100
PS3, Line 100: dren.
> Otherwise the current_type cannot be used as an lvalue to walk the ColumnTy
This compiled without problems for me:

const ColumnType* current_type = &(
    tuple_desc_->table_desc()->col_descs()[root_type_index].type());


http://gerrit.cloudera.org:8080/#/c/21061/6/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/21061/6/testdata/workloads/functional-query/queries/QueryTest/iceberg-metadata-tables.test@715
PS6, Line 715: select null_value_counts from functional_parquet.iceberg_query_metadata.all_files;
Thanks for pointing out this test that queries maps.
Nit: now it's duplicated here and on L663.

But shouldn't this throw an exception, i.e. maps are not yet supported? Before this change we threw exceptions for collections.



-- 
To view, visit http://gerrit.cloudera.org:8080/21061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieb9bac1822a17bd3cd074b4b98e4d010703cecb1
Gerrit-Change-Number: 21061
Gerrit-PatchSet: 6
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 29 Feb 2024 11:05:58 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables

Posted by "Daniel Becker (Code Review)" <ge...@cloudera.org>.
Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/21061 )

Change subject: IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables
......................................................................


Patch Set 3:

(4 comments)

Finished the C++ part, will continue with the Java part later.

http://gerrit.cloudera.org:8080/#/c/21061/3/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/21061/3/be/src/exec/iceberg-metadata/iceberg-row-reader.cc@194
PS3, Line 194:   DCHECK(slot_desc != nullptr);
Could also DCHECK that 'slot_desc' is a collection.


http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-row-reader.cc@203
PS3, Line 203: array_size
'remaining_array_size' could be better because we're decreasing it in the loop at L228.


http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-row-reader.cc@205
PS3, Line 205: array_size
I think writing "[remaining]_array_size > 0" is easier to understand as it is more explicit. Also on L216.


http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-row-reader.cc@207
PS3, Line 207: num_rows
'num_rows' could be misleading, IIANM it is the number of elements in the array, not the number of rows in a table. CollectionValueBuilder::GetFreeMemory() also uses 'num_tuples'.



-- 
To view, visit http://gerrit.cloudera.org:8080/21061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieb9bac1822a17bd3cd074b4b98e4d010703cecb1
Gerrit-Change-Number: 21061
Gerrit-PatchSet: 3
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Mon, 26 Feb 2024 18:48:27 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21061 )

Change subject: IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables
......................................................................


Patch Set 2:

Build Failed 

https://jenkins.impala.io/job/gerrit-code-review-checks/15290/ : Initial code review checks failed. See linked job for details on the failure.


-- 
To view, visit http://gerrit.cloudera.org:8080/21061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieb9bac1822a17bd3cd074b4b98e4d010703cecb1
Gerrit-Change-Number: 21061
Gerrit-PatchSet: 2
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Fri, 23 Feb 2024 11:39:49 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables

Posted by "Daniel Becker (Code Review)" <ge...@cloudera.org>.
Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/21061 )

Change subject: IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables
......................................................................


Patch Set 3:

(30 comments)

Thanks Tamás! So far I have only reviewed the files until iceberg-row-reader.cc up until the function WriteStructSlot(). I'll come back for the rest.

http://gerrit.cloudera.org:8080/#/c/21061/3//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/21061/3//COMMIT_MSG@13
PS3, Line 13: d
Colon or comma needed before the class name.


http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/CMakeLists.txt
File be/src/exec/iceberg-metadata/CMakeLists.txt:

http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/CMakeLists.txt@27
PS3, Line 27:   iceberg-metadata-scanner.cc
Nit: shouldn't we follow alphabetical ordering with these files?


http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-metadata-scan-node.h
File be/src/exec/iceberg-metadata/iceberg-metadata-scan-node.h:

http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-metadata-scan-node.h@81
PS3, Line 81: Java
It's no longer a Java object.


http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-metadata-scan-node.h@98
PS3, Line 98: scan_prepare_timer_
Now it's used in Open(), is the name still appropriate?


http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-metadata-scan-node.cc
File be/src/exec/iceberg-metadata/iceberg-metadata-scan-node.cc:

http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-metadata-scan-node.cc@55
PS3, Line 55:   SCOPED_TIMER(scan_prepare_timer_);
Some of the work has been moved from Prepare() to Open(), this could be mentioned in the commit message.


http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-metadata-scanner.h
File be/src/exec/iceberg-metadata/iceberg-metadata-scanner.h:

http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-metadata-scanner.h@46
PS3, Line 46: &
In Impala we usually take mutable parameters by pointer, can we also do it here?


http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-metadata-scanner.h@50
PS3, Line 50: jobject
Can't we take it as a const ref? Applies to the other jobject input parameters of the other methods too.


http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-metadata-scanner.h@51
PS3, Line 51: &
See L46.


http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-metadata-scanner.h@55
PS3, Line 55: &
See L46.


http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-metadata-scanner.h@58
PS3, Line 58: GetNextItem
Could be GetNextArrayItem() to make it clearer that it refers to arrays.


http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-metadata-scanner.h@70
PS3, Line 70:   inline static jmethodID iceberg_accessor_get_ = nullptr;
Some of the jmethodID names are prefixed with the class, some are not. Is there a reason for that?


http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-metadata-scanner.h@82
PS3, Line 82: metadata
Shouldn't it be "metadata table"?


http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-metadata-scanner.h@96
PS3, Line 96: jaccessors_
Does it still exist?


http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-metadata-scanner.h@113
PS3, Line 113: &
See L46.


http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-metadata-scanner.h@115
PS3, Line 115: &
See L46.


http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-metadata-scanner.cc
File be/src/exec/iceberg-metadata/iceberg-metadata-scanner.cc:

http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-metadata-scanner.cc@75
PS3, Line 75:   iceberg_metadata_scanner_ctor_, jtable_, jstr_metadata_table_name);
Nit: add indentation.


http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-metadata-scanner.cc@89
PS3, Line 89: SlotDescriptor*
Can this be const?


http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-metadata-scanner.cc@96
PS3, Line 96: does
Nit: do.


http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-metadata-scanner.cc@96
PS3, Line 96: not have their nested type's tuple available
Are there also slots that have their nested type's tuple, e.g. arrays? Where are they handled?


http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-metadata-scanner.cc@100
PS3, Line 100: const_cast
Why do we need this const_cast? Cant 'current_type' be a const pointer?


http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-metadata-scanner.cc@118
PS3, Line 118:   if (!struct_slot_desc->type().IsStructType()) return Status::OK();
When does this happen? Shouldn't we instead DCHECK that the type is a struct?


http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-metadata-scanner.cc@124
PS3, Line 124: IsComplexType
What if 'child_slot_desc' is a collection? In InitSlotIdFieldIdMap() we only recurse if the type is struct.


http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-metadata-scanner.cc@147
PS3, Line 147: find
We could store the result of this call and pass the field_id to GetValueByFieldId() so that it doesn't have to retrieve it again.


http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-metadata-scanner.cc@151
PS3, Line 151: ARRAY
Isn't is inside a struct if isTupleOfStructSlot() is true?


http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-metadata-scanner.cc@195
PS3, Line 195:   out << "Metadata table name: " << metadata_table_name_ << "; ";
Shouldn't we add some more info in the debug string, e.g. that it is an IcebergMetadataScanner, and also something before the string of 'tuple_desc_', that it is the tuple of the metadata table?


http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-row-reader.h
File be/src/exec/iceberg-metadata/iceberg-row-reader.h:

http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-row-reader.h@20
PS3, Line 20: 
Nit: unneeded empty line.


http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-row-reader.h@38
PS3, Line 38: s
Nit: can it translate a single StructLike into multiple rows? Otherwise use singular "row".


http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-row-reader.h@99
PS3, Line 99:   jclass JavaClassForImpalaType(const ColumnType type);
Couldn't it be static?


http://gerrit.cloudera.org:8080/#/c/21061/3/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/21061/3/be/src/exec/iceberg-metadata/iceberg-row-reader.cc@100
PS3, Line 100: Type not used
Can you clarify this?


http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-row-reader.cc@184
PS3, Line 184:   DCHECK(slot_desc != nullptr);
We could also DCHECK that 'slot_desc' has struct type.



-- 
To view, visit http://gerrit.cloudera.org:8080/21061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieb9bac1822a17bd3cd074b4b98e4d010703cecb1
Gerrit-Change-Number: 21061
Gerrit-PatchSet: 3
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Mon, 26 Feb 2024 14:44:30 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21061 )

Change subject: IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables
......................................................................


Patch Set 7:

Build Failed 

https://jenkins.impala.io/job/gerrit-code-review-checks/15361/ : Initial code review checks failed. See linked job for details on the failure.


-- 
To view, visit http://gerrit.cloudera.org:8080/21061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieb9bac1822a17bd3cd074b4b98e4d010703cecb1
Gerrit-Change-Number: 21061
Gerrit-PatchSet: 7
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 29 Feb 2024 14:07:39 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables

Posted by "Tamas Mate (Code Review)" <ge...@cloudera.org>.
Tamas Mate has uploaded a new patch set (#10). ( http://gerrit.cloudera.org:8080/21061 )

Change subject: IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables
......................................................................

IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables

This commit adds support for reading ARRAY columns inside Iceberg
Metadata tables.

The change starts with some refactoring, to consolidate accessing JVM
through JNI a new backend class was introduced, IcebergMetadataScanner.
This class is the C++ part of the Java IcebergMetadataScanner, it is
responsible to manage the Java scanner object.

In Iceberg array types do not have accessors, so structs inside arrays
have to be accessed by position, for the value obtaining logics have been
changed to allow access by position.

The IcebergRowReader needed an IcebergMetadataScanner, so that it can
iterate over the arrays returned by the scanner and add them to the
collection.

This change will not cover MAP, these slots are set to NULL, it will
be done in IMPALA-12611.

Testing:
 - Added E2E tests.

Change-Id: Ieb9bac1822a17bd3cd074b4b98e4d010703cecb1
---
M be/src/exec/iceberg-metadata/CMakeLists.txt
M be/src/exec/iceberg-metadata/iceberg-metadata-scan-node.cc
M be/src/exec/iceberg-metadata/iceberg-metadata-scan-node.h
A be/src/exec/iceberg-metadata/iceberg-metadata-scanner.cc
A be/src/exec/iceberg-metadata/iceberg-metadata-scanner.h
M be/src/exec/iceberg-metadata/iceberg-row-reader.cc
M be/src/exec/iceberg-metadata/iceberg-row-reader.h
M be/src/service/impalad-main.cc
M fe/src/main/java/org/apache/impala/analysis/FromClause.java
M fe/src/main/java/org/apache/impala/util/IcebergMetadataScanner.java
M testdata/workloads/functional-query/queries/QueryTest/iceberg-metadata-tables.test
11 files changed, 734 insertions(+), 292 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/61/21061/10
-- 
To view, visit http://gerrit.cloudera.org:8080/21061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ieb9bac1822a17bd3cd074b4b98e4d010703cecb1
Gerrit-Change-Number: 21061
Gerrit-PatchSet: 10
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>

[Impala-ASF-CR] IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21061 )

Change subject: IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables
......................................................................


Patch Set 10: Verified-1

Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/10334/


-- 
To view, visit http://gerrit.cloudera.org:8080/21061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieb9bac1822a17bd3cd074b4b98e4d010703cecb1
Gerrit-Change-Number: 21061
Gerrit-PatchSet: 10
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Fri, 01 Mar 2024 03:39:30 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables

Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
Gabor Kaszab has posted comments on this change. ( http://gerrit.cloudera.org:8080/21061 )

Change subject: IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables
......................................................................


Patch Set 8: Code-Review+2


-- 
To view, visit http://gerrit.cloudera.org:8080/21061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieb9bac1822a17bd3cd074b4b98e4d010703cecb1
Gerrit-Change-Number: 21061
Gerrit-PatchSet: 8
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 29 Feb 2024 15:29:28 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21061 )

Change subject: IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables
......................................................................


Patch Set 1:

Build Failed 

https://jenkins.impala.io/job/gerrit-code-review-checks/15288/ : Initial code review checks failed. See linked job for details on the failure.


-- 
To view, visit http://gerrit.cloudera.org:8080/21061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieb9bac1822a17bd3cd074b4b98e4d010703cecb1
Gerrit-Change-Number: 21061
Gerrit-PatchSet: 1
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Fri, 23 Feb 2024 10:19:06 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables

Posted by "Tamas Mate (Code Review)" <ge...@cloudera.org>.
Tamas Mate has uploaded a new patch set (#6). ( http://gerrit.cloudera.org:8080/21061 )

Change subject: IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables
......................................................................

IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables

This commit adds support for reading ARRAY columns inside Iceberg
Metadata tables.

The change starts with some refactoring, to consolidate accessing JVM
through JNI a new backend class was introduced, IcebergMetadataScanner.
This class is the C++ part of the Java IcebergMetadataScanner, it is
responsible to manage the Java scanner object.

In Iceberg array types do not have accessors, so structs inside arrays
have to be accessed by position, for the alue obtaining logics have been
changed to allow access by position.

The IcebergRowReader needed an IcebergMetadataScanner, so that it can
iterate over the arrays returned by the scanner and add them to the
collection.

Testing:
 - Added E2E tests.

Change-Id: Ieb9bac1822a17bd3cd074b4b98e4d010703cecb1
---
M be/src/exec/iceberg-metadata/CMakeLists.txt
M be/src/exec/iceberg-metadata/iceberg-metadata-scan-node.cc
M be/src/exec/iceberg-metadata/iceberg-metadata-scan-node.h
A be/src/exec/iceberg-metadata/iceberg-metadata-scanner.cc
A be/src/exec/iceberg-metadata/iceberg-metadata-scanner.h
M be/src/exec/iceberg-metadata/iceberg-row-reader.cc
M be/src/exec/iceberg-metadata/iceberg-row-reader.h
M be/src/service/impalad-main.cc
M fe/src/main/java/org/apache/impala/analysis/FromClause.java
M fe/src/main/java/org/apache/impala/planner/IcebergScanPlanner.java
M fe/src/main/java/org/apache/impala/util/IcebergMetadataScanner.java
M testdata/workloads/functional-query/queries/QueryTest/iceberg-metadata-tables.test
12 files changed, 746 insertions(+), 293 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/61/21061/6
-- 
To view, visit http://gerrit.cloudera.org:8080/21061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ieb9bac1822a17bd3cd074b4b98e4d010703cecb1
Gerrit-Change-Number: 21061
Gerrit-PatchSet: 6
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>

[Impala-ASF-CR] IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables

Posted by "Tamas Mate (Code Review)" <ge...@cloudera.org>.
Tamas Mate has posted comments on this change. ( http://gerrit.cloudera.org:8080/21061 )

Change subject: IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables
......................................................................


Patch Set 11:

I have not figured out the reason behind the clang-tidy failures yet:
The fail is:
```
FAILED: Unified backend test executable returned an error when trying
        to list tests.
Command: /home/ubuntu/Impala/bin/run-binary.sh /home/ubuntu/Impala/be/build/release//service/unifiedbetests --gtest_list_tests
Return Code: -11
```
Which suggests that the unifiedbetests failed with SIGSEGV.

As PS8 was fine, I narrowed down the issue to one commit:
```
commit 9b8e43e9a1ae871e676cded8e9eddc90aa96f314
IMPALA-12426: QueryStateRecord Refactor
```
Looks like this these changes are colliding, but I do not see why ATM.


-- 
To view, visit http://gerrit.cloudera.org:8080/21061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieb9bac1822a17bd3cd074b4b98e4d010703cecb1
Gerrit-Change-Number: 21061
Gerrit-PatchSet: 11
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Fri, 01 Mar 2024 16:12:30 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21061 )

Change subject: IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables
......................................................................


Patch Set 10:

Build Failed 

https://jenkins.impala.io/job/gerrit-code-review-checks/15370/ : Initial code review checks failed. See linked job for details on the failure.


-- 
To view, visit http://gerrit.cloudera.org:8080/21061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieb9bac1822a17bd3cd074b4b98e4d010703cecb1
Gerrit-Change-Number: 21061
Gerrit-PatchSet: 10
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 29 Feb 2024 23:09:34 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables

Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
Gabor Kaszab has posted comments on this change. ( http://gerrit.cloudera.org:8080/21061 )

Change subject: IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables
......................................................................


Patch Set 3:

(9 comments)

Thanks for the patch, Tamas! I checked the tests and the Java part. Will take a deeper look at the rest tomorrow.

http://gerrit.cloudera.org:8080/#/c/21061/3//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/21061/3//COMMIT_MSG@18
PS3, Line 18: the the
nit: duplicate 'the'


http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-metadata-scanner.h
File be/src/exec/iceberg-metadata/iceberg-metadata-scanner.h:

http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-metadata-scanner.h@21
PS3, Line 21: #include "exec/scan-node.h"
Can we not include scan-node.h?


http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/service/impalad-main.cc
File be/src/service/impalad-main.cc:

http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/service/impalad-main.cc@32
PS3, Line 32: #include "exec/iceberg-metadata/iceberg-metadata-scan-node.h"
This include can be removed I think


http://gerrit.cloudera.org:8080/#/c/21061/3/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/21061/3/fe/src/main/java/org/apache/impala/analysis/FromClause.java@169
PS3, Line 169:         tblRef.getDesc().getType().isMapType()) {
If I'm not mistaken, this check is for the case when a collection is provided in the FROM clause. If this use case is deliberately supported then please add test coverage too.


http://gerrit.cloudera.org:8080/#/c/21061/3/fe/src/main/java/org/apache/impala/util/IcebergMetadataScanner.java
File fe/src/main/java/org/apache/impala/util/IcebergMetadataScanner.java:

http://gerrit.cloudera.org:8080/#/c/21061/3/fe/src/main/java/org/apache/impala/util/IcebergMetadataScanner.java@116
PS3, Line 116: StructLke
typo


http://gerrit.cloudera.org:8080/#/c/21061/3/fe/src/main/java/org/apache/impala/util/IcebergMetadataScanner.java@137
PS3, Line 137:     private Iterator<T> iterator;
If we store the iterator as a member, is there a guarantee that the underlying array is not destroyed?


http://gerrit.cloudera.org:8080/#/c/21061/3/fe/src/main/java/org/apache/impala/util/IcebergMetadataScanner.java@146
PS3, Line 146:         return iterator.next();
nit: could fit into the line above


http://gerrit.cloudera.org:8080/#/c/21061/3/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/21061/3/testdata/workloads/functional-query/queries/QueryTest/iceberg-metadata-tables.test@567
PS3, Line 567: # Test 11 : Query ARRAY type columns
The test above is also "Test 11". I don't think that giving a number of these tests make sense, it just makes it complicated to maintain and to add anything to the middle.


http://gerrit.cloudera.org:8080/#/c/21061/3/testdata/workloads/functional-query/queries/QueryTest/iceberg-metadata-tables.test@591
PS3, Line 591: row_regex:'/test-warehouse/iceberg_test/hadoop_catalog/ice/iceberg_v2_delete_both_eq_and_pos/data/.*','NULL'
The name of the files in this table is fix, so we could spell the full path out in the expected results, in my opinion.



-- 
To view, visit http://gerrit.cloudera.org:8080/21061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieb9bac1822a17bd3cd074b4b98e4d010703cecb1
Gerrit-Change-Number: 21061
Gerrit-PatchSet: 3
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Mon, 26 Feb 2024 16:22:30 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables

Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
Gabor Kaszab has posted comments on this change. ( http://gerrit.cloudera.org:8080/21061 )

Change subject: IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables
......................................................................


Patch Set 6:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/21061/6//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/21061/6//COMMIT_MSG@18
PS6, Line 18: alue
nit: value


http://gerrit.cloudera.org:8080/#/c/21061/6/be/src/exec/iceberg-metadata/iceberg-row-reader.h
File be/src/exec/iceberg-metadata/iceberg-row-reader.h:

http://gerrit.cloudera.org:8080/#/c/21061/6/be/src/exec/iceberg-metadata/iceberg-row-reader.h@21
PS6, Line 21: #include "exec/scan-node.h"
I think even including scan-node.h in the header could be avoided by forward declarations


http://gerrit.cloudera.org:8080/#/c/21061/3/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/21061/3/be/src/exec/iceberg-metadata/iceberg-row-reader.cc@222
PS3, Line 222:       if (ExecNode::EvalConjuncts(scan_node_->conjunct_evals().data(),
> I just realised that with the current implementation it won't be possible t
well, if this code doesn't work then let's remove it. What would be needed to make this work? FROM clause collections, and predicates on the ITEM, right?


http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-row-reader.cc@248
PS3, Line 248:     } case TYPE_STRUCT: {
> Yes, but I think it helps to understand what is happening here.
But then we return nullptr for TYPE_STRUCT that could cause issues on the caller side. If this method is not expected to be called for TYPE_STRUCT then it's cleaner to get rid of that case.


http://gerrit.cloudera.org:8080/#/c/21061/3/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/21061/3/fe/src/main/java/org/apache/impala/analysis/FromClause.java@169
PS3, Line 169:       throw new AnalysisException("Querying collection types (ARRAY/MAP) in FROM " +
> Adding Jira:  IMPALA-12853
Thanks for creating a Jira! Could you also mention the Jira ID here in a comment?


http://gerrit.cloudera.org:8080/#/c/21061/6/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/21061/6/testdata/workloads/functional-query/queries/QueryTest/iceberg-metadata-tables.test@624
PS6, Line 624: '/test-warehouse/iceberg_test/hadoop_catalog/ice/iceberg_v2_delete_both_eq_and_pos/data/delete-074a9e19e61b766e-652a169e00000001_800513971_data.0.parq','NULL'
Could you also add the content col to the select list so that we can see which files are eq-delete files. Would make it easier to verify that only eq-delete files (content=2) have eq-id list


http://gerrit.cloudera.org:8080/#/c/21061/6/testdata/workloads/functional-query/queries/QueryTest/iceberg-metadata-tables.test@663
PS6, Line 663: select null_value_counts from functional_parquet.iceberg_query_metadata.all_files;
This seems a duplicate test with the one on L715


http://gerrit.cloudera.org:8080/#/c/21061/6/testdata/workloads/functional-query/queries/QueryTest/iceberg-metadata-tables.test@715
PS6, Line 715: select null_value_counts from functional_parquet.iceberg_query_metadata.all_files;
Shouldn't we get a not supported error when querying map columns from a metadata table? Returning nulls might be misleading



-- 
To view, visit http://gerrit.cloudera.org:8080/21061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieb9bac1822a17bd3cd074b4b98e4d010703cecb1
Gerrit-Change-Number: 21061
Gerrit-PatchSet: 6
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 29 Feb 2024 11:33:24 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables

Posted by "Daniel Becker (Code Review)" <ge...@cloudera.org>.
Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/21061 )

Change subject: IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables
......................................................................


Patch Set 8:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/21061/8/be/src/exec/iceberg-metadata/iceberg-row-reader.h
File be/src/exec/iceberg-metadata/iceberg-row-reader.h:

http://gerrit.cloudera.org:8080/#/c/21061/8/be/src/exec/iceberg-metadata/iceberg-row-reader.h@101
PS8, Line 101: ColumnType
This could be a const ref



-- 
To view, visit http://gerrit.cloudera.org:8080/21061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieb9bac1822a17bd3cd074b4b98e4d010703cecb1
Gerrit-Change-Number: 21061
Gerrit-PatchSet: 8
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 29 Feb 2024 15:28:37 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables

Posted by "Daniel Becker (Code Review)" <ge...@cloudera.org>.
Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/21061 )

Change subject: IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables
......................................................................


Patch Set 8: Code-Review+1


-- 
To view, visit http://gerrit.cloudera.org:8080/21061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieb9bac1822a17bd3cd074b4b98e4d010703cecb1
Gerrit-Change-Number: 21061
Gerrit-PatchSet: 8
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 29 Feb 2024 15:26:46 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21061 )

Change subject: IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables
......................................................................


Patch Set 3:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/15291/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
To view, visit http://gerrit.cloudera.org:8080/21061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieb9bac1822a17bd3cd074b4b98e4d010703cecb1
Gerrit-Change-Number: 21061
Gerrit-PatchSet: 3
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Fri, 23 Feb 2024 13:07:38 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21061 )

Change subject: IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables
......................................................................


Patch Set 9: Verified-1

Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/10327/


-- 
To view, visit http://gerrit.cloudera.org:8080/21061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieb9bac1822a17bd3cd074b4b98e4d010703cecb1
Gerrit-Change-Number: 21061
Gerrit-PatchSet: 9
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 29 Feb 2024 20:37:16 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21061 )

Change subject: IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables
......................................................................


Patch Set 6:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/15356/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
To view, visit http://gerrit.cloudera.org:8080/21061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieb9bac1822a17bd3cd074b4b98e4d010703cecb1
Gerrit-Change-Number: 21061
Gerrit-PatchSet: 6
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 29 Feb 2024 10:08:10 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21061 )

Change subject: IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables
......................................................................


Patch Set 9: Code-Review+2


-- 
To view, visit http://gerrit.cloudera.org:8080/21061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieb9bac1822a17bd3cd074b4b98e4d010703cecb1
Gerrit-Change-Number: 21061
Gerrit-PatchSet: 9
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 29 Feb 2024 15:58:22 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21061 )

Change subject: IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables
......................................................................


Patch Set 12:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/10353/ DRY_RUN=false


-- 
To view, visit http://gerrit.cloudera.org:8080/21061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieb9bac1822a17bd3cd074b4b98e4d010703cecb1
Gerrit-Change-Number: 21061
Gerrit-PatchSet: 12
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 05 Mar 2024 10:08:42 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21061 )

Change subject: IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables
......................................................................


Patch Set 12: Verified+1


-- 
To view, visit http://gerrit.cloudera.org:8080/21061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieb9bac1822a17bd3cd074b4b98e4d010703cecb1
Gerrit-Change-Number: 21061
Gerrit-PatchSet: 12
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 05 Mar 2024 14:49:33 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables

Posted by "Tamas Mate (Code Review)" <ge...@cloudera.org>.
Tamas Mate has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/21061 )

Change subject: IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables
......................................................................

IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables

This commit adds support for reading ARRAY columns inside Iceberg
Metadata tables.

The change starts with some refactoring, to consolidate accessing JVM
through JNI a new backend class was introduced, IcebergMetadataScanner.
This class is the C++ part of the Java IcebergMetadataScanner, it is
responsible to manage the Java scanner object.

In Iceberg array types do not have accessors, so structs inside arrays
have to be accessed by position, for the alue obtaining logics have been
changed to allow access by position.

The IcebergRowReader needed an IcebergMetadataScanner, so that it can
iterate over the arrays returned by the scanner and add them to the
collection.

Testing:
 - Added E2E tests.

Change-Id: Ieb9bac1822a17bd3cd074b4b98e4d010703cecb1
---
M be/src/exec/iceberg-metadata/CMakeLists.txt
M be/src/exec/iceberg-metadata/iceberg-metadata-scan-node.cc
M be/src/exec/iceberg-metadata/iceberg-metadata-scan-node.h
A be/src/exec/iceberg-metadata/iceberg-metadata-scanner.cc
A be/src/exec/iceberg-metadata/iceberg-metadata-scanner.h
M be/src/exec/iceberg-metadata/iceberg-row-reader.cc
M be/src/exec/iceberg-metadata/iceberg-row-reader.h
M be/src/service/impalad-main.cc
M fe/src/main/java/org/apache/impala/analysis/FromClause.java
M fe/src/main/java/org/apache/impala/planner/IcebergScanPlanner.java
M fe/src/main/java/org/apache/impala/util/IcebergMetadataScanner.java
M testdata/workloads/functional-query/queries/QueryTest/iceberg-metadata-tables.test
12 files changed, 746 insertions(+), 293 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/61/21061/5
-- 
To view, visit http://gerrit.cloudera.org:8080/21061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ieb9bac1822a17bd3cd074b4b98e4d010703cecb1
Gerrit-Change-Number: 21061
Gerrit-PatchSet: 5
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>

[Impala-ASF-CR] IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables

Posted by "Tamas Mate (Code Review)" <ge...@cloudera.org>.
Tamas Mate has uploaded a new patch set (#7). ( http://gerrit.cloudera.org:8080/21061 )

Change subject: IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables
......................................................................

IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables

This commit adds support for reading ARRAY columns inside Iceberg
Metadata tables.

The change starts with some refactoring, to consolidate accessing JVM
through JNI a new backend class was introduced, IcebergMetadataScanner.
This class is the C++ part of the Java IcebergMetadataScanner, it is
responsible to manage the Java scanner object.

In Iceberg array types do not have accessors, so structs inside arrays
have to be accessed by position, for the value obtaining logics have been
changed to allow access by position.

The IcebergRowReader needed an IcebergMetadataScanner, so that it can
iterate over the arrays returned by the scanner and add them to the
collection.

Testing:
 - Added E2E tests.

Change-Id: Ieb9bac1822a17bd3cd074b4b98e4d010703cecb1
---
M be/src/exec/iceberg-metadata/CMakeLists.txt
M be/src/exec/iceberg-metadata/iceberg-metadata-scan-node.cc
M be/src/exec/iceberg-metadata/iceberg-metadata-scan-node.h
A be/src/exec/iceberg-metadata/iceberg-metadata-scanner.cc
A be/src/exec/iceberg-metadata/iceberg-metadata-scanner.h
M be/src/exec/iceberg-metadata/iceberg-row-reader.cc
M be/src/exec/iceberg-metadata/iceberg-row-reader.h
M be/src/service/impalad-main.cc
M fe/src/main/java/org/apache/impala/analysis/FromClause.java
M fe/src/main/java/org/apache/impala/planner/IcebergScanPlanner.java
M fe/src/main/java/org/apache/impala/util/IcebergMetadataScanner.java
M testdata/workloads/functional-query/queries/QueryTest/iceberg-metadata-tables.test
12 files changed, 735 insertions(+), 293 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/61/21061/7
-- 
To view, visit http://gerrit.cloudera.org:8080/21061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ieb9bac1822a17bd3cd074b4b98e4d010703cecb1
Gerrit-Change-Number: 21061
Gerrit-PatchSet: 7
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>

[Impala-ASF-CR] IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables

Posted by "Tamas Mate (Code Review)" <ge...@cloudera.org>.
Tamas Mate has uploaded a new patch set (#8). ( http://gerrit.cloudera.org:8080/21061 )

Change subject: IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables
......................................................................

IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables

This commit adds support for reading ARRAY columns inside Iceberg
Metadata tables.

The change starts with some refactoring, to consolidate accessing JVM
through JNI a new backend class was introduced, IcebergMetadataScanner.
This class is the C++ part of the Java IcebergMetadataScanner, it is
responsible to manage the Java scanner object.

In Iceberg array types do not have accessors, so structs inside arrays
have to be accessed by position, for the value obtaining logics have been
changed to allow access by position.

The IcebergRowReader needed an IcebergMetadataScanner, so that it can
iterate over the arrays returned by the scanner and add them to the
collection.

This change will not cover MAP, these slots are set to NULL, it will
be done in IMPALA-12611.

Testing:
 - Added E2E tests.

Change-Id: Ieb9bac1822a17bd3cd074b4b98e4d010703cecb1
---
M be/src/exec/iceberg-metadata/CMakeLists.txt
M be/src/exec/iceberg-metadata/iceberg-metadata-scan-node.cc
M be/src/exec/iceberg-metadata/iceberg-metadata-scan-node.h
A be/src/exec/iceberg-metadata/iceberg-metadata-scanner.cc
A be/src/exec/iceberg-metadata/iceberg-metadata-scanner.h
M be/src/exec/iceberg-metadata/iceberg-row-reader.cc
M be/src/exec/iceberg-metadata/iceberg-row-reader.h
M be/src/service/impalad-main.cc
M fe/src/main/java/org/apache/impala/analysis/FromClause.java
M fe/src/main/java/org/apache/impala/planner/IcebergScanPlanner.java
M fe/src/main/java/org/apache/impala/util/IcebergMetadataScanner.java
M testdata/workloads/functional-query/queries/QueryTest/iceberg-metadata-tables.test
12 files changed, 734 insertions(+), 293 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/61/21061/8
-- 
To view, visit http://gerrit.cloudera.org:8080/21061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ieb9bac1822a17bd3cd074b4b98e4d010703cecb1
Gerrit-Change-Number: 21061
Gerrit-PatchSet: 8
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>

[Impala-ASF-CR] IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21061 )

Change subject: IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables
......................................................................


Patch Set 8:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/15365/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
To view, visit http://gerrit.cloudera.org:8080/21061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieb9bac1822a17bd3cd074b4b98e4d010703cecb1
Gerrit-Change-Number: 21061
Gerrit-PatchSet: 8
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 29 Feb 2024 15:51:15 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21061 )

Change subject: IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables
......................................................................


Patch Set 3: Verified+1


-- 
To view, visit http://gerrit.cloudera.org:8080/21061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieb9bac1822a17bd3cd074b4b98e4d010703cecb1
Gerrit-Change-Number: 21061
Gerrit-PatchSet: 3
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Fri, 23 Feb 2024 19:37:48 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21061 )

Change subject: IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables
......................................................................


Patch Set 3:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/10295/ DRY_RUN=true


-- 
To view, visit http://gerrit.cloudera.org:8080/21061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieb9bac1822a17bd3cd074b4b98e4d010703cecb1
Gerrit-Change-Number: 21061
Gerrit-PatchSet: 3
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Fri, 23 Feb 2024 15:03:11 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables

Posted by "Tamas Mate (Code Review)" <ge...@cloudera.org>.
Tamas Mate has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/21061 )

Change subject: IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables
......................................................................

IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables

This commit adds support for reading ARRAY columns inside Iceberg
Metadata tables.

The change starts with some refactoring, to consolidate accessing JVM
through JNI a new backend class was introduced IcebergMetadataScanner.
This class is the C++ part of the Java IcebergMetadataScanner, it is
responsible to manage the Java scanner object.

In Iceberg array types do not have accessors, so structs inside arrays
have to be accessed by position, for the the value obtaining logics have
been changed to allow access by position.

The IcebergRowReader needed an IcebergMetadataScanner, so that it can
iterate over the arrays returned by the scanner and add them to the
collection.

Testing:
 - Added E2E tests.

Change-Id: Ieb9bac1822a17bd3cd074b4b98e4d010703cecb1
---
M be/src/exec/iceberg-metadata/CMakeLists.txt
M be/src/exec/iceberg-metadata/iceberg-metadata-scan-node.cc
M be/src/exec/iceberg-metadata/iceberg-metadata-scan-node.h
A be/src/exec/iceberg-metadata/iceberg-metadata-scanner.cc
A be/src/exec/iceberg-metadata/iceberg-metadata-scanner.h
M be/src/exec/iceberg-metadata/iceberg-row-reader.cc
M be/src/exec/iceberg-metadata/iceberg-row-reader.h
M be/src/service/impalad-main.cc
M fe/src/main/java/org/apache/impala/analysis/FromClause.java
M fe/src/main/java/org/apache/impala/planner/IcebergScanPlanner.java
M fe/src/main/java/org/apache/impala/util/IcebergMetadataScanner.java
M testdata/workloads/functional-query/queries/QueryTest/iceberg-metadata-tables.test
12 files changed, 622 insertions(+), 238 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/61/21061/2
-- 
To view, visit http://gerrit.cloudera.org:8080/21061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ieb9bac1822a17bd3cd074b4b98e4d010703cecb1
Gerrit-Change-Number: 21061
Gerrit-PatchSet: 2
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>

[Impala-ASF-CR] IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables

Posted by "Daniel Becker (Code Review)" <ge...@cloudera.org>.
Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/21061 )

Change subject: IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables
......................................................................


Patch Set 3:

(6 comments)

Thanks, I finished PS3.

http://gerrit.cloudera.org:8080/#/c/21061/3/fe/src/main/java/org/apache/impala/util/IcebergMetadataScanner.java
File fe/src/main/java/org/apache/impala/util/IcebergMetadataScanner.java:

http://gerrit.cloudera.org:8080/#/c/21061/3/fe/src/main/java/org/apache/impala/util/IcebergMetadataScanner.java@119
PS3, Line 119: Object
Why isn't this type 'StructLike', like in GetValueByPosition()?


http://gerrit.cloudera.org:8080/#/c/21061/3/fe/src/main/java/org/apache/impala/util/IcebergMetadataScanner.java@127
PS3, Line 127: classTypeClass
Wouldn't "classType" be enough for the name?


http://gerrit.cloudera.org:8080/#/c/21061/3/fe/src/main/java/org/apache/impala/util/IcebergMetadataScanner.java@145
PS3, Line 145: iterator != null
Can 'iterator' be NULL? If not, this could be a Preconditions check instead.


http://gerrit.cloudera.org:8080/#/c/21061/3/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/21061/3/testdata/workloads/functional-query/queries/QueryTest/iceberg-metadata-tables.test@a566
PS3, Line 566: 
Why did we delete this test?


http://gerrit.cloudera.org:8080/#/c/21061/3/testdata/workloads/functional-query/queries/QueryTest/iceberg-metadata-tables.test@566
PS3, Line 566: ####
We could have a test that tries to query a MAP.


http://gerrit.cloudera.org:8080/#/c/21061/3/testdata/workloads/functional-query/queries/QueryTest/iceberg-metadata-tables.test@611
PS3, Line 611: select equality_ids from (select equality_ids from functional_parquet.iceberg_v2_delete_both_eq_and_pos.all_files) s;
We could also have an inline view with a WITH clause.



-- 
To view, visit http://gerrit.cloudera.org:8080/21061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieb9bac1822a17bd3cd074b4b98e4d010703cecb1
Gerrit-Change-Number: 21061
Gerrit-PatchSet: 3
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 27 Feb 2024 10:41:21 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables

Posted by "Tamas Mate (Code Review)" <ge...@cloudera.org>.
Tamas Mate has posted comments on this change. ( http://gerrit.cloudera.org:8080/21061 )

Change subject: IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables
......................................................................


Patch Set 10:

Carrying over +2 after rebase.


-- 
To view, visit http://gerrit.cloudera.org:8080/21061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieb9bac1822a17bd3cd074b4b98e4d010703cecb1
Gerrit-Change-Number: 21061
Gerrit-PatchSet: 10
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 29 Feb 2024 22:50:05 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21061 )

Change subject: IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables
......................................................................


Patch Set 10:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/10334/ DRY_RUN=false


-- 
To view, visit http://gerrit.cloudera.org:8080/21061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieb9bac1822a17bd3cd074b4b98e4d010703cecb1
Gerrit-Change-Number: 21061
Gerrit-PatchSet: 10
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 29 Feb 2024 22:50:19 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables

Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
Gabor Kaszab has posted comments on this change. ( http://gerrit.cloudera.org:8080/21061 )

Change subject: IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables
......................................................................


Patch Set 12: Code-Review+2


-- 
To view, visit http://gerrit.cloudera.org:8080/21061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieb9bac1822a17bd3cd074b4b98e4d010703cecb1
Gerrit-Change-Number: 21061
Gerrit-PatchSet: 12
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 05 Mar 2024 14:56:39 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables

Posted by "Daniel Becker (Code Review)" <ge...@cloudera.org>.
Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/21061 )

Change subject: IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables
......................................................................


Patch Set 7:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/21061/6/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/21061/6/testdata/workloads/functional-query/queries/QueryTest/iceberg-metadata-tables.test@715
PS6, Line 715: # Describe all the metadata tables once
> We decided to go with 'NULL's, because otherwise it would be nearly impossi
Ok, I'm going to add support for MAPs probably soon anyway. But please mention it in the commit message.



-- 
To view, visit http://gerrit.cloudera.org:8080/21061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieb9bac1822a17bd3cd074b4b98e4d010703cecb1
Gerrit-Change-Number: 21061
Gerrit-PatchSet: 7
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 29 Feb 2024 13:54:14 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables

Posted by "Tamas Mate (Code Review)" <ge...@cloudera.org>.
Tamas Mate has posted comments on this change. ( http://gerrit.cloudera.org:8080/21061 )

Change subject: IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables
......................................................................


Patch Set 7:

(3 comments)

Thanks again for the swift reviews!

http://gerrit.cloudera.org:8080/#/c/21061/7/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/21061/7/be/src/exec/iceberg-metadata/iceberg-row-reader.cc@250
PS7, Line 250:       VLOG(3) << "Skipping unsupported column type: " << type.type;
> ahh, I just noticed that we just write a log line if there is an unsupporte
It is about skipping the resolution currently and making these slots NULL later. It cannot be skipped if a DCHECK catches it.


http://gerrit.cloudera.org:8080/#/c/21061/6/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/21061/6/testdata/workloads/functional-query/queries/QueryTest/iceberg-metadata-tables.test@715
PS6, Line 715: # Describe all the metadata tables once
> Ok, I'm going to add support for MAPs probably soon anyway. But please ment
Done


http://gerrit.cloudera.org:8080/#/c/21061/6/testdata/workloads/functional-query/queries/QueryTest/iceberg-metadata-tables.test@715
PS6, Line 715: # Describe all the metadata tables once
> Expand complex types would come after we have MAP support anyway. So I stil
Keeping it as NULL has already been approved by you and Zoltan a few months ago, given that MAP support is around the corner I would just leave it as is.

reference: https://gerrit.cloudera.org/#/c/20010/



-- 
To view, visit http://gerrit.cloudera.org:8080/21061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieb9bac1822a17bd3cd074b4b98e4d010703cecb1
Gerrit-Change-Number: 21061
Gerrit-PatchSet: 7
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 29 Feb 2024 15:23:24 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables

Posted by "Tamas Mate (Code Review)" <ge...@cloudera.org>.
Tamas Mate has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/21061 )

Change subject: IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables
......................................................................

IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables

This commit adds support for reading ARRAY columns inside Iceberg
Metadata tables.

The change starts with some refactoring, to consolidate accessing JVM
through JNI a new backend class was introduced IcebergMetadataScanner.
This class is the C++ part of the Java IcebergMetadataScanner, it is
responsible to manage the Java scanner object.

In Iceberg array types do not have accessors, so structs inside arrays
have to be accessed by position, for the the value obtaining logics have
been changed to allow access by position.

The IcebergRowReader needed an IcebergMetadataScanner, so that it can
iterate over the arrays returned by the scanner and add them to the
collection.

Testing:
 - Added E2E tests.

Change-Id: Ieb9bac1822a17bd3cd074b4b98e4d010703cecb1
---
M be/src/exec/iceberg-metadata/CMakeLists.txt
M be/src/exec/iceberg-metadata/iceberg-metadata-scan-node.cc
M be/src/exec/iceberg-metadata/iceberg-metadata-scan-node.h
A be/src/exec/iceberg-metadata/iceberg-metadata-scanner.cc
A be/src/exec/iceberg-metadata/iceberg-metadata-scanner.h
M be/src/exec/iceberg-metadata/iceberg-row-reader.cc
M be/src/exec/iceberg-metadata/iceberg-row-reader.h
M be/src/service/impalad-main.cc
M fe/src/main/java/org/apache/impala/analysis/FromClause.java
M fe/src/main/java/org/apache/impala/planner/IcebergScanPlanner.java
M fe/src/main/java/org/apache/impala/util/IcebergMetadataScanner.java
M testdata/workloads/functional-query/queries/QueryTest/iceberg-metadata-tables.test
12 files changed, 621 insertions(+), 238 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/61/21061/3
-- 
To view, visit http://gerrit.cloudera.org:8080/21061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ieb9bac1822a17bd3cd074b4b98e4d010703cecb1
Gerrit-Change-Number: 21061
Gerrit-PatchSet: 3
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>

[Impala-ASF-CR] IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21061 )

Change subject: IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/21061/1/be/src/exec/iceberg-metadata/iceberg-row-reader.h
File be/src/exec/iceberg-metadata/iceberg-row-reader.h:

http://gerrit.cloudera.org:8080/#/c/21061/1/be/src/exec/iceberg-metadata/iceberg-row-reader.h@94
PS1, Line 94:       SlotDescriptor* slot_desc, Tuple* tuple, MemPool* tuple_data_pool, RuntimeState* state);
line too long (94 > 90)



-- 
To view, visit http://gerrit.cloudera.org:8080/21061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieb9bac1822a17bd3cd074b4b98e4d010703cecb1
Gerrit-Change-Number: 21061
Gerrit-PatchSet: 1
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Fri, 23 Feb 2024 09:55:06 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables

Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/21061 )

Change subject: IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables
......................................................................


Patch Set 11:

> Patch Set 11:
> 
> I have not figured out the reason behind the clang-tidy failures yet:
> The fail is:
> ```
> FAILED: Unified backend test executable returned an error when trying
>         to list tests.
> Command: /home/ubuntu/Impala/bin/run-binary.sh /home/ubuntu/Impala/be/build/release//service/unifiedbetests --gtest_list_tests
> Return Code: -11
> ```
> Which suggests that the unifiedbetests failed with SIGSEGV.
> 
> As PS8 was fine, I narrowed down the issue to one commit:
> ```
> commit 9b8e43e9a1ae871e676cded8e9eddc90aa96f314
> IMPALA-12426: QueryStateRecord Refactor
> ```
> Looks like this these changes are colliding, but I do not see why ATM.

I hacked be/src/service/CMakeLists.txt and be/src/service/query-state-record-test.cc in https://gerrit.cloudera.org/c/21100/4, and that seems to help pass this issue.


-- 
To view, visit http://gerrit.cloudera.org:8080/21061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieb9bac1822a17bd3cd074b4b98e4d010703cecb1
Gerrit-Change-Number: 21061
Gerrit-PatchSet: 11
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Sun, 03 Mar 2024 18:10:16 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables

Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
Gabor Kaszab has posted comments on this change. ( http://gerrit.cloudera.org:8080/21061 )

Change subject: IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables
......................................................................


Patch Set 3:

(14 comments)

I managed to check the c++ part too

http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-metadata-scanner.h
File be/src/exec/iceberg-metadata/iceberg-metadata-scanner.h:

http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-metadata-scanner.h@58
PS3, Line 58: jobject& result
This should be pointer as Daniel said


http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-metadata-scanner.h@93
PS3, Line 93:   /// Accessor map for the scan result, pairs the slot ids with the java Accessor objects.
This is not a map of accessors now but a slot ID to field ID mapping.


http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-metadata-scanner.h@103
PS3, Line 103: Accessor collection
I think this comment is off now.


http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-metadata-scanner.h@109
PS3, Line 109: const SlotDescriptor*
Don't we use const ref types for immutable params instead of const pointers?


http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-metadata-scanner.h@113
PS3, Line 113: SlotDescriptor*
const ref instead


http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-metadata-scanner.cc
File be/src/exec/iceberg-metadata/iceberg-metadata-scanner.cc:

http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-metadata-scanner.cc@88
PS3, Line 88: Status IcebergMetadataScanner::InitSlotIdFieldIdMap(JNIEnv* env) {
Could you help me understand if this functiondoes or should go into collections, e.g. get the field ids for struct members inside collections?


http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-metadata-scanner.cc@145
PS3, Line 145: clazz
I saw this param name elsewhere too, but not sure if this is something conventional or if it makes something, but seems a weird name for me.


http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-metadata-scanner.cc@155
PS3, Line 155: Primitive inside an ARRAY
Can we DCHECK on this fact?


http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-row-reader.h
File be/src/exec/iceberg-metadata/iceberg-row-reader.h:

http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-row-reader.h@22
PS3, Line 22: #include "exec/iceberg-metadata/iceberg-metadata-scanner.h"
I think we can get rid of this include with a forward declaration.


http://gerrit.cloudera.org:8080/#/c/21061/3/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/21061/3/be/src/exec/iceberg-metadata/iceberg-row-reader.cc@114
PS3, Line 114:     env->DeleteLocalRef(accessed_value);
I'm a bit lost with these refs, but aren't local refs released automatically when going out from the scope?


http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-row-reader.cc@222
PS3, Line 222:       if (ExecNode::EvalConjuncts(scan_node_->conjunct_evals().data(),
Could you add a test that exercises the path where EvalConjuncts is false?


http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-row-reader.cc@237
PS3, Line 237: For
nit: I'd fing 'From' instead of 'For' easier to understand


http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-row-reader.cc@248
PS3, Line 248:     } case TYPE_STRUCT: {
Would it work to simply remove this case?


http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-row-reader.cc@252
PS3, Line 252: return list_cl_;
Is this java.lang.util.List? Please add a comment similarly to the other types



-- 
To view, visit http://gerrit.cloudera.org:8080/21061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieb9bac1822a17bd3cd074b4b98e4d010703cecb1
Gerrit-Change-Number: 21061
Gerrit-PatchSet: 3
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 27 Feb 2024 16:43:07 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21061 )

Change subject: IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables
......................................................................


Patch Set 5:

Build Failed 

https://jenkins.impala.io/job/gerrit-code-review-checks/15355/ : Initial code review checks failed. See linked job for details on the failure.


-- 
To view, visit http://gerrit.cloudera.org:8080/21061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieb9bac1822a17bd3cd074b4b98e4d010703cecb1
Gerrit-Change-Number: 21061
Gerrit-PatchSet: 5
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 29 Feb 2024 09:35:24 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables

Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
Gabor Kaszab has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/21061 )

Change subject: IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables
......................................................................

IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables

This commit adds support for reading ARRAY columns inside Iceberg
Metadata tables.

The change starts with some refactoring, to consolidate accessing JVM
through JNI a new backend class was introduced, IcebergMetadataScanner.
This class is the C++ part of the Java IcebergMetadataScanner, it is
responsible to manage the Java scanner object.

In Iceberg array types do not have accessors, so structs inside arrays
have to be accessed by position, for the value obtaining logics have been
changed to allow access by position.

The IcebergRowReader needed an IcebergMetadataScanner, so that it can
iterate over the arrays returned by the scanner and add them to the
collection.

This change will not cover MAP, these slots are set to NULL, it will
be done in IMPALA-12611.

Testing:
 - Added E2E tests.

Change-Id: Ieb9bac1822a17bd3cd074b4b98e4d010703cecb1
Reviewed-on: http://gerrit.cloudera.org:8080/21061
Tested-by: Impala Public Jenkins <im...@cloudera.com>
Reviewed-by: Gabor Kaszab <ga...@cloudera.com>
---
M be/src/exec/iceberg-metadata/CMakeLists.txt
M be/src/exec/iceberg-metadata/iceberg-metadata-scan-node.cc
M be/src/exec/iceberg-metadata/iceberg-metadata-scan-node.h
A be/src/exec/iceberg-metadata/iceberg-metadata-scanner.cc
A be/src/exec/iceberg-metadata/iceberg-metadata-scanner.h
M be/src/exec/iceberg-metadata/iceberg-row-reader.cc
M be/src/exec/iceberg-metadata/iceberg-row-reader.h
M be/src/service/impalad-main.cc
M fe/src/main/java/org/apache/impala/analysis/FromClause.java
M fe/src/main/java/org/apache/impala/util/IcebergMetadataScanner.java
M testdata/workloads/functional-query/queries/QueryTest/iceberg-metadata-tables.test
11 files changed, 734 insertions(+), 292 deletions(-)

Approvals:
  Impala Public Jenkins: Verified
  Gabor Kaszab: Looks good to me, approved

-- 
To view, visit http://gerrit.cloudera.org:8080/21061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ieb9bac1822a17bd3cd074b4b98e4d010703cecb1
Gerrit-Change-Number: 21061
Gerrit-PatchSet: 13
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>