-
-
Notifications
You must be signed in to change notification settings - Fork 18.1k
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
EHN: allow for to_sql multi
method with oracle backend
#51648
EHN: allow for to_sql multi
method with oracle backend
#51648
Conversation
4aa6162
to
716862a
Compare
Any chance that we can add tests for this? |
I'd gladly write tests, but I have no idea how. I can try mocking a SQLAlchemy db connection with an oracle back-end and then checking if the generated SQL is valid. Would that be OK? I presume the test would go somewhere in here? |
stmt = insert(self.table).values(data) | ||
result = conn.execute(stmt) | ||
stmt = insert(self.table) | ||
result = conn.execute(stmt, data) |
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.
Why does this have to be different for Oracle specifically? I thought sqlalchemy should have made this call agnostic to the DB flavor?
Also in regards to testing, we don't really have the capabilities to test oracle but not opposed to accepting as is.
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.
Hi, this is different for Oracle specifically since insert(self.table).values(date)
instructs SQLAlchemy to produce a multi-row insert statement (e.g. INSERT INTO some_table(col1, col2) VALUES (1, 2), (3, 4))
, which are only supported by Oracle 23c+, while passing the insert(self.table)
statement in with the data to conn.execute
produces an executemany
call that Oracle supports.
I've merged main into my branch, could we reopen please?
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 add a comment about the Oracle case why .values(data)
isn't used here?
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 have done that
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.
Looks like there's a merge conflict and some pre-commit errors
Thanks for the pull request, but it appears to have gone stale. If interested in continuing, please merge in the main branch, address any review comments and/or failing tests, and we can reopen. |
@mroeschke I tried my best to address your question and merged in the main. Could we reopen, please? |
7e11605
to
1667b3c
Compare
Thanks @jacadzaca |
doc/source/whatsnew/v2.2.0.rst
file if fixing a bug or adding a new feature.