vinishjail97 commented on code in PR #835:
URL: https://github.com/apache/incubator-xtable/pull/835#discussion_r4158846120
##########
xtable-core/src/main/java/org/apache/xtable/hudi/HudiConversionSource.java:
##########
@@ -124,10 +124,17 @@ public InternalSnapshot getCurrentSnapshot() {
@Override
public TableChange getTableChangeForCommit(HoodieInstant
hoodieInstantForDiff) {
HoodieActiveTimeline activeTimeline = metaClient.getActiveTimeline();
+ // The set of commits visible as-of the diff commit is ordered by
completion time on table
+ // version 9 (timeline layout V2) and by requested time on version 6.
HoodieTimeline visibleTimeline =
- activeTimeline
- .filterCompletedInstants()
- .findInstantsBeforeOrEquals(hoodieInstantForDiff.requestedTime());
+ usesCompletionTimeOrdering()
Review Comment:
Used `getTimelineLayout().getInstantComparator().orderingComparator()` in
2bef501 for the backlog merge and the latest commit. It orders by requested
time on layout V1 and by completion time on V2. The other branches select
different instants, and no Hudi util seems to cover both, so they stay.
##########
xtable-core/src/main/java/org/apache/xtable/hudi/HudiConversionTarget.java:
##########
@@ -648,14 +723,16 @@ private HoodieWriteConfig getWriteConfig(
HoodieMetadataConfig.newBuilder()
.enable(true)
.withProperties(properties)
- // Hudi 1.2.0 couples the partition-stats index to the
column-stats index. For
- // partitioned tables, the partition-stats generation path
rebuilds a file-system
- // view over the committed external parquet files and groups
them by fileId.
- // XTable's externally-registered files have non-Hudi names
whose fileId cannot be
- // parsed once the "_hudiext" marker is stripped, which
leads to failures. So
- // column stats are only enabled for un-partitioned tables
for now. Tracked in
- // https://github.com/apache/incubator-xtable/issues/832.
-
.withMetadataIndexColumnStats(!metaClient.getTableConfig().isTablePartitioned())
+ // Build the column-stats index for all tables. The
partition-stats index is
+ // disabled independently: its generation path rebuilds a
file-system view over
+ // the committed external parquet files and groups them by
fileId, but XTable's
+ // externally-registered files have non-Hudi names whose
fileId cannot be parsed
+ // once the "_hudiext" marker is stripped, which leads to
failures on partitioned
+ // tables. Disabling partition stats (while keeping column
stats) requires the
+ // independent toggle added in
https://github.com/apache/hudi/pull/19111. Tracked
Review Comment:
Updated in 2bef501.
##########
xtable-core/src/test/java/org/apache/xtable/hudi/TestHudiConversionSource.java:
##########
@@ -71,6 +72,7 @@ void
testIsIncrementalSyncSafeFromWithNullEarliestCommitToRetainNoCleanInstants(
when(mockMetaClient.getActiveTimeline()).thenReturn(mockActiveTimeline);
when(mockMetaClient.getTableConfig()).thenReturn(mockTableConfig);
when(mockTableConfig.isMetadataTableAvailable()).thenReturn(false);
+ when(mockTableConfig.getTableVersion()).thenReturn(HoodieTableVersion.SIX);
Review Comment:
Done in 2bef501. The three tests now run on SIX and NINE.
##########
xtable-core/src/test/java/org/apache/xtable/hudi/TestBaseFileUpdatesExtractor.java:
##########
@@ -145,7 +145,7 @@ void convertDiff() {
BaseFileUpdatesExtractor extractor =
BaseFileUpdatesExtractor.of(CONTEXT, new CachingPath(tableBasePath));
BaseFileUpdatesExtractor.ReplaceMetadata replaceMetadata =
- extractor.convertDiff(diff, COMMIT_TIME, HoodieIndexVersion.V1);
+ extractor.convertDiff(diff, COMMIT_TIME, HoodieIndexVersion.V1, false);
Review Comment:
Added `convertDiffWithFileGroupPrefix` in 2bef501. It covers a partitioned
file, an unpartitioned file, a removed file, and a `bucket-3` directory without
a prefix.
##########
xtable-core/src/main/java/org/apache/xtable/hudi/HudiConversionTarget.java:
##########
@@ -648,14 +723,16 @@ private HoodieWriteConfig getWriteConfig(
HoodieMetadataConfig.newBuilder()
.enable(true)
.withProperties(properties)
- // Hudi 1.2.0 couples the partition-stats index to the
column-stats index. For
- // partitioned tables, the partition-stats generation path
rebuilds a file-system
- // view over the committed external parquet files and groups
them by fileId.
- // XTable's externally-registered files have non-Hudi names
whose fileId cannot be
- // parsed once the "_hudiext" marker is stripped, which
leads to failures. So
- // column stats are only enabled for un-partitioned tables
for now. Tracked in
- // https://github.com/apache/incubator-xtable/issues/832.
-
.withMetadataIndexColumnStats(!metaClient.getTableConfig().isTablePartitioned())
+ // Build the column-stats index for all tables. The
partition-stats index is
+ // disabled independently: its generation path rebuilds a
file-system view over
+ // the committed external parquet files and groups them by
fileId, but XTable's
+ // externally-registered files have non-Hudi names whose
fileId cannot be parsed
+ // once the "_hudiext" marker is stripped, which leads to
failures on partitioned
+ // tables. Disabling partition stats (while keeping column
stats) requires the
+ // independent toggle added in
https://github.com/apache/hudi/pull/19111. Tracked
+ // in https://github.com/apache/incubator-xtable/issues/832.
+ .withMetadataIndexColumnStats(true)
Review Comment:
Good catch. A test showed that Hudi marks column_stats available but indexes
only the files in the new commit, because the bootstrap skips the `_hudiext`
names. Hudi's Spark reader keeps files without stats, so results stay correct,
but old files get no data skipping. A stacked PR will fall back to a snapshot
sync when the index is missing, and register the existing files again with
their stats.
##########
xtable-core/src/main/java/org/apache/xtable/hudi/BaseFileUpdatesExtractor.java:
##########
@@ -250,7 +257,42 @@ private String getFileId(Path filePath) {
if (isFileCreatedByHudiWriter(fileName)) {
return FSUtils.getFileId(fileName);
}
- return fileName;
+ // External bucketed files keep their file-group prefix as part of the
fileId so the prefix can
+ // be recovered when Hudi resolves the physical path of the externally
created file.
+ return externalFileGroupPrefix(filePath)
+ .map(prefix -> prefix + "/" + fileName)
+ .orElse(fileName);
+ }
+
+ /**
+ * Returns the external file-group prefix (e.g. Paimon's {@code bucket-N})
when the file's
+ * immediate parent directory denotes a file group within the partition
rather than a partition
+ * segment, otherwise empty.
+ */
+ private Optional<String> externalFileGroupPrefix(Path filePath) {
+ Path parent = filePath.getParent();
+ if (parent == null) {
+ return Optional.empty();
+ }
+ String parentName = parent.getName();
+ return EXTERNAL_FILE_GROUP_PREFIX_PATTERN.matcher(parentName).matches()
+ ? Optional.of(parentName)
+ : Optional.empty();
+ }
+
+ /**
+ * Resolves the true Hudi partition path for a file, stripping any trailing
external file-group
+ * prefix directory (e.g. {@code bucket-N}) so it is not treated as part of
the partition.
+ */
+ private String truePartitionPath(Path tableBasePath, Path filePath) {
Review Comment:
Replaced in 2bef501. The automatic fallback is removed. The Hudi target
throws with a message to run one FULL sync, and the PR description lists this
as a breaking change.
--
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]
