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

Allow returning result for write queries #1990

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

MonilBhavsar
Copy link
Contributor

@MonilBhavsar MonilBhavsar commented Dec 2, 2024

Details

SQLite supports RETURNING clause(https://www.sqlite.org/lang_returning.html) that supports both read and write operations at a time.
Modify the SQLite handler to allow passing result reference for WRITE queries

Fixed Issues

Required for Expensify/App#51832

Tests

Tests with PR https://github.com/Expensify/Auth/pull/13393


Internal Testing Reminder: when changing bedrock, please compile auth against your new changes

@MonilBhavsar MonilBhavsar self-assigned this Dec 2, 2024
@MonilBhavsar MonilBhavsar changed the title Monil return result for writes Allow returning result for write queries Dec 2, 2024
@MonilBhavsar MonilBhavsar marked this pull request as ready for review December 2, 2024 12:42
@MonilBhavsar
Copy link
Contributor Author

@tylerkaraszewski will you please review this

Copy link
Contributor

@tylerkaraszewski tylerkaraszewski left a comment

Choose a reason for hiding this comment

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

Please add tests.

sqlitecluster/SQLite.cpp Show resolved Hide resolved
Copy link
Contributor

@tylerkaraszewski tylerkaraszewski left a comment

Choose a reason for hiding this comment

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

I've approved this

sqlitecluster/SQLite.cpp Show resolved Hide resolved
@MonilBhavsar MonilBhavsar force-pushed the monil-returnResultForWrites branch from 62b73c5 to 72bb681 Compare December 9, 2024 17:45
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.

2 participants