wwj6591812 commented on PR #7994: URL: https://github.com/apache/paimon/pull/7994#issuecomment-4569761600
> Thanks for the fix. I think the current exhausted flag does not cover the real `RawFileSplitRead` path yet. > > In `RawFileSplitRead#createFileReader`, the same `BitmapIndexResult` is first passed into `FormatReaderContext` and then `DataFileRecordReader#readBatchInternal` applies it again via `iterator.selection(selection)` before the reader is wrapped by `ApplyBitmapIndexRecordReader`. Because of that inner selection, the wrapped iterator can return `null` as soon as the bitmap iterator is exhausted, without ever exposing a row whose `returnedPosition() > last` to `ApplyBitmapIndexFileRecordIterator`. In that case `exhausted` remains `false`, and `RecordReaderIterator` still keeps calling `readBatch()` until EOF. > > The new unit test does not model this because its underlying reader returns unfiltered rows. I added a local variant where `CountingFileRecordReader#readBatch()` returns `batch.selection(selection)`, which is equivalent to the `DataFileRecordReader` behavior above. With `totalRows = 100`, `batchSize = 20`, and `limit = 10`, the result rows are correct, but the underlying `readBatchCount()` is `6` instead of `1`. > > A minimal fix could be to mark the reader exhausted when returning the last selected row, for example when `bitmap.contains(position)` and `position >= last`, or otherwise avoid/apply only one layer of bitmap selection and add a test that simulates the already-selected underlying iterator. Thanks again for the careful review and for pointing out the gap in the real RawFileSplitRead path. Your feedback was very helpful. You were right that our original exhausted flag did not cover the case where the underlying iterator is already selection-filtered in DataFileRecordReader. In that path, the wrapped iterator can return null once the selected rows in the current batch are consumed, without ever reaching position > last, so readBatch() could still continue until EOF. We have updated the fix accordingly: We still keep the original reader-level exhausted guard in ApplyBitmapIndexRecordReader. In ApplyBitmapIndexFileRecordIterator, we now also mark the reader exhausted when returning the last selected row (position >= last), in addition to the existing position > last handling. We added a unit test where the underlying reader returns batch.selection(selection), matching the DataFileRecordReader behavior you described. With totalRows = 100, batchSize = 20, and limit = 10, the result is still 10 rows, and readBatch() is now called only once. We really appreciate you taking the time to review this in detail. Could you please take another look when convenient? Thx. @leaves12138 -- 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]
