Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/20429 )
Change subject: IMPALA-12409: Don't allow EXTERNAL Iceberg tables to point another Iceberg table in Hive catalog ...................................................................... Patch Set 3: (5 comments) Thanks for the comments! http://gerrit.cloudera.org:8080/#/c/20429/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20429/2//COMMIT_MSG@10 PS2, Line 10: Hive Catalog > I might miss something but why is this patch restricted to Hive catalog? I Disallowing that for other catalogs would limit Impala's ability to load existing Iceberg tables from arbitrary catalogs. For existing Iceberg tables in Hive Catalog there is no need to create external Iceberg tables that point to them. http://gerrit.cloudera.org:8080/#/c/20429/2/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java File fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java: http://gerrit.cloudera.org:8080/#/c/20429/2/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java@778 PS2, Line 778: if (isExternalWithNoPurge() && IcebergUtil.isHiveCatalog(getTblProperties())) { > Never mind, I tried it out, and with 'external.table.purge'=true the table You could also specify a schema for such tables, e.g.: create table ice_tt (i int) stored by iceberg tblproperties ('iceberg.table_identifier'='default.ice_t'); But the table creation will fail if the target table already exists. Also, during table loading we would throw an exception for such tables as well. Added a test for this. http://gerrit.cloudera.org:8080/#/c/20429/2/testdata/workloads/functional-query/queries/QueryTest/iceberg-negative.test File testdata/workloads/functional-query/queries/QueryTest/iceberg-negative.test: http://gerrit.cloudera.org:8080/#/c/20429/2/testdata/workloads/functional-query/queries/QueryTest/iceberg-negative.test@103 PS2, Line 103: functional_ > nit: I love the name functioanal :D :D Fixed it. http://gerrit.cloudera.org:8080/#/c/20429/2/tests/query_test/test_iceberg.py File tests/query_test/test_iceberg.py: http://gerrit.cloudera.org:8080/#/c/20429/2/tests/query_test/test_iceberg.py@1096 PS2, Line 1096: self.run_stmt_in_hive > Just curious: Is there a similar working going on Hive side to reject setti 'iceberg.table_identifier' is something that only Impala uses. They might disallow 'iceberg.mr.table.identifier' in the future, but currently Hive ignores this property, so probably they don't really care about it. But anyway, yeah, if they change the behavior we will fix the tests. http://gerrit.cloudera.org:8080/#/c/20429/2/tests/query_test/test_iceberg.py@1103 PS2, Line 1103: self.run_stmt_in_hive(""" > We have already altered the 'iceberg.table_identifier' of this table so ref Oops, earlier I had UNSET TBLPROPERTIES but it got removed when I moved code around. Thanks for catching this. -- To view, visit http://gerrit.cloudera.org:8080/20429 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifb0d7f0e7ec40fba356bd58b43f68d070432de71 Gerrit-Change-Number: 20429 Gerrit-PatchSet: 3 Gerrit-Owner: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Noemi Pap-Takacs <[email protected]> Gerrit-Reviewer: Tamas Mate <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Thu, 31 Aug 2023 09:43:08 +0000 Gerrit-HasComments: Yes
