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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add support for SQLAlchemy 1.4 #177

Merged
merged 228 commits into from
May 21, 2021
Merged

feat: Add support for SQLAlchemy 1.4 #177

merged 228 commits into from
May 21, 2021

Conversation

jimfulton
Copy link
Contributor

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.
The moral equivalent of    "where foo in (@bar)", where bar is an array
which actually need to be  "where foo in unnest (@bar)".
Other fixes:
- Handle BIGINT
- Fix string leteral formatting (and start type-specific adaptations).
The SQLAlchemy like convenience functions (e.g. ) escape incorrectly for
BigQuery, so re-escape.
We could make that work, if we want to. :)
鈥y keys, etc., that BigQuery doesn't have.
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.
- 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.
鈥aceholder

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.
@product-auto-label product-auto-label bot added the api: bigquery Issues related to the googleapis/python-bigquery-sqlalchemy API. label May 17, 2021
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label May 17, 2021
@jimfulton jimfulton marked this pull request as ready for review May 18, 2021 13:40
@jimfulton jimfulton requested a review from a team as a code owner May 18, 2021 13:40
@@ -0,0 +1,3 @@
sqlalchemy>=1.4.13
#sqlalchemy==1.3.24
Copy link
Contributor

Choose a reason for hiding this comment

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

redundant comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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


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

Choose a reason for hiding this comment

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

do we want to leave this breakpoint in?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope! Thanks.

super(BigQueryCompiler, self).__init__(
dialect, statement, column_keys, inline, **kwargs
)
super(BigQueryCompiler, self).__init__(dialect, statement, *args, **kwargs)
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@@ -0,0 +1 @@
sqlalchemy==1.3.24
Copy link
Contributor

Choose a reason for hiding this comment

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

why Is that different than 3.9 constraint on SQLA?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

@alonme Thanks for taking a look!

Copy link

@plamut plamut left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
pybigquery/_helpers.py Outdated Show resolved Hide resolved
noxfile.py Show resolved Hide resolved
@jimfulton
Copy link
Contributor Author

@plamut thanks for the review!

Copy link

@plamut plamut left a comment

Choose a reason for hiding this comment

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

LGTM

(with that mandatory disclaimer :) )

@jimfulton jimfulton merged commit b7b6000 into master May 21, 2021
@jimfulton jimfulton deleted the riversnake-sqla-14 branch May 21, 2021 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigquery Issues related to the googleapis/python-bigquery-sqlalchemy API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Problem with SQLAlchemy 1.4.0
4 participants