the-other-tim-brown commented on code in PR #835:
URL: https://github.com/apache/incubator-xtable/pull/835#discussion_r4116901814
##########
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:
Do we have any tests where we take a target that has no col-stats and then
we sync a new commit? This would simulate the migration path where there is an
existing Hudi target and now we are adding col-stats to the target. It would be
good to verify that this creates the full stats for the table and doesn't leave
things in a bad state that will impact a query planner in a negative way.
##########
xtable-core/src/main/java/org/apache/xtable/hudi/BaseFileUpdatesExtractor.java:
##########
@@ -84,12 +92,62 @@ public class BaseFileUpdatesExtractor {
* @param partitionedDataFiles files grouped by partition to sync
* @param metaClient the meta client for the Hudi table with the latest
timeline state loaded
* @param commit The current commit started by the Hudi client
+ * @param useExternalFileGroupPrefix whether a trailing {@code bucket-N}
directory is a file-group
+ * prefix (true only for a Paimon source)
* @return The information needed to create a "replace" commit for the Hudi
table
*/
ReplaceMetadata extractSnapshotChanges(
List<PartitionFileGroup> partitionedDataFiles,
HoodieTableMetaClient metaClient,
- String commit) {
+ String commit,
+ boolean useExternalFileGroupPrefix) {
+ return withFileSystemView(
+ metaClient,
+ (fsView, metadataConfig) ->
+ extractFromFsView(
+ partitionedDataFiles,
+ commit,
+ fsView,
+ metaClient,
+ metadataConfig,
+ useExternalFileGroupPrefix));
+ }
+
+ /**
+ * Returns true when the table still has live file groups under a {@code
bucket-N} partition.
+ * XTable 0.4.0 registered Paimon files under {@code <partition>/bucket-N},
and the incremental
+ * sync now keys the same files by {@code <partition>}, so a snapshot sync
must replace those file
+ * groups first. The replaced file groups are no longer live, so the check
is false after that
+ * sync.
+ *
+ * @param metaClient the meta client for the Hudi table with the latest
timeline state loaded
+ * @return true if a file group exists under a partition path that ends in
{@code bucket-N}
+ */
+ boolean
hasFileGroupsInExternalFileGroupPrefixPartitions(HoodieTableMetaClient
metaClient) {
Review Comment:
This method is called from `isIncrementalSyncSafe` and forces a full FSView
load which will load the Hudi Metadata table as well if I remember correctly.
This is quite heavy weight for the incremental sync check. Is there a more
lightweight approach we can take?
--
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]
