Arnab Karmakar 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 2: (7 comments) Thanks for the comments! http://gerrit.cloudera.org:8080/#/c/24386/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24386/1//COMMIT_MSG@7 PS1, Line 7: IMPALA-14589 > From now on please create a separate tickets/sub-tasks for each change. Sure. http://gerrit.cloudera.org:8080/#/c/24386/1//COMMIT_MSG@26 PS1, Line 26: BINARY, TIMESTAMP > What are the challenges with BINARY and TIMESTAMP? 1. BINARY: No way to display binary defaults, no hex() support; IcebergSchemaConverter#convertIcebergLiteralToString wasn't extended for it in [Part 2]. 2. TIMESTAMP: LiteralExpr doesn't support TIMESTAMP, so write-default parsing would fail in InsertStmt#getDefaultExpr. Initial-default works because the backend handles it at the scanner level, bypassing LiteralExpr. Avoided adding support otherwise either users wouldn't be able to omit TIMESTAMP cols in INSERT stmts or they'll have to manually DROP the DEFAULT. 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 not supported " + : "for this table type: " + c.toStrin > This can be misleading since Iceberg tables also support a few options. Done 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 icebergAlterDefault = t instanceof FeIcebergTable && : (newColDef_.hasDefaultValue() > Shouldn't it be: Done 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: (ver == null || ver < 3) { > I think we shouldn't auto-upgrade tables to V3. Provide an error message in Done 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: onverter.pars > Does this mean users can't set empty string as default value? Nice catch! We were just using a sentinel value of empty string "" for dropping defaults in the codebase. With this we wouldn't have been able to set empty string as default value. The actual behaviour should be that the catalog should erase the write-default key itself from the metadata.json file. Now we are doing this with UpdateSchema#updateColumnDefault() and we can now also set empty string as DEFAULT values. 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 str_col SET DEFAULT 'hello'; > What happens if the user issues CHANGE COLUMN to change multiple things? It'll be rejected with an error message: Unsupported column options in ALTER TABLE CHANGE COLUMN statement -- 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: 2 Gerrit-Owner: Arnab Karmakar <[email protected]> Gerrit-Reviewer: Arnab Karmakar <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Thu, 04 Jun 2026 09:29:37 +0000 Gerrit-HasComments: Yes
