-
Notifications
You must be signed in to change notification settings - Fork 45
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(get_view_names): Use proper schema #1082
Conversation
🤖 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 |
for more information, see https://pre-commit.ci
… into fix-get-views
for more information, see https://pre-commit.ci
@Mause I am not sure I understand CI failures here 🤔 could you take a look? |
@Mause bump |
Thanks! This has now been released in 0.13.2 Also sorry for the delay 😬 |
@Mause no worries! Again, thanks a lot for the lib :) |
Hey @Mause ! Thanks for this lib :)
When working with it to wire DuckDB to Superset I have noticed that Superset does not display views created in the DuckDB.
The problem was that
get_view_names
was using rawtable_schema
in theWHERE
clause, which in case of Superset includes database name, so it becomes something likefoodb.barschema
. Which means that the WHERE clause in the function to get the view names never succeeds, as it tries to comparetable_name
to a string that includes database name.get_table_names
uses_build_query_where
function that splits database and schema name, so to fix the bug I have used the same approach that_build_query_where
function uses to split the schema and db name. We can't really_build_query_where
function itself because of how the naming difference(ininformation_schema.tables
schema field is namedtable_schema
and notschema_name
as it is named in theduckdb_tables()
orduckdb_views()
).I also initially tried using
duckdb_views()
but that one returns all system views as well, which we don't really need