vinishjail97 commented on code in PR #19869:
URL: https://github.com/apache/hudi/pull/19869#discussion_r3983157831
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/metadata/index/BaseIndexer.java:
##########
@@ -71,4 +75,15 @@ public void postInitialization(HoodieTableMetaClient
metadataMetaClient, HoodieD
public List<IndexPartitionAndRecords> buildRestore(IndexRestoreContext
context) {
return Collections.emptyList();
}
+
+ /**
+ * Fails when a clustering commit reaches a table without record keys. The
record index and the secondary index
+ * key the rows of such a table by file path and row position, and
clustering rewrites the rows into new files,
+ * so neither index could follow them. Every other operation type either
keeps the keys or registers new files.
+ */
+ protected void checkClusteringKeepsRecordKeys(HoodieCommitMetadata
commitMetadata) {
Review Comment:
Fixed in af6ddfa1b5b6. Good catch, thanks. The check now lives in
`HoodieBackedTableMetadataWriter` as a private method that both
`update(HoodieCommitMetadata, ...)` and `completeStreamingCommit` call first,
so it runs ahead of the streaming/batch fork on every engine. It fires only for
a `CLUSTER` commit when the record index or a secondary index exists and the
table has no record key; the `BaseIndexer` method and the two indexer calls are
removed. The two indexer unit tests moved to
`TestHoodieBackedTableMetadataWriter`, one per path plus a pass-through case.
`TestExternalFileRecordAndSecondaryIndex` now runs real clustering via
`scheduleClustering` and `cluster`, parameterized over
`hoodie.metadata.streaming.write.enabled` on (the Spark default) and off, and
asserts the rejection plus that both indexes still point at the registered
files. One detail worth knowing: the test sets
`hoodie.datasource.write.keygenerator.class` to `NonpartitionedKeyGenerator`.
The default `SimpleKeyGenerator` cannot be constructed for an unpartitioned
table without a record key field, and the record-based clustering path fails in
`RecordContext` before any metadata write, so the row writer with that key
generator is the only configuration under which clustering on such a table
reaches the metadata commit at all.
On `RecordIndexMapper:78`: I left it as is. The mapper emits records only
for write statuses that carry record delegates, which files registered from
outside never have, so the raw encoding there is unreachable for a key-less
table.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/metadata/SecondaryIndexRecordGenerationUtils.java:
##########
@@ -245,35 +326,32 @@ public static <T> HoodieData<HoodieRecord>
readSecondaryKeysFromFileSlices(Hoodi
return engineContext.emptyHoodieData();
}
final int parallelism = Math.min(fileSlices.size(),
secondaryIndexMaxParallelism);
- final StoragePath basePath = metaClient.getBasePath();
- HoodieSchema tableSchema;
- try {
- tableSchema = new TableSchemaResolver(metaClient).getTableSchema();
- } catch (Exception e) {
- throw new HoodieException("Failed to get latest schema for " +
metaClient.getBasePath(), e);
- }
ReaderContextFactory<T> readerContextFactory =
engineContext.getReaderContextFactory(metaClient);
engineContext.setJobStatus(activeModule, "Secondary Index: reading
secondary keys from " + fileSlices.size() + " file slices");
HoodieFileFormat baseFileFormat =
metaClient.getTableConfig().getBaseFileFormat();
return engineContext.parallelize(fileSlices,
parallelism).flatMap(partitionAndBaseFile -> {
- final String partition = partitionAndBaseFile.getPartitionPath();
final FileSlice fileSlice = partitionAndBaseFile.getFileSlice();
- Option<StoragePath> dataFilePath =
Option.ofNullable(fileSlice.getBaseFile().map(baseFile ->
FSUtils.getAbsoluteFilePath(basePath, partition,
baseFile.getFileName())).orElseGet(null));
- HoodieSchema readerSchema;
- if (dataFilePath.isPresent()) {
- readerSchema = HoodieIOFactory.getIOFactory(metaClient.getStorage())
- .getFileFormatUtils(baseFileFormat)
- .readSchema(metaClient.getStorage(), dataFilePath.get());
- } else {
- readerSchema = tableSchema;
- }
+ // the storage path keeps the directory prefix of a file written outside
Hudi, which its file name alone loses
+ Option<StoragePath> dataFilePath =
fileSlice.getBaseFile().map(HoodieBaseFile::getStoragePath);
+ // a file slice without a base file has only log files, whose schema is
the table schema
+ HoodieSchema readerSchema = dataFilePath.isPresent()
+ ?
HoodieIOFactory.getIOFactory(metaClient.getStorage()).getFileFormatUtils(baseFileFormat).readSchema(metaClient.getStorage(),
dataFilePath.get())
+ : resolveTableSchema(metaClient);
Review Comment:
Fixed in af6ddfa1b5b6. `readSecondaryKeysFromFileSlices` now resolves the
table schema once on the driver, guarded by `fileSlices.stream().anyMatch(slice
-> !slice.getFileSlice().getBaseFile().isPresent())`, and captures it as an
`Option<HoodieSchema>` in the closure. Slices with a base file still read the
schema from the file.
--
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]
