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

feat: Add support and tests for DML returning clauses #805

Merged
merged 17 commits into from
Nov 22, 2022

Conversation

c2nes
Copy link
Contributor

@c2nes c2nes commented Sep 12, 2022

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.

@c2nes c2nes requested review from a team as code owners September 12, 2022 14:28
@product-auto-label product-auto-label bot added size: l Pull request size is large. api: spanner Issues related to the googleapis/python-spanner API. labels Sep 12, 2022
@c2nes c2nes changed the title Add support and tests for DML returning clauses feat: Add support and tests for DML returning clauses Sep 12, 2022
@c2nes
Copy link
Contributor Author

c2nes commented Sep 12, 2022

Note, integration tests are not expected to pass until THEN RETURN support is launched.

@c2nes
Copy link
Contributor Author

c2nes commented Sep 15, 2022

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 execute_sql vs execute_update being used in various case. After this change, execute_sql will be used in all cases. Previously, behavior depended on autocommit mode and the precise statement type,

Autocommit INSERT UPDATE DELETE
On execute_update execute_update execute_update
Off execute_update execute_sql execute_sql

@IlyaFaer
Copy link
Contributor

IlyaFaer commented Sep 21, 2022

@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 rowcount in the DB API - 'cause the backend was only counting updated rows, but not inserted.

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...

@asthamohta
Copy link
Contributor

@IlyaFaer from my understanding the value will only be returned if there is a returning clause. @rajatbhatta can you confirm this.
Also yes makes sense, running all the tests. Let's do that.

@c2nes
Copy link
Contributor Author

c2nes commented Sep 22, 2022

@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.

@IlyaFaer
Copy link
Contributor

IlyaFaer commented Sep 23, 2022

@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:
spanner_sqlalchemy: googleapis/python-spanner-sqlalchemy#247
spanner_django: googleapis/python-spanner-django#796

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.
@c2nes
Copy link
Contributor Author

c2nes commented Sep 23, 2022

@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 stats in rowcount, but it seems this is False when the row count is 0. I switched it to stats is not None.

Copy link
Contributor

@IlyaFaer IlyaFaer left a 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

@c2nes
Copy link
Contributor Author

c2nes commented Sep 26, 2022

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.

@rajatbhatta rajatbhatta added the automerge Merge the pull request once unit tests and other checks pass. label Nov 9, 2022
@gcf-merge-on-green
Copy link
Contributor

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.

@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Nov 9, 2022
@rajatbhatta rajatbhatta added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 15, 2022
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 15, 2022
@rajatbhatta rajatbhatta added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 15, 2022
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 15, 2022
@rajatbhatta rajatbhatta added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 16, 2022
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 16, 2022
@rajatbhatta rajatbhatta added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 16, 2022
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 16, 2022
@rajatbhatta rajatbhatta added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 22, 2022
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 22, 2022
@rajatbhatta rajatbhatta added the automerge Merge the pull request once unit tests and other checks pass. label Nov 22, 2022
@gcf-merge-on-green gcf-merge-on-green bot merged commit 81505cd into googleapis:main Nov 22, 2022
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Nov 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner Issues related to the googleapis/python-spanner API. size: l Pull request size is large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants