feat: Comment/description support, bug fixes and better test coverage #138

jimfulton · 2021-05-05T20:21:54Z

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 #89🦕

Runs SQLAlchemy dialect-compliance tests (as system tests).
100% unit-test coverage.
Support for table and column comments/descriptions (requiring SQLAlchemy 1.2 or higher).
Fixes bugs found while debugging tests, including:
- Handling of in queries.
- String literals with special characters.
- Use BIGNUMERIC when necessary.
- Missing types: BIGINT, SMALLINT, Boolean, REAL, CHAR, NCHAR, VARCHAR, NVARCHAR, TEXT, VARBINARY, DECIMAL
- Literal bytes, dates, times, datetimes, timestamps, and arrays.
- Get view definitions.
When executing parameterized queries, the new BigQuery DB API parameter syntax is used to pass type information. This is helpful when the DB API can't determine type information from values, or can't determine it correctly.

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.

We get it from the creds.

tests/sqlalchemy_dialect_compliance/README.rst

noxfile.py

plamut

Good that we caught and fixed a bug!

The changes look good, I just found two more typos and what seems like a bug with overriding default kwarg values in fauxdbi. No tests currently seem to use that, thus no failure.

Co-authored-by: Peter Lamut <plamut@users.noreply.github.com>

plamut · 2021-05-11T13:36:01Z

tests/unit/fauxdbi.py

-                    # necessary.
-                    **dict(name=name, type=type, notnull=notnull, **custom)
-                )
+                return self._get_field(name=name, type=type, notnull=notnull, **custom)


If I understood correctly, custom, by design, can never contain name, type, or notnull keys?

Design? You give me so much credit! ;) You may be right. I can imagine overriding type, especially when dealing with arrays and structs, although I may be able to automate that using naming patterns. (e.g. array_int_ or somesuch).

You probably know better. :)

If we want to use overriding, the code needs to be changed to avoid errors (duplicate keyword arguments), otherwise we can leave it as is.

I removed the overriding. If we need it later, we can add it then.

jimfulton · 2021-05-11T14:17:19Z

Waaaaa, tests are failing spuriously, I think due to a race between tests. I added a sleep yesterday afternoon, which I thought fixed it, but maybe that was just because it was late in the day. :(

They don't fail or fail far less often when I run from home.

shollyman · 2021-05-11T18:09:32Z

tests/sqlalchemy_dialect_compliance/conftest.py

+    for schema in "test_schema", "test_pybigquery_sqla":
+        for table_item in client.list_tables(f"{client.project}.{schema}"):
+            table_id = table_item.table_id
+            client.query(


Your timing issues may be related to this. It seems that you're invoking the query, but not waiting for it to complete. Possibly just racy?

And get rid of the sleep, at least termporarily.

It didn't buy us anything.

jimfulton · 2021-05-12T12:26:24Z

@plamut are you requesting changes? :)

plamut

No, it was just late yesterday and didn't get to submit an approval - doing this now. :)
Looks like a good piece of work, thanks!

Disclaimer - I don't know all the ins and outs of writing a new SQAlchemy dialect, as I have not studied all the requirements (especially not some less- or non-documented ones) . Did the reviews on a best-effort basis.

Any additional pair of eyes would be welcome, if anybody else has some free capacity.

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

yoshi-kokoro removed kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels May 10, 2021

jimfulton added kokoro:force-run Add this label to force Kokoro to re-run the tests. kokoro:run Add this label to force Kokoro to re-run the tests. labels May 10, 2021

yoshi-kokoro removed kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels May 10, 2021

jimfulton added 3 commits May 10, 2021 16:00

don't need to call out the project in the test config.

ddd3399

We get it from the creds.

Try sleeping between tests to see if we can avoid some weird failures.

e063e3e

some minimal docs on the dialect tests.

7530acf

plamut reviewed May 11, 2021

View reviewed changes

tests/sqlalchemy_dialect_compliance/README.rst Outdated Show resolved Hide resolved

tests/sqlalchemy_dialect_compliance/README.rst Outdated Show resolved Hide resolved

noxfile.py Show resolved Hide resolved

plamut suggested changes May 11, 2021

View reviewed changes

jimfulton and others added 4 commits May 11, 2021 07:02

Update tests/sqlalchemy_dialect_compliance/README.rst

4d99090

Co-authored-by: Peter Lamut <plamut@users.noreply.github.com>

Update tests/sqlalchemy_dialect_compliance/README.rst

6b2a6b6

Co-authored-by: Peter Lamut <plamut@users.noreply.github.com>

simplify by getting rid of unnecessary and broken overrriding.

bc40df2

merge upstream

ec4961c

plamut reviewed May 11, 2021

View reviewed changes

Try sleeping longer to avoid races between tests.

16e302d

shollyman reviewed May 11, 2021

View reviewed changes

jimfulton added 7 commits May 11, 2021 12:38

make sure we wait for the drops at the beginning of the session.

1e11ce2

And get rid of the sleep, at least termporarily.

rerun on table not found

ff35acf

Try simpler regex

b4c3c23

Try different error

18ebe26

don't sleep between tests.

fbbd563

It didn't buy us anything.

blacken

a103ae8

removed unused import

5809a9c

plamut approved these changes May 12, 2021

View reviewed changes

jimfulton merged commit fb7c188 into master May 12, 2021

jimfulton deleted the riversnake-tests branch May 12, 2021 12:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Comment/description support, bug fixes and better test coverage #138

feat: Comment/description support, bug fixes and better test coverage #138

jimfulton commented May 5, 2021 •

edited

plamut left a comment

plamut May 11, 2021

jimfulton May 11, 2021

plamut May 11, 2021

jimfulton May 12, 2021

jimfulton commented May 11, 2021

shollyman May 11, 2021

jimfulton commented May 12, 2021

plamut left a comment •

edited

Navigation Menu

feat: Comment/description support, bug fixes and better test coverage #138

feat: Comment/description support, bug fixes and better test coverage #138

Conversation

jimfulton commented May 5, 2021 • edited

plamut left a comment

Choose a reason for hiding this comment

plamut May 11, 2021

Choose a reason for hiding this comment

jimfulton May 11, 2021

Choose a reason for hiding this comment

plamut May 11, 2021

Choose a reason for hiding this comment

jimfulton May 12, 2021

Choose a reason for hiding this comment

jimfulton commented May 11, 2021

shollyman May 11, 2021

Choose a reason for hiding this comment

jimfulton commented May 12, 2021

plamut left a comment • edited

Choose a reason for hiding this comment

jimfulton commented May 5, 2021 •

edited

plamut left a comment •

edited