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(db_api): make rowcount property NotImplemented #603

Merged
merged 3 commits into from
Oct 14, 2021

Conversation

IlyaFaer
Copy link
Contributor

@IlyaFaer IlyaFaer commented Sep 30, 2021

The original issue: googleapis/python-spanner-sqlalchemy#121
As we're not able to implement rowcount property according to the PEP-249, erasing its functionality (which is actual for only single one case - UPDATE query in autocommit mode).

@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Sep 30, 2021
@product-auto-label product-auto-label bot added the api: spanner Issues related to the googleapis/python-spanner API. label Sep 30, 2021
@IlyaFaer IlyaFaer added the type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. label Sep 30, 2021
@IlyaFaer IlyaFaer marked this pull request as ready for review September 30, 2021 09:31
@IlyaFaer IlyaFaer requested a review from a team as a code owner September 30, 2021 09:31
@larkee larkee added the automerge Merge the pull request once unit tests and other checks pass. label Oct 13, 2021
@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 Oct 14, 2021
@IlyaFaer IlyaFaer added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 14, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 14, 2021
@larkee larkee merged commit b5a567f into googleapis:main Oct 14, 2021
@IlyaFaer IlyaFaer deleted the rowcount_not_implemented branch October 15, 2021 07:18
asthamohta pushed a commit to asthamohta/python-spanner that referenced this pull request Nov 11, 2021
Co-authored-by: larkee <31196561+larkee@users.noreply.github.com>
@vi3k6i5
Copy link
Contributor

vi3k6i5 commented Jan 12, 2022

Updates with autocommit is default in Django, which breaks with these changes, can we undo these changes? @IlyaFaer

@IlyaFaer
Copy link
Contributor Author

@vi3k6i5, these changes were changed not very long ago, here #628

@vi3k6i5
Copy link
Contributor

vi3k6i5 commented Jan 13, 2022

@IlyaFaer : The fix in #628 is not working for Django. In Django model object update in auto-commit mode checks for success by getting the _row_count value which is now returning -1 and thus Django thinks that the update failed. Can you share what might break if we completely undo this PR ?

@IlyaFaer
Copy link
Contributor Author

IlyaFaer commented Jan 13, 2022

@vi3k6i5, I don't think it'll break anything, you know, more.

But, as I see it, if we'll start to really count rows, it'll be incorrect in many cases (most likely Django will still be failing), because results are streamed piece by piece. We're just not able to always know how many results we've got.

Plus to this, there will be an operative property rowcount, which users will try to rely on, and it'll bring confusion (like it was in the first version).

Could you show traceback of the failure or something? If it's only actual for tests, I guess we can just skip them (as we're not able to support rowcount, why should we test it). If it's real case... well, let's see.

@vi3k6i5
Copy link
Contributor

vi3k6i5 commented Feb 2, 2022

Hi @IlyaFaer

I am not able to run model object update query because of the change in this PR:

Example:
author_kent = Author( first_name="Arthur", last_name="Kent", rating=Decimal("4.1"), ) author_kent.save() # Works file author_kent.rating = Decimal("4.2") author_kent.save() # Fails with following error

Error:
`
E grpc._channel._InactiveRpcError: <_InactiveRpcError of RPC that terminated with:
E status = StatusCode.ALREADY_EXISTS
E details = "Failed to insert row with primary key ({pk#id:837916644807504432}) due to previously existing row"
E debug_error_string = "{"created":"@1643794129.022745000","description":"Error received from peer ipv4:0.0.0.0:9010","file":"src/core/lib/surface/call.cc","file_line":1075,"grpc_message":"Failed to insert row with primary key ({pk#id:837916644807504432}) due to previously existing row","grpc_status":6}"
E >

../../.virtualenvs/temp_2/lib/python3.9/site-packages/grpc/_channel.py:849: _InactiveRpcError
`

Reason for the failure is pretty simple:

  1. During author_kent.save() Django tries to first update the value always, if that is successful then the query is ended else it tries to insert.
  2. The way django checks if a update was successful or not is by checking row_count > 0 Link

Since we are returning -1 for row_count, django assumes the update has failed and tries to insert data again which fails because there is already an entry in db with the same primary key.

Solution would be to revert the row_count changes.

Can you elaborate where the row_count returns incorrect result which does not match the expectation set in PEP 249

Specifically point 7 and point 9.

Thanks

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. cla: yes This human has signed the Contributor License Agreement. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants