Dbapi2 by artembo · Pull Request #161 · tarantool/tarantool-python

artembo · 2020-07-17T11:16:36Z

Added dbapi module accomplished according to pep-249 specification

Totktonada · 2020-08-18T22:27:44Z

It looks as the interesting feature. Thank you!

After the first very brief glance:

Please, keep Python 2 supported: CI should work for it. You can either support Python 2 in the database API implementation (it is preferred option) or disable the corresponding tests on Python 2.
Please, keep the history flat: rebase the patchset to get rid of the merge commit. (Don't hesitate to force-push your branch, it is fine for a short-term feature/bugfix branch.)
It would be good to have at least some basic test cases.
I would mention the relevant issue (Execute Sql Support #159) in the commit message, which resolves it and in the PR description.
Please, add a commit to remove Ubuntu Disco, it is EOL and fails in CI (within the patchset or in a separate PR, as you wish).

artembo · 2020-08-19T17:05:21Z

It would be good to have at least some basic test cases.

@Totktonada Many thanks for the review!
Could you please clarify what kind of tests do you mean? I used dbapi-compliance package which contains all the unit tests according to pep-249 specification.

Totktonada · 2020-08-19T21:29:31Z

Please, keep the history flat: rebase the patchset to get rid of the merge commit. (Don't hesitate to force-push your branch, it is fine for a short-term feature/bugfix branch.)

I didn't meant squashing all changes into one commit, but get rid of merge commits. It is better to keep commits atomic.

It would be good to have at least some basic test cases.

<...>
Could you please clarify what kind of tests do you mean? I used dbapi-compliance package which contains all the unit tests according to pep-249 specification.

My bad, I missed the inheritance.

Totktonada · 2020-08-19T22:17:47Z

I didn't meant squashing all changes into one commit, but get rid of merge commits. It is better to keep commits atomic.

That's my bad too. I see 'Dbapi2 (#160)' commit and had thought that it is the merge commit. Now I see that it is just usual commit.

Totktonada · 2020-08-20T20:44:08Z

Pushed the .gitignore update (e49f5f0) and the Ubuntu Disco removal (21e3ebf) to master.

Totktonada

I looked over first two commits and over commit messages. Please, consider comments below. I'll continue with looking into the Database API implementation itself.

Comments re commit messages

General:

Please, keep commit message body within 72 symbols.

Regarding 'Set auth request schema to zero in req header':

There is no description of the problem that is resolved here. Just in case, it is the following. It is possible that a database schema is changed on the server during reconnect (or between a last request and reconnect) and the client tries to connect with an old schema id. It is not (generally) possible to fetch the new schema before authentication, so the only way to reconnect is to send authentication request without schema id. Since we don't use any information of the schema, it is harmless.

However it is already in master, see aa8f115 and #141. let's drop the commit.

Regarding 'Add sql execute to connection':

I would close the relevant issue from the commit message: #159.

Regarding 'Add PEP-249 dbapi module (#160)':

Unit tests were taken from dbapi-compliance [3] package.
https://github.com/baztian/dbapi-compliance/

But it is added in the next commit, not here.

will be added in the connector when the feature is stable it Tarantool itself

is stable -> will be stable?

Regarding 'Add unit tests for dbapi module':

removed EOL python 3.4 from appveyor test

This is not actual anymore.

used installer.sh from tarantool.io to set up Tarantool 2.4 version in test.sh

Is it necessary to change the version? If so, a reason should be explained. However I would rather keep everything as is here and test all versions after #124.

BTW, are those tests skipped when we're run testing against 1.10? It seems no, but should.

Totktonada

Comments regarding changes for the base connector, but not dbapi.py itself (commit 'Add PEP-249 dbapi module (#160)').

BTW, I would remove the link to PR #160 from the commit header, because it refers to a draft implementation. I guess it is not what a reader would expect when (s)he will see this link within master branch.

Totktonada

Comments re dbapi.py (commit 'Add PEP-249 dbapi module (#160)').

I would not consider it as thorough review. I proposed many changes and it would be better to look again after your modifications. Let's ping me, when the PR will be ready.

While looking into this, I marked a status of the standard support. It is below. Please, note that it is not exhaustive status, just my random notes.

PEP 249 Database API

Support status

Core features

!! <cursor>.description (result set metadata) is not supported yet.

!! No types are defined.

Cursors are emulated.

Prepared statements are emulated.

Transactions are not supported (not supported in tarantool).

Optional <cursor>.callproc() and <cursor>.nextset() are no supported (no SQL/PMS support in tarantool).

Extensions

Supported:

"DB-API extension connection. used"
"DB-API extension cursor.lastrowid used"

Unsupported:

TBD

Totktonada · 2020-08-22T06:33:23Z

Regarding 'set msgpack max version to 0.5.6 for test environment and deps' commit: I think it is quite simple to finish PR #156, this would be better than the workaround.

Totktonada

Looked over the first three commits after the long delay. No objections in general.

Totktonada

Comments for 'Add pep-249 dbapi module'.

I think it is in the good shape.

Closes #159 Co-authored-by: Denis Ignatenko <[email protected]>

It is set up to be converted to lists by default. Django expects row type to be tuple. Also tuples tend to perform better than lists and it is good if it can be configurable. Closes #166

See [1] for details. The main motivation for the module creation was the integration Django with Tarantool database through django-tarantool database backend [2] which requires dbapi connector for the database. The most of the optional extensions and methods were ignored because Django does not require them. Anyway, feel free to suggest its implementation as needed. Interactive transactions are not currently supported by Tarantool and theirs implementation will be added in the connector when the feature is stable in Tarantool itself. [1] https://www.python.org/dev/peps/pep-0249/ [2] https://github.com/artembo/django-tarantool Co-authored-by: Denis Ignatenko <[email protected]>

Used dbapi-compliance [1] package to test module according to pep-249 specification. Not implemented features are skipped in the tests. Added dbapi-compliance package to test.sh requirements and appveyor.yml [1] https://github.com/baztian/dbapi-compliance/

Totktonada

I didn't look again over the whole patchset (sorry), but since all comments from the previous review was fixed, I think, everything should be okay now.

LGTM.

Totktonada · 2020-10-30T19:38:51Z

I'm glad that you found strengths to finish the work on the Database API support. It required significant effort, but you accepted the challenge and wins!

artembo force-pushed the dbapi2 branch from 3ffd484 to d98b0e9 Compare July 17, 2020 13:51

artembo linked an issue Aug 16, 2020 that may be closed by this pull request

Execute Sql Support #159

Closed

artembo requested a review from Totktonada August 16, 2020 10:33

artembo self-assigned this Aug 16, 2020

artembo force-pushed the dbapi2 branch 3 times, most recently from 3a6f812 to 69fffb1 Compare August 19, 2020 16:43

artembo force-pushed the dbapi2 branch 2 times, most recently from 14d3b62 to ab1314a Compare August 19, 2020 18:04

artembo force-pushed the dbapi2 branch from ab1314a to ec49ff7 Compare August 19, 2020 21:46

artembo force-pushed the dbapi2 branch from ec49ff7 to 1d34fbc Compare August 19, 2020 23:04

artembo marked this pull request as ready for review August 20, 2020 06:59

artembo force-pushed the dbapi2 branch 6 times, most recently from 08bfd6d to 9eb8832 Compare August 21, 2020 09:51

Totktonada reviewed Aug 21, 2020

View reviewed changes

Totktonada reviewed Aug 22, 2020

View reviewed changes

Comment thread tarantool/connection.py Resolved

artembo marked this pull request as draft August 22, 2020 17:27

artembo force-pushed the dbapi2 branch from 9eb8832 to 6b4c050 Compare August 22, 2020 17:28

artembo force-pushed the dbapi2 branch 2 times, most recently from 89223e7 to 6f43c09 Compare September 1, 2020 17:05

Totktonada mentioned this pull request Sep 2, 2020

Enlight pathways for new contributors tarantool/doc#1444

Open

artembo force-pushed the dbapi2 branch from 3662592 to 680a6e9 Compare September 6, 2020 21:00

artembo marked this pull request as ready for review September 6, 2020 21:01

artembo requested a review from Totktonada September 6, 2020 21:01

artembo force-pushed the dbapi2 branch 2 times, most recently from 908d382 to 92c71b3 Compare September 7, 2020 09:54

artembo force-pushed the dbapi2 branch 3 times, most recently from af23872 to 4f51516 Compare October 18, 2020 20:31

artembo force-pushed the dbapi2 branch from 4f51516 to 07b8109 Compare October 26, 2020 11:06

Totktonada reviewed Oct 26, 2020

View reviewed changes

Comment thread tarantool/connection.py Resolved

Comment thread tarantool/request.py Resolved

Comment thread unit/suites/test_execute.py Resolved

Totktonada reviewed Oct 26, 2020

View reviewed changes

Comment thread test.sh Resolved

Comment thread unit/suites/test_dbapi.py Resolved

Comment thread unit/suites/test_dbapi.py Resolved

Comment thread unit/suites/test_dbapi.py Resolved

Totktonada reviewed Oct 26, 2020

View reviewed changes

Comment thread tarantool/dbapi.py Resolved

Comment thread tarantool/dbapi.py Resolved

Comment thread tarantool/dbapi.py Resolved

Comment thread tarantool/dbapi.py Resolved

Comment thread tarantool/dbapi.py Resolved

artembo force-pushed the dbapi2 branch 2 times, most recently from b14844e to d308fa5 Compare October 28, 2020 20:02

Totktonada reviewed Oct 30, 2020

View reviewed changes

Comment thread unit/suites/test_execute.py Resolved

artembo and others added 4 commits October 30, 2020 18:46

Add sql execute to Connection

c529767

Closes #159 Co-authored-by: Denis Ignatenko <[email protected]>

Make use_list param configurable

00d315b

It is set up to be converted to lists by default. Django expects row type to be tuple. Also tuples tend to perform better than lists and it is good if it can be configurable. Closes #166

artembo force-pushed the dbapi2 branch from d308fa5 to 5160aaf Compare October 30, 2020 16:09

Totktonada approved these changes Oct 30, 2020

View reviewed changes

Totktonada merged commit 1fff812 into master Oct 30, 2020

kyukhin added teamE prio6 labels Nov 19, 2020

DifferentialOrange deleted the dbapi2 branch November 9, 2022 10:35

Conversation

artembo commented Jul 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Totktonada commented Aug 18, 2020

Uh oh!

artembo commented Aug 19, 2020

Uh oh!

Totktonada commented Aug 19, 2020

Uh oh!

Totktonada commented Aug 19, 2020

Uh oh!

Totktonada commented Aug 20, 2020

Uh oh!

Totktonada left a comment

Choose a reason for hiding this comment

Comments re commit messages

General:

Regarding 'Set auth request schema to zero in req header':

Regarding 'Add sql execute to connection':

Regarding 'Add PEP-249 dbapi module (#160)':

Regarding 'Add unit tests for dbapi module':

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Totktonada left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Totktonada left a comment

Choose a reason for hiding this comment

PEP 249 Database API

Support status

Core features

Extensions

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Totktonada commented Aug 22, 2020

Uh oh!

Totktonada left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Totktonada left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

artembo commented Jul 17, 2020 •

edited

Loading