wwj6591812 commented on PR #8116: URL: https://github.com/apache/paimon/pull/8116#issuecomment-4923742876
> Thanks for the PR. I have one concern about the actual optimization benefit here. > > `RecordReader` / `TableRead` are lazy. If the upper Limit operator already pulls only N records and then stops/closes the source, the underlying split reader should not be fully drained anyway. In that case, wrapping `MergeFileSplitRead` with `LimitRecordReader` mostly duplicates the upper-layer limit behavior and may not bring a real SQL runtime optimization. > > The current tests prove that `TableRead.withLimit` returns only N rows when the reader is fully consumed, but they do not demonstrate reduced IO / fewer opened data files / less merge work in an actual engine LIMIT query. > > Could you clarify the concrete execution path where the Paimon reader would still be drained despite an outer LIMIT? If the main purpose is to fix the `TableRead.withLimit` contract for PK merge-read, I think the PR description should state that more explicitly. If it is intended as a performance optimization, it would be better to add a benchmark or a test showing fewer opened file readers / merge sections compared with the previous behavior. Thanks for pointing this out. I agree that an outer SQL LIMIT can already stop a lazy TableRead / RecordReader, so this PR should not be presented as a general SQL runtime optimization. I updated the focus to the TableRead.withLimit contract for PK merge-read when the returned reader is fully consumed. I also added a focused test with a tracking FileReaderFactory: with the split-level limit it only opens the first needed file reader, while the unbounded merge read opens all file readers when drained. Filters still disable this split-level limit for correctness. -- 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]
