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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

create mem table should not override current tables #1269

Closed
jimexist opened this issue Nov 8, 2021 · 12 comments · Fixed by #1288
Closed

create mem table should not override current tables #1269

jimexist opened this issue Nov 8, 2021 · 12 comments · Fixed by #1288
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@jimexist
Copy link
Member

jimexist commented Nov 8, 2021

Is your feature request related to a problem or challenge? Please describe what you are trying to do.

With #1252 dropping supported, we should not override existing table with create table as statement.

Describe the solution you'd like

current behavior:

❯ create table users as values ('a', 'b');
0 rows in set. Query took 0.001 seconds.
❯ create table users as values ('a', 'b');
0 rows in set. Query took 0.001 seconds.
❯ \d
+---------------+--------------------+------------+------------+
| table_catalog | table_schema       | table_name | table_type |
+---------------+--------------------+------------+------------+
| datafusion    | public             | users      | BASE TABLE |
| datafusion    | information_schema | tables     | VIEW       |
| datafusion    | information_schema | columns    | VIEW       |
+---------------+--------------------+------------+------------+
3 rows in set. Query took 0.003 seconds.

expecting

statement to err on the second create.

Describe alternatives you've considered
A clear and concise description of any alternative solutions or features you've considered.

Additional context
Add any other context or screenshots about the feature request here.

@jimexist jimexist added the enhancement New feature or request label Nov 8, 2021
@jimexist
Copy link
Member Author

jimexist commented Nov 8, 2021

FYI @Dandandan

@Dandandan
Copy link
Contributor

Yes agree @jimexist .
Maybe we can also add support for create or replace to not error if it already exists.

@jimexist jimexist changed the title create mem table should be override current tables create mem table should not override current tables Nov 8, 2021
@alamb alamb added the good first issue Good for newcomers label Nov 8, 2021
@alamb
Copy link
Contributor

alamb commented Nov 8, 2021

I think this would be a good first issue for someone who wanted to tackle it

@liukun4515
Copy link
Contributor

I think this is a bug for management of table metadata.
If the new table is not compatible or conflict with existing table, datafusion should throw error and provide error type.

@roeap
Copy link
Contributor

roeap commented Nov 12, 2021

I'd be happy to give this a shot, but would need to understand the desired behaviour a bit better. i.e. should an error be returned only if the table is registered via the context, or should this be pushed down to the catalogue/schema level?

@xudong963
Copy link
Member

I think this is a bug for management of table metadata. If the new table is not compatible or conflict with existing table, datafusion should throw error and provide error type.

Yes, I agree, I tested in pg.
FYI

postgres=# create table users as values ('a', 'b');
SELECT 1
postgres=# create table users as values ('a', 'b');
ERROR:  relation "users" already exists

@jimexist
Copy link
Member Author

isn't that just about name conflict? no matter if the new schema is compatible or conflict with the existing one it shall not be allowed

@liukun4515
Copy link
Contributor

isn't that just about name conflict? no matter if the new schema is compatible or conflict with the existing one it shall not be allowed

Yes.
In the user side, user should get the error message which can provide the error type.
In the PG, db will give the error message like “table name already exists"

@liukun4515
Copy link
Contributor

we should forbid this in the SQL parse phase.

@alamb
Copy link
Contributor

alamb commented Nov 13, 2021

I suggest we support the postgres model here (it does not seem to have create or replace table)

create table foo (...); -- errors if a table named foo already exists
drop table foo (...); -- errors if the table does not already exists

That way it is super clear that we aren't replacing tables and the user has to explicitly drop the existing one

Bonus points for supporting create table ... if not exists, but perhaps as a follow on PR

@alamb
Copy link
Contributor

alamb commented Nov 13, 2021

Here is what postgres does:

postgres=# create table foo(x int);
CREATE TABLE

postgres=# create table foo(x int);
ERROR:  relation "foo" already exists

@liukun4515
Copy link
Contributor

I suggest we support the postgres model here (it does not seem to have create or replace table)

create table foo (...); -- errors if a table named foo already exists
drop table foo (...); -- errors if the table does not already exists

That way it is super clear that we aren't replacing tables and the user has to explicitly drop the existing one

Bonus points for supporting create table ... if not exists, but perhaps as a follow on PR

I can give another pull request to implement the drop table foo and create table .... if not exists

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants