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.

Sign up for GitHub

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

fix: POSTCOMPILE expansion in SQLAlchemy 1.4.27+ #408

Merged
merged 4 commits into from
Feb 22, 2022
Merged

fix: POSTCOMPILE expansion in SQLAlchemy 1.4.27+ #408

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
256e044
fix: POSTCOMPILE expansion in SQLAlchemy 1.4.27+
jethron Feb 3, 2022
ed93927
build: Widen sqlalchemy version support to include 1.4.27
jethron Feb 17, 2022
9133c20
fix: IN expansion compliance test
jethron Feb 18, 2022
5724486
fix(coverage): skip expansion coverage checks
jethron Feb 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ def readme():
# https://github.com/googleapis/python-bigquery-sqlalchemy/issues/386
# and
# https://github.com/googleapis/python-bigquery-sqlalchemy/issues/385
"sqlalchemy>=1.2.0,<=1.4.26",
"sqlalchemy>=1.2.0,<=1.4.27",
"future",
],
extras_require=extras,
Expand Down
18 changes: 12 additions & 6 deletions sqlalchemy_bigquery/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -341,16 +341,19 @@ def group_by_clause(self, select, **kw):

__sqlalchemy_version_info = tuple(map(int, sqlalchemy.__version__.split(".")))

__expandng_text = (
__expanding_text = (
"EXPANDING" if __sqlalchemy_version_info < (1, 4) else "POSTCOMPILE"
)

# https://github.com/sqlalchemy/sqlalchemy/commit/f79df12bd6d99b8f6f09d4bf07722638c4b4c159
__expanding_conflict = "" if __sqlalchemy_version_info < (1, 4, 27) else "__"

__in_expanding_bind = _helpers.substitute_string_re_method(
fr"""
\sIN\s\( # ' IN ('
(
\[ # Expanding placeholder
{__expandng_text} # e.g. [EXPANDING_foo_1]
{__expanding_conflict}\[ # Expanding placeholder
{__expanding_text} # e.g. [EXPANDING_foo_1]
_[^\]]+ #
\]
(:[A-Z0-9]+)? # type marker (e.g. ':INT64'
Expand Down Expand Up @@ -431,7 +434,9 @@ def visit_notendswith_op_binary(self, binary, operator, **kw):

__placeholder = re.compile(r"%\(([^\]:]+)(:[^\]:]+)?\)s$").match

__expanded_param = re.compile(fr"\(\[" fr"{__expandng_text}" fr"_[^\]]+\]\)$").match
__expanded_param = re.compile(
fr"\({__expanding_conflict}\[" fr"{__expanding_text}" fr"_[^\]]+\]\)$"
).match

__remove_type_parameter = _helpers.substitute_string_re_method(
r"""
Expand Down Expand Up @@ -521,9 +526,10 @@ def visit_bindparam(

assert_(param != "%s", f"Unexpected param: {param}")

if bindparam.expanding:
if bindparam.expanding: # pragma: NO COVER
assert_(self.__expanded_param(param), f"Unexpected param: {param}")
param = param.replace(")", f":{bq_type})")
if self.__sqlalchemy_version_info < (1, 4, 27):
Copy link
Collaborator

Choose a reason for hiding this comment

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

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. 🤔

param = param.replace(")", f":{bq_type})")
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.


else:
m = self.__placeholder(param)
Expand Down