From 8dcb7a3101c2e4bcfbf0977de011b0a849c1361c Mon Sep 17 00:00:00 2001 From: Jim Fulton Date: Fri, 23 Jul 2021 13:05:13 -0400 Subject: [PATCH 1/9] Add last_query fixture to make it easier to look at recent queries --- tests/unit/conftest.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py index 1c78b12d..e9af45db 100644 --- a/tests/unit/conftest.py +++ b/tests/unit/conftest.py @@ -71,6 +71,17 @@ def ex(sql, *args, **kw): conn.close() +@pytest.fixture() +def last_query(faux_conn): + + def last_query(sql, params=None, offset=1): + actual_sql, actual_params = faux_conn.test_data["execute"][-offset] + assert actual_sql == sql + if params is not None: + assert actual_params == params + + return last_query + @pytest.fixture() def metadata(): return sqlalchemy.MetaData() From 17f050796d9bccd3584797843a35bc1dfd2a8347 Mon Sep 17 00:00:00 2001 From: Jim Fulton Date: Tue, 17 Aug 2021 10:13:57 -0400 Subject: [PATCH 2/9] Don't expand in lists for SQLAlchemy 1.4 --- sqlalchemy_bigquery/base.py | 63 ++++++----- tests/system/test_sqlalchemy_bigquery.py | 21 ++++ tests/unit/conftest.py | 3 +- tests/unit/fauxdbi.py | 18 ++- tests/unit/test_select.py | 133 +++++++++++------------ 5 files changed, 136 insertions(+), 102 deletions(-) diff --git a/sqlalchemy_bigquery/base.py b/sqlalchemy_bigquery/base.py index db7336f6..0816b69e 100644 --- a/sqlalchemy_bigquery/base.py +++ b/sqlalchemy_bigquery/base.py @@ -400,6 +400,13 @@ def visit_bindparam( skip_bind_expression=False, **kwargs, ): + unnest = False + if (bindparam.expanding and not isinstance(bindparam.type, NullType)): + if getattr(bindparam, 'expand_op', None) is not None: + assert bindparam.expand_op.__name__.endswith('in_op') # in in + bindparam.expanding = False + unnest = True + param = super(BigQueryCompiler, self).visit_bindparam( bindparam, within_columns_clause, @@ -409,40 +416,42 @@ def visit_bindparam( ) type_ = bindparam.type - if isinstance(type_, NullType): - return param + if not isinstance(type_, NullType): - if ( - isinstance(type_, Numeric) - and (type_.precision is None or type_.scale is None) - and isinstance(bindparam.value, Decimal) - ): - t = bindparam.value.as_tuple() + if ( + isinstance(type_, Numeric) + and (type_.precision is None or type_.scale is None) + and isinstance(bindparam.value, Decimal) + ): + t = bindparam.value.as_tuple() - if type_.precision is None: - type_.precision = len(t.digits) + if type_.precision is None: + type_.precision = len(t.digits) - if type_.scale is None and t.exponent < 0: - type_.scale = -t.exponent + if type_.scale is None and t.exponent < 0: + type_.scale = -t.exponent - bq_type = self.dialect.type_compiler.process(type_) - if bq_type[-1] == ">" and bq_type.startswith("ARRAY<"): - # Values get arrayified at a lower level. - bq_type = bq_type[6:-1] - bq_type = self.__remove_type_parameter(bq_type) + bq_type = self.dialect.type_compiler.process(type_) + if bq_type[-1] == ">" and bq_type.startswith("ARRAY<"): + # Values get arrayified at a lower level. + bq_type = bq_type[6:-1] + bq_type = self.__remove_type_parameter(bq_type) - assert_(param != "%s", f"Unexpected param: {param}") + assert_(param != "%s", f"Unexpected param: {param}") - if bindparam.expanding: - assert_(self.__expanded_param(param), f"Unexpected param: {param}") - param = param.replace(")", f":{bq_type})") + if bindparam.expanding: + assert_(self.__expanded_param(param), f"Unexpected param: {param}") + param = param.replace(")", f":{bq_type})") - else: - m = self.__placeholder(param) - if m: - name, type_ = m.groups() - assert_(type_ is None) - param = f"%({name}:{bq_type})s" + else: + m = self.__placeholder(param) + if m: + name, type_ = m.groups() + assert_(type_ is None) + param = f"%({name}:{bq_type})s" + + if unnest: + param = f'UNNEST({param})' return param diff --git a/tests/system/test_sqlalchemy_bigquery.py b/tests/system/test_sqlalchemy_bigquery.py index 0fe878b2..e1c01c4a 100644 --- a/tests/system/test_sqlalchemy_bigquery.py +++ b/tests/system/test_sqlalchemy_bigquery.py @@ -34,6 +34,8 @@ import datetime import decimal +sqlalchemy_version_info = tuple(map(int, sqlalchemy.__version__.split("."))) + ONE_ROW_CONTENTS_EXPANDED = [ 588, @@ -686,3 +688,22 @@ def test_has_table(engine, engine_using_test_dataset, bigquery_dataset): assert engine_using_test_dataset.has_table(f"{bigquery_dataset}.sample") is True assert engine_using_test_dataset.has_table("sample_alt") is False + + +@pytest.mark.skipif( + sqlalchemy_version_info < (1, 4), + reason="requires sqlalchemy 1.4 or higher", +) +def test_huge_in(): + engine = sqlalchemy.create_engine("bigquery://") + conn = engine.connect() + try: + assert list(conn.execute( + sqlalchemy.select([sqlalchemy.literal(-1).in_(list(range(99999)))]) + )) == [(False,)] + except Exception as e: + error = True + else: + error = False + + assert not error, "execution failed" diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py index e9af45db..c71be56e 100644 --- a/tests/unit/conftest.py +++ b/tests/unit/conftest.py @@ -77,8 +77,7 @@ def last_query(faux_conn): def last_query(sql, params=None, offset=1): actual_sql, actual_params = faux_conn.test_data["execute"][-offset] assert actual_sql == sql - if params is not None: - assert actual_params == params + assert actual_params == params return last_query diff --git a/tests/unit/fauxdbi.py b/tests/unit/fauxdbi.py index 56c44e0f..597f8b01 100644 --- a/tests/unit/fauxdbi.py +++ b/tests/unit/fauxdbi.py @@ -261,11 +261,19 @@ def __handle_problematic_literal_inserts( else: return operation - __handle_unnest = substitute_string_re_method( - r"UNNEST\(\[ ([^\]]+)? \]\)", # UNNEST([ ... ]) - flags=re.IGNORECASE, - repl=r"(\1)", - ) + @substitute_re_method( + r""" + UNNEST\( + ( + \[ (?P[^\]]+)? \] # UNNEST([ ... ]) + | + ([?]) # UNNEST(?) + ) + \) + """, + flags=re.IGNORECASE | re.VERBOSE) + def __handle_unnest(self, m): + return '(' + (m.group('exp') or '?') + ')' def __handle_true_false(self, operation): # Older sqlite versions, like those used on the CI servers diff --git a/tests/unit/test_select.py b/tests/unit/test_select.py index 5d49ae68..aa110e34 100644 --- a/tests/unit/test_select.py +++ b/tests/unit/test_select.py @@ -27,6 +27,7 @@ from conftest import ( setup_table, + sqlalchemy_version_info, sqlalchemy_1_3_or_higher, sqlalchemy_1_4_or_higher, sqlalchemy_before_1_4, @@ -213,18 +214,6 @@ def test_disable_quote(faux_conn): assert faux_conn.test_data["execute"][-1][0] == ("SELECT `t`.foo \nFROM `t`") -def _normalize_in_params(query, params): - # We have to normalize parameter names, because they - # change with sqlalchemy versions. - newnames = sorted( - ((p, f"p_{i}") for i, p in enumerate(sorted(params))), key=lambda i: -len(i[0]) - ) - for old, new in newnames: - query = query.replace(old, new) - - return query, {new: params[old] for old, new in newnames} - - @sqlalchemy_before_1_4 def test_select_in_lit_13(faux_conn): [[isin]] = faux_conn.execute( @@ -239,63 +228,70 @@ def test_select_in_lit_13(faux_conn): @sqlalchemy_1_4_or_higher -def test_select_in_lit(faux_conn): - [[isin]] = faux_conn.execute( - sqlalchemy.select([sqlalchemy.literal(1).in_([1, 2, 3])]) - ) - assert isin - assert _normalize_in_params(*faux_conn.test_data["execute"][-1]) == ( - "SELECT %(p_0:INT64)s IN " - "UNNEST([ %(p_1:INT64)s, %(p_2:INT64)s, %(p_3:INT64)s ]) AS `anon_1`", - {"p_1": 1, "p_2": 2, "p_3": 3, "p_0": 1}, +def test_select_in_lit(faux_conn, last_query): + faux_conn.execute(sqlalchemy.select([sqlalchemy.literal(1).in_([1, 2, 3])])) + last_query( + 'SELECT %(param_1:INT64)s IN UNNEST(%(param_2:INT64)s) AS `anon_1`', + {'param_1': 1, 'param_2': [1, 2, 3]}, ) -def test_select_in_param(faux_conn): +def test_select_in_param(faux_conn, last_query): [[isin]] = faux_conn.execute( sqlalchemy.select( [sqlalchemy.literal(1).in_(sqlalchemy.bindparam("q", expanding=True))] ), dict(q=[1, 2, 3]), ) - assert isin - assert faux_conn.test_data["execute"][-1] == ( - "SELECT %(param_1:INT64)s IN UNNEST(" - "[ %(q_1:INT64)s, %(q_2:INT64)s, %(q_3:INT64)s ]" - ") AS `anon_1`", - {"param_1": 1, "q_1": 1, "q_2": 2, "q_3": 3}, - ) + if sqlalchemy_version_info >= (1, 4): + last_query('SELECT %(param_1:INT64)s IN UNNEST(%(q:INT64)s) AS `anon_1`', + {'param_1': 1, 'q': [1, 2, 3]}, + ) + else: + assert isin + last_query( + "SELECT %(param_1:INT64)s IN UNNEST(" + "[ %(q_1:INT64)s, %(q_2:INT64)s, %(q_3:INT64)s ]" + ") AS `anon_1`", + {"param_1": 1, "q_1": 1, "q_2": 2, "q_3": 3}, + ) -def test_select_in_param1(faux_conn): +def test_select_in_param1(faux_conn, last_query): [[isin]] = faux_conn.execute( sqlalchemy.select( [sqlalchemy.literal(1).in_(sqlalchemy.bindparam("q", expanding=True))] ), dict(q=[1]), ) - assert isin - assert faux_conn.test_data["execute"][-1] == ( - "SELECT %(param_1:INT64)s IN UNNEST(" "[ %(q_1:INT64)s ]" ") AS `anon_1`", - {"param_1": 1, "q_1": 1}, - ) + if sqlalchemy_version_info >= (1, 4): + last_query('SELECT %(param_1:INT64)s IN UNNEST(%(q:INT64)s) AS `anon_1`', + {'param_1': 1, 'q': [1]}, + ) + else: + assert isin + last_query( + "SELECT %(param_1:INT64)s IN UNNEST(" "[ %(q_1:INT64)s ]" ") AS `anon_1`", + {"param_1": 1, "q_1": 1}, + ) @sqlalchemy_1_3_or_higher -def test_select_in_param_empty(faux_conn): +def test_select_in_param_empty(faux_conn, last_query): [[isin]] = faux_conn.execute( sqlalchemy.select( [sqlalchemy.literal(1).in_(sqlalchemy.bindparam("q", expanding=True))] ), dict(q=[]), ) - assert not isin - assert faux_conn.test_data["execute"][-1] == ( - "SELECT %(param_1:INT64)s IN(NULL) AND (1 != 1) AS `anon_1`" - if sqlalchemy.__version__ >= "1.4" - else "SELECT %(param_1:INT64)s IN UNNEST([ ]) AS `anon_1`", - {"param_1": 1}, - ) + if sqlalchemy_version_info >= (1, 4): + last_query('SELECT %(param_1:INT64)s IN UNNEST(%(q:INT64)s) AS `anon_1`', + {'param_1': 1, 'q': []}) + else: + assert not isin + last_query( + "SELECT %(param_1:INT64)s IN UNNEST([ ]) AS `anon_1`", + {"param_1": 1}) @sqlalchemy_before_1_4 @@ -312,47 +308,48 @@ def test_select_notin_lit13(faux_conn): @sqlalchemy_1_4_or_higher -def test_select_notin_lit(faux_conn): - [[isnotin]] = faux_conn.execute( +def test_select_notin_lit(faux_conn, last_query): + faux_conn.execute( sqlalchemy.select([sqlalchemy.literal(0).notin_([1, 2, 3])]) ) - assert isnotin - - assert _normalize_in_params(*faux_conn.test_data["execute"][-1]) == ( - "SELECT (%(p_0:INT64)s NOT IN " - "UNNEST([ %(p_1:INT64)s, %(p_2:INT64)s, %(p_3:INT64)s ])) AS `anon_1`", - {"p_0": 0, "p_1": 1, "p_2": 2, "p_3": 3}, - ) + last_query( + 'SELECT (%(param_1:INT64)s NOT IN UNNEST(%(param_2:INT64)s)) AS `anon_1`', + {'param_1': 0, 'param_2': [1, 2, 3]}) -def test_select_notin_param(faux_conn): +def test_select_notin_param(faux_conn, last_query): [[isnotin]] = faux_conn.execute( sqlalchemy.select( [sqlalchemy.literal(1).notin_(sqlalchemy.bindparam("q", expanding=True))] ), dict(q=[1, 2, 3]), ) - assert not isnotin - assert faux_conn.test_data["execute"][-1] == ( - "SELECT (%(param_1:INT64)s NOT IN UNNEST(" - "[ %(q_1:INT64)s, %(q_2:INT64)s, %(q_3:INT64)s ]" - ")) AS `anon_1`", - {"param_1": 1, "q_1": 1, "q_2": 2, "q_3": 3}, - ) + if sqlalchemy_version_info >= (1, 4): + last_query( + 'SELECT (%(param_1:INT64)s NOT IN UNNEST(%(q:INT64)s)) AS `anon_1`', + {'param_1': 1, 'q': [1, 2, 3]}) + else: + assert not isnotin + last_query( + "SELECT (%(param_1:INT64)s NOT IN UNNEST(" + "[ %(q_1:INT64)s, %(q_2:INT64)s, %(q_3:INT64)s ]" + ")) AS `anon_1`", + {"param_1": 1, "q_1": 1, "q_2": 2, "q_3": 3}) @sqlalchemy_1_3_or_higher -def test_select_notin_param_empty(faux_conn): +def test_select_notin_param_empty(faux_conn, last_query): [[isnotin]] = faux_conn.execute( sqlalchemy.select( [sqlalchemy.literal(1).notin_(sqlalchemy.bindparam("q", expanding=True))] ), dict(q=[]), ) - assert isnotin - assert faux_conn.test_data["execute"][-1] == ( - "SELECT (%(param_1:INT64)s NOT IN(NULL) OR (1 = 1)) AS `anon_1`" - if sqlalchemy.__version__ >= "1.4" - else "SELECT (%(param_1:INT64)s NOT IN UNNEST([ ])) AS `anon_1`", - {"param_1": 1}, - ) + if sqlalchemy_version_info >= (1, 4): + last_query( + 'SELECT (%(param_1:INT64)s NOT IN UNNEST(%(q:INT64)s)) AS `anon_1`', + {'param_1': 1, 'q': []}) + else: + assert isnotin + last_query("SELECT (%(param_1:INT64)s NOT IN UNNEST([ ])) AS `anon_1`", + {"param_1": 1}) From ff2191853542d31b2d386964f2d296fcf895a365 Mon Sep 17 00:00:00 2001 From: Jim Fulton Date: Tue, 17 Aug 2021 11:44:21 -0400 Subject: [PATCH 3/9] blacken/lint --- sqlalchemy_bigquery/base.py | 8 ++-- tests/system/test_sqlalchemy_bigquery.py | 13 +++--- tests/unit/conftest.py | 2 +- tests/unit/fauxdbi.py | 5 ++- tests/unit/test_select.py | 55 +++++++++++++----------- 5 files changed, 46 insertions(+), 37 deletions(-) diff --git a/sqlalchemy_bigquery/base.py b/sqlalchemy_bigquery/base.py index 0816b69e..5c42c382 100644 --- a/sqlalchemy_bigquery/base.py +++ b/sqlalchemy_bigquery/base.py @@ -401,9 +401,9 @@ def visit_bindparam( **kwargs, ): unnest = False - if (bindparam.expanding and not isinstance(bindparam.type, NullType)): - if getattr(bindparam, 'expand_op', None) is not None: - assert bindparam.expand_op.__name__.endswith('in_op') # in in + if bindparam.expanding and not isinstance(bindparam.type, NullType): + if getattr(bindparam, "expand_op", None) is not None: + assert bindparam.expand_op.__name__.endswith("in_op") # in in bindparam.expanding = False unnest = True @@ -451,7 +451,7 @@ def visit_bindparam( param = f"%({name}:{bq_type})s" if unnest: - param = f'UNNEST({param})' + param = f"UNNEST({param})" return param diff --git a/tests/system/test_sqlalchemy_bigquery.py b/tests/system/test_sqlalchemy_bigquery.py index e1c01c4a..ea7c4416 100644 --- a/tests/system/test_sqlalchemy_bigquery.py +++ b/tests/system/test_sqlalchemy_bigquery.py @@ -691,17 +691,18 @@ def test_has_table(engine, engine_using_test_dataset, bigquery_dataset): @pytest.mark.skipif( - sqlalchemy_version_info < (1, 4), - reason="requires sqlalchemy 1.4 or higher", + sqlalchemy_version_info < (1, 4), reason="requires sqlalchemy 1.4 or higher", ) def test_huge_in(): engine = sqlalchemy.create_engine("bigquery://") conn = engine.connect() try: - assert list(conn.execute( - sqlalchemy.select([sqlalchemy.literal(-1).in_(list(range(99999)))]) - )) == [(False,)] - except Exception as e: + assert list( + conn.execute( + sqlalchemy.select([sqlalchemy.literal(-1).in_(list(range(99999)))]) + ) + ) == [(False,)] + except Exception: error = True else: error = False diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py index c71be56e..d5de554e 100644 --- a/tests/unit/conftest.py +++ b/tests/unit/conftest.py @@ -73,7 +73,6 @@ def ex(sql, *args, **kw): @pytest.fixture() def last_query(faux_conn): - def last_query(sql, params=None, offset=1): actual_sql, actual_params = faux_conn.test_data["execute"][-offset] assert actual_sql == sql @@ -81,6 +80,7 @@ def last_query(sql, params=None, offset=1): return last_query + @pytest.fixture() def metadata(): return sqlalchemy.MetaData() diff --git a/tests/unit/fauxdbi.py b/tests/unit/fauxdbi.py index 597f8b01..631996af 100644 --- a/tests/unit/fauxdbi.py +++ b/tests/unit/fauxdbi.py @@ -271,9 +271,10 @@ def __handle_problematic_literal_inserts( ) \) """, - flags=re.IGNORECASE | re.VERBOSE) + flags=re.IGNORECASE | re.VERBOSE, + ) def __handle_unnest(self, m): - return '(' + (m.group('exp') or '?') + ')' + return "(" + (m.group("exp") or "?") + ")" def __handle_true_false(self, operation): # Older sqlite versions, like those used on the CI servers diff --git a/tests/unit/test_select.py b/tests/unit/test_select.py index aa110e34..5d9deadc 100644 --- a/tests/unit/test_select.py +++ b/tests/unit/test_select.py @@ -231,8 +231,8 @@ def test_select_in_lit_13(faux_conn): def test_select_in_lit(faux_conn, last_query): faux_conn.execute(sqlalchemy.select([sqlalchemy.literal(1).in_([1, 2, 3])])) last_query( - 'SELECT %(param_1:INT64)s IN UNNEST(%(param_2:INT64)s) AS `anon_1`', - {'param_1': 1, 'param_2': [1, 2, 3]}, + "SELECT %(param_1:INT64)s IN UNNEST(%(param_2:INT64)s) AS `anon_1`", + {"param_1": 1, "param_2": [1, 2, 3]}, ) @@ -244,8 +244,9 @@ def test_select_in_param(faux_conn, last_query): dict(q=[1, 2, 3]), ) if sqlalchemy_version_info >= (1, 4): - last_query('SELECT %(param_1:INT64)s IN UNNEST(%(q:INT64)s) AS `anon_1`', - {'param_1': 1, 'q': [1, 2, 3]}, + last_query( + "SELECT %(param_1:INT64)s IN UNNEST(%(q:INT64)s) AS `anon_1`", + {"param_1": 1, "q": [1, 2, 3]}, ) else: assert isin @@ -265,15 +266,16 @@ def test_select_in_param1(faux_conn, last_query): dict(q=[1]), ) if sqlalchemy_version_info >= (1, 4): - last_query('SELECT %(param_1:INT64)s IN UNNEST(%(q:INT64)s) AS `anon_1`', - {'param_1': 1, 'q': [1]}, - ) + last_query( + "SELECT %(param_1:INT64)s IN UNNEST(%(q:INT64)s) AS `anon_1`", + {"param_1": 1, "q": [1]}, + ) else: assert isin last_query( "SELECT %(param_1:INT64)s IN UNNEST(" "[ %(q_1:INT64)s ]" ") AS `anon_1`", {"param_1": 1, "q_1": 1}, - ) + ) @sqlalchemy_1_3_or_higher @@ -285,13 +287,15 @@ def test_select_in_param_empty(faux_conn, last_query): dict(q=[]), ) if sqlalchemy_version_info >= (1, 4): - last_query('SELECT %(param_1:INT64)s IN UNNEST(%(q:INT64)s) AS `anon_1`', - {'param_1': 1, 'q': []}) + last_query( + "SELECT %(param_1:INT64)s IN UNNEST(%(q:INT64)s) AS `anon_1`", + {"param_1": 1, "q": []}, + ) else: assert not isin last_query( - "SELECT %(param_1:INT64)s IN UNNEST([ ]) AS `anon_1`", - {"param_1": 1}) + "SELECT %(param_1:INT64)s IN UNNEST([ ]) AS `anon_1`", {"param_1": 1} + ) @sqlalchemy_before_1_4 @@ -309,12 +313,11 @@ def test_select_notin_lit13(faux_conn): @sqlalchemy_1_4_or_higher def test_select_notin_lit(faux_conn, last_query): - faux_conn.execute( - sqlalchemy.select([sqlalchemy.literal(0).notin_([1, 2, 3])]) - ) + faux_conn.execute(sqlalchemy.select([sqlalchemy.literal(0).notin_([1, 2, 3])])) last_query( - 'SELECT (%(param_1:INT64)s NOT IN UNNEST(%(param_2:INT64)s)) AS `anon_1`', - {'param_1': 0, 'param_2': [1, 2, 3]}) + "SELECT (%(param_1:INT64)s NOT IN UNNEST(%(param_2:INT64)s)) AS `anon_1`", + {"param_1": 0, "param_2": [1, 2, 3]}, + ) def test_select_notin_param(faux_conn, last_query): @@ -326,15 +329,17 @@ def test_select_notin_param(faux_conn, last_query): ) if sqlalchemy_version_info >= (1, 4): last_query( - 'SELECT (%(param_1:INT64)s NOT IN UNNEST(%(q:INT64)s)) AS `anon_1`', - {'param_1': 1, 'q': [1, 2, 3]}) + "SELECT (%(param_1:INT64)s NOT IN UNNEST(%(q:INT64)s)) AS `anon_1`", + {"param_1": 1, "q": [1, 2, 3]}, + ) else: assert not isnotin last_query( "SELECT (%(param_1:INT64)s NOT IN UNNEST(" "[ %(q_1:INT64)s, %(q_2:INT64)s, %(q_3:INT64)s ]" ")) AS `anon_1`", - {"param_1": 1, "q_1": 1, "q_2": 2, "q_3": 3}) + {"param_1": 1, "q_1": 1, "q_2": 2, "q_3": 3}, + ) @sqlalchemy_1_3_or_higher @@ -347,9 +352,11 @@ def test_select_notin_param_empty(faux_conn, last_query): ) if sqlalchemy_version_info >= (1, 4): last_query( - 'SELECT (%(param_1:INT64)s NOT IN UNNEST(%(q:INT64)s)) AS `anon_1`', - {'param_1': 1, 'q': []}) + "SELECT (%(param_1:INT64)s NOT IN UNNEST(%(q:INT64)s)) AS `anon_1`", + {"param_1": 1, "q": []}, + ) else: assert isnotin - last_query("SELECT (%(param_1:INT64)s NOT IN UNNEST([ ])) AS `anon_1`", - {"param_1": 1}) + last_query( + "SELECT (%(param_1:INT64)s NOT IN UNNEST([ ])) AS `anon_1`", {"param_1": 1} + ) From 9343b86ffc2b4b5cfeeef59699c2159d33885712 Mon Sep 17 00:00:00 2001 From: Jim Fulton Date: Thu, 19 Aug 2021 13:02:39 -0400 Subject: [PATCH 4/9] Simplify visit_bindparam and punt on literal binds, to work less hard and to fix 252 --- sqlalchemy_bigquery/base.py | 59 +++++++++++++++++++------------------ tests/unit/test_select.py | 14 +++++++++ 2 files changed, 45 insertions(+), 28 deletions(-) diff --git a/sqlalchemy_bigquery/base.py b/sqlalchemy_bigquery/base.py index 3d728f94..babaf82b 100644 --- a/sqlalchemy_bigquery/base.py +++ b/sqlalchemy_bigquery/base.py @@ -411,8 +411,12 @@ def visit_bindparam( skip_bind_expression=False, **kwargs, ): + type_ = bindparam.type unnest = False - if bindparam.expanding and not isinstance(bindparam.type, NullType): + if (bindparam.expanding and + not isinstance(type_, NullType) and + not literal_binds + ): if getattr(bindparam, "expand_op", None) is not None: assert bindparam.expand_op.__name__.endswith("in_op") # in in bindparam.expanding = False @@ -426,40 +430,39 @@ def visit_bindparam( **kwargs, ) - type_ = bindparam.type - if not isinstance(type_, NullType): + if literal_binds or isinstance(type_, NullType): + return param - if ( - isinstance(type_, Numeric) - and (type_.precision is None or type_.scale is None) - and isinstance(bindparam.value, Decimal) - ): - t = bindparam.value.as_tuple() + if ( + isinstance(type_, Numeric) + and (type_.precision is None or type_.scale is None) + and isinstance(bindparam.value, Decimal) + ): + t = bindparam.value.as_tuple() - if type_.precision is None: - type_.precision = len(t.digits) + if type_.precision is None: + type_.precision = len(t.digits) - if type_.scale is None and t.exponent < 0: - type_.scale = -t.exponent + if type_.scale is None and t.exponent < 0: + type_.scale = -t.exponent - bq_type = self.dialect.type_compiler.process(type_) - if bq_type[-1] == ">" and bq_type.startswith("ARRAY<"): - # Values get arrayified at a lower level. - bq_type = bq_type[6:-1] - bq_type = self.__remove_type_parameter(bq_type) + bq_type = self.dialect.type_compiler.process(type_) + if bq_type[-1] == ">" and bq_type.startswith("ARRAY<"): + # Values get arrayified at a lower level. + bq_type = bq_type[6:-1] + bq_type = self.__remove_type_parameter(bq_type) - assert_(param != "%s", f"Unexpected param: {param}") + assert_(param != "%s", f"Unexpected param: {param}") - if bindparam.expanding: - assert_(self.__expanded_param(param), f"Unexpected param: {param}") - param = param.replace(")", f":{bq_type})") + if bindparam.expanding: + assert_(self.__expanded_param(param), f"Unexpected param: {param}") + param = param.replace(")", f":{bq_type})") - else: - m = self.__placeholder(param) - if m: - name, type_ = m.groups() - assert_(type_ is None) - param = f"%({name}:{bq_type})s" + else: + m = self.__placeholder(param) + name, type_ = m.groups() + assert_(type_ is None) + param = f"%({name}:{bq_type})s" if unnest: param = f"UNNEST({param})" diff --git a/tests/unit/test_select.py b/tests/unit/test_select.py index 5d9deadc..57a1ee1f 100644 --- a/tests/unit/test_select.py +++ b/tests/unit/test_select.py @@ -360,3 +360,17 @@ def test_select_notin_param_empty(faux_conn, last_query): last_query( "SELECT (%(param_1:INT64)s NOT IN UNNEST([ ])) AS `anon_1`", {"param_1": 1} ) + + +def test_literal_binds_kwarg_with_an_IN_operator_252(faux_conn): + table = setup_table(faux_conn, "test", sqlalchemy.Column("val", sqlalchemy.Integer), + initial_data=[dict(val=i) for i in range(3)]) + q = sqlalchemy.select([table.c.val]).where(table.c.val.in_([2])) + + def nstr(q): + return ' '.join(str(q).strip().split()) + + assert nstr(q.compile(faux_conn.engine, + compile_kwargs={"literal_binds": True} + ) + ) == 'SELECT `test`.`val` FROM `test` WHERE `test`.`val` IN (2)' From c2c0bf4d74b747b27c1716298f3a908286b8ab47 Mon Sep 17 00:00:00 2001 From: Jim Fulton Date: Thu, 19 Aug 2021 13:13:50 -0400 Subject: [PATCH 5/9] blacken --- sqlalchemy_bigquery/base.py | 7 ++++--- tests/unit/test_select.py | 18 +++++++++++------- 2 files changed, 15 insertions(+), 10 deletions(-) diff --git a/sqlalchemy_bigquery/base.py b/sqlalchemy_bigquery/base.py index babaf82b..51000b57 100644 --- a/sqlalchemy_bigquery/base.py +++ b/sqlalchemy_bigquery/base.py @@ -413,9 +413,10 @@ def visit_bindparam( ): type_ = bindparam.type unnest = False - if (bindparam.expanding and - not isinstance(type_, NullType) and - not literal_binds + if ( + bindparam.expanding + and not isinstance(type_, NullType) + and not literal_binds ): if getattr(bindparam, "expand_op", None) is not None: assert bindparam.expand_op.__name__.endswith("in_op") # in in diff --git a/tests/unit/test_select.py b/tests/unit/test_select.py index 57a1ee1f..9436f39a 100644 --- a/tests/unit/test_select.py +++ b/tests/unit/test_select.py @@ -363,14 +363,18 @@ def test_select_notin_param_empty(faux_conn, last_query): def test_literal_binds_kwarg_with_an_IN_operator_252(faux_conn): - table = setup_table(faux_conn, "test", sqlalchemy.Column("val", sqlalchemy.Integer), - initial_data=[dict(val=i) for i in range(3)]) + table = setup_table( + faux_conn, + "test", + sqlalchemy.Column("val", sqlalchemy.Integer), + initial_data=[dict(val=i) for i in range(3)], + ) q = sqlalchemy.select([table.c.val]).where(table.c.val.in_([2])) def nstr(q): - return ' '.join(str(q).strip().split()) + return " ".join(str(q).strip().split()) - assert nstr(q.compile(faux_conn.engine, - compile_kwargs={"literal_binds": True} - ) - ) == 'SELECT `test`.`val` FROM `test` WHERE `test`.`val` IN (2)' + assert ( + nstr(q.compile(faux_conn.engine, compile_kwargs={"literal_binds": True})) + == "SELECT `test`.`val` FROM `test` WHERE `test`.`val` IN (2)" + ) From 04c57513cca941522f51646be9fd9d09c048be31 Mon Sep 17 00:00:00 2001 From: Jim Fulton Date: Thu, 19 Aug 2021 15:38:39 -0400 Subject: [PATCH 6/9] Restored some code needed by compliance tests and added unit test to cover it. Also fixed constraints --- sqlalchemy_bigquery/base.py | 7 ++++--- testing/constraints-3.6.txt | 2 +- tests/unit/test_compliance.py | 16 ++++++++++++++++ 3 files changed, 21 insertions(+), 4 deletions(-) diff --git a/sqlalchemy_bigquery/base.py b/sqlalchemy_bigquery/base.py index 51000b57..17261a53 100644 --- a/sqlalchemy_bigquery/base.py +++ b/sqlalchemy_bigquery/base.py @@ -461,9 +461,10 @@ def visit_bindparam( else: m = self.__placeholder(param) - name, type_ = m.groups() - assert_(type_ is None) - param = f"%({name}:{bq_type})s" + if m: + name, type_ = m.groups() + assert_(type_ is None) + param = f"%({name}:{bq_type})s" if unnest: param = f"UNNEST({param})" diff --git a/testing/constraints-3.6.txt b/testing/constraints-3.6.txt index 1785edd0..e5ed0b2a 100644 --- a/testing/constraints-3.6.txt +++ b/testing/constraints-3.6.txt @@ -6,5 +6,5 @@ # e.g., if setup.py has "foo >= 1.14.0, < 2.0.0dev", sqlalchemy==1.2.0 google-auth==1.25.0 -google-cloud-bigquery==2.19.0 +google-cloud-bigquery==2.24.1 google-api-core==1.30.0 diff --git a/tests/unit/test_compliance.py b/tests/unit/test_compliance.py index 7cae1825..5faaca5f 100644 --- a/tests/unit/test_compliance.py +++ b/tests/unit/test_compliance.py @@ -200,3 +200,19 @@ def test_group_by_composed(faux_conn): select([sqlalchemy.func.count(table.c.id), expr]).group_by(expr).order_by(expr) ) assert_result(faux_conn, stmt, [(1, 3), (1, 5), (1, 7)]) + + +def test_cast_type_decorator(faux_conn, last_query): + # [artial dup of: + # sqlalchemy.testing.suite.test_types.CastTypeDecoratorTest.test_special_type + # That test failes without code that's otherwise not covered by the unit tests. + + class StringAsInt(sqlalchemy.TypeDecorator): + impl = sqlalchemy.String(50) + + def bind_expression(self, col): + return sqlalchemy.cast(col, String(50)) + + t = setup_table(faux_conn, "t", Column("x", StringAsInt())) + faux_conn.execute(t.insert(), [{"x": x} for x in [1, 2, 3]]) + last_query('INSERT INTO `t` (`x`) VALUES (CAST(%(x:STRING)s AS STRING))', {'x': 3}) From bc615e67f6c2b05008f78e4b36ca6d62eb4afe8a Mon Sep 17 00:00:00 2001 From: Jim Fulton Date: Thu, 19 Aug 2021 15:44:43 -0400 Subject: [PATCH 7/9] blacken --- tests/unit/test_compliance.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/test_compliance.py b/tests/unit/test_compliance.py index 5faaca5f..9ba27cf6 100644 --- a/tests/unit/test_compliance.py +++ b/tests/unit/test_compliance.py @@ -215,4 +215,4 @@ def bind_expression(self, col): t = setup_table(faux_conn, "t", Column("x", StringAsInt())) faux_conn.execute(t.insert(), [{"x": x} for x in [1, 2, 3]]) - last_query('INSERT INTO `t` (`x`) VALUES (CAST(%(x:STRING)s AS STRING))', {'x': 3}) + last_query("INSERT INTO `t` (`x`) VALUES (CAST(%(x:STRING)s AS STRING))", {"x": 3}) From 295b1bc96a5e6e8d89543daa92713531256630c7 Mon Sep 17 00:00:00 2001 From: Jim Fulton Date: Mon, 23 Aug 2021 12:13:00 -0600 Subject: [PATCH 8/9] Add a comment explainin the expanding logic. --- sqlalchemy_bigquery/base.py | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/sqlalchemy_bigquery/base.py b/sqlalchemy_bigquery/base.py index 2963db6f..da3ac6e8 100644 --- a/sqlalchemy_bigquery/base.py +++ b/sqlalchemy_bigquery/base.py @@ -418,6 +418,27 @@ def visit_bindparam( and not isinstance(type_, NullType) and not literal_binds ): + # Normally, when performing an IN operation, like: + # + # foo IN (some_sequence) + # + # SQAlchemy passes `foo` as a parameter and unpacks + # `some_sequence` and passes each element as a parameter. + # This mechanism is refered to as "expanding". It's + # inefficient and can't handle large arrays. (It's also + # very complicated, but that's not the issue we care about + # here. :) ) BigQuery lets us use arrays directly in this + # context, we just need to call UNNEST on an array when + # it's used in IN. + # + # So, if we get an `expanding` flag, and if we have a known type + # (and don't have literal binds, which are implemented in-line in + # in the SQL), we turn off expanding and we set an unnest flag + # so that we add an UNNEST() call (below). + # + # The NullType/known-type check has to do with some extreme + # edge cases having to do with empty in-lists that get special + # hijinks from SQLAlchemy that we don't want to disturb. :) if getattr(bindparam, "expand_op", None) is not None: assert bindparam.expand_op.__name__.endswith("in_op") # in in bindparam.expanding = False From 6e32630b1f1a7eac0ced11cbe705ae88f272b3b6 Mon Sep 17 00:00:00 2001 From: Jim Fulton Date: Mon, 23 Aug 2021 12:19:20 -0600 Subject: [PATCH 9/9] removed dup test --- tests/unit/test_select.py | 18 ------------------ 1 file changed, 18 deletions(-) diff --git a/tests/unit/test_select.py b/tests/unit/test_select.py index 747f11a5..9436f39a 100644 --- a/tests/unit/test_select.py +++ b/tests/unit/test_select.py @@ -378,21 +378,3 @@ def nstr(q): nstr(q.compile(faux_conn.engine, compile_kwargs={"literal_binds": True})) == "SELECT `test`.`val` FROM `test` WHERE `test`.`val` IN (2)" ) - - -def test_literal_binds_kwarg_with_an_IN_operator_252(faux_conn): - table = setup_table( - faux_conn, - "test", - sqlalchemy.Column("val", sqlalchemy.Integer), - initial_data=[dict(val=i) for i in range(3)], - ) - q = sqlalchemy.select([table.c.val]).where(table.c.val.in_([2])) - - def nstr(q): - return " ".join(str(q).strip().split()) - - assert ( - nstr(q.compile(faux_conn.engine, compile_kwargs={"literal_binds": True})) - == "SELECT `test`.`val` FROM `test` WHERE `test`.`val` IN (2)" - )