Zoltan Borok-Nagy has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/24042 )

Change subject: IMPALA-14592: Read Row Lineage of Iceberg tables
......................................................................


Patch Set 4:

(6 comments)

Thanks for the comments!

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

http://gerrit.cloudera.org:8080/#/c/24042/3//COMMIT_MSG@17
PS3, Line 17: or
> nit: or
Done


http://gerrit.cloudera.org:8080/#/c/24042/3//COMMIT_MSG@28
PS3, Line 28: _file_row_id
> What about _FILE_ROW_ID, similarly with fully uppercase words like the virt
I was using lower-case to emphasize they differ from virtual columns, and they 
are more similar to regular columns. I'm also trying to use this convention in 
the tests.


http://gerrit.cloudera.org:8080/#/c/24042/3/common/fbs/IcebergObjects.fbs
File common/fbs/IcebergObjects.fbs:

http://gerrit.cloudera.org:8080/#/c/24042/3/common/fbs/IcebergObjects.fbs@52
PS3, Line 52:   first_row_id : long;
> it could be -1 by default as it is the sentinel value
When upgrading V2 to V3, at some point we need to mutate this first_row_id 
member to an actual value. If -1 is the default, we cannot mutate the field in 
the Flatbuffer, because default values are not stored by the flatbuffer 
objects. Therefore we need a non-default sentinel value which we can mutate to 
an actual value.

Other approach would be to somehow invalidate old file descriptors when we 
notice that first-row-id assignement happened. This could happen in 
IcebergFileMetadataLoader, but it'd require re-loading lots of file 
descriptors. Though it is a once in a table lifetime event.


http://gerrit.cloudera.org:8080/#/c/24042/3/fe/src/main/java/org/apache/impala/catalog/FeIcebergTable.java
File fe/src/main/java/org/apache/impala/catalog/FeIcebergTable.java:

http://gerrit.cloudera.org:8080/#/c/24042/3/fe/src/main/java/org/apache/impala/catalog/FeIcebergTable.java@317
PS3, Line 317: signed for
> Magic number. Is this constant defined in Iceberg library?
Yes, replaced it.


http://gerrit.cloudera.org:8080/#/c/24042/3/fe/src/main/java/org/apache/impala/catalog/FeIcebergTable.java@320
PS3, Line 320: ber which
> Magic number.
Done


http://gerrit.cloudera.org:8080/#/c/24042/3/fe/src/main/java/org/apache/impala/catalog/IcebergFileMetadataLoader.java
File fe/src/main/java/org/apache/impala/catalog/IcebergFileMetadataLoader.java:

http://gerrit.cloudera.org:8080/#/c/24042/3/fe/src/main/java/org/apache/impala/catalog/IcebergFileMetadataLoader.java@198
PS3, Line 198:  id
> nit: delete duplicate 'of'
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I71b1076b25c9e7a0a6c9428b24abc986f5382c71
Gerrit-Change-Number: 24042
Gerrit-PatchSet: 4
Gerrit-Owner: Zoltan Borok-Nagy <[email protected]>
Gerrit-Reviewer: Arnab Karmakar <[email protected]>
Gerrit-Reviewer: Impala Public Jenkins <[email protected]>
Gerrit-Reviewer: Noemi Pap-Takacs <[email protected]>
Gerrit-Reviewer: Peter Rozsa <[email protected]>
Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]>
Gerrit-Comment-Date: Tue, 10 Mar 2026 14:08:20 +0000
Gerrit-HasComments: Yes

Reply via email to