-
Notifications
You must be signed in to change notification settings - Fork 93
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
Add Rust ordered message stream implementation #1197
Conversation
Thanks for spiking out this PR. At a quick glance, it looks like this is reading all messages into memory and storing them in a heap – is that right? If so, this would not be usable with larger MCAP files, so while it is a good proof of concept, I don't think it would make sense to include in the upstream repo. Did you have additional work planned on this PR? |
@jtbandes yeah good callout, I was looking at the Python implementation which I think roughly does the same thing iiuc. The go implementation does not read everything in-memory, but does require a seekable stream - I think the challenge with not reading the whole stream in Rust is that LinearReader moves the |
Looking at the Python now, it seems to only do the in-memory sort if using a NonSeekingReader; for a SeekingReader the ordering is handled by MessageQueue here: https://github.com/foxglove/mcap/blame/main/python/mcap/mcap/reader.py#L288 I'm not very familiar with the Rust implementation yet but it may be true that a refactor is needed - @james-rms may know more. |
ping @james-rms on making a decision here on whether we want to push forward with the approach, do something different, or close |
fwiw, happy to close this - we're not using it at the moment, and timestamp ordered reading would be better suited to a version of this library built on top of |
Changelog
Add OrderedMessageStream implementation for reading timestamp ordered messages in Rust.
Docs
Happy to add a docs change!
Description
The Python API allows reading in message-timestamp order:
todo
This PR brings roughly the same functionality to Rust:
Fixes: #659