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

AUTO_INCREMENT #159

Open
KyGost opened this issue Mar 3, 2021 · 12 comments
Open

AUTO_INCREMENT #159

KyGost opened this issue Mar 3, 2021 · 12 comments
Labels
enhancement New feature or request

Comments

@KyGost
Copy link
Contributor

KyGost commented Mar 3, 2021

It would be quite nice to have an AUTO_INCREMENT feature for INTEGER columns.

There's a few approaches to how this could work though:

SQLite automatically acts on INTEGER PRIMARY KEY columns, backfilling where values are removed while the keyword specifies that backfill should not occur.
https://sqlite.org/autoinc.html

MySQL acts based on the current largest value in the column when the keyword is used.
https://dev.mysql.com/doc/refman/8.0/en/example-auto-increment.html

MSSQL stores a value and uses that. The value can be altered.
https://docs.microsoft.com/en-us/sql/t-sql/statements/create-table-transact-sql-identity-property?view=sql-server-ver15

Anyone have any preference?

I'm not sure that I like how SQLite increments by default.

I kinda like the MySQL method; MSSQL sounds a bit messy and I worry for the potential issues of backfilling (SQLite).

@panarch panarch added the enhancement New feature or request label Mar 3, 2021
@panarch
Copy link
Member

panarch commented Mar 3, 2021

I'm also familiar with MySQL's AUTO_INCREMENT syntax.

There can be something to consider about the implementation plan.
I'm not sure that current Store trait methods are enough to support AUTO_INCREMENT.
Then, we might need to add another optional store trait for this?

@KyGost
Copy link
Contributor Author

KyGost commented Mar 4, 2021

@KyGost
Copy link
Contributor Author

KyGost commented Mar 4, 2021

I tried a number of ways writing it how I might expect it to be writable and with the current sqlparser, id INTEGER CONSTRAINT AUTO_INCREMENT NOT NULL seems to be the only reasonable way.

(With the NOT NULL always required)

Thoughts on AUTO_INCREMENT vs AUTOINCREMENT?

@KyGost
Copy link
Contributor Author

KyGost commented Mar 4, 2021

nb:

id INTEGER CONSTRAINT AUTO_INCREMENT NOT NULL

Gives us:

[ColumnDef { name: Ident { value: "id", quote_style: None }, data_type: Int, collation: None, options: [ColumnOptionDef { name: Some(Ident { value: "AUTO_INCREMENT", quote_style: None }), option: NotNull }] }]

@KyGost
Copy link
Contributor Author

KyGost commented Mar 4, 2021

CONSTRAINT AUTO_INCREMENT UNIQUE & CONSTRAINT AUTO_INCREMENT PRIMARY KEY also works

@KyGost
Copy link
Contributor Author

KyGost commented Mar 5, 2021

I was trying to implement this using a validation check but it seems this would actually be quite simple by using the existing row/default code.

The question just becomes how to decide the value to use.

Doing a scan seems it would be rather expensive but once indexes are in (esp considering AUTO_INCREMENT will probably be very often PK) it shouldn't be too bad.

@KyGost
Copy link
Contributor Author

KyGost commented Mar 5, 2021

Hmmmmm
That wouldn't work for multi row nor simultaneous inserts.
I suppose maybe the value will need to be stored in a schema.

If this is the case it would probably be most efficient to just use the MSSQL method of just storing and reseeding as needed.

If we wanted it to work otherwise it would probably need to check the value each update/insert (specified or not) and update it when that value is bigger &| maybe when values are deleted.

@KyGost
Copy link
Contributor Author

KyGost commented Mar 5, 2021

@panarch do you think it would be better to add a generated parameter on schema or leave it to the store and add a new prefix for sled?

@panarch
Copy link
Member

panarch commented Mar 5, 2021

@KyGost
In the case of MySQL syntax, we might be able to use CREATE TABLE Test (id INTEGER AUTO_INCREMENT NOT NULL) without using CONSTRAINT keyword

..
ColumnOptionDef {
    name: None,
    option: DialectSpecific([
        Word(Word { value: "AUTO_INCREMENT", quote_style: None, keyword: AUTO_INCREMENT })
    ]) 
..

This above is supported by the latest version of sqlparser.

However, GlueSQL is currently using sqlparser@v0.6.1, the latest is v0.8.0. I'll migrate to newer ASAP.

The reason you needed to use CONSTRAINT keyword is because of sqlparser?

@KyGost
Copy link
Contributor Author

KyGost commented Mar 5, 2021

@panarch
Ah awesome!

Because of splparser?

Yup.

@panarch
Copy link
Member

panarch commented Mar 5, 2021

And the design I briefly thought was something like this.

Adding optional store trait

pub trait AutoIncrement<T: Debug>  {
    fn generate_id() -> T;
}

and implementation idea for SledStorage

  1. fetching the biggest item
  2. add 1, and return

However, without index this above implementation plan can be quite inefficient, so it would be good to do something more on this, like you suggested in the thread.
(This would be ok with bulk insert INSERT INTO Table VALUES ("item1"), ("item2"), ...; but if someone tries to split insert into independent INSERT queries, then it can become O(n^2) hell.)

In anyway, the point would be... having store trait api as simple as possible

@panarch
Copy link
Member

panarch commented Mar 5, 2021

@KyGost 🚀 #160 🚀

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

Successfully merging a pull request may close this issue.

2 participants