feat: seekable reader#530
Conversation
There was a problem hiding this comment.
Pull request overview
Adds seek support for Avro container readers by tracking block boundaries (offset + record count) during iteration and exposing an API to seek back to a previously-seen block for Read + Seek inputs.
Changes:
- Introduced
BlockPositionand internal position tracking to record block start offsets as blocks are loaded. - Added
Reader::{current_block,data_start,seek_to_block}(seek API gated onSeek) plus tests validating seeking between blocks. - Added a new error detail variant for seek failures.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
avro/src/reader/mod.rs |
Exposes BlockPosition and adds reader-level block position + seek APIs with new tests. |
avro/src/reader/block.rs |
Implements BlockPosition, PositionTracker, and block-level seek + block-boundary tracking. |
avro/src/lib.rs |
Re-exports BlockPosition from the crate root. |
avro/src/error.rs |
Adds Details::SeekToBlock for I/O seek errors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Do you have a use case for this functionality ? |
|
|
||
| self.current_block_info = Some(BlockPosition { | ||
| offset: block_start, | ||
| message_count: block_len as usize, |
There was a problem hiding this comment.
| message_count: block_len as usize, | |
| message_count: self.message_count, |
There was a problem hiding this comment.
I actually wrote this part specifically this way to ensure it would not lost, if a refactoring or other change are applied. It cannot rely on the meaning of the self.message_count and its current value if those two will be separated into different places of code
There was a problem hiding this comment.
You can move self.message_count = block_len as usize down next to self.current_block_info to reduce that risk. As I do think Martin's suggestion is reasonable.
There was a problem hiding this comment.
I'm not arguing, any of this would work and I can change, that's no problem. But I'm curios to understand why you think this would be better?
My idea is just using the source of truth variable, so there is no way it gets a wrong number. Using the self.message_count works with the current code, but it doesn't give any guarantee if it changes. So I'm wondering why the second approach is better?
I need to read just a few records from a large Avro file, and without this it's incredibly inefficient as I need to read the whole file from the start each time. |
|
I'm wondering if you guys expect me to make some changes to the PR or we're waiting for something else? |
|
I don't have further comments, @martin-g how about you? |
AFAIU you need to read the Avro data once to collect the offsets to be able to seek later, right ? |
Right. First time I needed that was when we had a separate index with records, so we were just opening the Avro file at a needed position. Very large files (audit and traces) and small index for positions. That was in Java, where seeking is supported. Now I need to show an Avro file on screen, using Rust. User can scroll the file back and forth. One option is keeping the whole file in memory, which is not optimal considering the usual sizes, or just keep the list of positions in file. The file opens from the start, so I always have all the previous positions. |
c51cc92 to
6d736c7
Compare
|
I've rebased this branch and fixed the conflicts. @martin-g will we put this in 0.22 or will it be pushed back to 0.23? |
|
Let's leave it for 0.23 |
| let next = self.codec.decompress(&mut self.buf); | ||
|
|
||
| // Make sure the position points only to a valid block | ||
| self.current_block_info = match next { |
There was a problem hiding this comment.
Should this be done earlier ?!
If there is a problem with the marker then any call to Reader::current_block() will return wrong data.
| /// Seek the underlying stream to `offset` and read the block there. | ||
| /// Validates the sync marker to confirm it's a real block boundary. | ||
| /// Returns an error if no valid block can be read at the offset | ||
| /// (e.g., the offset is at or past EOF). |
There was a problem hiding this comment.
| let reader = if track_positions { | ||
| PositionTracker::Tracking { | ||
| inner: reader, | ||
| pos: 0, |
There was a problem hiding this comment.
If the underlying reader R is already partially read then pos: 0 won't be correct.
I am not sure how feasible this is though.
We can add one more optional field to ReaderBuilder - initial_position, with a default value of 0.
Added a feature to seek to a particular block when reading an Avro file.
The Reader now provides the current Block position and for a Seek'able input it can seek to the specific position, assuming it's a valid start position of a block.