You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Gabor Kaszab (Code Review)" <ge...@cloudera.org> on 2024/10/01 14:12:46 UTC

[Impala-ASF-CR] IMPALA-11265: Part2: Store Iceberg file descriptors in encoded format

Gabor Kaszab has uploaded this change for review. ( http://gerrit.cloudera.org:8080/21869


Change subject: IMPALA-11265: Part2: Store Iceberg file descriptors in encoded format
......................................................................

IMPALA-11265: Part2: Store Iceberg file descriptors in encoded format

The file descriptors in HdfsPartition are cached as byte arrays to keep
the memory footprint low. They are transformed into actual
FileDescriptor objects once queried.
This patch changes IcebergContentFileStore to similarly use byte arrays
as an internal representation for file descriptors. Note, file
descriptors for Iceberg tables have 2 components: one is the same as in
HdfsPartition and the other stores Iceberg specific file metadata in an
addiitonal byte array.

Measurements and observations:
 - I have a test table that has 110k data files. For this table the JVM
   memory usage in the catalogd got reduced from 80MB to 65MB.
 - Both HdfsPartition.FileDescriptor and IcebergContentFileStore use
   flatbuffers and in turn byte arrays to represent file descriptors and
   these byte arrays are shared between these 2 places. As a result
   there is no redundancy in storing the file descriptors both for the
   Iceberg and the Hdfs table.
 - There is no measurable difference in planning times with this patch.

Change-Id: I9d7794df999bdaf118158eace26cea610f911c0a
---
M fe/src/main/java/org/apache/impala/analysis/OptimizeStmt.java
M fe/src/main/java/org/apache/impala/catalog/FeIcebergTable.java
M fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java
M fe/src/main/java/org/apache/impala/catalog/IcebergContentFileStore.java
M fe/src/main/java/org/apache/impala/catalog/IcebergTable.java
M fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergCtasTarget.java
6 files changed, 192 insertions(+), 100 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I9d7794df999bdaf118158eace26cea610f911c0a
Gerrit-Change-Number: 21869
Gerrit-PatchSet: 1
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>

[Impala-ASF-CR] IMPALA-11265: Part2: Store Iceberg file descriptors in encoded format

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

Change subject: IMPALA-11265: Part2: Store Iceberg file descriptors in encoded format
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/21869/1/fe/src/main/java/org/apache/impala/catalog/IcebergContentFileStore.java
File fe/src/main/java/org/apache/impala/catalog/IcebergContentFileStore.java:

http://gerrit.cloudera.org:8080/#/c/21869/1/fe/src/main/java/org/apache/impala/catalog/IcebergContentFileStore.java@217
PS1, Line 217:   public FileDescriptor getDeleteFileDescriptor(String pathHash) {
Note to self: Now when we query the same pathHash multiple times, the objects we return won't be the same. This causes some hiccups when we put the FDs for equality deletes into a HashSet and we expect the duplicates being filtered by this. However, since they are different objects the EQ-deletes files will be stored redundantly in the IcebergScanNode that would mean we read some eq-delete files redundantly. This doesn't cause correctness issue fortunately.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9d7794df999bdaf118158eace26cea610f911c0a
Gerrit-Change-Number: 21869
Gerrit-PatchSet: 1
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
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, 29 Oct 2024 13:17:09 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11265: Part2: Store Iceberg file descriptors in encoded format

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

Change subject: IMPALA-11265: Part2: Store Iceberg file descriptors in encoded format
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/21869/1/fe/src/main/java/org/apache/impala/catalog/IcebergContentFileStore.java
File fe/src/main/java/org/apache/impala/catalog/IcebergContentFileStore.java:

http://gerrit.cloudera.org:8080/#/c/21869/1/fe/src/main/java/org/apache/impala/catalog/IcebergContentFileStore.java@123
PS1, Line 123:       if (!fileDescMap_.containsKey(pathHash)) return null;
Instead of checking 'containsKey()', we could use the result of fileDescMap_.get(pathHash) - if the map doesn't contain 'pathHash', the result will be null. That is one less lookup, although probably it's not very important, so if you feel this is clearer it's ok for me too.

Also L224.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9d7794df999bdaf118158eace26cea610f911c0a
Gerrit-Change-Number: 21869
Gerrit-PatchSet: 1
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Sat, 05 Oct 2024 09:54:06 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11265: Part2: Store Iceberg file descriptors in encoded format

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

Change subject: IMPALA-11265: Part2: Store Iceberg file descriptors in encoded format
......................................................................


Patch Set 5:

carry +2 from Zoltan


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9d7794df999bdaf118158eace26cea610f911c0a
Gerrit-Change-Number: 21869
Gerrit-PatchSet: 5
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
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: Fri, 22 Nov 2024 12:56:34 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11265: Part2: Store Iceberg file descriptors in encoded format

Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/21869 )

Change subject: IMPALA-11265: Part2: Store Iceberg file descriptors in encoded format
......................................................................


Patch Set 1:

(7 comments)

Nice findings and improvement!

http://gerrit.cloudera.org:8080/#/c/21869/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/21869/1//COMMIT_MSG@9
PS1, Line 9: The file descriptors in HdfsPartition are cached as byte arrays to keep
           : the memory footprint low.
To make it easier to understand, you could elaborate that putting a byte array to a ByteBuffer, then putting the ByteBuffer to an FbFileDesc/FbFileMetadata, then putting them into an HdfsPartition.FileDescriptor object all increases the size of the object.


http://gerrit.cloudera.org:8080/#/c/21869/1//COMMIT_MSG@16
PS1, Line 16: addiitonal
typo


http://gerrit.cloudera.org:8080/#/c/21869/1/fe/src/main/java/org/apache/impala/analysis/OptimizeStmt.java
File fe/src/main/java/org/apache/impala/analysis/OptimizeStmt.java:

http://gerrit.cloudera.org:8080/#/c/21869/1/fe/src/main/java/org/apache/impala/analysis/OptimizeStmt.java@240
PS1, Line 240: .Builder(iceTable, selectedContentFiles).build()
IcebergContentFileStore could just have a constructor that takes these two arguments.


http://gerrit.cloudera.org:8080/#/c/21869/1/fe/src/main/java/org/apache/impala/catalog/IcebergContentFileStore.java
File fe/src/main/java/org/apache/impala/catalog/IcebergContentFileStore.java:

http://gerrit.cloudera.org:8080/#/c/21869/1/fe/src/main/java/org/apache/impala/catalog/IcebergContentFileStore.java@72
PS1, Line 72:   private static final Function<EncodedFileDescriptor, FileDescriptor> FROM_BYTES =
            :       new Function<EncodedFileDescriptor, FileDescriptor>() {
Is there any advantage using a Function object over defining a method?
I see it can be used in Lists.transform(), but could you use a lambda like "l -> FromBytes(l)" or a method reference like "IcebergContentFileStore::FromBytes" (or "FromBytes" might work as well)? This way we could get rid of the boilerplate code required for the Function object.


http://gerrit.cloudera.org:8080/#/c/21869/1/fe/src/main/java/org/apache/impala/catalog/IcebergContentFileStore.java@101
PS1, Line 101: assert
Probably a Precondition check would be better here.


http://gerrit.cloudera.org:8080/#/c/21869/1/fe/src/main/java/org/apache/impala/catalog/IcebergContentFileStore.java@402
PS1, Line 402:       return fileDesc.cloneWithFileMetadata(
             :           IcebergUtil.createIcebergMetadata(icebergTable_, contentFile));
Would it make sense to return EncodedFileDescriptor here and also switch to EncodedFileDescriptor in the private add*() methods? We would save creating an unnecessary FileDescriptor object and invoking TO_BYTES.apply() for each file descriptor.


http://gerrit.cloudera.org:8080/#/c/21869/1/fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergCtasTarget.java
File fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergCtasTarget.java:

http://gerrit.cloudera.org:8080/#/c/21869/1/fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergCtasTarget.java@109
PS1, Line 109: new IcebergContentFileStore.Builder().build();
             :     } catch (IOException e) {
             :       throw new ImpalaRuntimeException(e.getMes
If we switched to a default constructor then we wouldn't need exception handling here.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9d7794df999bdaf118158eace26cea610f911c0a
Gerrit-Change-Number: 21869
Gerrit-PatchSet: 1
Gerrit-Owner: 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, 01 Oct 2024 17:06:11 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11265: Part2: Store Iceberg file descriptors in encoded format

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

Change subject: IMPALA-11265: Part2: Store Iceberg file descriptors in encoded format
......................................................................


Patch Set 2:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/21869/1/fe/src/main/java/org/apache/impala/analysis/OptimizeStmt.java
File fe/src/main/java/org/apache/impala/analysis/OptimizeStmt.java:

http://gerrit.cloudera.org:8080/#/c/21869/1/fe/src/main/java/org/apache/impala/analysis/OptimizeStmt.java@240
PS1, Line 240: (iceTable, selectedContentFiles);
> IcebergContentFileStore could just have a constructor that takes these two 
Done


http://gerrit.cloudera.org:8080/#/c/21869/1/fe/src/main/java/org/apache/impala/catalog/IcebergContentFileStore.java
File fe/src/main/java/org/apache/impala/catalog/IcebergContentFileStore.java:

http://gerrit.cloudera.org:8080/#/c/21869/1/fe/src/main/java/org/apache/impala/catalog/IcebergContentFileStore.java@72
PS1, Line 72:     Preconditions.checkNotNull(encodedFd.fileMetadata_);
            : 
> Is there any advantage using a Function object over defining a method?
Done


http://gerrit.cloudera.org:8080/#/c/21869/1/fe/src/main/java/org/apache/impala/catalog/IcebergContentFileStore.java@101
PS1, Line 101: ivate 
> Probably a Precondition check would be better here.
Done


http://gerrit.cloudera.org:8080/#/c/21869/1/fe/src/main/java/org/apache/impala/catalog/IcebergContentFileStore.java@123
PS1, Line 123:     // Adds a file to the map. If this is a new entry, then add it to the list as well.
> Instead of checking 'containsKey()', we could use the result of fileDescMap
I think this is more readable as it is now and the lookup is O(1) so shouldn't matter.


http://gerrit.cloudera.org:8080/#/c/21869/1/fe/src/main/java/org/apache/impala/catalog/IcebergContentFileStore.java@217
PS1, Line 217:     }
> Note to self: Now when we query the same pathHash multiple times, the objec
As a solution I overrided the equals() and hashCode() functions for the FileDescriptors returned by this  ContentFileStore.


http://gerrit.cloudera.org:8080/#/c/21869/1/fe/src/main/java/org/apache/impala/catalog/IcebergContentFileStore.java@402
PS1, Line 402: 
             : 
> Would it make sense to return EncodedFileDescriptor here and also switch to
Done


http://gerrit.cloudera.org:8080/#/c/21869/1/fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergCtasTarget.java
File fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergCtasTarget.java:

http://gerrit.cloudera.org:8080/#/c/21869/1/fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergCtasTarget.java@109
PS1, Line 109: 
             : 
             :   private void createIcebergSchema(List<ColumnD
> If we switched to a default constructor then we wouldn't need exception han
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9d7794df999bdaf118158eace26cea610f911c0a
Gerrit-Change-Number: 21869
Gerrit-PatchSet: 2
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
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: Wed, 30 Oct 2024 15:46:44 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11265: Part2: Store Iceberg file descriptors in encoded format

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

Change subject: IMPALA-11265: Part2: Store Iceberg file descriptors in encoded format
......................................................................

IMPALA-11265: Part2: Store Iceberg file descriptors in encoded format

The file descriptors in HdfsPartition are cached as byte arrays to keep
the memory footprint low. They are transformed into actual
FileDescriptor objects once queried.
This patch changes IcebergContentFileStore to similarly use byte arrays
as an internal representation for file descriptors. Note, file
descriptors for Iceberg tables have 2 components: one is the same as in
HdfsPartition and the other stores Iceberg specific file metadata in an
additional byte array.

Measurements and observations:
 - I have a test table that has 110k data files. For this table the JVM
   memory usage in the catalogd got reduced from 80MB to 65MB.
 - Both HdfsPartition.FileDescriptor and IcebergContentFileStore use
   flatbuffers and in turn byte arrays to represent file descriptors and
   these byte arrays are shared between these 2 places. As a result
   there is no redundancy in storing the file descriptors both for the
   Iceberg and the Hdfs table.
 - There is no measurable difference in planning times with this patch.

Change-Id: I9d7794df999bdaf118158eace26cea610f911c0a
Reviewed-on: http://gerrit.cloudera.org:8080/21869
Tested-by: Impala Public Jenkins <im...@cloudera.com>
Reviewed-by: Gabor Kaszab <ga...@cloudera.com>
---
M fe/src/main/java/org/apache/impala/analysis/OptimizeStmt.java
M fe/src/main/java/org/apache/impala/catalog/FeIcebergTable.java
M fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java
M fe/src/main/java/org/apache/impala/catalog/IcebergContentFileStore.java
M fe/src/main/java/org/apache/impala/catalog/IcebergTable.java
M fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergCtasTarget.java
M fe/src/main/java/org/apache/impala/planner/IcebergScanPlanner.java
A fe/src/test/java/org/apache/impala/catalog/IcebergContentFileStoreTest.java
8 files changed, 311 insertions(+), 111 deletions(-)

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

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I9d7794df999bdaf118158eace26cea610f911c0a
Gerrit-Change-Number: 21869
Gerrit-PatchSet: 6
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
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>

[Impala-ASF-CR] IMPALA-11265: Part2: Store Iceberg file descriptors in encoded format

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

Change subject: IMPALA-11265: Part2: Store Iceberg file descriptors in encoded format
......................................................................


Patch Set 4:

Build Failed 

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9d7794df999bdaf118158eace26cea610f911c0a
Gerrit-Change-Number: 21869
Gerrit-PatchSet: 4
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
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: Thu, 21 Nov 2024 13:38:35 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11265: Part2: Store Iceberg file descriptors in encoded format

Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
Hello Daniel Becker, Zoltan Borok-Nagy, Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/21869

to look at the new patch set (#2).

Change subject: IMPALA-11265: Part2: Store Iceberg file descriptors in encoded format
......................................................................

IMPALA-11265: Part2: Store Iceberg file descriptors in encoded format

The file descriptors in HdfsPartition are cached as byte arrays to keep
the memory footprint low. They are transformed into actual
FileDescriptor objects once queried.
This patch changes IcebergContentFileStore to similarly use byte arrays
as an internal representation for file descriptors. Note, file
descriptors for Iceberg tables have 2 components: one is the same as in
HdfsPartition and the other stores Iceberg specific file metadata in an
addiitonal byte array.

Measurements and observations:
 - I have a test table that has 110k data files. For this table the JVM
   memory usage in the catalogd got reduced from 80MB to 65MB.
 - Both HdfsPartition.FileDescriptor and IcebergContentFileStore use
   flatbuffers and in turn byte arrays to represent file descriptors and
   these byte arrays are shared between these 2 places. As a result
   there is no redundancy in storing the file descriptors both for the
   Iceberg and the Hdfs table.
 - There is no measurable difference in planning times with this patch.

Change-Id: I9d7794df999bdaf118158eace26cea610f911c0a
---
M fe/src/main/java/org/apache/impala/analysis/OptimizeStmt.java
M fe/src/main/java/org/apache/impala/catalog/FeIcebergTable.java
M fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java
M fe/src/main/java/org/apache/impala/catalog/IcebergContentFileStore.java
M fe/src/main/java/org/apache/impala/catalog/IcebergTable.java
M fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergCtasTarget.java
6 files changed, 184 insertions(+), 111 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9d7794df999bdaf118158eace26cea610f911c0a
Gerrit-Change-Number: 21869
Gerrit-PatchSet: 2
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
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>

[Impala-ASF-CR] IMPALA-11265: Part2: Store Iceberg file descriptors in encoded format

Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/21869 )

Change subject: IMPALA-11265: Part2: Store Iceberg file descriptors in encoded format
......................................................................


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/21869/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/21869/2//COMMIT_MSG@16
PS2, Line 16: addiitonal
typo


http://gerrit.cloudera.org:8080/#/c/21869/2/fe/src/main/java/org/apache/impala/catalog/IcebergContentFileStore.java
File fe/src/main/java/org/apache/impala/catalog/IcebergContentFileStore.java:

http://gerrit.cloudera.org:8080/#/c/21869/2/fe/src/main/java/org/apache/impala/catalog/IcebergContentFileStore.java@337
PS2, Line 337:     if (contentFile.content() == FileContent.EQUALITY_DELETES) {
             :       Preconditions.checkState(
             :           icebergMetadata.icebergMetadata().equalityFieldIdsLength() > 0);
             :     }
Probably it's better to throw such exceptions from IcebergScanPlanner. If we throw these during table loading it'll mean Impala won't be able to do anything with the table.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9d7794df999bdaf118158eace26cea610f911c0a
Gerrit-Change-Number: 21869
Gerrit-PatchSet: 2
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
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: Wed, 13 Nov 2024 14:53:42 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11265: Part2: Store Iceberg file descriptors in encoded format

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

Change subject: IMPALA-11265: Part2: Store Iceberg file descriptors in encoded format
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/21869/2/fe/src/main/java/org/apache/impala/catalog/IcebergContentFileStore.java
File fe/src/main/java/org/apache/impala/catalog/IcebergContentFileStore.java:

http://gerrit.cloudera.org:8080/#/c/21869/2/fe/src/main/java/org/apache/impala/catalog/IcebergContentFileStore.java@71
PS2, Line 71:   private static FileDescriptor decode(EncodedFileDescriptor encodedFd) {
Maybe write a Java unit test that runs decode(encode(fd)) and checks the result.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9d7794df999bdaf118158eace26cea610f911c0a
Gerrit-Change-Number: 21869
Gerrit-PatchSet: 2
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
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: Thu, 31 Oct 2024 16:24:13 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11265: Part2: Store Iceberg file descriptors in encoded format

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

Change subject: IMPALA-11265: Part2: Store Iceberg file descriptors in encoded format
......................................................................


Patch Set 4:

Build Failed 

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9d7794df999bdaf118158eace26cea610f911c0a
Gerrit-Change-Number: 21869
Gerrit-PatchSet: 4
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
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: Thu, 21 Nov 2024 15:20:31 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11265: Part2: Store Iceberg file descriptors in encoded format

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

Change subject: IMPALA-11265: Part2: Store Iceberg file descriptors in encoded format
......................................................................


Patch Set 5:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9d7794df999bdaf118158eace26cea610f911c0a
Gerrit-Change-Number: 21869
Gerrit-PatchSet: 5
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
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: Fri, 22 Nov 2024 12:54:16 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11265: Part2: Store Iceberg file descriptors in encoded format

Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/21869 )

Change subject: IMPALA-11265: Part2: Store Iceberg file descriptors in encoded format
......................................................................


Patch Set 4: Code-Review+2

LGTM!


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9d7794df999bdaf118158eace26cea610f911c0a
Gerrit-Change-Number: 21869
Gerrit-PatchSet: 4
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
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: Thu, 21 Nov 2024 13:35:30 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11265: Part2: Store Iceberg file descriptors in encoded format

Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
Hello Andrew Sherman, Daniel Becker, Zoltan Borok-Nagy, Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/21869

to look at the new patch set (#3).

Change subject: IMPALA-11265: Part2: Store Iceberg file descriptors in encoded format
......................................................................

IMPALA-11265: Part2: Store Iceberg file descriptors in encoded format

The file descriptors in HdfsPartition are cached as byte arrays to keep
the memory footprint low. They are transformed into actual
FileDescriptor objects once queried.
This patch changes IcebergContentFileStore to similarly use byte arrays
as an internal representation for file descriptors. Note, file
descriptors for Iceberg tables have 2 components: one is the same as in
HdfsPartition and the other stores Iceberg specific file metadata in an
additional byte array.

Measurements and observations:
 - I have a test table that has 110k data files. For this table the JVM
   memory usage in the catalogd got reduced from 80MB to 65MB.
 - Both HdfsPartition.FileDescriptor and IcebergContentFileStore use
   flatbuffers and in turn byte arrays to represent file descriptors and
   these byte arrays are shared between these 2 places. As a result
   there is no redundancy in storing the file descriptors both for the
   Iceberg and the Hdfs table.
 - There is no measurable difference in planning times with this patch.

Change-Id: I9d7794df999bdaf118158eace26cea610f911c0a
---
M fe/src/main/java/org/apache/impala/analysis/OptimizeStmt.java
M fe/src/main/java/org/apache/impala/catalog/FeIcebergTable.java
M fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java
M fe/src/main/java/org/apache/impala/catalog/IcebergContentFileStore.java
M fe/src/main/java/org/apache/impala/catalog/IcebergTable.java
M fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergCtasTarget.java
M fe/src/main/java/org/apache/impala/planner/IcebergScanPlanner.java
A fe/src/test/java/org/apache/impala/catalog/IcebergContentFileStoreTest.java
8 files changed, 311 insertions(+), 111 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9d7794df999bdaf118158eace26cea610f911c0a
Gerrit-Change-Number: 21869
Gerrit-PatchSet: 3
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
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>

[Impala-ASF-CR] IMPALA-11265: Part2: Store Iceberg file descriptors in encoded format

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

Change subject: IMPALA-11265: Part2: Store Iceberg file descriptors in encoded format
......................................................................


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/21869/2/fe/src/main/java/org/apache/impala/catalog/IcebergContentFileStore.java
File fe/src/main/java/org/apache/impala/catalog/IcebergContentFileStore.java:

http://gerrit.cloudera.org:8080/#/c/21869/2/fe/src/main/java/org/apache/impala/catalog/IcebergContentFileStore.java@83
PS2, Line 83:       public boolean equals(Object obj) {
Wouldn't it make sense to include the equals() and hashCode() overrides in the definition of FileDescriptor, instead of using a subclass here?


http://gerrit.cloudera.org:8080/#/c/21869/2/fe/src/main/java/org/apache/impala/catalog/IcebergContentFileStore.java@205
PS2, Line 205: pathHashAndFd
This could be a local variable in the loops below, I think it's good to limit the scope of variables.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9d7794df999bdaf118158eace26cea610f911c0a
Gerrit-Change-Number: 21869
Gerrit-PatchSet: 2
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
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: Thu, 07 Nov 2024 10:34:48 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11265: Part2: Store Iceberg file descriptors in encoded format

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

Change subject: IMPALA-11265: Part2: Store Iceberg file descriptors in encoded format
......................................................................


Patch Set 2:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/17306/ : 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/21869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9d7794df999bdaf118158eace26cea610f911c0a
Gerrit-Change-Number: 21869
Gerrit-PatchSet: 2
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
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: Wed, 30 Oct 2024 16:10:24 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11265: Part2: Store Iceberg file descriptors in encoded format

Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/21869 )

Change subject: IMPALA-11265: Part2: Store Iceberg file descriptors in encoded format
......................................................................


Patch Set 3: Code-Review+1

(2 comments)

LGTM!

http://gerrit.cloudera.org:8080/#/c/21869/3/fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergCtasTarget.java
File fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergCtasTarget.java:

http://gerrit.cloudera.org:8080/#/c/21869/3/fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergCtasTarget.java@20
PS3, Line 20: import java.io.IOException;
Unused import


http://gerrit.cloudera.org:8080/#/c/21869/3/fe/src/main/java/org/apache/impala/planner/IcebergScanPlanner.java
File fe/src/main/java/org/apache/impala/planner/IcebergScanPlanner.java:

http://gerrit.cloudera.org:8080/#/c/21869/3/fe/src/main/java/org/apache/impala/planner/IcebergScanPlanner.java@595
PS3, Line 595: )
Could you please add an error message, so we won't only get IllegalStateException without any explanation?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9d7794df999bdaf118158eace26cea610f911c0a
Gerrit-Change-Number: 21869
Gerrit-PatchSet: 3
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
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: Wed, 20 Nov 2024 14:04:20 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11265: Part2: Store Iceberg file descriptors in encoded format

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

Change subject: IMPALA-11265: Part2: Store Iceberg file descriptors in encoded format
......................................................................


Patch Set 3: Code-Review+1

+1 to show my comment was addressed, thanks


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9d7794df999bdaf118158eace26cea610f911c0a
Gerrit-Change-Number: 21869
Gerrit-PatchSet: 3
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
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: Wed, 20 Nov 2024 17:48:27 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11265: Part2: Store Iceberg file descriptors in encoded format

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

Change subject: IMPALA-11265: Part2: Store Iceberg file descriptors in encoded format
......................................................................


Patch Set 5: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9d7794df999bdaf118158eace26cea610f911c0a
Gerrit-Change-Number: 21869
Gerrit-PatchSet: 5
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
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: Fri, 22 Nov 2024 18:26:09 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11265: Part2: Store Iceberg file descriptors in encoded format

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

Change subject: IMPALA-11265: Part2: Store Iceberg file descriptors in encoded format
......................................................................


Patch Set 1:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/17076/ : 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/21869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9d7794df999bdaf118158eace26cea610f911c0a
Gerrit-Change-Number: 21869
Gerrit-PatchSet: 1
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 01 Oct 2024 14:39:08 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11265: Part2: Store Iceberg file descriptors in encoded format

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

Change subject: IMPALA-11265: Part2: Store Iceberg file descriptors in encoded format
......................................................................


Patch Set 2:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/21869/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/21869/2//COMMIT_MSG@16
PS2, Line 16: addiitonal
> typo
Done


http://gerrit.cloudera.org:8080/#/c/21869/2/fe/src/main/java/org/apache/impala/catalog/IcebergContentFileStore.java
File fe/src/main/java/org/apache/impala/catalog/IcebergContentFileStore.java:

http://gerrit.cloudera.org:8080/#/c/21869/2/fe/src/main/java/org/apache/impala/catalog/IcebergContentFileStore.java@71
PS2, Line 71:   private static FileDescriptor decode(EncodedFileDescriptor encodedFd) {
> Maybe write a Java unit test that runs decode(encode(fd)) and checks the re
Done


http://gerrit.cloudera.org:8080/#/c/21869/2/fe/src/main/java/org/apache/impala/catalog/IcebergContentFileStore.java@83
PS2, Line 83:       public boolean equals(Object obj) {
> Wouldn't it make sense to include the equals() and hashCode() overrides in 
I also thought of that. I went for this approach because these functions would only be useful for FileDescriptors coming from the IcebergContentFileStore, so I figured the less noise I add to HdfsPartition.FileDescriptor the less regression I cause :)


http://gerrit.cloudera.org:8080/#/c/21869/2/fe/src/main/java/org/apache/impala/catalog/IcebergContentFileStore.java@205
PS2, Line 205: pathHashAndFd
> This could be a local variable in the loops below, I think it's good to lim
well, this is a copy-pasted code from FeIcebergTable.

Done


http://gerrit.cloudera.org:8080/#/c/21869/2/fe/src/main/java/org/apache/impala/catalog/IcebergContentFileStore.java@337
PS2, Line 337:     if (contentFile.content() == FileContent.EQUALITY_DELETES) {
             :       Preconditions.checkState(
             :           icebergMetadata.icebergMetadata().equalityFieldIdsLength() > 0);
             :     }
> Probably it's better to throw such exceptions from IcebergScanPlanner. If w
Good point. I moved this to IcebergScanPlanner.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9d7794df999bdaf118158eace26cea610f911c0a
Gerrit-Change-Number: 21869
Gerrit-PatchSet: 2
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
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, 18 Nov 2024 10:46:49 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11265: Part2: Store Iceberg file descriptors in encoded format

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

Change subject: IMPALA-11265: Part2: Store Iceberg file descriptors in encoded format
......................................................................


Patch Set 3:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/17491/ : 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/21869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9d7794df999bdaf118158eace26cea610f911c0a
Gerrit-Change-Number: 21869
Gerrit-PatchSet: 3
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
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, 18 Nov 2024 11:12:45 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11265: Part2: Store Iceberg file descriptors in encoded format

Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
Hello Andrew Sherman, Daniel Becker, Zoltan Borok-Nagy, Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/21869

to look at the new patch set (#4).

Change subject: IMPALA-11265: Part2: Store Iceberg file descriptors in encoded format
......................................................................

IMPALA-11265: Part2: Store Iceberg file descriptors in encoded format

The file descriptors in HdfsPartition are cached as byte arrays to keep
the memory footprint low. They are transformed into actual
FileDescriptor objects once queried.
This patch changes IcebergContentFileStore to similarly use byte arrays
as an internal representation for file descriptors. Note, file
descriptors for Iceberg tables have 2 components: one is the same as in
HdfsPartition and the other stores Iceberg specific file metadata in an
additional byte array.

Measurements and observations:
 - I have a test table that has 110k data files. For this table the JVM
   memory usage in the catalogd got reduced from 80MB to 65MB.
 - Both HdfsPartition.FileDescriptor and IcebergContentFileStore use
   flatbuffers and in turn byte arrays to represent file descriptors and
   these byte arrays are shared between these 2 places. As a result
   there is no redundancy in storing the file descriptors both for the
   Iceberg and the Hdfs table.
 - There is no measurable difference in planning times with this patch.

Change-Id: I9d7794df999bdaf118158eace26cea610f911c0a
---
M fe/src/main/java/org/apache/impala/analysis/OptimizeStmt.java
M fe/src/main/java/org/apache/impala/catalog/FeIcebergTable.java
M fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java
M fe/src/main/java/org/apache/impala/catalog/IcebergContentFileStore.java
M fe/src/main/java/org/apache/impala/catalog/IcebergTable.java
M fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergCtasTarget.java
M fe/src/main/java/org/apache/impala/planner/IcebergScanPlanner.java
A fe/src/test/java/org/apache/impala/catalog/IcebergContentFileStoreTest.java
8 files changed, 311 insertions(+), 111 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/69/21869/4
-- 
To view, visit http://gerrit.cloudera.org:8080/21869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9d7794df999bdaf118158eace26cea610f911c0a
Gerrit-Change-Number: 21869
Gerrit-PatchSet: 4
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
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>

[Impala-ASF-CR] IMPALA-11265: Part2: Store Iceberg file descriptors in encoded format

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

Change subject: IMPALA-11265: Part2: Store Iceberg file descriptors in encoded format
......................................................................


Patch Set 4:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/21869/3/fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergCtasTarget.java
File fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergCtasTarget.java:

http://gerrit.cloudera.org:8080/#/c/21869/3/fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergCtasTarget.java@20
PS3, Line 20: import java.util.ArrayList;
> Unused import
Done


http://gerrit.cloudera.org:8080/#/c/21869/3/fe/src/main/java/org/apache/impala/planner/IcebergScanPlanner.java
File fe/src/main/java/org/apache/impala/planner/IcebergScanPlanner.java:

http://gerrit.cloudera.org:8080/#/c/21869/3/fe/src/main/java/org/apache/impala/planner/IcebergScanPlanner.java@595
PS3, Line 595: ,
> Could you please add an error message, so we won't only get IllegalStateExc
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9d7794df999bdaf118158eace26cea610f911c0a
Gerrit-Change-Number: 21869
Gerrit-PatchSet: 4
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
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: Thu, 21 Nov 2024 13:21:34 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11265: Part2: Store Iceberg file descriptors in encoded format

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

Change subject: IMPALA-11265: Part2: Store Iceberg file descriptors in encoded format
......................................................................


Patch Set 5: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9d7794df999bdaf118158eace26cea610f911c0a
Gerrit-Change-Number: 21869
Gerrit-PatchSet: 5
Gerrit-Owner: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
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, 25 Nov 2024 14:14:55 +0000
Gerrit-HasComments: No