feat: Add support for SQLAlchemy 1.4 #177

jimfulton · 2021-05-17T20:19:36Z

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)

Fixes #83 🦕

The source of the dependency bug is in old versions of google-cloud-core that depend on too-old versions of google-api-core.

Some tests are still failing, but we're far enough along that we have the right shape, I think.

@bar

The moral equivalent of "where foo in (@bar)", where bar is an array which actually need to be "where foo in unnest (@bar)".

Doh!

Other fixes: - Handle BIGINT - Fix string leteral formatting (and start type-specific adaptations).

…ures

The SQLAlchemy like convenience functions (e.g. ) escape incorrectly for BigQuery, so re-escape.

We could make that work, if we want to. :)

…nt_digits

…ry keys, etc., that BigQuery doesn't have.

#78

…xpect

The source of the dependency bug is in old versions of google-cloud-core that depend on too-old versions of google-api-core.

So we don't have t mock at the api level.

#78

- Run tests in temporary directory rather than sharing memory connections. Because simpler. :) - Introduce cross-connection state and record queries in it, so tests can make assertions bout generated queries.

…laceholder The BigQuery Python Client supports an extended placeholder syntax that includes type information, as in `%(foo:INT64)s` (named) or `%(:INT64)s` (unnamed). When we know the type, include it in the placeholder.

The numeric tests now tun since we started passing type info from sqla to bigquery. The CTE tests test features that don't exist in bigquery.

Although the test isn't actually testing dialect code. Maybe it should be skipped instead. Also set the profile test pasth to a more reasonable value, although it doesn't seem to be used. <shrug>

Also inlined colspecs code, because there wasn't much and it facilitated separating literal processing into a function.

alonme · 2021-05-19T05:56:14Z

testing/constraints-3.9.txt

@@ -0,0 +1,3 @@
+sqlalchemy>=1.4.13
+#sqlalchemy==1.3.24


redundant comment

No. This makes it easy for me to test with 1.3. :) I want to make sure I don't break 1.3 but the compliance tests are too expensive to tun twice, so I just run them manually.

On second thought, let's run the compliance test for both in CI.

alonme · 2021-05-19T18:06:13Z

pybigquery/sqlalchemy_bigquery.py

@@ -57,6 +58,12 @@
 FIELD_ILLEGAL_CHARACTERS = re.compile(r"[^\w]+")


+def assert_(cond, message="Assertion failed"):  # pragma: NO COVER
+    if not cond:
+        breakpoint()


do we want to leave this breakpoint in?

Nope! Thanks.

alonme · 2021-05-19T18:08:10Z

pybigquery/sqlalchemy_bigquery.py

-        super(BigQueryCompiler, self).__init__(
-            dialect, statement, column_keys, inline, **kwargs
-        )
+        super(BigQueryCompiler, self).__init__(dialect, statement, *args, **kwargs)


When i worked on this change, i was a bit worried about the consequences of users passing "inline" as a named parameter.

AFAIR - it was removed from SQLA right?

I wasn't thinking about users passing inline. :)

Yes, SQLA removed it in 1.4. I want this code to work with earlier versions and since we're only using statement, I'm just passing the later arguments along.

alonme · 2021-05-19T18:11:11Z

testing/constraints-3.8.txt

@@ -0,0 +1 @@
+sqlalchemy==1.3.24


why Is that different than 3.9 constraint on SQLA?

To make sure we run with 1.3 in some of the unit tests.

There are code paths needed for 1.3 that aren't exercised with 1.4. Without this, we wouldn't have the required 100% test coverage (and wouldn't be testing with 1.3).

jimfulton · 2021-05-20T20:45:04Z

@alonme Thanks for taking a look!

plamut

Generally looks fine, although I know far too few details about SQLALchemy dialects to give a definite verdict.

The version check is something that would be nice to fix, the rest are just trivial remarks.

pybigquery/sqlalchemy_bigquery.py

pybigquery/_helpers.py

noxfile.py

Also be a little DRYer.

jimfulton · 2021-05-21T13:16:08Z

@plamut thanks for the review!

plamut

LGTM

(with that mandatory disclaimer :) )

…problem.

Jim Fulton added 30 commits April 12, 2021 08:39

Fixed a dependency problem that caysed test failures in Python 3.6.

0fc64fa

The source of the dependency bug is in old versions of google-cloud-core that depend on too-old versions of google-api-core.

Started implementing SqlAlchemy dialect-compliance tests

bfdda68

Some tests are still failing, but we're far enough along that we have the right shape, I think.

Handle parameters in in

41cb2dd

The moral equivalent of "where foo in (@bar)", where bar is an array which actually need to be "where foo in unnest (@bar)".

fixed regex to allow multi-character parameter names.

631c060

Doh!

Fixed rendering of string literals.

aa85cb9

Provide default values for primary keys and other fixes...

c7da257

Other fixes: - Handle BIGINT - Fix string leteral formatting (and start type-specific adaptations).

Narrowed skips and license comment diagnosed reasons for skipped fail…

3916239

…ures

use Google license

46b30b4

Fixed like (% and _) escapes.

36a0033

The SQLAlchemy like convenience functions (e.g. ) escape incorrectly for BigQuery, so re-escape.

Handle BIGNUMERIC

d079fbe

Skip tests that want to stpre floats as numeric.

7e60691

We could make that work, if we want to. :)

skip tests for offsets without limits. BigQuery doesn't allow that.

4a1bafd

BIGNUMERIC lets us handle many significant digits.test_many_significa…

e07b1f0

…nt_digits

Skip LongNameBlowoutTest because it requires features (indexes, prima…

656af06

…ry keys, etc., that BigQuery doesn't have.

We have to rewrite these tests, because of forcing use_labels=True

a085db6

#78

BigQuery returns deleted and updated rows, which SQLAlchemy doesn't e…

681ea47

…xpect

Fixed a dependency problem that caysed test failures in Python 3.6.

cde5bf8

The source of the dependency bug is in old versions of google-cloud-core that depend on too-old versions of google-api-core.

Provide a bigquery mock based on sqlite

76cf078

So we don't have t mock at the api level.

Don't force labels in select.

ee46a1e

#78

merge riversnake-fix-78

62432a3

No need to work around forced labels anymore.

a54b946

Updated sqlite-bases mock

1320f32

- Run tests in temporary directory rather than sharing memory connections. Because simpler. :) - Introduce cross-connection state and record queries in it, so tests can make assertions bout generated queries.

Added test that types are injected into parameter placeholders.

4f098fd

Oops, need to call super even when there are where clauses.

2c76342

Don't skip some numeric tests and skip some CTE tests

be1e215

The numeric tests now tun since we started passing type info from sqla to bigquery. The CTE tests test features that don't exist in bigquery.

Can't coerce datetime to date.

39f1955

reenable some tests now that we pass type infor to bigquery

9e266f2

Enable error on warnings -- a test depends on it.

fa84c95

Although the test isn't actually testing dialect code. Maybe it should be skipped instead. Also set the profile test pasth to a more reasonable value, although it doesn't seem to be used. <shrug>

Added table and column comment support.

b01d2ce

Also inlined colspecs code, because there wasn't much and it facilitated separating literal processing into a function.

Jim Fulton added 6 commits May 17, 2021 13:19

ported a compliance test to hit an unreached path.

08c781c

different sql is generated for in before and after 1.4

e06fac2

made substitute_re_method more methody

ce87a54

brought back some code paths needed by sqlalchemy 1.3

da3ce60

make sure we test 1.3

26c16a7

lint/blacken

6996562

product-auto-label bot added the api: bigquery Issues related to the googleapis/python-bigquery-sqlalchemy API. label May 17, 2021

Merge branch 'master' into riversnake-sqla-14

55ecde6

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label May 17, 2021

removed some no-longer relevant text

9ae173a

jimfulton marked this pull request as ready for review May 18, 2021 13:40

jimfulton requested a review from a team as a code owner May 18, 2021 13:40

jimfulton mentioned this pull request May 18, 2021

Problem with SQLAlchemy 1.4.0 #83

Closed

Merge branch 'master' into riversnake-sqla-14

d0edaea

jimfulton mentioned this pull request May 19, 2021

feat: add support for SQLAlchemy 1.4 #84

Closed

cwegrzyn mentioned this pull request May 19, 2021

Support sqlalchemy 1.4 bluelabsio/records-mover#166

Closed

alonme reviewed May 19, 2021

View reviewed changes

Jim Fulton added 2 commits May 19, 2021 13:39

Removed breakpoint from code that should never be called. :)

2c5d992

run system and compliance tests with both sqla 1.3 and 1.4

070814e

plamut reviewed May 21, 2021

View reviewed changes

pybigquery/sqlalchemy_bigquery.py Outdated Show resolved Hide resolved

pybigquery/_helpers.py Outdated Show resolved Hide resolved

noxfile.py Show resolved Hide resolved

Jim Fulton added 3 commits May 21, 2021 07:10

be more careful about sqlalchemy version comparison

0fc9db6

Also be a little DRYer.

use wraps as a decorator

64206c0

say why we're using 2 python versions

41edaa5

plamut approved these changes May 21, 2021

View reviewed changes

Jim Fulton added 2 commits May 21, 2021 09:36

blacken

738faa0

don't wait for compliance tests to finish to find out we have a lint …

4d80d23

…problem.

jimfulton merged commit b7b6000 into master May 21, 2021

jimfulton deleted the riversnake-sqla-14 branch May 21, 2021 17:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add support for SQLAlchemy 1.4 #177

feat: Add support for SQLAlchemy 1.4 #177

jimfulton commented May 17, 2021

alonme May 19, 2021

jimfulton May 19, 2021

jimfulton May 19, 2021

alonme May 19, 2021

jimfulton May 19, 2021

alonme May 19, 2021

jimfulton May 19, 2021

alonme May 19, 2021

jimfulton May 19, 2021

jimfulton commented May 20, 2021

plamut left a comment

jimfulton commented May 21, 2021

plamut left a comment

		@@ -0,0 +1,3 @@
		sqlalchemy>=1.4.13
		#sqlalchemy==1.3.24

		@@ -0,0 +1 @@
		sqlalchemy==1.3.24

feat: Add support for SQLAlchemy 1.4 #177

feat: Add support for SQLAlchemy 1.4 #177

Conversation

jimfulton commented May 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimfulton commented May 20, 2021

plamut left a comment

Choose a reason for hiding this comment

jimfulton commented May 21, 2021

plamut left a comment

Choose a reason for hiding this comment