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

[8.x] Adds "asRowid" modifier, making a column an alias of "rowid" #35814

Closed
wants to merge 1 commit into from

Conversation

Max13
Copy link
Contributor

@Max13 Max13 commented Jan 7, 2021

After #35792, I thought the best would be to write a modifier allowing the user to explicitly specify a column as rowid.

While, IMHO, laravel should implement every increments and id asRowId by default, this would be a breaking change in behavior. Maybe for the next major release, we can do the opposite, create a ->trueAutoIncrement() modifier to explicitly use the SQLite AUTOINCREMENT mechanism.

This modifier won't do anything on other drivers.

@taylorotwell
Copy link
Member

Thanks for your pull request to Laravel!

Unfortunately, I'm going to delay merging this code for now. To preserve our ability to adequately maintain the framework, we need to be very careful regarding the amount of code we include.

If possible, please consider releasing your code as a package so that the community can still take advantage of your contributions!

If you feel absolutely certain that this code corrects a bug in the framework, please "@" mention me in a follow-up comment with further explanation so that GitHub will send me a notification of your response.

@Max13
Copy link
Contributor Author

Max13 commented Jan 7, 2021

@taylorotwell Sorry to ping you if at the end you still consider this PR to be delayed or anything else.

To make it short, every SQLite table (which aren't specifically WITHOUT ROWID) have a rowid column which has the "increment" attributes (is an INTEGER (64bits signed), starts at 1, is autoincrementing, is unique across a table and is really fast searching). BUT because of a bug in SQLite, if you don't use the exact column type "INTEGER" (and not "INT", not "BIGINT", …) as a PRIMARY KEY, then the column won't be an alias of rowid and will be a duplicate. Note that AUTOINCREMENT keyword isn't used as it will use a different algorithm:

Imposes extra CPU, memory, disk space, and disk I/O overhead and should be avoided if not strictly needed.

Currently, laravel creates a INTEGER NOT NULL PRIMARY KEY AUTOINCREMENT for every increments, which means these ids are both not alias for rowid (so double work) and are using AUTOINCREMENT which in most cases isn't necessary.

As changing default behavior of laravel in this situation would be a breaking change, I suggested with this PR the creation of a modifier instead, allowing a user to write $table->increments('id')->asRowid() and it will make the column an alias for rowid (without doing anything to other drivers). I'm aware this is not a laravel bug, but adding ->asRowid() to $table->increments('id') is shorter than rewriting it to $table->integer('id')->nullable()->primary() (as rowid is nullable because of the mentioned bug).

@ratatatKE
Copy link

ratatatKE commented Jan 8, 2021

@taylorotwell Sorry to ping you if at the end you still consider this PR to be delayed or anything else.

To make it short, every SQLite table (which aren't specifically WITHOUT ROWID) have a rowid column which has the "increment" attributes (is an INTEGER (64bits signed), starts at 1, is autoincrementing, is unique across a table and is really fast searching). BUT because of a bug in SQLite, if you don't use the exact column type "INTEGER" (and not "INT", not "BIGINT", …) as a PRIMARY KEY, then the column won't be an alias of rowid and will be a duplicate. Note that AUTOINCREMENT keyword isn't used as it will use a different algorithm:

Imposes extra CPU, memory, disk space, and disk I/O overhead and should be avoided if not strictly needed.

Currently, laravel creates a INTEGER NOT NULL PRIMARY KEY AUTOINCREMENT for every increments, which means these ids are both not alias for rowid (so double work) and are using AUTOINCREMENT which in most cases isn't necessary.

As changing default behavior of laravel in this situation would be a breaking change, I suggested with this PR the creation of a modifier instead, allowing a user to write $table->increments('id')->asRowid() and it will make the column an alias for rowid (without doing anything to other drivers). I'm aware this is not a laravel bug, but adding ->asRowid() to $table->increments('id') is shorter than rewriting it to $table->integer('id')->nullable()->primary() (as rowid is nullable because of the mentioned bug).

@Max13 Laravel migrations typically don't work well with SQLite (Think in memory database testing & migrations). However majority of the time I think it's better Laravel does not bend over to accommodate these but rather leave these to be handled by the developer using SQLite.

That said, for all it's worth I'd also suggest the method name be asSqliteRowId() since this rowid specification affects only the sqlite database driver.

@Max13
Copy link
Contributor Author

Max13 commented Jan 9, 2021

@ratatatKE I agree with you that these kind of "issues" should be handled by the dev. But, I disagree about this specific issue because (IMO) Laravel isn't simply implementing the SQL standards but it's rather simplifying developers life.

SQLite should have respected the standards, but what was originally a bug became a feature (if I may call it that way), then it becomes relevant for Laravel to implement things (here, the Schema Grammar) the most efficient way for the developer. Note that SQLite themselves advise against using a duplicate of rowid and using AUTOINCREMENT, I consider this a specificity of their syntax, that's why I think in this situation, Laravel should take it in consideration.

Thank you for your idea, I might change the modifier in another commit

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

Successfully merging this pull request may close these issues.

None yet

3 participants