[GitHub] [iceberg] gaborkaszab commented on a diff in pull request #6006: Core,Spark: Refactor to move "copy-on-write" and "merge-on-read" literals to constants

[GitBox <gi...@apache.org>]

gaborkaszab commented on code in PR #6006:
URL: https://github.com/apache/iceberg/pull/6006#discussion_r998250473


##########
core/src/main/java/org/apache/iceberg/TableProperties.java:
##########
@@ -332,6 +332,8 @@ private TableProperties() {}
   public static final String MERGE_MODE = "write.merge.mode";
   public static final String MERGE_MODE_DEFAULT = "copy-on-write";
 
+  public static final String COPY_ON_WRITE_MODE = "copy-on-write";
+  public static final String MERGE_ON_READ_MODE = "merge-on-read";

Review Comment:
   MERGE_MODE_COPY_ON_WRITE would indicate that this is used for merge mode, but actually it's also for delete and update mode.
   
   I wouldn't delete MERGE_MODE_DEFAULT because in my opinion that is for different purpose than the ones I introduced here. Let's say that someone wants to change the default from "copy-on-write" to "merge-on-read". Then only this DEFAULT variable has to be changed. However, with your suggestion one has to search for the uses of COPY_ON_WRITE_MODE (if we stick with the original name) and replace them with MERGE_ON_READ_MODE, but not all of them only the ones where the default is required.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org