[GitHub] [hudi] xushiyan commented on a diff in pull request #5771: [HUDI-4071] Relax record key requirement and write with minimal options

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

xushiyan commented on code in PR #5771:
URL: https://github.com/apache/hudi/pull/5771#discussion_r931043291


##########
hudi-common/src/main/java/org/apache/hudi/common/table/HoodieTableConfig.java:
##########
@@ -235,6 +235,13 @@ public class HoodieTableConfig extends HoodieConfig {
       .withDocumentation("Comma-separated list of metadata partitions that have been completely built and in-sync with data table. "
           + "These partitions are ready for use by the readers");
 
+  public static final ConfigProperty<Boolean> APPEND_ONLY_TABLE = ConfigProperty
+      .key("hoodie.table.append.only")
+      .defaultValue(false)
+      .sinceVersion("0.12.0")
+      .withDocumentation("When enabled, the writer will simply bulk insert without meta fields assuming that workload is append-only and there are no updates."
+          + " This is enabled only when users have not configured any operation type, record key and precombine field.");

Review Comment:
   ```suggestion
             + " This will be auto-enabled when users have not configured any operation type, record key, or precombine field.");
   ```



##########
hudi-common/src/main/java/org/apache/hudi/common/table/HoodieTableMetaClient.java:
##########
@@ -396,6 +398,16 @@ public HoodieArchivedTimeline getArchivedTimeline(String startTs) {
    * @param properties Properties from writeConfig.
    */
   public void validateTableProperties(Properties properties) {

Review Comment:
   it's good to explicitly say `writeConfigProps` here  instead of `properties`



##########
hudi-common/src/main/java/org/apache/hudi/common/table/HoodieTableMetaClient.java:
##########
@@ -438,7 +450,7 @@ public static HoodieTableMetaClient initTableAndGetMetaClient(Configuration hado
 
     // if anything other than default archive log folder is specified, create that too
     String archiveLogPropVal = new HoodieConfig(props).getStringOrDefault(HoodieTableConfig.ARCHIVELOG_FOLDER);
-    if (!StringUtils.isNullOrEmpty(archiveLogPropVal)) {
+    if (!isNullOrEmpty(archiveLogPropVal)) {

Review Comment:
   nonEmpty()



##########
hudi-common/src/main/java/org/apache/hudi/common/table/HoodieTableMetaClient.java:
##########
@@ -1008,7 +1029,7 @@ public Properties build() {
         tableConfig.setValue(HoodieTableConfig.CREATE_SCHEMA, tableCreateSchema);
       }
 
-      if (!StringUtils.isNullOrEmpty(archiveLogFolder)) {
+      if (!isNullOrEmpty(archiveLogFolder)) {

Review Comment:
   nonEmpty()



##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/hudi/HoodieSparkSqlWriter.scala:
##########
@@ -118,6 +118,19 @@ object HoodieSparkSqlWriter {
 
       operation = WriteOperationType.INSERT
     }
+    // If no record key field and precombine field is provided, assume it's an append-only workload and do bulk insert
+    if (!hoodieConfig.contains(RECORDKEY_FIELD)
+      && !hoodieConfig.contains(PRECOMBINE_FIELD)) {
+      if (!optParams.contains(OPERATION.key)
+        || optParams.getOrDefault(OPERATION.key, OPERATION.defaultValue).equalsIgnoreCase(BULK_INSERT_OPERATION_OPT_VAL)) {
+        log.warn(s"$RECORDKEY_FIELD and $PRECOMBINE_FIELD is not specified " +

Review Comment:
   ```suggestion
           log.warn(s"Neither $RECORDKEY_FIELD nor $PRECOMBINE_FIELD is specified; " +
   ```



##########
hudi-common/src/main/java/org/apache/hudi/common/table/HoodieTableMetaClient.java:
##########
@@ -396,6 +398,16 @@ public HoodieArchivedTimeline getArchivedTimeline(String startTs) {
    * @param properties Properties from writeConfig.
    */
   public void validateTableProperties(Properties properties) {
+    // Once table is configured to be append-only, it cannot be mutable or allow setting record key or precombine fields for updates
+    if (getTableConfig().isAppendOnlyTable()) {
+      boolean appendOnlyTable = Boolean.parseBoolean(
+          properties.getProperty(HoodieTableConfig.APPEND_ONLY_TABLE.key(), String.valueOf(HoodieTableConfig.APPEND_ONLY_TABLE.defaultValue())));
+      checkArgument(appendOnlyTable, String.format("%s is enabled. Please recreate the table with record key to support mutable tables.", HoodieTableConfig.APPEND_ONLY_TABLE.key()));
+      String recordKeyFields = properties.getProperty(HoodieTableConfig.RECORDKEY_FIELDS.key());
+      checkState(isNullOrEmpty(recordKeyFields), String.format("%s set for an append-only table", HoodieTableConfig.RECORDKEY_FIELDS.key()));
+      String precombineField = properties.getProperty(HoodieTableConfig.PRECOMBINE_FIELD.key());

Review Comment:
   there can be other alias to this from table config?



##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/hudi/HoodieSparkSqlWriter.scala:
##########
@@ -118,6 +118,19 @@ object HoodieSparkSqlWriter {
 
       operation = WriteOperationType.INSERT
     }
+    // If no record key field and precombine field is provided, assume it's an append-only workload and do bulk insert
+    if (!hoodieConfig.contains(RECORDKEY_FIELD)
+      && !hoodieConfig.contains(PRECOMBINE_FIELD)) {
+      if (!optParams.contains(OPERATION.key)
+        || optParams.getOrDefault(OPERATION.key, OPERATION.defaultValue).equalsIgnoreCase(BULK_INSERT_OPERATION_OPT_VAL)) {

Review Comment:
   you already checked contains(), no need to getOrDefault. Returning default makes the logic less future-proof



-- 
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: commits-unsubscribe@hudi.apache.org

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