-
Notifications
You must be signed in to change notification settings - Fork 130
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Fix metadata reflection without a default dataset #1089
base: main
Are you sure you want to change the base?
Conversation
c35e20e
to
7d72652
Compare
🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use -- conventional-commit-lint bot |
969685f
to
4a006bf
Compare
Hi, any chance I could get the tests approved to run on the PR? I've been trying to setup the system tests locally, but they're a bit onerous (understandably). |
Sorry for the delay, just started running the tests! |
no worries, thank you! |
64ab97f
to
75a933a
Compare
@lingchin if this is a breaking change as Jacob indicates, then we need to have some sort of deprecation warning that the break is coming and get this into the schedule.
|
Fixes #838 and fixes #1088. 🦕
The
get_table_names
andget_view_names
methods are supposed to return the bare names (no{schema}.
prefix) of the resources for a single schema/dataset (whereschema=None
is the "default schema", not "all schemas"). However, theBigQueryDialect
implementation returns:schema
: if the connection has a default dataset{schema}.
prefixed names for one schema: if the connection doesn't have a default dataset and theschema
arg is a string{schema}.
prefixed names for all schemas: if the connection doesn't have a default dataset and theschema
arg isNone
The bolded parts cause issues outlined in #1088. This PR fixes the
get_table_names
andget_view_names
implementations to return bare names as SQLAlchemy expects.This is a breaking change for a subset of users:
Metadata.reflect()
without aschema
argument were probably expecting to reflect all datasets, but they would now not reflect anything.public
) - so without a default set on the connection string, we don't have anything to search and return an empty list.This has been working for me locally with
alembic
+ no default dataset (the models specify the schema) +include_schemas=True
(required for cross-schema stuff).