-
Notifications
You must be signed in to change notification settings - Fork 87
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
feat: Add support and tests for DML returning clauses #805
Conversation
Note, integration tests are not expected to pass until |
For reference, this was the previous PR where most of the special casing for INSERT statements was removed, #679 I also wanted to call out some changes with regard to
|
@c2nes, @asthamohta, does it mean Spanner backend now returns a number of affected rows for INSERT statements? Because that's why we were not fully supporting Before merging it, we'll have to run tests with this branch on Django and SQLAlchemy. What's working for the DB API, may still be not okay for higher-level packages. UPDATE: okay, I see that rows are counted for INSERT statements. Let's see... |
@IlyaFaer from my understanding the value will only be returned if there is a returning clause. @rajatbhatta can you confirm this. |
@asthamohta @IlyaFaer: Row counts should be available for all DML statements, with or without a returning clause. The new system tests included in this PR check row counts for INSERT, UPDATE, and DELETE statements with and without a THEN RETURN clause. |
@c2nes, one test failed for spanner_django: FAIL: test_empty_update (update.tests.SimpleTest)
Update changes the right number of rows for an empty queryset
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/runner/work/python-spanner-django/python-spanner-django/django_tests_dir/django/tests/update/tests.py", line 31, in test_empty_update
self.assertEqual(num_updated, 0)
AssertionError: -1 != 0 All the other tests seem to be passing fine: |
By setting the `GOOGLE_CLOUD_TESTS_SPANNER_HOST` environment variable you can now run tests against an alternate Spanner API endpoint. This is particularly useful for running system tests against a pre-production deployment.
For historical reasons it seems the INSERT codepath and that for UPDATE/DELETE were separated, but today there appears to be no practical differences in how these DML statements are handled. This change removes most of the special handling for INSERTs and uses existing methods for UPDATEs/DELETEs instead. The one remaining exception is the automatic addition of a WHERE clause to UPDATE and DELETE statements lacking one, which does not apply to INSERT statements.
Previously, rowcount was only available after executing an UPDATE or DELETE in autocommit mode. This change extends this support so that a rowcount is available for all DML statements, regardless of whether autocommit is enabled.
This change adds support and tests for a returning clause in DML statements. This is done by moving executing of all DML to use `execute_sql`, which is already used when not in autocommit mode.
36dd5d4
to
f5e8153
Compare
@IlyaFaer thank you for catching that! I pushed up a fix and added a test case to cover this. I was previously testing the truthiness of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@c2nes, @asthamohta, okay, now all the tests are passing fine on all dependent APIs. I think we can merge it
We will need to wait until the server-side changes are available to merge. I'll reach back out once that happens. |
Merge-on-green attempted to merge your PR for 6 hours, but it was not mergeable because either one of your required status checks failed, one of your required reviews was not approved, or there is a do not merge label. Learn more about your required status checks here: https://help.github.com/en/github/administering-a-repository/enabling-required-status-checks. You can remove and reapply the label to re-run the bot. |
This change adds support for DML returning clauses and includes a few prerequisite changes.
I would suggest reviewing commit-by-commit. The commit messages provide additional context and are reproduced below,
feat: Support custom endpoint when running tests
By setting the
GOOGLE_CLOUD_TESTS_SPANNER_HOST
environment variable you can now run tests against an alternate Spanner API endpoint. This is particularly useful for running system tests against a pre-production deployment.refactor(dbapi): Remove most special handling of INSERTs
For historical reasons it seems the INSERT codepath and that for UPDATE/DELETE were separated, but today there appears to be no practical differences in how these DML statements are handled. This change removes most of the special handling for INSERTs and uses existing methods for UPDATEs/DELETEs instead. The one remaining exception is the automatic addition of a WHERE clause to UPDATE and DELETE statements lacking one, which does not apply to INSERT statements.
feat(dbapi): Add full support for rowcount
Previously, rowcount was only available after executing an UPDATE or DELETE in autocommit mode. This change extends this support so that a rowcount is available for all DML statements, regardless of whether autocommit is enabled.
feat: Add support for returning clause in DML
This change adds support and tests for a returning clause in DML statements. This is done by moving executing of all DML to use
execute_sql
, which is already used when not in autocommit mode.