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

Improve sqlite iterator interface #33

Merged
merged 3 commits into from
Sep 18, 2018

Conversation

anhosi
Copy link
Contributor

@anhosi anhosi commented Sep 17, 2018

The SqliteStatementWrapper::QueryResult::Iterator represents the result set of a sqlite query that needs to be stepped. Using a copy of such a iterator would also step the statement and looping the original iterator would "miss" result rows. To avoid this surprising behavior this PR changes the iterator interface to disable copies.

anhosi and others added 3 commits September 14, 2018 15:09
Reason: multiple iterators for the same query result would interfere
with each other as a single result row can only be retrieved
once. These side effects would be very surprising and are therefore
forbidden.
@anhosi
Copy link
Contributor Author

anhosi commented Sep 17, 2018

CI

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

@@ -51,14 +51,14 @@ SqliteStatementWrapper::~SqliteStatementWrapper()
std::shared_ptr<SqliteStatementWrapper> SqliteStatementWrapper::execute_and_reset()
{
int return_code = sqlite3_step(statement_);
if (!isQueryOk(return_code)) {
if (!is_query_ok(return_code)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

just as a remark. I know it's only single threaded for now, but I believe operations like that can benefit from atomic compare_exchange.

@Karsten1987 Karsten1987 merged commit cf84366 into ros2:master Sep 18, 2018
@anhosi anhosi deleted the bug/sqlite_iterator_interface branch September 19, 2018 07:15
james-rms pushed a commit to james-rms/rosbag2 that referenced this pull request Nov 17, 2022
…ackage.xml (ros2#33)

Fix build failure for galactic and test foxy/galactic in CI.

Foxy build currently fails (ros2#29). We can fix it in the future, or remove it if it’s impractical to fix it.
james-rms pushed a commit to james-rms/rosbag2 that referenced this pull request Nov 17, 2022
…ackage.xml (ros2#33)

Fix build failure for galactic and test foxy/galactic in CI.

Foxy build currently fails (ros2#29). We can fix it in the future, or remove it if it’s impractical to fix it.

Signed-off-by: James Smith <james@foxglove.dev>
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.

None yet

4 participants