Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make IndexInput#prefetch take an offset. #13363

Merged
merged 3 commits into from
May 14, 2024

Conversation

jpountz
Copy link
Contributor

@jpountz jpountz commented May 13, 2024

This makes IndexInput#prefetch take an offset instead of being relative to the current position. This avoids requiring callers to seek only to call prefetch().

This makes `IndexInput#prefetch` take an offset instead of being relative to
the current position. This avoids requiring callers to seek only to call
`prefetch()`.
@jpountz jpountz requested a review from uschindler May 13, 2024 15:31
@rmuir
Copy link
Member

rmuir commented May 13, 2024

I like this much better from api perspective: it closer maps to madvise() and to me is more straightforward. Especially as the current PRs out there to use it (terms and postings) are using absolute FPs and seeking back-n-forth to workaround the current API.

@jpountz jpountz merged commit 46f1f95 into apache:main May 14, 2024
3 checks passed
@jpountz jpountz deleted the index_input_prefetch_absolute branch May 14, 2024 16:43
@ChrisHegarty
Copy link
Contributor

++ this absolute addressing is much nicer. We could even add prefetch to the RandomAccessInput interface, though I'm not sure how much that is used.

Copy link
Contributor

@uschindler uschindler left a comment

Choose a reason for hiding this comment

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

Quick review after coming back from vacation: looks fine and seems to work.

I only don't like the we now have absolute APIs in a stream-like interface. So my choice for this API would have been in RandomAccessInput. But others seem to want to have it like that.

I don't care!

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.

None yet

4 participants