Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/24088 )
Change subject: IMPALA-14589: Add support for Iceberg V3 default values ...................................................................... Patch Set 1: (9 comments) Thanks for working on this! http://gerrit.cloudera.org:8080/#/c/24088/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24088/1//COMMIT_MSG@9 PS1, Line 9: schema nit: please use the 72-char limit in the body of the commit message http://gerrit.cloudera.org:8080/#/c/24088/1//COMMIT_MSG@27 PS1, Line 27: Impala is able to set default values in ALTER TABLE ADD/CHANGE COLUMN, for Kudu tables. We could also enable it for Iceberg tables to set initial-default/write-default. It could be implemented in a different CR. http://gerrit.cloudera.org:8080/#/c/24088/1/be/src/exec/file-metadata-utils.cc File be/src/exec/file-metadata-utils.cc: http://gerrit.cloudera.org:8080/#/c/24088/1/be/src/exec/file-metadata-utils.cc@176 PS1, Line 176: LOG(INFO) It should be VLOG_FILE or VLOG_ROW. Same goes for L184. http://gerrit.cloudera.org:8080/#/c/24088/1/be/src/exec/parquet/hdfs-parquet-scanner.cc File be/src/exec/parquet/hdfs-parquet-scanner.cc: http://gerrit.cloudera.org:8080/#/c/24088/1/be/src/exec/parquet/hdfs-parquet-scanner.cc@649 PS1, Line 649: // For Iceberg V3 tables, check if the column has an initial-default value : bool has_default_value = false; : if (scan_node_->hdfs_table()->IsIcebergTable()) { : const SchemaPath& col_path = slot_desc->col_path(); : if (col_path.size() == 1) { : const ColumnDescriptor& col_desc = : scan_node_->hdfs_table()->col_descs()[col_path.front()]; : has_default_value = !col_desc.initial_default().empty(); : } : } This could be moved to FileMetadataUtils::NeedDataInFile(). Then the below code changes (L659-669, and L805-L825) are not needed. http://gerrit.cloudera.org:8080/#/c/24088/1/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java File fe/src/main/java/org/apache/impala/analysis/InsertStmt.java: http://gerrit.cloudera.org:8080/#/c/24088/1/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java@998 PS1, Line 998: if (isIcebergTarget() && tblColumn instanceof IcebergColumn) { Can you please file a Jira ticket that it should be done for the MERGE-statement as well? (Currently MERGE is not supported for V3 tables). http://gerrit.cloudera.org:8080/#/c/24088/1/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java@1008 PS1, Line 1008: ) You could add: "Please specify value for column '{}' explicitly." http://gerrit.cloudera.org:8080/#/c/24088/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/24088/1/testdata/workloads/functional-query/queries/QueryTest/iceberg-v3-default-values.test@136 PS1, Line 136: write-default = -1 You could change the write-default in iceberg_v3_default_value/metadata/v4.metadata.json to a different value then the initial-default. http://gerrit.cloudera.org:8080/#/c/24088/1/testdata/workloads/functional-query/queries/QueryTest/iceberg-v3-default-values.test@163 PS1, Line 163: INSERT INTO iceberg_v3_default_value (i) VALUES (5), (6); Please add tests for INSERT-overwrite as well. http://gerrit.cloudera.org:8080/#/c/24088/1/tests/query_test/test_iceberg.py File tests/query_test/test_iceberg.py: http://gerrit.cloudera.org:8080/#/c/24088/1/tests/query_test/test_iceberg.py@2341 PS1, Line 2341: parquet Can we have tests for ORC/Avro as well? -- To view, visit http://gerrit.cloudera.org:8080/24088 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9f1be994a336b30b17b17819091417d777a39be9 Gerrit-Change-Number: 24088 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, 11 Mar 2026 16:23:14 +0000 Gerrit-HasComments: Yes
