Gabor Kaszab has posted comments on this change. ( http://gerrit.cloudera.org:8080/21125 )
Change subject: IMPALA-12611: Add support to MAP type Iceberg Metadata table columns ...................................................................... Patch Set 3: (4 comments) Thanks for the change, Dani! Frankly, reviewing these JNI changes isn't the easiest so I mostly focused on the tests so far. I'll keep reading the c++ implementation meanwhile. http://gerrit.cloudera.org:8080/#/c/21125/3/fe/src/main/java/org/apache/impala/util/IcebergMetadataScanner.java File fe/src/main/java/org/apache/impala/util/IcebergMetadataScanner.java: http://gerrit.cloudera.org:8080/#/c/21125/3/fe/src/main/java/org/apache/impala/util/IcebergMetadataScanner.java@163 PS3, Line 163: return iterator_.next(); nit: merge with line above http://gerrit.cloudera.org:8080/#/c/21125/3/testdata/workloads/functional-query/queries/QueryTest/iceberg-metadata-tables.test File testdata/workloads/functional-query/queries/QueryTest/iceberg-metadata-tables.test: http://gerrit.cloudera.org:8080/#/c/21125/3/testdata/workloads/functional-query/queries/QueryTest/iceberg-metadata-tables.test@761 PS3, Line 761: ==== Checked the Iceberg-Spark page for metadata tables: https://iceberg.apache.org/docs/latest/spark-queries/#snapshots This query seems interesting: select h.made_current_at, s.operation, h.snapshot_id, h.is_current_ancestor, s.summary['spark.app.id'] from prod.db.table.history h join prod.db.table.snapshots s on h.snapshot_id = s.snapshot_id order by made_current_at; Do you know if such a query is feasible now with our map implementation? http://gerrit.cloudera.org:8080/#/c/21125/3/testdata/workloads/functional-query/queries/QueryTest/iceberg-metadata-tables.test@764 PS3, Line 764: # Describe all the metadata tables once entries table seems to have embedded nested type cols. Could you try to add test coverage for them? http://gerrit.cloudera.org:8080/#/c/21125/3/testdata/workloads/functional-query/queries/QueryTest/iceberg-metadata-tables.test@765 PS3, Line 765: #### Can you add a test that queries different collections, arrays, maps? -- To view, visit http://gerrit.cloudera.org:8080/21125 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8a8b3a574ca45c893315c3b41b33ce4e0eff865a Gerrit-Change-Number: 21125 Gerrit-PatchSet: 3 Gerrit-Owner: Daniel Becker <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Noemi Pap-Takacs <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Wed, 27 Mar 2024 16:57:14 +0000 Gerrit-HasComments: Yes
