Skip to content

Making scan limit of RecordBatchReaderExec handle row limit efficiently#36

Merged
jiayuasu merged 2 commits intoapache:mainfrom
Kontinuation:apply-limit
Sep 8, 2025
Merged

Making scan limit of RecordBatchReaderExec handle row limit efficiently#36
jiayuasu merged 2 commits intoapache:mainfrom
Kontinuation:apply-limit

Conversation

@Kontinuation
Copy link
Member

limit in RecordBatchReaderExec is supposed to be a number of rows. However, it is currently being applied as a number of batches. This patch fixes this by implementing a structure that keeps track of the number of iterated rows and stops early.

This patch also fixes another problem: the usage of RwLock in RecordBatchReaderExec is inappropriate, since we only access the underlying reader protected by the lock in write mode. We use a Mutex here instead, and removed unsafe impl Sync for RecordBatchReaderExec.

@Kontinuation Kontinuation requested a review from Copilot September 8, 2025 06:52
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes two issues in RecordBatchReaderExec: the limit parameter now correctly applies to row count instead of batch count, and replaces inappropriate RwLock usage with Mutex for better thread safety. The changes improve both correctness and performance of the record batch reader functionality.

  • Implements proper row-based limiting with RowLimitedIterator that tracks consumed rows and handles batch truncation
  • Replaces RwLock with Mutex and removes unsafe Sync implementation for better thread safety
  • Adds comprehensive test coverage for various row limit scenarios including edge cases with empty batches

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@jiayuasu jiayuasu merged commit bfb46a6 into apache:main Sep 8, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants