yuqi1129 commented on code in PR #13487:
URL: https://github.com/apache/gravitino/pull/13487#discussion_r4228412033


##########
core/src/main/java/org/apache/gravitino/storage/relational/service/StatisticMetaService.java:
##########
@@ -63,6 +77,20 @@ public List<StatisticEntity> listStatisticsByEntity(
         .collect(Collectors.toList());
   }
 
+  /**
+   * Creates or replaces statistics of a metadata object by name.
+   *
+   * <p>The name keeps the historical upsert name and metric, but the write is 
no longer a blind

Review Comment:
   Fixed in 81bb9b328e: renamed the method to `writeStatisticsWithVersion` 
(updated the `JDBCBackend` call site) and dropped the explanatory sentence. The 
`@Monitored` `baseMetricName` literal is unchanged, so the emitted metric stays 
the same.



##########
core/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/base/StatisticBaseSQLProvider.java:
##########
@@ -33,47 +33,41 @@
 
 public class StatisticBaseSQLProvider {
 
-  public String batchInsertStatisticPOsOnDuplicateKeyUpdate(
-      @Param("statisticPOs") List<StatisticPO> statisticPOs) {
-    return "<script>"
-        + "INSERT INTO "
+  /** Inserts a new live statistic without upsert fallback. */
+  public String insertStatisticPO(@Param("statisticPO") StatisticPO 
statisticPO) {
+    return "INSERT INTO "
         + STATISTIC_META_TABLE_NAME
         + " (statistic_id, statistic_name, statistic_value, metalake_id, 
metadata_object_id,"
-        + " metadata_object_type, audit_info, current_version, last_version, 
deleted_at) VALUES "
-        + "<foreach collection='statisticPOs' item='item' separator=','>"
-        + "(#{item.statisticId}, "
-        + "#{item.statisticName}, "
-        + "#{item.statisticValue}, "
-        + "#{item.metalakeId}, "
-        + "#{item.metadataObjectId}, "
-        + "#{item.metadataObjectType}, "
-        + "#{item.auditInfo}, "
-        + "#{item.currentVersion}, "
-        + "#{item.lastVersion}, "
-        + "#{item.deletedAt})"
-        + "</foreach>"
-        + " ON DUPLICATE KEY UPDATE "
-        + "  statistic_value = VALUES(statistic_value),"
-        + "  audit_info = VALUES(audit_info),"
-        + "  current_version = VALUES(current_version),"
-        + "  last_version = VALUES(last_version),"
-        + "  deleted_at = VALUES(deleted_at)"
-        + "</script>";
+        + " metadata_object_type, audit_info, current_version, last_version, 
deleted_at) VALUES"
+        + " (#{statisticPO.statisticId}, #{statisticPO.statisticName},"
+        + " #{statisticPO.statisticValue}, #{statisticPO.metalakeId},"
+        + " #{statisticPO.metadataObjectId}, 
#{statisticPO.metadataObjectType},"
+        + " #{statisticPO.auditInfo}, 1, 1, 0)";
   }
 
-  public String batchDeleteStatisticPOs(
-      @Param("entityId") Long entityId, @Param("statisticNames") List<String> 
statisticNames) {
-    return "<script>"
-        + "UPDATE "
+  /** Updates the value and advances the version when the observed row still 
matches. */
+  public String updateStatisticPOWithVersion(
+      @Param("statisticPO") StatisticPO statisticPO, @Param("previous") 
StatisticPO previous) {
+    return "UPDATE "
+        + STATISTIC_META_TABLE_NAME
+        + " SET statistic_value = #{statisticPO.statisticValue},"
+        + " audit_info = #{statisticPO.auditInfo},"
+        + " last_version = current_version, current_version = current_version 
+ 1"
+        + " WHERE statistic_id = #{previous.statisticId}"
+        + " AND metadata_object_id = #{previous.metadataObjectId}"
+        + " AND statistic_name = #{previous.statisticName}"
+        + " AND current_version = #{previous.currentVersion} AND deleted_at = 
0";
+  }
+
+  /** Soft-deletes the observed statistic and advances its version. */
+  public String deleteStatisticPOWithVersion(@Param("previous") StatisticPO 
previous) {
+    return "UPDATE "
         + STATISTIC_META_TABLE_NAME
         + softDeleteSQL()
-        + " WHERE "
-        + " statistic_name IN ("
-        + "<foreach collection='statisticNames' item='item' separator=','>"
-        + " #{item}"
-        + "</foreach>"
-        + " ) AND deleted_at = 0 AND metadata_object_id = #{entityId}"
-        + "</script>";
+        + ", last_version = current_version, current_version = current_version 
+ 1"
+        + " WHERE statistic_id = #{previous.statisticId}"
+        + " AND metadata_object_id = #{previous.metadataObjectId}"
+        + " AND current_version = #{previous.currentVersion} AND deleted_at = 
0";

Review Comment:
   Fixed in 81bb9b328e: `deleteStatisticPOWithVersion` now also matches `AND 
statistic_name = #{previous.statisticName}`, so both CAS statements use the 
same predicate.



##########
core/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/base/StatisticBaseSQLProvider.java:
##########
@@ -33,47 +33,41 @@
 
 public class StatisticBaseSQLProvider {
 
-  public String batchInsertStatisticPOsOnDuplicateKeyUpdate(
-      @Param("statisticPOs") List<StatisticPO> statisticPOs) {
-    return "<script>"
-        + "INSERT INTO "
+  /** Inserts a new live statistic without upsert fallback. */
+  public String insertStatisticPO(@Param("statisticPO") StatisticPO 
statisticPO) {
+    return "INSERT INTO "
         + STATISTIC_META_TABLE_NAME
         + " (statistic_id, statistic_name, statistic_value, metalake_id, 
metadata_object_id,"
-        + " metadata_object_type, audit_info, current_version, last_version, 
deleted_at) VALUES "
-        + "<foreach collection='statisticPOs' item='item' separator=','>"
-        + "(#{item.statisticId}, "
-        + "#{item.statisticName}, "
-        + "#{item.statisticValue}, "
-        + "#{item.metalakeId}, "
-        + "#{item.metadataObjectId}, "
-        + "#{item.metadataObjectType}, "
-        + "#{item.auditInfo}, "
-        + "#{item.currentVersion}, "
-        + "#{item.lastVersion}, "
-        + "#{item.deletedAt})"
-        + "</foreach>"
-        + " ON DUPLICATE KEY UPDATE "
-        + "  statistic_value = VALUES(statistic_value),"
-        + "  audit_info = VALUES(audit_info),"
-        + "  current_version = VALUES(current_version),"
-        + "  last_version = VALUES(last_version),"
-        + "  deleted_at = VALUES(deleted_at)"
-        + "</script>";
+        + " metadata_object_type, audit_info, current_version, last_version, 
deleted_at) VALUES"
+        + " (#{statisticPO.statisticId}, #{statisticPO.statisticName},"
+        + " #{statisticPO.statisticValue}, #{statisticPO.metalakeId},"
+        + " #{statisticPO.metadataObjectId}, 
#{statisticPO.metadataObjectType},"
+        + " #{statisticPO.auditInfo}, 1, 1, 0)";

Review Comment:
   Fixed in 81bb9b328e: the Javadoc on `StatisticMetaMapper.insertStatisticPO` 
now states that the statement fixes both versions to 1 and `deleted_at` to 0, 
ignoring those fields in the supplied PO.



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to