the-other-tim-brown commented on code in PR #569:
URL: https://github.com/apache/incubator-xtable/pull/569#discussion_r1957174855


##########
xtable-api/src/test/java/org/apache/xtable/model/metadata/TestTableSyncMetadata.java:
##########
@@ -76,4 +96,26 @@ void failToParseJsonWithMissingLastSyncedInstant() {
             TableSyncMetadata.fromJson(
                 
"{\"instantsToConsiderForNextSync\":[\"2020-08-21T11:15:30Z\",\"2024-01-21T12:15:30Z\"],\"version\":0}"));
   }
+
+  @Test
+  void parseJsonWithNewFieldsAdded() {

Review Comment:
   Is this already covered in the test above?



##########
xtable-api/src/test/java/org/apache/xtable/model/metadata/TestTableSyncMetadata.java:
##########
@@ -56,7 +59,24 @@ private static Stream<Arguments> provideMetadataAndJson() {
             
"{\"lastInstantSynced\":\"2020-07-04T10:15:30Z\",\"instantsToConsiderForNextSync\":[],\"version\":0}"),
         Arguments.of(
             TableSyncMetadata.of(Instant.parse("2020-07-04T10:15:30.00Z"), 
null),
-            "{\"lastInstantSynced\":\"2020-07-04T10:15:30Z\",\"version\":0}"));
+            "{\"lastInstantSynced\":\"2020-07-04T10:15:30Z\",\"version\":0}"),
+        // New version of metadata and JSON where two new fields are added

Review Comment:
   "new" will be relative in the future so we should mention the names of the 
fields added



##########
xtable-core/src/main/java/org/apache/xtable/delta/DeltaConversionTarget.java:
##########
@@ -258,10 +258,15 @@ public Optional<String> getTargetCommitIdentifier(String 
sourceIdentifier) {
         Optional<TableSyncMetadata> optionalMetadata =
             TableSyncMetadata.fromJson(sourceMetadataJson.get());
         if (!optionalMetadata.isPresent()) {
-          return Optional.empty();
+          continue;
         }
 
         TableSyncMetadata metadata = optionalMetadata.get();
+        // For backward compatability since the older version JSON might not 
contain new fields

Review Comment:
   I think this will be covered at line 270 so we don't need the extra 
condition right?
   
   Is there a test case where we will have a `null` to cover this case?



-- 
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-unsubscr...@xtable.apache.org

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

Reply via email to