voonhous commented on code in PR #19869:
URL: https://github.com/apache/hudi/pull/19869#discussion_r3965132171
##########
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:
Agree, and the same holds on the RLI side: `BaseFileRecordParsingUtils:205`
now always passes `basePath`, so the
`checkArgument(relativeFilePath.isPresent())` in `ParquetUtils` can never fire.
`MetaFieldsMode` selective modes leave `_hoodie_record_key` null on disk with
the field still in the schema, and `BaseHoodieWriteClient:1607-1616` explicitly
steers those tables to `RECORD_INDEX`, so after this change they would silently
index `path_pos` keys. Could both sides gate on
`tableConfig.isRecordKeyPopulated()` (or the `_hudiext` marker) instead of
schema presence / a null value?
##########
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: `BaseRecordIndexer.buildUpdate:147` calls
`getRecordIndexAdditionalUpserts` unconditionally, so a write-stat-free replace
commit deletes from RLI while `getSecondaryIndexUpdates` returns at the
`allWriteStats.isEmpty()` check, before the new union at
`SecondaryIndexRecordGenerationUtils:210`. Both commits in the functional test
carry a write stat, so the asymmetry is not visible there. Could the
replaced-file-group SI generation move above that early return, with a third
drop-only commit in `TestExternalFileRecordAndSecondaryIndex` asserting the SI
entries disappear?
##########
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++;
Review Comment:
For a base-file-only slice these line up: the file group reader has no
record buffer and no instant-range filter for that shape, and the counter
increments after `getRecordKey`. The one divergence is an external file group
that later acquires log files: `getLatestMergedFileSliceBeforeOrOn` hands back
a merged slice, so SI counts merged rows while RLI (`ParquetUtils`) counts
base-file rows. Could `getRecordKey` fail loudly when `fileSlice.getLogFiles()`
is non-empty, so that case cannot silently mis-key?
--
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]
