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: Comment/description support, bug fixes and better test coverage #138

Merged
merged 159 commits into from May 12, 2021

Conversation

@jimfulton
Copy link
Contributor

@jimfulton jimfulton commented May 5, 2021

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.
jimfulton added 30 commits Apr 12, 2021
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.
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
Copy link

@plamut plamut left a comment

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.

jimfulton and others added 4 commits May 11, 2021
# necessary.
**dict(name=name, type=type, notnull=notnull, **custom)
)
return self._get_field(name=name, type=type, notnull=notnull, **custom)
Copy link

@plamut plamut May 11, 2021

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

Copy link
Contributor Author

@jimfulton jimfulton May 11, 2021

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

Copy link

@plamut plamut May 11, 2021

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.

Copy link
Contributor Author

@jimfulton jimfulton May 12, 2021

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

@jimfulton
Copy link
Contributor Author

@jimfulton jimfulton commented May 11, 2021

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.

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?

@jimfulton
Copy link
Contributor Author

@jimfulton jimfulton commented May 12, 2021

@plamut are you requesting changes? :)

plamut
plamut approved these changes May 12, 2021
Copy link

@plamut plamut left a comment

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 jimfulton merged commit fb7c188 into master May 12, 2021
3 checks passed
@jimfulton jimfulton deleted the riversnake-tests branch May 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants