fix: POSTCOMPILE expansion in SQLAlchemy 1.4.27+ #408

jethron · 2022-02-03T09:39:15Z

Handle the new double underscore prefix for POSTCOMPILE variables introduced in SQLAlchemy 1.4.27 onwards. (See commit sqlalchemy#f79df12bd6d99b8f6f09d4bf07722638c4b4c159)

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
Ensure the tests and linter pass
Code coverage does not decrease (if any source code was changed)
Appropriate docs were updated (if necessary)

Related to #385 🦕

Actually encountered this issue in pybigquery (dependency for great_expectations), not sure if there's a backporting process or if that's just completely unsupported now?

tswast · 2022-02-10T23:59:37Z

Actually encountered this issue in pybigquery (dependency for great_expectations), not sure if there's a backporting process or if that's just completely unsupported now?

We don't have the resources to backport to pybigquery.

jethron · 2022-02-11T11:03:27Z

Actually encountered this issue in pybigquery (dependency for great_expectations), not sure if there's a backporting process or if that's just completely unsupported now?

We don't have the resources to backport to pybigquery.

All good, thought I'd ask. :)

Not sure what the deal is with the compliance tests. Pushed a commit that gets it at least running for 3.8 on my local but not sure it'll pass... Any advice?

tswast · 2022-02-15T15:32:02Z

Not sure what the deal is with the compliance tests. Pushed a commit that gets it at least running for 3.8 on my local but not sure it'll pass... Any advice?

I've got a fix coming in #401 The older versions of SQLAlchemy aren't compatible with pytest 7+. Just waiting on a teammate to review the PR.

jethron · 2022-02-16T02:51:18Z

Ah, I see!
Thanks, force pushed again without that commit.

tswast · 2022-02-17T18:30:37Z

@jethron It looks like you haven't enabled "allow commits from maintainers".

Could you sync with main and also update setup.py to allow SQLAlchemy 1.4.27?

Handle the new double underscore prefix for POSTCOMPILE variables introduced in this version.

jethron · 2022-02-17T22:37:31Z

Sorry @tswast, TIL isaacs/github#1681 will fork to my user account next time.

Rebased and updated setup.py. I just went to 1.4.27 even though I think this should work for higher too, but not sure if that will add further incompitabilities or not.

jethron · 2022-02-18T06:15:38Z

Think that fixes the failing test. Not positive on what that line does, so I've just wrapped it in the same if-guard for 1.4.27+. Can I get another force-run?

tswast · 2022-02-18T14:57:31Z

sqlalchemy_bigquery/base.py

@@ -523,7 +528,8 @@ def visit_bindparam(

        if bindparam.expanding:
            assert_(self.__expanded_param(param), f"Unexpected param: {param}")
-            param = param.replace(")", f":{bq_type})")
+            if self.__sqlalchemy_version_info < (1, 4, 27):
+                param = param.replace(")", f":{bq_type})")


I'll need to take a closer look at this. I believe this is required to pass in the type information about sometimes ambiguous query parameters.

Looking at the failing test, there ends up with an extra trailing parentheses and the type information doesn't appear to have been extracted.

[SQL: select id FROM some_table WHERE z IN (%(q_1)s, %(q_2)s, %(q_3)s:STRING) ORDER BY id]

It does seem this change is necessary, but I wish I understood what had changed in 1.4.27 that requires it.

tswast · 2022-02-18T16:11:17Z

sqlalchemy_bigquery/base.py

@@ -523,7 +528,8 @@ def visit_bindparam(

        if bindparam.expanding:
            assert_(self.__expanded_param(param), f"Unexpected param: {param}")
-            param = param.replace(")", f":{bq_type})")
+            if self.__sqlalchemy_version_info < (1, 4, 27):


Looks like we're missing unit test coverage of entering the if bindparam.expanding if statement, but SQLAlchemy >= 1.4.27

sqlalchemy_bigquery/base.py 481 0 172 1 99% 531->541

I would have thought that the update to setup.py would make sure the tests run, though. 🤔

tswast · 2022-02-18T16:13:49Z

Compliance tests and system tests are passing! 🎉 Thanks so much.

Hopefully we can figure out the coverage issue. If not, I wouldn't be too opposed to a # pragma: NO COVER comment

tswast · 2022-02-22T16:57:36Z

Thanks so much for the contribution @jethron ! I'm planning on releasing these changes today in 1.4.0 #415

jethron requested a review from a team February 3, 2022 09:39

jethron requested a review from a team as a code owner February 3, 2022 09:39

jethron requested a review from tswast February 3, 2022 09:39

product-auto-label bot added the api: bigquery Issues related to the googleapis/python-bigquery-sqlalchemy API. label Feb 3, 2022

tswast added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 11, 2022

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 11, 2022

jethron changed the title ~~Fix POSTCOMPILE expansion in SQLAlchemy 1.4.27+~~ fix: POSTCOMPILE expansion in SQLAlchemy 1.4.27+ Feb 11, 2022

jethron added 2 commits February 18, 2022 09:25

fix: POSTCOMPILE expansion in SQLAlchemy 1.4.27+

256e044

Handle the new double underscore prefix for POSTCOMPILE variables introduced in this version.

build: Widen sqlalchemy version support to include 1.4.27

ed93927

tswast added kokoro:force-run Add this label to force Kokoro to re-run the tests. owlbot:run Add this label to trigger the Owlbot post processor. labels Feb 17, 2022

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Feb 17, 2022

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 17, 2022

tswast approved these changes Feb 17, 2022

View reviewed changes

fix: IN expansion compliance test

9133c20

tswast requested changes Feb 18, 2022

View reviewed changes

tswast added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 18, 2022

yoshi-kokoro removed kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Feb 18, 2022

tswast reviewed Feb 18, 2022

View reviewed changes

tswast added the owlbot:run Add this label to trigger the Owlbot post processor. label Feb 18, 2022

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Feb 18, 2022

fix(coverage): skip expansion coverage checks

5724486

tswast added kokoro:force-run Add this label to force Kokoro to re-run the tests. owlbot:run Add this label to trigger the Owlbot post processor. labels Feb 22, 2022

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Feb 22, 2022

tswast approved these changes Feb 22, 2022

View reviewed changes

yoshi-kokoro removed kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Feb 22, 2022

tswast merged commit 7844813 into googleapis:main Feb 22, 2022

release-please bot mentioned this pull request Feb 22, 2022

chore(main): release 1.4.0 #415

Merged

jethron deleted the expansion-fix branch February 23, 2022 12:55

tswast mentioned this pull request May 25, 2022

support SQLAlchemy 1.4.28+ #385

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: POSTCOMPILE expansion in SQLAlchemy 1.4.27+ #408

fix: POSTCOMPILE expansion in SQLAlchemy 1.4.27+ #408

jethron commented Feb 3, 2022 •

edited

Loading

tswast commented Feb 10, 2022

jethron commented Feb 11, 2022

tswast commented Feb 15, 2022

jethron commented Feb 16, 2022

tswast commented Feb 17, 2022

jethron commented Feb 17, 2022

jethron commented Feb 18, 2022

tswast Feb 18, 2022

tswast Feb 18, 2022

tswast Feb 18, 2022

tswast commented Feb 18, 2022

tswast commented Feb 22, 2022

fix: POSTCOMPILE expansion in SQLAlchemy 1.4.27+ #408

fix: POSTCOMPILE expansion in SQLAlchemy 1.4.27+ #408

Conversation

jethron commented Feb 3, 2022 • edited Loading

tswast commented Feb 10, 2022

jethron commented Feb 11, 2022

tswast commented Feb 15, 2022

jethron commented Feb 16, 2022

tswast commented Feb 17, 2022

jethron commented Feb 17, 2022

jethron commented Feb 18, 2022

tswast Feb 18, 2022

Choose a reason for hiding this comment

tswast Feb 18, 2022

Choose a reason for hiding this comment

tswast Feb 18, 2022

Choose a reason for hiding this comment

tswast commented Feb 18, 2022

tswast commented Feb 22, 2022

jethron commented Feb 3, 2022 •

edited

Loading