Tamas Mate has posted comments on this change. ( http://gerrit.cloudera.org:8080/20951 )
Change subject: IMPALA-12598: Allow multiple equality filed id lists for Iceberg tables ...................................................................... Patch Set 3: (6 comments) Hi Gabor, thank you for the feature, it looks great. Marked some nits and I have a question, what was the reason behind moving from Thrift to FlatBuffer? http://gerrit.cloudera.org:8080/#/c/20951/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20951/3//COMMIT_MSG@7 PS3, Line 7: d nit: d http://gerrit.cloudera.org:8080/#/c/20951/3//COMMIT_MSG@9 PS3, Line 9: has nit: have It refers to the plural object. http://gerrit.cloudera.org:8080/#/c/20951/3/common/fbs/IcebergObjects.fbs File common/fbs/IcebergObjects.fbs: http://gerrit.cloudera.org:8080/#/c/20951/3/common/fbs/IcebergObjects.fbs@53 PS3, Line 53: equality_field_ids I saw this in multiple places, the new term became equality_fiel_ids, in many places it was left as equality_ids. I will mark some and I think we should use one of them to avoid confusion and to be consistent. Iceberg uses equality_ids to refer to equality field ids. https://iceberg.apache.org/spec/ http://gerrit.cloudera.org:8080/#/c/20951/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/20951/3/fe/src/main/java/org/apache/impala/planner/IcebergScanPlanner.java@126 PS3, Line 126: private Set<Integer> allEqualityFieldIds_ = new HashSet<>(); : private Map<List<Integer>, Set<FileDescriptor>> equalityIdsToDeleteFiles_ = : new HashMap<>(); nit: Here we use both equality ids and equality field ids. http://gerrit.cloudera.org:8080/#/c/20951/3/fe/src/main/java/org/apache/impala/planner/IcebergScanPlanner.java@510 PS3, Line 510: d nit: D http://gerrit.cloudera.org:8080/#/c/20951/3/testdata/workloads/functional-query/queries/QueryTest/iceberg-v2-read-equality-deletes.test File testdata/workloads/functional-query/queries/QueryTest/iceberg-v2-read-equality-deletes.test: http://gerrit.cloudera.org:8080/#/c/20951/3/testdata/workloads/functional-query/queries/QueryTest/iceberg-v2-read-equality-deletes.test@114 PS3, Line 114: filed nit: field -- To view, visit http://gerrit.cloudera.org:8080/20951 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3e52d7a5800bf1b479f0c234679be92442d09f79 Gerrit-Change-Number: 20951 Gerrit-PatchSet: 3 Gerrit-Owner: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Tamas Mate <tma...@apache.org> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Fri, 26 Jan 2024 10:33:25 +0000 Gerrit-HasComments: Yes
