-
Notifications
You must be signed in to change notification settings - Fork 248
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
added seek interface #836
added seek interface #836
Conversation
fb6caf3
to
04d27ab
Compare
rosbag2_storage/include/rosbag2_storage/storage_interfaces/read_only_interface.hpp
Show resolved
Hide resolved
...g2_storage_default_plugins/include/rosbag2_storage_default_plugins/sqlite/sqlite_storage.hpp
Outdated
Show resolved
Hide resolved
rosbag2_transport/test/rosbag2_transport/mock_sequential_reader.hpp
Outdated
Show resolved
Hide resolved
rosbag2_storage/include/rosbag2_storage/storage_interfaces/read_only_interface.hpp
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
rosbag2_storage_default_plugins/src/rosbag2_storage_default_plugins/sqlite/sqlite_storage.cpp
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am requesting for changes in sqlite_storage.cpp
to cleanup logic since in current stage it's misleading further in sequential_reader
rosbag2_storage_default_plugins/src/rosbag2_storage_default_plugins/sqlite/sqlite_storage.cpp
Show resolved
Hide resolved
see updated description for why query can't be simplified.
- added seek(time) interface to read_only_interface - default sqlite implementation - added seek implementation to sequential reader (assumes multi-file is in time order) - updated set_filter/reset_filter to be able to update in between reads without resetting to beginning - added tests for seeking/filtering both single and multi-file bags fix mock for seek_time remove extra line moved setting up filters and seek_time for new file into `load_current_file` split up seek tests added docs to read_only_interface for seek, set_filter & reset_filter Signed-off-by: Sonia Jin <diegothemuich@gmail.com>
5e85787
to
fde201b
Compare
…and doesnt enforce weakly monotonic reads in time Signed-off-by: Sonia Jin <diegothemuich@gmail.com>
- added seek(time) interface to read_only_interface - default sqlite implementation - added seek implementation to sequential reader (assumes multi-file is in time order) - updated set_filter/reset_filter to be able to update in between reads without resetting to beginning - added tests for seeking/filtering both single and multi-file bags Signed-off-by: Sonia Jin <diegothemuich@gmail.com> Signed-off-by: Wojciech Jaworski <wojciech.jaworski@robotec.ai>
Per #824, created
seek(timestamp)
method inrosbag2_storage::storage_interfaces::ReadOnlyInterface
API.seek(timestamp)
seek
implementation keeps the storage filter, but will re-run the query with a new seek_timeset_filter
/reset_filter
to be able to be called in between reads in place. This means that if the message topics were in the order of:AAA
after reading two messages (AAA
&BBB
), the next message read would be theAAA
on the 4th row.read_next()
callshas_next()
to ensure that reader rolls over to next file at end of current filehas_next()
calls itself recursively until either there is a next row or there are no more files (doesn't assume that the next file will definitely contain valid row)NOTE
Because of continued confusion in the comments, I'm highlighting here the difference between
storage_interfaces::ReadOnlyInterface->seek(t)
SequentialReader->seek(t)
The differences are as follows:
SqliteStorage
DOES guarantee time-ordered reads regardless of write order. This means that callingseek(t)
will also guarantee subsequentread_next()
calls to return messages in time order until the end of the file.SequentialReader
DOES NOT guarantee time-ordered reads if individual files within a bag are not in non-overlapping sequential time order ("Time normalize" bag written from API so that splits are in order #842). This means that, with or without callingseek(t)
, there may be messages that occur at an earlier timestamp than previously-read messages when one file rolls over to the next file.SequentialReader->seek(t)
will only guarantee that all messages and only messages with timestamp >=t
will be read (satisfying any topic filters).SqliteStorage
, messages are queried in time order. The implicit tiebreaker between messages of the same timestamp is on the row id. This PR makes this second-degree ordering explicit. When we modify topic filters "in place", we actually rerun the query of the db file. In order to prevent re-reading previously read messages, we filter the timestamp to prevent reading "older" messages, AND we filter row-id to prevent reading already-read messages at the current timestamp. Note that if we don't assume write order (consistent with existing SqliteStorage implementation), we need to do both. For example:msgA
andmsgE
, then we need to check against the last read message timet1
to prevent previously read messagesmsgB
andmsgC
to be re-read, and to check against the row idrow0
to make sure thatmsgA
doesn't get re-read. This is why in theSqliteStorage
implementation we updateseek_time_
andseek_row_id_
with every read, to make sure that we can make the correct in-place update query. Thus the filter query is necessarily(timestamp > last_t) or (timestamp = last_t and row_id > last_rid)
.SequentialReader
, this is not applicable. When setting/resetting topic filters the "in place pointer" is kept by reference to the current file. We can assume that all files previous to the current file have already been read, so the setting/resetting of topic filters just occurs on the current & subsequent files. Within the current file, the "in place" message pointer is already handled by the single-file storage implementation.