Add filter for reading selective topics

[mabelzhang]

This allows the user to specify a list of topic names to read.

Signed-off-by: Mabel Zhang mabel@openrobotics.org

[Karsten1987]

Solid PR, thanks for that.

a few outstanding nits, but mainly one comment for shifting the test to rosbag2_cpp instead of only testing the default storage plugin.

[Karsten1987]

I would appreciate having this test actually happening in rosbag2_cpp so it can be applied to more than just SQLite3.

[mabelzhang]

I had trouble creating the test. Can you suggest a test file in rosbag2_cpp that I can look at as an example of how to put messages into a reader? I tried adding it in rosbag2_cpp/test/rosbag2_cpp/test_sequential_reader.cpp, but I'm not able to get a valid object returned from read_next() - I get null pointers. Maybe because the object is a MockStorage?

The other problem is since the actual filtering is only implemented in sqlite3 right now, we wouldn't be able to get the right filtering behavior from a SequentialReader using MockStorage, right?

What I have done locally is I added a new class to test_sequential_reader.cpp, changed the reader_ object from a Reader to a SequentialReader, since the set_filter() is only in SequentialReader right now. If we want it to be in Reader, it also needs to be in BaseReadInterface and rosbag2_compression::SequentialCompressionReader. Do we want to define set_filter() beyond SequentialReader?

[Karsten1987]

I guess you have two options here. Either you modify the mock functions in a way to take the filter logic into account or you shift the test over to rosbag2_tests where we have a fully set up architecture. This also runs the sqlite3 plugin and still gives you the opportunity to test the c++ API.

Do you think it makes sense to apply the filter to each individual reader instance? Then we should add the two functions (set_filter()/reset_filter()). I would assume that these filters are generic enough (filter by topic etc) to apply for each reader.

[mabelzhang]

I spent some time putting the tests in rosbag2_cpp and rosbag2_tests. I did both halfway, and I think I finally figured out what the tests in these 3 packages do.

In the rosbag2_cpp tests, test_sequential_reader.cpp is intentionally using a mock object in mock_storage.cpp so that it tests the functions in the SequentialReader only, just to verify that reader_ calls functions inside storage_. The tests here are independent of the Storage object.

So with re/set_filter(), in the current setup, all that should be tested in rosbag2_cpp is that reader_.set_filter() will call storage_.set_filter(), and no checks on whether the returned objects are the correct topic names. That can be added - I have this locally.

The tests in rosbag2_storage test the actual function of the storage. So set_filter() here should be tested with the actual returned messages' topic names. This is the present state of the PR. I think we should keep these tests, because there's nowhere else that does unit test on the actual functionality of the Storage objects.

The tests in rosbag2_tests execute the command line, which would require implementing the topic filter in rosbag2_transport. That has not been implemented yet. It would require enough changes that maybe it should be in a separate PR? With rosbag2_transport tests, test_play.cpp uses a mock_sequential_reader.cpp mock object, which has its functions implemented. This can be updated with set_filter() to mock the functionality of the storage - I have this locally.

So my question is what we want to go into this PR - I have a mess of both routes on my local computer, with new tests in all 3 packages, and I haven't pushed anything. I think both the rosbag2_storage and rosbag2_cpp tests should be kept. In that case, I would just add the few lines to rosbag2_cpp tests as mentioned above to this PR, and leave this test in rosbag2_storage as well.

If we also want to add end-to-end tests and implement a new flag on rosbag2_transport, I can put that in a new branch and open a new PR. Otherwise I would just get rid of those changes. That option did not exist in ROS 1 bag playback and I don't know what flag we want to make it - maybe like ROS 1 bag record where -O is for the file name, and a subsequent list is the selective topics.

[Karsten1987]

I agree with basically everything you said here.

I would still appreciate some basic tests in rosbag2_cpp for things like what happens if you call the set_filter function twice in a row or test for the exception to be thrown if the storage is not initialized. That shouldn't take too much time, I think.

The functionality can be tested in the sqlite3 implementation of it (I think that's what you meant with rosbag2_storage, but I think it's indeed the default storage plugin).
But I would also assume we could get around rosbag2_transport or the ros2cli in the system tests (rosbag2_tests) by directly calling the rosbag2_cpp API. But I am okay with putting this in a separate PR to address once the option is exposed through the command line interface. I would ask you though in this case to open a new ticket for it to keep track of the outstanding work. It's up to you, really, which way you want to go. I am okay with both.

[mabelzhang]

Okay. I added basic tests in rosbag2_cpp in 5e09b58.

I tried making a new test in rosbag2_tests and calling the SequentialReader::open() in rosbag2_cpp to open the bag in rosbag2_tests/resources/cdr_test. For some reason it doesn't open:

[22.820s] 4: [ERROR] [1585629564.565606326] [rosbag2_storage]: Could not open 'cdr_test.db3' with 'sqlite3'. Error: Failed to read from bag: File 'cdr_test.db3' does not exist!
[22.820s] 4: [ERROR] [1585629564.565750830] [rosbag2_storage]: Could not load/open plugin with storage id 'sqlite3'.

In fact, I cannot even open cdr_test and wrong_rmw_test with ros2 bag play on the command line. I get the same error:

$ ros2 bag play cdr_test
[ERROR] [rosbag2_storage]: Could not open 'cdr_test.db3' with 'sqlite3'. Error: Failed to read from bag: File 'cdr_test.db3' does not exist!
[ERROR] [rosbag2_storage]: Could not load/open plugin with storage id 'sqlite3'.
[ERROR] [rosbag2_transport]: Failed to play: No storage could be initialized. Abort

But ros2 bag info cdr_test works.
Is this expected? If so, then I won't be able to call Reader directly from rosbag2_tests. I will open a new ticket to expose the topic selection to rosbag2_transport, and then test using the existing test_rosbag2_play_end_to_end.cpp.

[Karsten1987]

@mabelzhang Any update on this?

[mabelzhang]

I created a standalone C++ file to test this, and for some reason whenever it calls set_filter(), it just terminates, without even printing the error if the file doesn't exist. It doesn't do that if I remove the set_filter() call. I need to look into what's wrong in the code I added.

[Karsten1987]

@mabelzhang have you had a chance to re-iterate over the tests?

[Karsten1987]

a few outstanding comments, but this looks good to me.
Please run CI after addressing the comments.

[Karsten1987]

total bike shedding, but I somehow feel it reads easier if you invert the logic:

if (nullptr == storage_) {
  throw ...
}

storage_->set_filter(storage_filter);

same below

[mabelzhang]

So here I just copied the pattern in the three existing functions above these lines...

[mabelzhang]

One or more items in the checks have been failing on every new push with things like, bash failed, sudo failed, failed to fetch due to connection timeout.

CI (I kept the Ubuntu distro to default focal, hope that's right):

• Linux
• Linux-aarch64
• macOS
• Windows []

[mabelzhang]

CI is passing:

• Linux
• Linux-aarch64
• macOS
• Windows

Note that with the tests, I'm only doing the packages I touched --packages-select rosbag2_cpp rosbag2_storage rosbag2_storage_default_plugins rosbag2_transport. I did not include rosbag2_tests in the above.

If I do everything with --packages-select-regex rosbag2*, then I saw Windows with this in rosbag2_tests:

C:\ci\ws\src\mabelzhang\rosbag2\rosbag2_tests\test\rosbag2_tests\test_rosbag2_record_end_to_end.cpp(470): error: Expected: (metadata.relative_file_paths.size()) >= (1), actual: 0 vs 1
Bagfile never split!

from this line:

    ASSERT_GE(metadata.relative_file_paths.size(), 1) << "Bagfile never split!";

It looks like something with metadata. It doesn't look like it could be from this PR?

[Karsten1987]

that end-to-end test is known to be failing on windows. We'll address this hopefully shortly.
CI looks good. Feel free to merge.