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

Add visit_try_cast to sqlalchemy compiler #474

Merged
merged 1 commit into from
Aug 29, 2024
Merged

Conversation

dwolfeu
Copy link
Member

@dwolfeu dwolfeu commented Jul 18, 2024

Description

I have added a visit_try_cast() method to trino.sqlalchemy.compiler.TrinoSQLCompiler. This fixes issue 473.

Non-technical explanation

This allows Trino's try_cast() to be used in SQLAlchemy.

Release notes

(x) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

Copy link

cla-bot bot commented Jul 18, 2024

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

@dwolfeu
Copy link
Member Author

dwolfeu commented Jul 18, 2024

I just emailed the signed CLA.

@hovaesco
Copy link
Member

Thanks for your contribution! Could you please add a test for it?

@dwolfeu
Copy link
Member Author

dwolfeu commented Jul 18, 2024

Sure! I should add one here, right?

@hovaesco
Copy link
Member

Sure! I should add one here, right?

Yes, correct.

Copy link

cla-bot bot commented Jul 18, 2024

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

@dwolfeu
Copy link
Member Author

dwolfeu commented Jul 18, 2024

Thanks for your contribution! Could you please add a test for it?

I have added a unit test.

@dwolfeu
Copy link
Member Author

dwolfeu commented Jul 23, 2024

@hovaesco hovaesco requested a review from hashhar July 23, 2024 07:01
@hovaesco
Copy link
Member

@cla-bot check

@cla-bot cla-bot bot added the cla-signed label Jul 23, 2024
Copy link

cla-bot bot commented Jul 23, 2024

The cla-bot has been summoned, and re-checked this pull request!

@hashhar
Copy link
Member

hashhar commented Aug 2, 2024

@dwolfeu Please make test conditional to run only with SQLAlchemy 2.x since it seems 1.x doesn't have try_cast? (or maybe it needs to be imported differently?).

@dwolfeu dwolfeu force-pushed the try_cast branch 2 times, most recently from 9afd094 to 39eb496 Compare August 3, 2024 13:37
@dwolfeu
Copy link
Member Author

dwolfeu commented Aug 3, 2024

@dwolfeu Please make test conditional to run only with SQLAlchemy 2.x since it seems 1.x doesn't have try_cast? (or maybe it needs to be imported differently?).

@hashhar Thanks for reviewing this PR. I have added an exception to the unit test. Three tests still fail, but that seems to be unrelated to my changes.

@hashhar
Copy link
Member

hashhar commented Aug 5, 2024

thanks, those are indeed unrelated.

@hovaesco can you help take a look at the types-all installation failure (seems it's been unmaintained for very long) - https://github.com/asottile-archive/types-all for the CI checks step.

The PyPy failures we can tackle separately since it's about Kerberos + PyPy incompat.

@dwolfeu
Copy link
Member Author

dwolfeu commented Aug 13, 2024

@hashhar @hovaesco Would it be possible to resolve this PR by the weekend or is that being too impatient?

Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

I've squashed (logically a single change) and rebased on master (to fix CI).

Will merge, sorry for the slow work.

@hashhar
Copy link
Member

hashhar commented Aug 29, 2024

Fixed some linter errors.

@hashhar hashhar merged commit 97c5a7f into trinodb:master Aug 29, 2024
12 checks passed
@dwolfeu
Copy link
Member Author

dwolfeu commented Aug 30, 2024

I've squashed (logically a single change) and rebased on master (to fix CI).

Will merge, sorry for the slow work.

Not at all! Thank you for the collaboration. :)

@dwolfeu dwolfeu deleted the try_cast branch August 30, 2024 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Using try_cast in SQLAlchemy
3 participants