-
Notifications
You must be signed in to change notification settings - Fork 18
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
IGNITE-12867 DBAPI support #39
base: master
Are you sure you want to change the base?
Conversation
5ee99e1
to
74e7785
Compare
@@ -0,0 +1,281 @@ | |||
# |
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.
I am strongly against to put dependency on sqlalchemy to thin client.
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.
I don't think it should be a mandatory requirement, but support for SQLAlchemy is important. I may yet break this out into a separate PR.
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.
I suppose that it should be additional module. Just start separate module in separate repo.
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.
We can of course rearrange repo, but it is not a good idea, imho. Currently there is not any options to make it "optional". Just create new module pyignite-sqlalchemy and develope it independently.
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.
And sqlalchemy is not important at all, it is just one of many others ORM, and you can barely find any db driver, that contains anything related to ORM in main repo
""" | ||
self._check_query_started() | ||
|
||
if size is None: |
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.
I suppose you should use low level API
pyignite.api.sql.sql_fields
pyignite.api.sql.sql_fields_cursor_get_page
You can open cursor using the first call and retrieve next pages using the next call. Please, see how current cursors are implemented.
pyignite/dbapi/__init__.py
Outdated
@@ -0,0 +1,87 @@ | |||
# | |||
# Copyright 2021 GridGain Systems, Inc. and Contributors. |
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.
Could you please change file header to appropriate one
threadsafety = 2 | ||
paramstyle = 'qmark' | ||
|
||
def connect(dsn=None, |
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.
What if ignite node is out? We should support multiple nodes
Still a way to go, but basically works.