hudi-agent commented on code in PR #19869:
URL: https://github.com/apache/hudi/pull/19869#discussion_r3965426768
##########
hudi-common/src/main/java/org/apache/hudi/metadata/BaseFileRecordParsingUtils.java:
##########
@@ -67,7 +69,9 @@ public static Iterator<HoodieRecord>
generateRLIMetadataHoodieRecordsForBaseFile
boolean isPartitionedRLI) {
String partition = writeStat.getPartitionPath();
String latestFileName = FSUtils.getFileNameFromPath(writeStat.getPath());
- String fileId = FSUtils.getFileId(latestFileName);
+ // a file written outside Hudi keeps its own name, which may contain
underscores, so the file id is parsed from the marker
+ String fileId =
FileNameParser.parseBaseFile(latestFileName).map(FileNameParser.BaseFileName::getFileId)
Review Comment:
🤖 With the default `_hoodie.writes.fileid.encoding` (0 = UUID),
`createRecordIndexUpdate` below calls `UUID.fromString` on this file id, so for
an external file id like `file_1.parquet` the first RLI update throws
`HoodieMetadataException("Invalid UUID or index")` unless the caller sets the
internal config to 1 (the new functional test does; only Kafka Connect does in
production). Since `HoodieRecordIndexInfo` stores `fileIdEncoding` per record,
would it make sense to fall back to raw-string encoding here when
`FileNameParser.parseBaseFile` reports an external file, so XTable-registered
tables don't need to know about that config?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/metadata/SecondaryIndexRecordGenerationUtils.java:
##########
@@ -205,6 +221,44 @@ public static <T> HoodieData<HoodieRecord>
convertWriteStatsToSecondaryIndexReco
return HoodieTableMetadataUtil.reduceByKeys(secondaryIndexRecords,
parallelism, false);
}
+ /**
+ * Generates delete records for every record of the file groups that the
given replace commit replaces without
+ * writing to them again, e.g. files written outside Hudi that are
superseded by newer files. Records that the same
Review Comment:
🤖 nit: `convertReplacedFileGroupsToSecondaryIndexRecords` takes 9
parameters, most of which are just threaded through from the caller. Might be
worth bundling the shared context (`writeConfig`, `metadataConfig`,
`dataMetaClient`, `engineContext`) so the signature is easier to read and
extend.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/metadata/index/record/BaseRecordIndexer.java:
##########
@@ -478,11 +478,48 @@ private HoodieData<HoodieRecord>
getRecordIndexAdditionalUpserts(
} else if (operationType == WriteOperationType.DELETE_PARTITION) {
// all records from the target partition(s) to be deleted from RLI
return getRecordIndexReplacedRecords((HoodieReplaceCommitMetadata)
commitMetadata, fsView);
+ } else if (commitMetadata instanceof HoodieReplaceCommitMetadata &&
operationType != WriteOperationType.CLUSTER) {
+ // a replace commit that is neither a table service nor an overwrite,
e.g. files written outside Hudi being
+ // registered in the table. The replaced file groups are dropped without
their records being rewritten under
+ // the same key, so the records of the replaced base files are deleted
from RLI unless this commit wrote them again.
Review Comment:
🤖 nit: the branch condition `commitMetadata instanceof
HoodieReplaceCommitMetadata && operationType != WriteOperationType.CLUSTER`
mixes an instanceof check with an operation-type check inline in the else-if
chain — could you pull this into a named boolean/helper (e.g.
`isExternalFileRegistrationReplaceCommit(...)`) so the intent ("replace commit
that isn't a known table-service replace") is clearer at the call site?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/metadata/SecondaryIndexRecordGenerationUtils.java:
##########
@@ -197,6 +207,12 @@ public static <T> HoodieData<HoodieRecord>
convertWriteStatsToSecondaryIndexReco
return records.iterator();
});
+ if (commitMetadata instanceof HoodieReplaceCommitMetadata) {
Review Comment:
🤖 Confirmed from the code: `BaseRecordIndexer.buildUpdate` calls
`getRecordIndexAdditionalUpserts` unconditionally (line 147), so the new
replace-commit branch emits RLI deletes even when `partitionToWriteStats` is
empty, while `SecondaryIndexer.getSecondaryIndexUpdates` returns at the
`allWriteStats.isEmpty()` check on line 147 before
`convertReplacedFileGroupsToSecondaryIndexRecords` can run. A drop-only replace
commit would leave SI with dangling entries; gating that early return on
`!(commitMetadata instanceof HoodieReplaceCommitMetadata &&
!getPartitionToReplaceFileIds().isEmpty())` should close the gap.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/metadata/SecondaryIndexRecordGenerationUtils.java:
##########
@@ -319,17 +380,32 @@ public boolean hasNext() {
while (recordIterator.hasNext()) {
T record = recordIterator.next();
Object secondaryKey =
readerContext.getRecordContext().getValue(record, requestedSchema,
secondaryKeyField);
- nextValidRecord = Pair.of(
- readerContext.getRecordContext().getRecordKey(record,
requestedSchema),
- secondaryKey == null ? null : secondaryKey.toString()
- );
+ nextValidRecord = Pair.of(getRecordKey(record), secondaryKey == null
? null : secondaryKey.toString());
+ rowPosition++;
return true;
}
// If no valid records are found
return false;
}
+ private String getRecordKey(T record) {
+ Object recordKey;
+ if (hasRecordKeyMetaField) {
+ recordKey = readerContext.getRecordContext().getValue(record,
requestedSchema, RECORD_KEY_METADATA_FIELD);
+ } else if (hasRecordKeyFields) {
+ recordKey = readerContext.getRecordContext().getRecordKey(record,
requestedSchema);
+ } else {
+ recordKey = null;
Review Comment:
🤖 One more angle on this: the branch order in `getRecordKey` means that
whenever the schema carries `_hoodie_record_key` (any `MetaFieldsMode !=
NONE`), the `hasRecordKeyFields` branch is never tried, so a keyed table in a
selective mode goes straight from a null meta column to a positional key rather
than deriving the key from its record key fields the way the old
`recordKeyExtractor` path would. Falling through to the key-fields extractor
when the meta value is null, and only generating `path_pos` when neither yields
a key, would keep both keyed layouts correct.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/metadata/index/record/BaseRecordIndexer.java:
##########
@@ -478,11 +478,48 @@ private HoodieData<HoodieRecord>
getRecordIndexAdditionalUpserts(
} else if (operationType == WriteOperationType.DELETE_PARTITION) {
// all records from the target partition(s) to be deleted from RLI
return getRecordIndexReplacedRecords((HoodieReplaceCommitMetadata)
commitMetadata, fsView);
+ } else if (commitMetadata instanceof HoodieReplaceCommitMetadata &&
operationType != WriteOperationType.CLUSTER) {
+ // a replace commit that is neither a table service nor an overwrite,
e.g. files written outside Hudi being
+ // registered in the table. The replaced file groups are dropped without
their records being rewritten under
+ // the same key, so the records of the replaced base files are deleted
from RLI unless this commit wrote them again.
+ return
getRecordIndexReplacedFileGroupRecords((HoodieReplaceCommitMetadata)
commitMetadata, fsView)
+ .mapToPair(r -> Pair.of(r.getKey(), r))
+ .leftOuterJoin(updatesFromWriteStatuses.mapToPair(r ->
Pair.of(r.getKey(), r)))
+ .values()
+ .filter(p -> !p.getRight().isPresent())
+ .map(Pair::getLeft);
} else {
return engineContext.emptyHoodieData();
}
}
+ /**
+ * Reads the record keys of the latest base file of every file group
replaced by the given commit and
+ * returns a delete record for each of them.
+ */
+ private HoodieData<HoodieRecord>
getRecordIndexReplacedFileGroupRecords(HoodieReplaceCommitMetadata
replaceCommitMetadata, Lazy<HoodieTableFileSystemView> fsView) {
Review Comment:
🤖 Checked this and it lines up: `readRecordKeysFromFileSliceSnapshot` is the
only RLI init path (`initialize` line 114), it hard-codes `fileIdEncoding = 0`
at line 288 while `buildUpdate` uses
`dataTableWriteConfig.getWritesFileIdEncoding()`, and the functional test sets
`withWritesFileIdEncoding(1)` up front so init never runs. This is reachable in
practice whenever RLI is enabled after the first replace commit (e.g. via the
async indexer), so a test in that order would catch it.
--
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]
