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 TABLE IF NOT EXISTS support #53

Closed
panarch opened this issue Aug 28, 2020 · 12 comments · Fixed by #68
Closed

CREATE TABLE IF NOT EXISTS support #53

panarch opened this issue Aug 28, 2020 · 12 comments · Fixed by #68
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@panarch
Copy link
Member

panarch commented Aug 28, 2020

IF NOT EXISTS syntax is not supported yet.
Let's make it work.

@panarch panarch added the enhancement New feature or request label Aug 28, 2020
@panarch panarch added this to the v0.2 milestone Aug 28, 2020
@panarch panarch added this to To do in v0.2 Aug 31, 2020
@panarch panarch added the good first issue Good for newcomers label Sep 4, 2020
@panarch
Copy link
Member Author

panarch commented Sep 4, 2020

It is expected to add src/tests/create_table.rs.

@zier-one
Copy link
Contributor

zier-one commented Sep 5, 2020

Hi, could I pick this up?

@panarch
Copy link
Member Author

panarch commented Sep 5, 2020

Sure! 👍

@zier-one
Copy link
Contributor

zier-one commented Sep 6, 2020

Hi, @panarch.
Through testing and reading the source code, I have found that the current behavior for creating tables is as follows:

  • When Table A does not exist, Table A is created and the result is successful.
  • When Table A already exists, create Table A. The result is successful and the old Table A will be overwritten.
  • The IF NOT EXISTS keyword will be ignored and will not work.

So I plan to implement the following two features:

  • If Table A already exists, creating Table A should return an error.
  • Make the IF NOT EXISTS keyword work.

Both of these features need to determine whether a particular table exists first, and I have observed that I can use the Store::fetch_schema function to know whether a table exists, but the problem is that the Store interface and the StoreMut interface don't seem to support transactions, So I can't guarantee that no other thread will make changes to the schema between the time I call Store::fetch_schema and StoreMut::insert_schema.

@panarch
Copy link
Member Author

panarch commented Sep 6, 2020

  • When Table A already exists, create Table A. The result is successful and the old Table A will be overwritten.
    This is obviously... yes it's a bug.
    I'm totally for your implementation plan.
    It's time to make a new friend to lonely StoreError::SchemaNotFound. 😀

And about transaction issue, you're right.
INSERT query also has a same issue, because it also requires Store::fetch_schema first.

Current implementation does not have transaction functionality.

I'm planning to provide Transaction as an one of traits in store/, and in the case of Transaction, not like Store and StoreMut it will be optional.
Programmers can choose whether implementing Transaction trait or not.
That new Transaction trait will provide not only SQL transactions using BEGIN & COMMIT, but also it will make existing mutation queries(CREATE, INSERT...) atomic.

So, it would be ok to ignore transaction issue.

And I'll also share major feature release plan soon.
Briefly.. I'm thinking about bumping minor version when GlueSQL has a new Store traits.

  • v0.2 - ALTER TABLE
  • v0.3 - FOREIGN KEY
  • v0.4 - TRANSACTION
  • v0.5 - INDEX

@ryanhossain9797
Copy link
Contributor

Hey,

* When Table A already exists, create Table A. The result is successful and the old Table A will be overwritten.

Are you sure about this behavior? From my experience, Create Table on an Existing Table does absolutely nothing at all. It doesn't harm the existing table either.

What I tested is

  • Create TableA
  • Insert 3 Rows
  • Create TableA
  • Insert 2 Rows
  • Select * From TableA

Got 5 rows back

@panarch
Copy link
Member Author

panarch commented Sep 6, 2020

No, that's certainly bug which must be fixed.
I also found this before but I forgot to add on issue stack.
In case of expected behavior, I have exactly same idea with you.

@zier-one
Copy link
Contributor

zier-one commented Sep 6, 2020

Hey,

* When Table A already exists, create Table A. The result is successful and the old Table A will be overwritten.

Are you sure about this behavior? From my experience, Create Table on an Existing Table does absolutely nothing at all. It doesn't harm the existing table either.

What I tested is

  • Create TableA
  • Insert 3 Rows
  • Create TableA
  • Insert 2 Rows
  • Select * From TableA

Got 5 rows back

You can try to create two tables with the same name but different structures, like:

CREATE TABLE Test (
    id INTEGER,
    num INTEGER,
    name TEXT
);

INSERT INTO Test (id, num, name) VALUES (1, 2, \"Hello\");


CREATE TABLE Test (
    id INTEGER,
    num INTEGER
);


INSERT INTO Test (id, num, name) VALUES (1, 2, \"Hello\"); # this insert will report an error

@panarch
Copy link
Member Author

panarch commented Sep 6, 2020

Correct behavior can be this

CREATE TABLE Test (
    id INTEGER,
    num INTEGER,
    name TEXT
);

INSERT INTO Test (id, num, name) VALUES (1, 2, \"Hello\");


CREATE TABLE Test (
    id INTEGER,
    num INTEGER
);
# this should report an error, may be... StoreError::SchemaAlreadyExists


INSERT INTO Test (id, num, name) VALUES (1, 2, \"Hello\"); # this insert should run

Current one has a bug.

@panarch panarch moved this from To do to In progress in v0.2 Sep 6, 2020
@zier-one
Copy link
Contributor

zier-one commented Sep 6, 2020

Correct behavior can be this

CREATE TABLE Test (
    id INTEGER,
    num INTEGER,
    name TEXT
);

INSERT INTO Test (id, num, name) VALUES (1, 2, \"Hello\");


CREATE TABLE Test (
    id INTEGER,
    num INTEGER
);
# this should report an error, may be... StoreError::SchemaAlreadyExists


INSERT INTO Test (id, num, name) VALUES (1, 2, \"Hello\"); # this insert should run

Current one has a bug.

That's right. I will fix it.

@ryanhossain9797
Copy link
Contributor

ryanhossain9797 commented Sep 6, 2020

Another thing I see

CREATE TABLE Test (
id INTEGER,
name TEXT
height FLOAT
);

INSERT INTO Test (id, name, height) VALUES (1, "Hello", 1.7);

CREATE TABLE Test (
id INTEGER,
age INTEGER
);

INSERT INTO Test (id, age) VALUES (2, 20); //works goes through

SELECT * FROM Test

Returns 2 rows, so the rows from the overwritten tables are still being counted, even though the structure has changed completely

@panarch
Copy link
Member Author

panarch commented Sep 6, 2020

@leoppro Yup, that's also a bug.
I added this to current issue stack. Issue #62

@panarch panarch removed this from In progress in v0.2 Oct 21, 2020
@panarch panarch removed this from the v0.2 milestone Oct 21, 2020
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.

3 participants