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: Implementing client side statements in dbapi (starting with commit) #1037

Merged
merged 8 commits into from Nov 23, 2023

Conversation

ankiaga
Copy link
Contributor

@ankiaga ankiaga commented Nov 17, 2023

Implementation to Commit the ongoing transaction whenever execute() method of Cursor is called with a sql query "COMMIT" (or "COMMIT TRANSACTION")
The plan is to later add more client side statements

@ankiaga ankiaga requested review from a team as code owners November 17, 2023 09:25
@product-auto-label product-auto-label bot added size: m Pull request size is medium. api: spanner Issues related to the googleapis/python-spanner API. labels Nov 17, 2023
@ankiaga ankiaga marked this pull request as draft November 17, 2023 09:25
@product-auto-label product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Nov 20, 2023
@ankiaga ankiaga changed the title Implementing client side statement in dbapi starting with commit feat: Implementing client side statements in dbapi (starting with commit) Nov 20, 2023
Copy link

conventional-commit-lint-gcf bot commented Nov 20, 2023

🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use automerge label. Good luck human!

-- conventional-commit-lint bot
https://conventionalcommits.org/

@ankiaga ankiaga marked this pull request as ready for review November 20, 2023 07:35
Copy link

@aseering aseering left a comment

Choose a reason for hiding this comment

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

Thanks for putting this together! No major concerns but a bunch of little comments. Let me know what you think.

google/cloud/spanner_dbapi/client_side_statement_parser.py Outdated Show resolved Hide resolved
tests/system/test_dbapi.py Show resolved Hide resolved
tests/system/test_dbapi.py Show resolved Hide resolved
google/cloud/spanner_dbapi/parse_utils.py Show resolved Hide resolved
google/cloud/spanner_dbapi/cursor.py Outdated Show resolved Hide resolved
@ankiaga
Copy link
Contributor Author

ankiaga commented Nov 22, 2023

Thanks @aseering for taking out time and providing valuable comments. Got to learn a lot from your comments

@ankiaga ankiaga added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 23, 2023
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 23, 2023
@ankiaga ankiaga merged commit eb41b0d into googleapis:main Nov 23, 2023
14 of 15 checks passed
@@ -174,12 +177,11 @@
RE_PYFORMAT = re.compile(r"(%s|%\([^\(\)]+\)s)+", re.DOTALL)


@deprecated(reason="This method is deprecated. Use _classify_stmt method")
Copy link
Contributor

Choose a reason for hiding this comment

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

this pulls in two new dependencies (deprecated, wrapt) into this library -- this can be replaced entirely with this inside the function body:

warnings.warn("This method is deprecated.  Use classify_statement method instead", stacklevel=2)

this uses warnings directly avoiding the two new dependencies (including one which has a C extension!)

Copy link
Contributor

Choose a reason for hiding this comment

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

would one be open to a PR to do this instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

@ankiaga Would you mind taking a look what would be the best solution here?

Copy link
Contributor

Choose a reason for hiding this comment

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

threw one together: #1120

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.

None yet

5 participants