Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/21578 )
Change subject: IMPALA-12857: Add flag to enable merge-on-read even if tables are configured with copy-on-write ...................................................................... Patch Set 1: (6 comments) Thanks for working on this! http://gerrit.cloudera.org:8080/#/c/21578/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21578/1//COMMIT_MSG@11 PS1, Line 11: ) nit: longer than 72 chars http://gerrit.cloudera.org:8080/#/c/21578/1/be/src/common/global-flags.cc File be/src/common/global-flags.cc: http://gerrit.cloudera.org:8080/#/c/21578/1/be/src/common/global-flags.cc@428 PS1, Line 428: enable Since merge-one-read is the default in Impala, I think a better name would be: "iceberg_always_allow_merge_on_read_operations". http://gerrit.cloudera.org:8080/#/c/21578/1/fe/src/main/java/org/apache/impala/analysis/IcebergDeleteImpl.java File fe/src/main/java/org/apache/impala/analysis/IcebergDeleteImpl.java: http://gerrit.cloudera.org:8080/#/c/21578/1/fe/src/main/java/org/apache/impala/analysis/IcebergDeleteImpl.java@46 PS1, Line 46: deleteMode != null && !deleteMode.equals("merge-on-read") Could be written as: "merge-on-read".equals(getModifyMode()) Or: Objects.equals(getModifyMode(), "merge-on-read") http://gerrit.cloudera.org:8080/#/c/21578/1/fe/src/main/java/org/apache/impala/analysis/IcebergDeleteImpl.java@45 PS1, Line 45: String deleteMode = getModifyMode(); : if (deleteMode != null && !deleteMode.equals("merge-on-read") : && !isMergeOnReadEnabled()) { : throw new AnalysisException(String.format("Unsupported delete mode: '%s' for " + : "Iceberg table: %s", deleteMode, originalTargetTable_.getFullName())); : } This could be moved to super.analyze(). The error message would be "Unsupported write mode" instead of "Unsupported delete mode" / "Unsupported update mode", but I think it should be OK. Or see my comment below at getModifyMode(). http://gerrit.cloudera.org:8080/#/c/21578/1/fe/src/main/java/org/apache/impala/analysis/IcebergDeleteImpl.java@86 PS1, Line 86: String getModifyMode() { : return originalTargetTable_.getIcebergApiTable().properties() : .get(TableProperties.DELETE_MODE); : } This method could just return "TableProperties.DELETE_MODE", then everything could be taken care by IcebergModifyImpl. The value of TableProperties.DELETE_MODE, namely "write.delete.mode", could be incorporated into the error message as well, e.g.: Unsupported write mode detected in table property 'write.delete.mode' of Iceberg table 'db.ice_t': 'copy-on-write' Or stg like that. http://gerrit.cloudera.org:8080/#/c/21578/1/fe/src/main/java/org/apache/impala/analysis/IcebergModifyImpl.java File fe/src/main/java/org/apache/impala/analysis/IcebergModifyImpl.java: http://gerrit.cloudera.org:8080/#/c/21578/1/fe/src/main/java/org/apache/impala/analysis/IcebergModifyImpl.java@120 PS1, Line 120: isMergeOnReadEnabled This should be 'isMergeOnReadAlwaysAllowed()' -- To view, visit http://gerrit.cloudera.org:8080/21578 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3800043e135beeedfb655a238c0644aaa0ef11f4 Gerrit-Change-Number: 21578 Gerrit-PatchSet: 1 Gerrit-Owner: Noemi Pap-Takacs <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Tue, 16 Jul 2024 15:57:57 +0000 Gerrit-HasComments: Yes
