vinishjail97 commented on code in PR #835:
URL: https://github.com/apache/incubator-xtable/pull/835#discussion_r4160929347
##########
xtable-api/src/main/java/org/apache/xtable/model/storage/InternalDataFile.java:
##########
@@ -52,4 +53,7 @@ public class InternalDataFile extends InternalFile {
@Builder.Default @NonNull List<ColumnStat> columnStats =
Collections.emptyList();
// last modified time in millis since epoch
long lastModified;
+ // directory between the partition path and the file name that groups files
within a partition,
+ // for example Paimon's bucket-N; empty when the file sits directly under
its partition
+ @Builder.Default @NonNull Optional<String> fileGroupPrefix =
Optional.empty();
Review Comment:
fileGroupPrefix is confusing with hudi's terminology here, any better name
for this?
##########
xtable-core/src/main/java/org/apache/xtable/hudi/HudiConversionSource.java:
##########
@@ -163,19 +172,24 @@ public CommitsBacklog<HoodieInstant> getCommitsBacklog(
@Override
public boolean isIncrementalSyncSafeFrom(Instant instant) {
- return doesCommitExistsAsOfInstant(instant) &&
!isAffectedByCleanupProcess(instant);
+ HoodieInstant commitAtInstant = getCommitAtInstant(instant);
+ if (commitAtInstant == null) {
+ return false;
+ }
+ // On table version 8+ the checkpoint is a completion time, but the
cleaner retains commits by
+ // requested time, so check the cleaner against the requested time of the
synced commit.
+ Instant cleanerCheckInstant =
+ usesCompletionTimeOrdering()
+ ?
HudiInstantUtils.parseFromInstantTime(commitAtInstant.requestedTime())
+ : instant;
+ return !isAffectedByCleanupProcess(cleanerCheckInstant);
}
@Override
public String getCommitIdentifier(HoodieInstant commit) {
return commit.requestedTime();
}
- private boolean doesCommitExistsAsOfInstant(Instant instant) {
Review Comment:
Can we keep this function from main instead of moving this into the caller?
##########
xtable-core/src/main/java/org/apache/xtable/hudi/BaseFileUpdatesExtractor.java:
##########
@@ -244,13 +243,32 @@ ReplaceMetadata convertDiff(
return ReplaceMetadata.of(partitionToReplacedFileIds, writeStatuses);
}
- private String getFileId(Path filePath) {
- String fileName = filePath.getName();
+ private String getFileId(InternalDataFile file) {
+ String fileName = new CachingPath(file.getPhysicalPath()).getName();
// if file was created by Hudi use original fileId, otherwise use the file
name as IDs
if (isFileCreatedByHudiWriter(fileName)) {
return FSUtils.getFileId(fileName);
}
- return fileName;
+ // Files under a file-group prefix keep the prefix as part of the fileId
so the prefix can be
+ // recovered when Hudi resolves the physical path of the externally
created file.
+ return file.getFileGroupPrefix().map(prefix -> prefix + "/" +
fileName).orElse(fileName);
+ }
+
+ /**
+ * Resolves the Hudi partition path for a file. A file-group prefix
directory (e.g. Paimon's
+ * {@code bucket-N}) is a file group within the partition (see Hudi PR
#17788), so it is stripped
+ * from the partition path.
+ */
+ private String truePartitionPath(Path tableBasePath, InternalDataFile file) {
Review Comment:
Any better name for `truePartitionPath`?
##########
xtable-core/src/main/java/org/apache/xtable/hudi/HudiConversionTarget.java:
##########
@@ -599,18 +687,25 @@ private void markInstantsAsCleaned(
private void runArchiver(
HoodieJavaTable<?> table, HoodieWriteConfig config,
HoodieEngineContext engineContext) {
- // trigger archiver manually
+ // trigger archiver manually, selecting the archiver implementation that
matches the table's
+ // timeline layout (V1 for table version 6, V2/LSM for table version 9).
try {
- HoodieTimelineArchiver archiver = new TimelineArchiverV1(config,
table);
+ @SuppressWarnings({"unchecked", "rawtypes"})
Review Comment:
Why are we adding SuppressWarnings inside a try-catch block? Can we avoid
this and fix the underlying issue?
--
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]
