the-other-tim-brown commented on code in PR #569:
URL: https://github.com/apache/incubator-xtable/pull/569#discussion_r1874528546
##########
xtable-core/src/main/java/org/apache/xtable/hudi/HudiConversionTarget.java:
##########
@@ -316,6 +323,53 @@ public String getTableFormat() {
return TableFormat.HUDI;
}
+ @Override
+ public Optional<String> getTargetCommitIdentifier(String sourceIdentifier) {
+ if (!metaClient.isPresent()) {
+ return Optional.empty();
+ }
+ return getTargetCommitIdentifier(sourceIdentifier, metaClient.get());
+ }
+
+ public Optional<String> getTargetCommitIdentifier(
+ String sourceIdentifier, HoodieTableMetaClient metaClient) {
+ long sourceIdentifierVal = Long.parseLong(sourceIdentifier);
Review Comment:
This is assuming that all sources will have identifiers that can be mapped
to longs, that may not be true in the future. Can we keep this as strings
throughout?
##########
xtable-core/src/test/java/org/apache/xtable/delta/TestDeltaSync.java:
##########
@@ -159,16 +161,22 @@ public void testCreateSnapshotControlFlow() throws
Exception {
InternalDataFile dataFile2 = getDataFile(2, Collections.emptyList(),
basePath);
InternalDataFile dataFile3 = getDataFile(3, Collections.emptyList(),
basePath);
- InternalSnapshot snapshot1 = buildSnapshot(table1, dataFile1, dataFile2);
- InternalSnapshot snapshot2 = buildSnapshot(table2, dataFile2, dataFile3);
+ InternalSnapshot snapshot1 = buildSnapshot(table1, "0", dataFile1,
dataFile2);
+ InternalSnapshot snapshot2 = buildSnapshot(table2, "1", dataFile2,
dataFile3);
TableFormatSync.getInstance()
.syncSnapshot(Collections.singletonList(conversionTarget), snapshot1);
+ Optional<String> targetIdentifier1 =
conversionTarget.getTargetCommitIdentifier("0");
validateDeltaTable(basePath, new HashSet<>(Arrays.asList(dataFile1,
dataFile2)), null);
+ assertTrue(targetIdentifier1.isPresent());
+ assertEquals("0", targetIdentifier1.get());
TableFormatSync.getInstance()
.syncSnapshot(Collections.singletonList(conversionTarget), snapshot2);
+ Optional<String> targetIdentifier2 =
conversionTarget.getTargetCommitIdentifier("1");
validateDeltaTable(basePath, new HashSet<>(Arrays.asList(dataFile2,
dataFile3)), null);
+ assertTrue(targetIdentifier2.isPresent());
+ assertEquals("1", targetIdentifier2.get());
Review Comment:
Let's have 1 test where we return an empty option as well
##########
xtable-core/src/main/java/org/apache/xtable/delta/DeltaConversionTarget.java:
##########
@@ -264,7 +330,8 @@ private void commitTransaction() {
transaction.updateMetadata(metadata, false);
transaction.commit(
actions,
- new
DeltaOperations.Update(Option.apply(Literal.fromObject("xtable-delta-sync"))));
+ new
DeltaOperations.Update(Option.apply(Literal.fromObject("xtable-delta-sync"))),
+ ScalaUtils.convertJavaMapToScala(getCommitTags()));
Review Comment:
What is the difference between putting this info in tags compared to the
metadata we are currently using? Should we consolidate?
##########
xtable-core/src/main/java/org/apache/xtable/hudi/HudiConversionTarget.java:
##########
@@ -316,6 +323,53 @@ public String getTableFormat() {
return TableFormat.HUDI;
}
+ @Override
+ public Optional<String> getTargetCommitIdentifier(String sourceIdentifier) {
+ if (!metaClient.isPresent()) {
+ return Optional.empty();
+ }
+ return getTargetCommitIdentifier(sourceIdentifier, metaClient.get());
+ }
+
+ public Optional<String> getTargetCommitIdentifier(
Review Comment:
Can this be made private or at least package private?
##########
xtable-core/src/main/java/org/apache/xtable/hudi/HudiConversionTarget.java:
##########
@@ -316,6 +323,53 @@ public String getTableFormat() {
return TableFormat.HUDI;
}
+ @Override
+ public Optional<String> getTargetCommitIdentifier(String sourceIdentifier) {
+ if (!metaClient.isPresent()) {
+ return Optional.empty();
+ }
+ return getTargetCommitIdentifier(sourceIdentifier, metaClient.get());
+ }
+
+ public Optional<String> getTargetCommitIdentifier(
+ String sourceIdentifier, HoodieTableMetaClient metaClient) {
+ long sourceIdentifierVal = Long.parseLong(sourceIdentifier);
+
+ HoodieTimeline completedTimeline =
metaClient.getActiveTimeline().filterCompletedInstants();
Review Comment:
We can filter this to the commits timeline (avoids looking at cleaner or
other commit types) with `getCommitsTimeline`
##########
xtable-core/src/main/java/org/apache/xtable/hudi/HudiConversionTarget.java:
##########
@@ -316,6 +323,53 @@ public String getTableFormat() {
return TableFormat.HUDI;
}
+ @Override
+ public Optional<String> getTargetCommitIdentifier(String sourceIdentifier) {
+ if (!metaClient.isPresent()) {
+ return Optional.empty();
+ }
+ return getTargetCommitIdentifier(sourceIdentifier, metaClient.get());
+ }
+
+ public Optional<String> getTargetCommitIdentifier(
+ String sourceIdentifier, HoodieTableMetaClient metaClient) {
+ long sourceIdentifierVal = Long.parseLong(sourceIdentifier);
+
+ HoodieTimeline completedTimeline =
metaClient.getActiveTimeline().filterCompletedInstants();
+
+ for (HoodieInstant instant : completedTimeline.getInstants()) {
+ try {
+ Option<byte[]> instantDetails =
metaClient.getActiveTimeline().getInstantDetails(instant);
+ if (!instantDetails.isPresent()) {
+ continue;
+ }
+
+ HoodieCommitMetadata commitMetadata =
+ HoodieCommitMetadata.fromBytes(instantDetails.get(),
HoodieCommitMetadata.class);
+ String sourceMetadataJson =
+
commitMetadata.getExtraMetadata().get(TableSyncMetadata.XTABLE_SOURCE_METADATA);
+ if (sourceMetadataJson == null) {
+ continue;
+ }
+
+ SourceMetadata sourceMetadata =
SourceMetadata.fromJson(sourceMetadataJson);
+
+ if (sourceIdentifier.equals(sourceMetadata.getSourceIdentifier())) {
+ return Optional.of(sourceIdentifier);
Review Comment:
Should we return the identifier of the target?
##########
xtable-core/src/main/java/org/apache/xtable/hudi/HudiConversionTarget.java:
##########
@@ -316,6 +323,53 @@ public String getTableFormat() {
return TableFormat.HUDI;
}
+ @Override
+ public Optional<String> getTargetCommitIdentifier(String sourceIdentifier) {
+ if (!metaClient.isPresent()) {
+ return Optional.empty();
+ }
+ return getTargetCommitIdentifier(sourceIdentifier, metaClient.get());
+ }
+
+ public Optional<String> getTargetCommitIdentifier(
+ String sourceIdentifier, HoodieTableMetaClient metaClient) {
+ long sourceIdentifierVal = Long.parseLong(sourceIdentifier);
+
+ HoodieTimeline completedTimeline =
metaClient.getActiveTimeline().filterCompletedInstants();
+
+ for (HoodieInstant instant : completedTimeline.getInstants()) {
+ try {
+ Option<byte[]> instantDetails =
metaClient.getActiveTimeline().getInstantDetails(instant);
Review Comment:
There is a similar block of code above (line 300) that also handles parsing
of the "replace commit" type which is what we'll be writing from XTable. Can
you extract that into a common helper method to avoid repeated code?
--
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
