Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/24386 )
Change subject: IMPALA-14589: [Part 4] Add Iceberg V3 DDL support for column defaults ...................................................................... Patch Set 1: (6 comments) Thanks for working on this! http://gerrit.cloudera.org:8080/#/c/24386/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24386/1//COMMIT_MSG@26 PS1, Line 26: BINARY, TIMESTAMP What are the challenges with BINARY and TIMESTAMP? http://gerrit.cloudera.org:8080/#/c/24386/1/fe/src/main/java/org/apache/impala/analysis/AlterTableAddColsStmt.java File fe/src/main/java/org/apache/impala/analysis/AlterTableAddColsStmt.java: http://gerrit.cloudera.org:8080/#/c/24386/1/fe/src/main/java/org/apache/impala/analysis/AlterTableAddColsStmt.java@140 PS1, Line 140: The specified column options are only supported " + : "in Kudu tables: " + c.toString()); This can be misleading since Iceberg tables also support a few options. http://gerrit.cloudera.org:8080/#/c/24386/1/fe/src/main/java/org/apache/impala/analysis/AlterTableAlterColStmt.java File fe/src/main/java/org/apache/impala/analysis/AlterTableAlterColStmt.java: http://gerrit.cloudera.org:8080/#/c/24386/1/fe/src/main/java/org/apache/impala/analysis/AlterTableAlterColStmt.java@162 PS1, Line 162: boolean icebergAlterDefaultOnly = t instanceof FeIcebergTable && : newColDef_.hasDefaultValue(); Shouldn't it be: boolean icebergAlterDefault = t instanceof FeIcebergTable && (newColDef_.hasDefaultValue() || isDropDefault_); http://gerrit.cloudera.org:8080/#/c/24386/1/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/24386/1/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java@718 PS1, Line 718: putGeneratedProperty(TableProperties.FORMAT_VERSION, "3"); I think we shouldn't auto-upgrade tables to V3. Provide an error message in this case, then the user can explicitly ask for a V3 table. http://gerrit.cloudera.org:8080/#/c/24386/1/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/24386/1/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java@223 PS1, Line 223: !ws.isEmpty() Does this mean users can't set empty string as default value? http://gerrit.cloudera.org:8080/#/c/24386/1/testdata/workloads/functional-query/queries/QueryTest/iceberg-v3-default-values.test File testdata/workloads/functional-query/queries/QueryTest/iceberg-v3-default-values.test: http://gerrit.cloudera.org:8080/#/c/24386/1/testdata/workloads/functional-query/queries/QueryTest/iceberg-v3-default-values.test@535 PS1, Line 535: ALTER TABLE iceberg_ddl_alter_defaults ALTER COLUMN d_col SET DEFAULT CAST('2024-01-15' AS DATE); What happens if the user issues CHANGE COLUMN to change multiple things? ALTER TABLE iceberg_ddl_alter_defaults CHANGE COLUMN d_col (new_d_col DATE DEFAULT '2024-01-15'); -- To view, visit http://gerrit.cloudera.org:8080/24386 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibec771f324a0a9534e767e4b1fc66b25b41483ae Gerrit-Change-Number: 24386 Gerrit-PatchSet: 1 Gerrit-Owner: Arnab Karmakar <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Wed, 03 Jun 2026 11:28:19 +0000 Gerrit-HasComments: Yes
