Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/19138 )
Change subject: IMPALA-11655: Impala should set write mode "merge-on-read" by default ...................................................................... Patch Set 1: (3 comments) Hi Zoltan, the code looks good. I just have couple nits to discuss if you don't mind. http://gerrit.cloudera.org:8080/#/c/19138/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/19138/1/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java@658 PS1, Line 658: "2" nit: maybe make this a static final String in IcebergTable.java, so we can track V2 specific codes? Also, does it makes more sense if it is better to try convert to integer and compare with ">= 2"? I understand there is only V1 and V2 now, but just in case. http://gerrit.cloudera.org:8080/#/c/19138/1/fe/src/main/java/org/apache/impala/util/IcebergUtil.java File fe/src/main/java/org/apache/impala/util/IcebergUtil.java: http://gerrit.cloudera.org:8080/#/c/19138/1/fe/src/main/java/org/apache/impala/util/IcebergUtil.java@839 PS1, Line 839: isAnyWriteModeSet nit: Result from this method is always negated in all invocation. Maybe consider to change this to "isNoWriteModeSet", and replace the logical operator from disjunction to conjunction? http://gerrit.cloudera.org:8080/#/c/19138/1/testdata/workloads/functional-query/queries/QueryTest/iceberg-alter.test File testdata/workloads/functional-query/queries/QueryTest/iceberg-alter.test: http://gerrit.cloudera.org:8080/#/c/19138/1/testdata/workloads/functional-query/queries/QueryTest/iceberg-alter.test@354 PS1, Line 354: ---- QUERY : CREATE TABLE iceberg_upgrade_v2_no_write_mode (i INT) STORED AS ICEBERG; : DESCRIBE FORMATTED iceberg_upgrade_v2_no_write_mode; : ---- RESULTS: VERIFY_IS_NOT_IN : '','write.delete.mode ','merge-on-read ' : '','write.update.mode ','merge-on-read ' : '','write.merge.mode ','merge-on-read ' nit: Add one more test where it explicitly set 'format-version'='1'? -- To view, visit http://gerrit.cloudera.org:8080/19138 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icaa32472cde98e21fb23f5461175db1bf401db3d Gerrit-Change-Number: 19138 Gerrit-PatchSet: 1 Gerrit-Owner: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Comment-Date: Thu, 13 Oct 2022 23:52:43 +0000 Gerrit-HasComments: Yes
