-
Notifications
You must be signed in to change notification settings - Fork 610
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(sql): only return tables in current_database
#9748
Conversation
It looks like
|
Pushed up the additional test. It passed without changes 🥳 |
Ok, I guess we need to decide on what convention we want to enforce? |
lol, I'm going to have to relax the cross-backend exception check a fair bit until we get Naty's |
ok, MSSQL is failing the way postgres and duckdb did before the fix. |
A hilarious error message from MySQL:
Need to fix that, too... edit: actually, you CAN create tables in other databases using |
Ok, this turned into a bit of a nightmare. I'll pick it back up tomorrow or Monday |
d8f8c5c
to
456f95a
Compare
@@ -1,5 +1,4 @@ | |||
CREATE USER 'ibis'@'localhost' IDENTIFIED BY 'ibis'; | |||
CREATE SCHEMA IF NOT EXISTS test_schema; | |||
GRANT CREATE, DROP ON *.* TO 'ibis'@'%'; | |||
GRANT CREATE,SELECT,DROP ON `test_schema`.* TO 'ibis'@'%'; | |||
GRANT CREATE,SELECT,DROP ON *.* TO 'ibis'@'%'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Necessary to allow the ibis user to operate on newly created databases.
"column_name", | ||
"data_type", | ||
sg.column("is_nullable").eq(sge.convert("YES")).as_("nullable"), | ||
query = sge.Describe( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DESCRIBE
in DuckDB used to be different than using information_schema
, but only for CSV files, and that case has been fixed in a version that we no longer support.
@@ -586,3 +586,9 @@ def drop_sink( | |||
) | |||
with self._safe_raw_sql(src): | |||
pass | |||
|
|||
@property | |||
def _session_temp_db(self) -> str | None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was actually enough to make tests pass, since otherwise get_schema
works just fine.
sg.column("is_nullable").eq(sge.convert("YES")).as_("nullable"), | ||
C.column_name, | ||
C.data_type, | ||
C.is_nullable.eq(sge.convert("YES")).as_("nullable"), | ||
) | ||
.from_(sg.table("columns", db="information_schema", catalog=catalog)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wish we could use DESCRIBE
here, but it doesn't contain a nullability column 😞
ed837a7
to
b6b0e1d
Compare
Exasol does indeed allow creating tables in other databases, but there was a bug in our code (we weren't quoting identifiers). |
The only remaining task is to decide how we want to scope temp tables versus non-temp tables in backends that support them. We can defer that to a follow-up IMO. |
56e3ed2
to
d6d2405
Compare
current_database
d6d2405
to
c64f759
Compare
DuckDB puts temp tables into a catalog named `temp` (not a `database`).
…ion and listing tables
…e of an actual bug
907227e
to
7929fe0
Compare
7929fe0
to
936873b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
Description of changes
This started as a fix of #9686 but it turned up a few issues in other backends.
The short version is: when we call
get_schema
without specifying acatalog.database
prefix, we get tables that exist in many (sometimes all) thedatabases on a given backend (that match the name passed to
get_schema
).Postgres has things like
search_path
to help with this, but the tables onsearch_path
are slightly different than what's returned by\dt
(just to make this extra fun).What's implemented here (and what I'm proposing as a general standard) is:
When you ask for a table named
foo
, we will look through thecurrent_database
and wherever temp tables are stored for a match and returnthe schema of that table.
My idea here is that anything that shows up in the output of
list_tables
(orcon.tables
) should be accessible viacon.table
without additional arguments.There's at least one edge-case which I discovered while writing this
description, which is if the temp table has the same name as a table in the
current_database
, the table returned will be the interleaved schema of the twotables, which is terrible.
I'll fix that, too.
Issues closed
Todo List of things to fix (not all in this PR):
temp
tables take precedence over concrete tables or vice-versaget_schema
because it doesn't have postgres-specific function