hudi-agent commented on code in PR #19373:
URL: https://github.com/apache/hudi/pull/19373#discussion_r3654144169
##########
hudi-common/src/main/java/org/apache/hudi/metadata/HoodieBackedTableMetadata.java:
##########
@@ -606,20 +609,39 @@ private ClosableIterator<IndexedRecord>
readSliceWithFilter(Predicate predicate,
baseFileReaders,
fileGroupReaderProps);
- HoodieFileGroupReader<IndexedRecord> fileGroupReader =
HoodieFileGroupReader.<IndexedRecord>builder()
- .withReaderContext(readerContext)
- .withHoodieTableMetaClient(metadataMetaClient)
- .withLatestCommitTime(latestMetadataInstantTime)
- .withBaseFileOption(fileSlice.getBaseFile())
- .withLogFiles(fileSlice.getLogFiles())
- .withPartitionPath(fileSlice.getPartitionPath())
- .withDataSchema(SCHEMA)
- .withRequestedSchema(SCHEMA)
- .withProps(fileGroupReaderProps)
- .withRecordBufferLoader(recordBufferLoader)
- .build();
-
- return fileGroupReader.getClosableIterator();
+ if (useLsmReader) {
+ return HoodieLsmFileGroupReader.<IndexedRecord>builder()
Review Comment:
🤖 The classic reader filters MDT log blocks by the EXACT_MATCH valid-instant
range (BaseHoodieLogRecordReader#isInRange), but HoodieLsmFileGroupReader only
applies getInstantRange() to the base file — its native-log iterators read
every log file unconditionally. Since the file slice from
getPartitionLatestMergedFileSlices isn't pre-filtered, could this surface
metadata for instants that completed on the MDT but aren't yet committed (or
were rolled back) on the data table? @danny0405 should the native logs be
filtered by delta-commit time against the valid instants here?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-common/src/main/java/org/apache/hudi/metadata/HoodieBackedTableMetadata.java:
##########
@@ -606,20 +609,39 @@ private ClosableIterator<IndexedRecord>
readSliceWithFilter(Predicate predicate,
baseFileReaders,
fileGroupReaderProps);
- HoodieFileGroupReader<IndexedRecord> fileGroupReader =
HoodieFileGroupReader.<IndexedRecord>builder()
- .withReaderContext(readerContext)
- .withHoodieTableMetaClient(metadataMetaClient)
- .withLatestCommitTime(latestMetadataInstantTime)
- .withBaseFileOption(fileSlice.getBaseFile())
- .withLogFiles(fileSlice.getLogFiles())
- .withPartitionPath(fileSlice.getPartitionPath())
- .withDataSchema(SCHEMA)
- .withRequestedSchema(SCHEMA)
- .withProps(fileGroupReaderProps)
- .withRecordBufferLoader(recordBufferLoader)
- .build();
-
- return fileGroupReader.getClosableIterator();
+ if (useLsmReader) {
+ return HoodieLsmFileGroupReader.<IndexedRecord>builder()
+ .withReaderContext(readerContext)
+ .withHoodieTableMetaClient(metadataMetaClient)
+ .withLatestCommitTime(latestMetadataInstantTime)
+ .withBaseFileOption(fileSlice.getBaseFile())
+ .withLogFiles(fileSlice.getLogFiles())
+ .withPartitionPath(fileSlice.getPartitionPath())
+ .withDataSchema(SCHEMA)
+ .withRequestedSchema(SCHEMA)
+ .withProps(fileGroupReaderProps)
+ .build()
+ .getClosableIterator();
+ } else {
+ return HoodieFileGroupReader.<IndexedRecord>builder()
Review Comment:
🤖 nit: the two builder chains share 8 identical `.with*()` calls and differ
only in `withRecordBufferLoader`. Could you check whether passing `null` to
`withRecordBufferLoader` is a no-op in `HoodieFileGroupReader`? If so, a single
builder with `withRecordBufferLoader(useLsmReader ? null : recordBufferLoader)`
(and a type switch via an interface/factory) would avoid two places to update
if the shared params ever change.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
--
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]
