Reverse read order API and sqlite storage implementation #1083

emersonknapp · 2022-09-08T21:32:40Z

Add a new BaseReadInterface::set_read_order API, and implement it for the SqliteStorage.

Adds 2 new capabilities: reverse-timestamp reading, and file-order reading. Does not implement reverse-file-order reading.

rosbag2_storage/include/rosbag2_storage/storage_interfaces/base_read_interface.hpp

emersonknapp · 2022-09-08T22:52:52Z

API sweep to expose to Python revealed a larger surface area than I suspected - need to fix up the SequentialReader to be able to iterate backward between storage files, if necessary. Moving back to draft

emersonknapp · 2022-09-24T00:35:56Z

Ready to go but now depends on #1098 for the reverse timestamp split-bag test to work correctly

MichaelOrlov

@emersonknapp Thanks for implementation and sorry for delay in review from my side.
It turns out a lot of changes in many files and I was need to check logic very carefully.

Please see my comments and suggestions inline.

rosbag2_cpp/include/rosbag2_cpp/reader_interfaces/base_reader_interface.hpp

rosbag2_compression/test/rosbag2_compression/mock_storage.hpp

rosbag2_cpp/include/rosbag2_cpp/reader.hpp

rosbag2_cpp/include/rosbag2_cpp/reader_interfaces/base_reader_interface.hpp

rosbag2_cpp/include/rosbag2_cpp/readers/sequential_reader.hpp

rosbag2_storage_default_plugins/src/rosbag2_storage_default_plugins/sqlite/sqlite_storage.cpp

rosbag2_transport/test/rosbag2_transport/mock_sequential_reader.hpp

rosbag2_cpp/test/rosbag2_cpp/test_sequential_reader.cpp

emersonknapp · 2022-10-01T00:04:59Z

Pulling the one last conversation out to main thread for visibility:

I am thinking now that maybe it doesn't make sense to set the row id at all on set_read_order - maybe the user needs to explicitly seek to the end of the file? The cases are a little bit ambiguous how it should behave.
Maybe,

If set_read_order(reverse=true) set before open, then on open automatically seek to end of file

If storage is already open, then set_read_order doesn't change the place, even if no messages have been written yet
Does that behavior make sense?

rosbag2_storage_default_plugins/src/rosbag2_storage_default_plugins/sqlite/sqlite_storage.cpp

emersonknapp · 2022-10-03T22:11:34Z

Gist: https://gist.githubusercontent.com/emersonknapp/c5fded23fae98a4aafcf2a94672512a4/raw/ad0dd75e0b643a5c5f1a91b71dd5457352fc9d46/ros2.repos
BUILD args: --packages-above-and-dependencies rosbag2_compression rosbag2_cpp rosbag2_py rosbag2_storage rosbag2_storage_default_plugins rosbag2_transport
TEST args: --packages-above rosbag2_compression rosbag2_cpp rosbag2_py rosbag2_storage rosbag2_storage_default_plugins rosbag2_transport
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/10937

Linux
Linux-aarch64
Windows

rosbag2_cpp/include/rosbag2_cpp/readers/sequential_reader.hpp

rosbag2_storage_default_plugins/src/rosbag2_storage_default_plugins/sqlite/sqlite_storage.cpp

MichaelOrlov

@emersonknapp
I found out that when we will have read order by file without reverse which is currently supported the seek(timestamp) will not really work and will reset our seek_row_id which is unexpected.

I don't see a descent solution for this case. Therefore I would suggest to disable rosbag2_storage::ReadOrder::File completely for sqlite_storage implementation and create a follow up task to properly support it. It looks like that we will need to make some overhaul in sqlite_storage.cpp and in logic related to how we preparing database for readout in different cases.

I have also concern that we are not really doing db readout on seek and set_read_order - it will complicate things if we will add more logic in those methods and someone would do set_read_order then right after that seek before reading out new message. However it will be Ok to have it as is currently is if will have only one sort by timestamp order.

emersonknapp · 2022-10-04T20:39:24Z

OK - I have disabled File order reading entirely

emersonknapp · 2022-10-04T20:41:48Z

Gist: https://gist.githubusercontent.com/emersonknapp/f4e47a0ee418ead1fcaeaa13f9330cbf/raw/ad0dd75e0b643a5c5f1a91b71dd5457352fc9d46/ros2.repos
BUILD args: --packages-above-and-dependencies rosbag2_compression rosbag2_cpp rosbag2_py rosbag2_storage rosbag2_storage_default_plugins rosbag2_transport
TEST args: --packages-above rosbag2_compression rosbag2_cpp rosbag2_py rosbag2_storage rosbag2_storage_default_plugins rosbag2_transport
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/10946

Linux
Linux-aarch64
Windows

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>

…reader tests Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>

MichaelOrlov

Tentatively approve.
I found out one printf in tests I think this is residual from debug/development. It would be nice to remove it. It's trivial and minor I think we can omit rerunning CI if it will be removed.

I am more worry about appeared warnings on Windows build about YAML_CPP_DLL redefinition. There are wasn't such warnings from previous CI run.
@emersonknapp Do you have any idea what caused those warnings on Windows build?

MichaelOrlov · 2022-10-05T00:35:27Z

rosbag2_cpp/test/rosbag2_cpp/test_sequential_reader.cpp

+  reader.close();
+
+  for (bool do_reset : {false, true}) {
+    printf("Testing with do_reset %d\n", do_reset);


Forgotten debug printf from yesterday?

Yep, removing. Also yes, the windows warnings were from being slightly out of date - I will rebase and rerun.

MichaelOrlov · 2022-10-05T00:57:06Z

@emersonknapp Ok It seems I know root cause of the warnings on Windows build.
It looks like they are appeared due to recent merge on rolling ros2/yaml_cpp_vendor#38 .
The fix for warning in rosbag2 was also recently merged in #964.

Could you please try to rebase your branch on top of latest rolling and re-run CI for Windows job?

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>

emersonknapp · 2022-10-05T23:21:08Z

Gist: https://gist.githubusercontent.com/emersonknapp/4790ab890f1abc4f1c7d22fcb809ef0d/raw/ad0dd75e0b643a5c5f1a91b71dd5457352fc9d46/ros2.repos
BUILD args: --packages-above-and-dependencies rosbag2_compression rosbag2_cpp rosbag2_py rosbag2_storage rosbag2_storage_default_plugins rosbag2_transport
TEST args: --packages-above rosbag2_compression rosbag2_cpp rosbag2_py rosbag2_storage rosbag2_storage_default_plugins rosbag2_transport
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/10951

Linux
Linux-aarch64
Windows

MichaelOrlov · 2022-10-06T05:20:57Z

@emersonknapp Thanks for your hard work on this PR and sorry for long review process.
It turns out that logic for file ordering became complicated and opened up a "can of warms".
Please don't be angry on me if I was to pedantic on code review.

emersonknapp · 2022-10-06T07:53:49Z

No worries, sorry if I was brusque yesterday, I was feeling grumpy because I hadn't had any coffee. Thanks for holding a high standard for the code

emersonknapp mentioned this pull request Sep 8, 2022

Add set_read_order reader API ros-tooling/rosbag2_storage_mcap#54

Merged

emersonknapp marked this pull request as ready for review September 8, 2022 21:43

emersonknapp requested a review from a team as a code owner September 8, 2022 21:43

emersonknapp requested review from hidmic and jhdcs and removed request for a team September 8, 2022 21:43

james-rms reviewed Sep 8, 2022

View reviewed changes

rosbag2_storage/include/rosbag2_storage/storage_interfaces/base_read_interface.hpp Outdated Show resolved Hide resolved

emersonknapp marked this pull request as draft September 8, 2022 22:52

emersonknapp mentioned this pull request Sep 15, 2022

ROS2: Use rosbag2_py Python API, remove all explicit SQLite3 logic ros-visualization/rqt_bag#121

Closed

emersonknapp force-pushed the emersonknapp/reverse-iteration branch 2 times, most recently from 67d0d4a to 05f171e Compare September 16, 2022 00:15

MichaelOrlov self-requested a review September 17, 2022 07:56

emersonknapp force-pushed the emersonknapp/reverse-iteration branch from bb7138b to 9b4cdcc Compare September 23, 2022 23:35

emersonknapp marked this pull request as ready for review September 23, 2022 23:36

emersonknapp requested a review from james-rms September 23, 2022 23:37

emersonknapp force-pushed the emersonknapp/reverse-iteration branch 2 times, most recently from d7abe6d to 356495c Compare September 26, 2022 23:31

emersonknapp mentioned this pull request Sep 26, 2022

Use rosbag2_py API instead of metadata.yaml + sqlite3 ros-visualization/rqt_bag#126

Merged

MichaelOrlov requested changes Sep 28, 2022

View reviewed changes

emersonknapp force-pushed the emersonknapp/reverse-iteration branch from 356495c to 167182a Compare September 29, 2022 20:49

MichaelOrlov reviewed Sep 29, 2022

View reviewed changes

rosbag2_cpp/test/rosbag2_cpp/test_sequential_reader.cpp Outdated Show resolved Hide resolved

MichaelOrlov reviewed Oct 1, 2022

View reviewed changes

rosbag2_storage_default_plugins/src/rosbag2_storage_default_plugins/sqlite/sqlite_storage.cpp Show resolved Hide resolved

emersonknapp force-pushed the emersonknapp/reverse-iteration branch from 504dc40 to 606b2f7 Compare October 3, 2022 21:04

emersonknapp requested review from MichaelOrlov and removed request for james-rms October 3, 2022 22:10

MichaelOrlov reviewed Oct 4, 2022

View reviewed changes

rosbag2_cpp/include/rosbag2_cpp/readers/sequential_reader.hpp Outdated Show resolved Hide resolved

rosbag2_storage_default_plugins/src/rosbag2_storage_default_plugins/sqlite/sqlite_storage.cpp Show resolved Hide resolved

MichaelOrlov reviewed Oct 4, 2022

View reviewed changes

emersonknapp force-pushed the emersonknapp/reverse-iteration branch from ae1b36e to 787f5e5 Compare October 4, 2022 20:37

emersonknapp requested a review from MichaelOrlov October 4, 2022 20:39

emersonknapp added 6 commits October 4, 2022 15:40

Reverse read order API and sqlite storage implementation

10ccd30

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>

Make common fake_data for tests, address some review comments

90fade7

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>

Cover inter-timestamp ordering and iterator-reset case in sequential …

36b96f0

…reader tests Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>

Disable reverse file ordering

d75256f

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>

Disable file order reading entirely in sqlite3

6ad4b4a

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>

Use const-ref for set_read_order

6a76616

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>

emersonknapp force-pushed the emersonknapp/reverse-iteration branch from 73a1750 to 6a76616 Compare October 4, 2022 22:40

MichaelOrlov approved these changes Oct 5, 2022

View reviewed changes

Remove dangling printf

901bfd3

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>

emersonknapp merged commit 16cb47f into rolling Oct 6, 2022

delete-merged-branch bot deleted the emersonknapp/reverse-iteration branch October 6, 2022 00:45

emersonknapp mentioned this pull request Nov 16, 2022

Redo #126 to Rolling branch ros-visualization/rqt_bag#133

Merged

MichaelOrlov mentioned this pull request Dec 20, 2022

Draft: [Humble] Don't reopen file for every seek if we don't have to. Search directionally for the correct file (backport #1117) #1201

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reverse read order API and sqlite storage implementation #1083

Reverse read order API and sqlite storage implementation #1083

emersonknapp commented Sep 8, 2022 •

edited

Loading

emersonknapp commented Sep 8, 2022

emersonknapp commented Sep 24, 2022 •

edited

Loading

MichaelOrlov left a comment

emersonknapp commented Oct 1, 2022

emersonknapp commented Oct 3, 2022 •

edited

Loading

MichaelOrlov left a comment

emersonknapp commented Oct 4, 2022

emersonknapp commented Oct 4, 2022

MichaelOrlov left a comment •

edited

Loading

MichaelOrlov Oct 5, 2022

emersonknapp Oct 5, 2022

MichaelOrlov commented Oct 5, 2022 •

edited

Loading

emersonknapp commented Oct 5, 2022

MichaelOrlov commented Oct 6, 2022

emersonknapp commented Oct 6, 2022

Reverse read order API and sqlite storage implementation #1083

Reverse read order API and sqlite storage implementation #1083

Conversation

emersonknapp commented Sep 8, 2022 • edited Loading

emersonknapp commented Sep 8, 2022

emersonknapp commented Sep 24, 2022 • edited Loading

MichaelOrlov left a comment

Choose a reason for hiding this comment

emersonknapp commented Oct 1, 2022

emersonknapp commented Oct 3, 2022 • edited Loading

MichaelOrlov left a comment

Choose a reason for hiding this comment

emersonknapp commented Oct 4, 2022

emersonknapp commented Oct 4, 2022

MichaelOrlov left a comment • edited Loading

Choose a reason for hiding this comment

MichaelOrlov Oct 5, 2022

Choose a reason for hiding this comment

emersonknapp Oct 5, 2022

Choose a reason for hiding this comment

MichaelOrlov commented Oct 5, 2022 • edited Loading

emersonknapp commented Oct 5, 2022

MichaelOrlov commented Oct 6, 2022

emersonknapp commented Oct 6, 2022

emersonknapp commented Sep 8, 2022 •

edited

Loading

emersonknapp commented Sep 24, 2022 •

edited

Loading

emersonknapp commented Oct 3, 2022 •

edited

Loading

MichaelOrlov left a comment •

edited

Loading

MichaelOrlov commented Oct 5, 2022 •

edited

Loading