feat: Comment/description support, bug fixes and better test coverage #138
Conversation
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.
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. :)
…ry 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.
…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.
yoshi-kokoro
removed
kokoro:run
jimfulton
added
kokoro:force-run
yoshi-kokoro
removed
kokoro:run
Co-authored-by: Peter Lamut <plamut@users.noreply.github.com>
Co-authored-by: Peter Lamut <plamut@users.noreply.github.com>
| # necessary. | ||
| **dict(name=name, type=type, notnull=notnull, **custom) | ||
| ) | ||
| return self._get_field(name=name, type=type, notnull=notnull, **custom) |
There was a problem hiding this comment.
If I understood correctly, custom, by design, can never contain name, type, or notnull keys?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I removed the overriding. If we need it later, we can add it then.
|
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( |
There was a problem hiding this comment.
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.
|
@plamut are you requesting changes? :) |
There was a problem hiding this 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.
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:
Fixes #89🦕
inqueries.