Skip to content
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 support for filtering topics via regular expressions #1034

Merged
merged 4 commits into from
Jul 5, 2022

Conversation

esteve
Copy link
Member

@esteve esteve commented Jun 28, 2022

This PR adds a new option --topics-regex that allows filtering topics via regular expressions.

@esteve esteve force-pushed the filter-regex branch 5 times, most recently from bc3e1a2 to fd879f7 Compare June 28, 2022 14:15
@audrow audrow changed the base branch from master to rolling June 28, 2022 14:50
@esteve
Copy link
Member Author

esteve commented Jun 29, 2022

I'm debugging an issue with the player and regular expressions. The following causes ros2 bag play to throw an exception:

$ ros2 bag play . --topics-regex /lidar_front/points_raw
[INFO] [1656517413.919186963] [rosbag2_storage]: Opened database './rosbag2_2020_09_23-15_58_07.db3' for READ_ONLY.
[INFO] [1656517413.919316194] [rosbag2_player]: Set rate to 1
[INFO] [1656517413.925218047] [rosbag2_player]: Adding keyboard callbacks.
[INFO] [1656517413.925259647] [rosbag2_player]: Press SPACE for Pause/Resume
[INFO] [1656517413.925286814] [rosbag2_player]: Press CURSOR_RIGHT for Play Next Message
[INFO] [1656517413.925312879] [rosbag2_player]: Press CURSOR_UP for Increase Rate 10%
[INFO] [1656517413.925341094] [rosbag2_player]: Press CURSOR_DOWN for Decrease Rate 10%
[INFO] [1656517413.925446615] [rosbag2_player]: Playback until timestamp: -1
[INFO] [1656517413.925790959] [rosbag2_storage]: Opened database './rosbag2_2020_09_23-15_58_07.db3' for READ_ONLY.
[ERROR] [1656517414.026479569] [rosbag2_player]: Failed to play: Error when preparing SQL statement 'SELECT data, timestamp, topics.name, messages.id FROM messages JOIN topics ON messages.topic_id = topics.id WHERE ((topics.name REGEXP '/lidar_front/points_raw')) AND (((timestamp = 1600901887524769241) AND (messages.id >= 0)) OR (timestamp > 1600901887524769241)) ORDER BY messages.timestamp, messages.id;'. SQLite error: (1): SQL logic error

however, when issuing that same query directly via the sqlite3 command line interface, I get the correct results.

@esteve
Copy link
Member Author

esteve commented Jun 29, 2022

It seems that for the REGEXP operator to work an application-defined function needs to be provided. I'm guessing that the sqlite3 command line tool already includes one, so in this case it'd be a matter of defining one in the C++ code.

@esteve esteve marked this pull request as ready for review June 30, 2022 10:06
@esteve esteve requested a review from a team as a code owner June 30, 2022 10:06
@esteve esteve requested review from emersonknapp and jhdcs and removed request for a team June 30, 2022 10:06
@esteve
Copy link
Member Author

esteve commented Jun 30, 2022

This now works, you can pass multiple arguments to --topics-regex and ros2 bag play will only play the topics that match the regular expressions (e.g. --topics-regex /lidar_.*/points_raw)

Copy link
Contributor

@jhdcs jhdcs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks okay to me, though I wouldn't mind a second opinion from another reviewer.

@MichaelOrlov MichaelOrlov self-requested a review July 1, 2022 07:15
@MichaelOrlov
Copy link
Contributor

This looks okay to me, though I wouldn't mind a second opinion from another reviewer.

I will review it in a couple days.

Copy link
Contributor

@MichaelOrlov MichaelOrlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@esteve Overall looks good to me.
Among one nitpick with pointers initialization.
Also could you please write a few unit tests to cover new topics_regex API ?

@esteve
Copy link
Member Author

esteve commented Jul 5, 2022

@MichaelOrlov I've added a test based on the the existing topics API, does it look enough or would you like more tests? Thanks.

esteve and others added 4 commits July 5, 2022 16:00
Signed-off-by: Esteve Fernandez <esteve.fernandez@tier4.jp>
…thin sqlite3

Signed-off-by: Esteve Fernandez <esteve.fernandez@tier4.jp>
Signed-off-by: Esteve Fernandez <esteve.fernandez@tier4.jp>
Signed-off-by: Esteve Fernandez <esteve.fernandez@tier4.jp>
Copy link
Contributor

@MichaelOrlov MichaelOrlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@esteve Thank you for your contribution!
Now it looks good. Approving.

@MichaelOrlov
Copy link
Contributor

MichaelOrlov commented Jul 5, 2022

Running CI:
Gist: https://gist.githubusercontent.com/MichaelOrlov/4902ef223bb586d0f7311fd7508bc496/raw/28022ca5ed769da33c9731136f0d03173f8dec13/ros2.repos
BUILD args: --packages-above-and-dependencies ros2bag rosbag2_py rosbag2_storage rosbag2_storage_default_plugins rosbag2_transport rosbag2_tests
TEST args: --packages-above ros2bag rosbag2_py rosbag2_storage rosbag2_storage_default_plugins rosbag2_transport rosbag2_tests
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/10473

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@MichaelOrlov MichaelOrlov merged commit 3f1bf5f into ros2:rolling Jul 5, 2022
esteve added a commit to esteve/rosbag2 that referenced this pull request Jul 6, 2022
* Added support for filtering topics via regular expressions

Signed-off-by: Esteve Fernandez <esteve.fernandez@tier4.jp>

* Added application function for evaluating regular expressions from within sqlite3

Signed-off-by: Esteve Fernandez <esteve.fernandez@tier4.jp>

* Use nullptr for null pointers

Signed-off-by: Esteve Fernandez <esteve.fernandez@tier4.jp>

* Added tests for topics-regex API

Signed-off-by: Esteve Fernandez <esteve.fernandez@tier4.jp>
@esteve esteve deleted the filter-regex branch July 6, 2022 15:56
MichaelOrlov pushed a commit that referenced this pull request Jul 28, 2022
…ressions (#1034)- (#1039)

* Added support for filtering topics via regular expressions (#1034)

* Added support for filtering topics via regular expressions

Signed-off-by: Esteve Fernandez <esteve.fernandez@tier4.jp>

* Added application function for evaluating regular expressions from within sqlite3

Signed-off-by: Esteve Fernandez <esteve.fernandez@tier4.jp>

* Use nullptr for null pointers

Signed-off-by: Esteve Fernandez <esteve.fernandez@tier4.jp>

* Added tests for topics-regex API

Signed-off-by: Esteve Fernandez <esteve.fernandez@tier4.jp>

* Renamed --topics-regex to --regex and -e

Signed-off-by: Esteve Fernandez <esteve.fernandez@tier4.jp>
esteve added a commit that referenced this pull request Aug 16, 2022
esteve added a commit that referenced this pull request Aug 16, 2022
…ular expressions (#1034)- (#1039)"

This reverts commit 193caee.

Signed-off-by: Esteve Fernandez <esteve.fernandez@tier4.jp>
MichaelOrlov pushed a commit that referenced this pull request Aug 17, 2022
…ular expressions (#1034)- (#1039)" (#1069)

This reverts commit 193caee.

Signed-off-by: Esteve Fernandez <esteve.fernandez@tier4.jp>

Signed-off-by: Esteve Fernandez <esteve.fernandez@tier4.jp>
@ros-discourse
Copy link

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-8-18-2022/27050/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants