[GitHub] [hudi] danny0405 commented on a diff in pull request #8605: [HUDI-6152] Fixed the check for older timestamps with second granularity during index tagLocation.

["danny0405 (via GitHub)" <gi...@apache.org>]

danny0405 commented on code in PR #8605:
URL: https://github.com/apache/hudi/pull/8605#discussion_r1181157260


##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/index/HoodieIndexUtils.java:
##########
@@ -170,9 +171,35 @@ public static List<String> filterKeysFromFile(Path filePath, List<String> candid
     return foundRecordKeys;
   }
 
+  /**
+   * Check if the given commit timestamp is valid for the timeline.
+   *
+   * The commit timestamp is considered to be valid if:
+   *   1. the commit timestamp is present in the timeline, or
+   *   2. the commit timestamp is less than the first commit timestamp in the timeline
+   *
+   * @param commitTimeline  The timeline
+   * @param commitTs        The commit timestamp to check
+   * @return                true if the commit timestamp is valid for the timeline
+   */
   public static boolean checkIfValidCommit(HoodieTimeline commitTimeline, String commitTs) {
-    // Check if the last commit ts for this row is 1) present in the timeline or
-    // 2) is less than the first commit ts in the timeline
-    return !commitTimeline.empty() && commitTimeline.containsOrBeforeTimelineStarts(commitTs);
+    if (commitTimeline.empty()) {
+      return false;
+    }
+
+    // Check for 0.8+ timestamps which have msec granularity
+    if (commitTimeline.containsOrBeforeTimelineStarts(commitTs)) {
+      return true;
+    }
+
+    // Check for older timestamp which have sec granularity and an extension of DEFAULT_MILLIS_EXT may have been added via Timeline operations
+    if (commitTs.length() == HoodieInstantTimeGenerator.MILLIS_INSTANT_TIMESTAMP_FORMAT_LENGTH && commitTs.endsWith(HoodieInstantTimeGenerator.DEFAULT_MILLIS_EXT)) {
+      final String actualOlderFormatTs = commitTs.substring(0, commitTs.length() - HoodieInstantTimeGenerator.DEFAULT_MILLIS_EXT.length());
+      if (commitTimeline.containsOrBeforeTimelineStarts(actualOlderFormatTs)) {
+        return true;
+      }
+    }

Review Comment:
   Shouldm't we fix this method instead? `commitTimeline.containsOrBeforeTimelineStarts` and should we have a version number for the timeline?



-- 
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