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 <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Wed, 30 Oct 2024 15:46:44 +0000 Gerrit-HasComments: Yes
