Zoltan Borok-Nagy 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) Thanks Gabor for working on this, the change looks great! 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@136 PS3, Line 136: filed typo: field http://gerrit.cloudera.org:8080/#/c/20951/3/fe/src/main/java/org/apache/impala/planner/IcebergScanPlanner.java@520 PS3, Line 520: [1,2] => "[1, 2] This way [11, 12] would come earlier than [2, 3]. It shouldn't make much difference, as this ordering is quite arbitrary anyway, so it should be OK I think. Alternatively, can't we just use/write a comparator that is able to compare two lists of integers? http://gerrit.cloudera.org:8080/#/c/20951/3/testdata/data/README File testdata/data/README: http://gerrit.cloudera.org:8080/#/c/20951/3/testdata/data/README@884 PS3, Line 884: You also mention NiFi in the commit message, but here I don't see any reference to it. Probably those were manual tests only, and the tables weren't added to the test corpus, right? Maybe it'd be worth clarifying in the commit message. http://gerrit.cloudera.org:8080/#/c/20951/3/testdata/datasets/functional/functional_schema_template.sql File testdata/datasets/functional/functional_schema_template.sql: http://gerrit.cloudera.org:8080/#/c/20951/3/testdata/datasets/functional/functional_schema_template.sql@3643 PS3, Line 3643: 'write.update.mode'='merge-on-read' Do we need this? This is the default for Impala. http://gerrit.cloudera.org:8080/#/c/20951/3/testdata/workloads/functional-planner/queries/PlannerTest/iceberg-v2-tables.test File testdata/workloads/functional-planner/queries/PlannerTest/iceberg-v2-tables.test: http://gerrit.cloudera.org:8080/#/c/20951/3/testdata/workloads/functional-planner/queries/PlannerTest/iceberg-v2-tables.test@1396 PS3, Line 1396: cardinality=2 All EQ delete group has cardinality=2, which means we don't have test for the ordering between them. http://gerrit.cloudera.org:8080/#/c/20951/3/testdata/workloads/functional-planner/queries/PlannerTest/iceberg-v2-tables.test@1543 PS3, Line 1543: ==== Would be nice to see some time travel plans. -- 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, 02 Feb 2024 15:22:30 +0000 Gerrit-HasComments: Yes
