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

SQLAlchemy: Ignore SQL's "FOR UPDATE" clause #584

Merged
merged 1 commit into from
Sep 29, 2023

Conversation

surister
Copy link

@surister surister commented Sep 28, 2023

Summary of the changes / Why this is an improvement

Pretty straightforward, now queries like select(table).with_for_update() will not generate a FOR UPDATE statement.

Resolves #577.

Checklist

  • Link to issue this PR refers to (if applicable):
  • CLA is signed

Copy link
Member

@amotl amotl left a comment

Choose a reason for hiding this comment

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

Thank you very much!

src/crate/client/sqlalchemy/compiler.py Show resolved Hide resolved
@amotl amotl requested review from seut and matriv September 28, 2023 14:13
Copy link
Contributor

@matriv matriv left a comment

Choose a reason for hiding this comment

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

Thx a lot for the PR @surister!
I have the same consideration with #582. SELECT... FOR UPDATE has transactional semantics for traditional RDBMS which CRATEDB doesn't support, shouldn't we somehow make inform the user about an unsupported query (log warning message) or at least add this in docs?

Comment on lines 296 to 318
def for_update_clause(self, select, **kw):
# CrateDB does not support currently 'FOR UPDATE' statements.
# See https://github.com/crate/crate-python/issues/577
return ''
Copy link
Member

@amotl amotl Sep 28, 2023

Choose a reason for hiding this comment

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

Good point about the user warning, @matriv. I've just added a corresponding improvement to the other patch with 154d77a. If you would be so kind to adjust the patch like that, @surister? 🙏

I think it will be good like this. If you insist on adding it also to the documentation, I can take this, so we do not need to bother @surister about finding a good spot ;].

Copy link
Contributor

@matriv matriv Sep 28, 2023

Choose a reason for hiding this comment

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

I think that logging a warning is enough, mainly for those users that quickly try out CrateDB as their backend (switching from some other DB), to be informed.

People working with CrateDB should already know it doesn't support transactions, so adding a docs entry about that to the python client is a bit too much, so I think it's fine to just log a warning.

Copy link
Member

@amotl amotl Sep 28, 2023

Choose a reason for hiding this comment

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

Thanks. I've amended the patch by adding a similar warning like on the other one.

@amotl
Copy link
Member

amotl commented Sep 28, 2023

Dear @surister,

after bringing in GH-582 and GH-583, I am going to wrap this up, if you don't have any objections. Users are waiting for another release, and my intention was only to bring in a few small maintenance updates and little improvements like this.

Your contribution was a perfect fit for that, and arrived just in time. Thank you again.

With kind regards,
Andreas.

@amotl amotl force-pushed the ignore_for_update branch 2 times, most recently from 49fb8c2 to 3ba3bc8 Compare September 28, 2023 19:28
@amotl amotl requested a review from matriv September 28, 2023 19:29
@amotl amotl changed the title SQLAlchemy: Ignore SQL's "FOR UPDATE" clase SQLAlchemy: Ignore SQL's "FOR UPDATE" clause Sep 28, 2023
src/crate/client/sqlalchemy/compiler.py Outdated Show resolved Hide resolved
Copy link
Contributor

@matriv matriv left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you both @surister and @amotl

@amotl amotl merged commit c1ac679 into crate:master Sep 29, 2023
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SQLAlchemy: Ignore FOR UPDATE clauses
4 participants