/diesel

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

[SQLite] SQLite has no support for the `DEFAULT` keyword #157

Closed
sgrif opened this Issue Jan 31, 2016 · 1 comment

Comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
  0.5
1 participant
@sgrif
@sgrif
Member

sgrif commented Jan 31, 2016

Note: PRs addressing this issue should target the diesel-sqlite-support branch, not master.

I don't see a good way to work around this and support batch inserts for the time being. I think the short term solution comes in 3 steps.

  • Make Insertable generic over the backend, so we can change the behavior for SQLite.
  • On SQLite, when inserting a single record, instead of generating INSERT INTO users (name, hair_color) VALUES ('Tess', DEFAULT), instead generate INSERT INTO users (name) VALUES ('Tess').
  • On SQLite, disallow batch inserts.

We can revisit batch inserts in the future. Single inserts are the overwhelmingly common case, so let's focus on that for now.

@sgrif sgrif added this to the 0.5 milestone Jan 31, 2016

sgrif added a commit that referenced this issue Jan 31, 2016

@sgrif
Disallow batch insert on SQLite 
Related to #157. Whether this functionality is eventually added back is
out of scope for 0.5. What is clear is that we need to fix the single
record case on SQLite, and that we cannot safely insert multiple records
in a single query on SQLite.

This does not fix the single record case, and does not resolve the
issue. However, the actual solution to that will be much more complex,
and this is comparatively simple -- and will be a pre-requisite for any
solution to the actual issue.
Loading status checks…
10caf03

@sgrif sgrif referenced this issue Jan 31, 2016

Merged

Disallow batch insert on SQLite #164

sgrif added a commit that referenced this issue Feb 1, 2016

@sgrif
Fix inserting default values on SQLite 
Our previous infrastructure relied on the ability to use the `DEFAULT`
keyword for values which were not present. This keyword does not exist
on SQLite. This means that the query:

```sql
INSERT INTO users (name, hair_color) VALUES ('Tess', DEFAULT)
```

needs to instead be written as

```sql
INSERT INTO users (name) VALUES ('Tess')
```

This does not work for batch inserts, where some of the rows provide
values, but others want the defaults. There is no way to express this in
a single query using SQLite. Because of this, we disallowed batch
inserts for SQLite entirely in 10caf03.
This issue will be revisted after 0.5 is released.

There were several changes included in this commit which are
tangentially related to this change.

We were able to remove the "context" concept from our query builder
entirely. This was only there to insert the `"DEFAULT"` keyword instead
of a bind param on nulls during insert. It was never something I was
super happy with, and it resulted in allocations that the compiler would
never be able to optimize away. With this change, we have a better place
to handle this.

I've had to alter codegen to specifically reference `Bound` instead of
using the `AsExpr` helper which is technically corrent. Without that
change, we get some very obscure lifetime errors that I honestly don't
fully understand. This is either a bug in Rust itself, or some *very*
obscure interaction with
https://github.com/sgrif/diesel/blob/8a33029/diesel/src/types/impls/mod.rs#L23-L29
that I do not understand (I believe it is a bug in Rust, given that it
only manifests in `where` clauses. I have been unable to isolate it in a
playground link, but I will follow up with the core team tomorrow on
this.)

The changes that were required to the `migrations` module are
unfortunate, but necessary. Even though SQLite is the *only* backend
which would not implement `SupportsDefaultKeyword`, I have no way to
represent this in the type system. As such, I can provide no evidence
that a tuple of `ColumnInsertValue`s always implements
`InsertValues<DB>` for all possible `DB`. This can go away once
specialization lands in stable.

Ultimately we can likely re-implement batch inserts for SQLite in the
future by having it execute one query per row under the hood. As has
been pointed out to me numerous times in other places, since SQLite is
an embedded databbase, there is no downside to doing 10 queries instead
of 1 (assuming we're in a transaction so there is no IO cost).

Fixes #157
3d15d94

@sgrif sgrif referenced this issue Feb 1, 2016

Merged

Fix inserting default values on SQLite #166

sgrif added a commit that referenced this issue Feb 1, 2016

@sgrif
Fix inserting default values on SQLite 
Our previous infrastructure relied on the ability to use the `DEFAULT`
keyword for values which were not present. This keyword does not exist
on SQLite. This means that the query:

```sql
INSERT INTO users (name, hair_color) VALUES ('Tess', DEFAULT)
```

needs to instead be written as

```sql
INSERT INTO users (name) VALUES ('Tess')
```

This does not work for batch inserts, where some of the rows provide
values, but others want the defaults. There is no way to express this in
a single query using SQLite. Because of this, we disallowed batch
inserts for SQLite entirely in 10caf03.
This issue will be revisted after 0.5 is released.

There were several changes included in this commit which are
tangentially related to this change.

We were able to remove the "context" concept from our query builder
entirely. This was only there to insert the `"DEFAULT"` keyword instead
of a bind param on nulls during insert. It was never something I was
super happy with, and it resulted in allocations that the compiler would
never be able to optimize away. With this change, we have a better place
to handle this.

I've had to alter codegen to specifically reference `Bound` instead of
using the `AsExpr` helper which is technically corrent. Without that
change, we get some very obscure lifetime errors that I honestly don't
fully understand. This is either a bug in Rust itself, or some *very*
obscure interaction with
https://github.com/sgrif/diesel/blob/8a33029/diesel/src/types/impls/mod.rs#L23-L29
that I do not understand (I believe it is a bug in Rust, given that it
only manifests in `where` clauses. I have been unable to isolate it in a
playground link, but I will follow up with the core team tomorrow on
this.)

The changes that were required to the `migrations` module are
unfortunate, but necessary. Even though SQLite is the *only* backend
which would not implement `SupportsDefaultKeyword`, I have no way to
represent this in the type system. As such, I can provide no evidence
that a tuple of `ColumnInsertValue`s always implements
`InsertValues<DB>` for all possible `DB`. This can go away once
specialization lands in stable.

Ultimately we can likely re-implement batch inserts for SQLite in the
future by having it execute one query per row under the hood. As has
been pointed out to me numerous times in other places, since SQLite is
an embedded databbase, there is no downside to doing 10 queries instead
of 1 (assuming we're in a transaction so there is no IO cost).

Fixes #157
b7608a5

sgrif added a commit that referenced this issue Feb 1, 2016

@sgrif
Fix inserting default values on SQLite 
Our previous infrastructure relied on the ability to use the `DEFAULT`
keyword for values which were not present. This keyword does not exist
on SQLite. This means that the query:

```sql
INSERT INTO users (name, hair_color) VALUES ('Tess', DEFAULT)
```

needs to instead be written as

```sql
INSERT INTO users (name) VALUES ('Tess')
```

This does not work for batch inserts, where some of the rows provide
values, but others want the defaults. There is no way to express this in
a single query using SQLite. Because of this, we disallowed batch
inserts for SQLite entirely in 10caf03.
This issue will be revisted after 0.5 is released.

There were several changes included in this commit which are
tangentially related to this change.

We were able to remove the "context" concept from our query builder
entirely. This was only there to insert the `"DEFAULT"` keyword instead
of a bind param on nulls during insert. It was never something I was
super happy with, and it resulted in allocations that the compiler would
never be able to optimize away. With this change, we have a better place
to handle this.

I've had to alter codegen to specifically reference `Bound` instead of
using the `AsExpr` helper which is technically corrent. Without that
change, we get some very obscure lifetime errors that I honestly don't
fully understand. This is either a bug in Rust itself, or some *very*
obscure interaction with
https://github.com/sgrif/diesel/blob/8a33029/diesel/src/types/impls/mod.rs#L23-L29
that I do not understand (I believe it is a bug in Rust, given that it
only manifests in `where` clauses. I have been unable to isolate it in a
playground link, but I will follow up with the core team tomorrow on
this.)

The changes that were required to the `migrations` module are
unfortunate, but necessary. Even though SQLite is the *only* backend
which would not implement `SupportsDefaultKeyword`, I have no way to
represent this in the type system. As such, I can provide no evidence
that a tuple of `ColumnInsertValue`s always implements
`InsertValues<DB>` for all possible `DB`. This can go away once
specialization lands in stable.

Ultimately we can likely re-implement batch inserts for SQLite in the
future by having it execute one query per row under the hood. As has
been pointed out to me numerous times in other places, since SQLite is
an embedded databbase, there is no downside to doing 10 queries instead
of 1 (assuming we're in a transaction so there is no IO cost).

Fixes #157
Loading status checks…
35ee3ea
@sgrif

This comment has been minimized.

Sign in to view
Member

sgrif commented Feb 1, 2016

Fixed by #166

@sgrif sgrif closed this Feb 1, 2016

sgrif added a commit that referenced this issue Feb 3, 2016

@sgrif
Disallow batch insert on SQLite 
Related to #157. Whether this functionality is eventually added back is
out of scope for 0.5. What is clear is that we need to fix the single
record case on SQLite, and that we cannot safely insert multiple records
in a single query on SQLite.

This does not fix the single record case, and does not resolve the
issue. However, the actual solution to that will be much more complex,
and this is comparatively simple -- and will be a pre-requisite for any
solution to the actual issue.
4483fa7

sgrif added a commit that referenced this issue Feb 3, 2016

@sgrif
Fix inserting default values on SQLite 
Our previous infrastructure relied on the ability to use the `DEFAULT`
keyword for values which were not present. This keyword does not exist
on SQLite. This means that the query:

```sql
INSERT INTO users (name, hair_color) VALUES ('Tess', DEFAULT)
```

needs to instead be written as

```sql
INSERT INTO users (name) VALUES ('Tess')
```

This does not work for batch inserts, where some of the rows provide
values, but others want the defaults. There is no way to express this in
a single query using SQLite. Because of this, we disallowed batch
inserts for SQLite entirely in 10caf03.
This issue will be revisted after 0.5 is released.

There were several changes included in this commit which are
tangentially related to this change.

We were able to remove the "context" concept from our query builder
entirely. This was only there to insert the `"DEFAULT"` keyword instead
of a bind param on nulls during insert. It was never something I was
super happy with, and it resulted in allocations that the compiler would
never be able to optimize away. With this change, we have a better place
to handle this.

I've had to alter codegen to specifically reference `Bound` instead of
using the `AsExpr` helper which is technically corrent. Without that
change, we get some very obscure lifetime errors that I honestly don't
fully understand. This is either a bug in Rust itself, or some *very*
obscure interaction with
https://github.com/sgrif/diesel/blob/8a33029/diesel/src/types/impls/mod.rs#L23-L29
that I do not understand (I believe it is a bug in Rust, given that it
only manifests in `where` clauses. I have been unable to isolate it in a
playground link, but I will follow up with the core team tomorrow on
this.)

The changes that were required to the `migrations` module are
unfortunate, but necessary. Even though SQLite is the *only* backend
which would not implement `SupportsDefaultKeyword`, I have no way to
represent this in the type system. As such, I can provide no evidence
that a tuple of `ColumnInsertValue`s always implements
`InsertValues<DB>` for all possible `DB`. This can go away once
specialization lands in stable.

Ultimately we can likely re-implement batch inserts for SQLite in the
future by having it execute one query per row under the hood. As has
been pointed out to me numerous times in other places, since SQLite is
an embedded databbase, there is no downside to doing 10 queries instead
of 1 (assuming we're in a transaction so there is no IO cost).

Fixes #157
205c807

sgrif added a commit that referenced this issue Feb 3, 2016

@sgrif
Disallow batch insert on SQLite 
Related to #157. Whether this functionality is eventually added back is
out of scope for 0.5. What is clear is that we need to fix the single
record case on SQLite, and that we cannot safely insert multiple records
in a single query on SQLite.

This does not fix the single record case, and does not resolve the
issue. However, the actual solution to that will be much more complex,
and this is comparatively simple -- and will be a pre-requisite for any
solution to the actual issue.
de312db

sgrif added a commit that referenced this issue Feb 3, 2016

@sgrif
Fix inserting default values on SQLite 
Our previous infrastructure relied on the ability to use the `DEFAULT`
keyword for values which were not present. This keyword does not exist
on SQLite. This means that the query:

```sql
INSERT INTO users (name, hair_color) VALUES ('Tess', DEFAULT)
```

needs to instead be written as

```sql
INSERT INTO users (name) VALUES ('Tess')
```

This does not work for batch inserts, where some of the rows provide
values, but others want the defaults. There is no way to express this in
a single query using SQLite. Because of this, we disallowed batch
inserts for SQLite entirely in 10caf03.
This issue will be revisted after 0.5 is released.

There were several changes included in this commit which are
tangentially related to this change.

We were able to remove the "context" concept from our query builder
entirely. This was only there to insert the `"DEFAULT"` keyword instead
of a bind param on nulls during insert. It was never something I was
super happy with, and it resulted in allocations that the compiler would
never be able to optimize away. With this change, we have a better place
to handle this.

I've had to alter codegen to specifically reference `Bound` instead of
using the `AsExpr` helper which is technically corrent. Without that
change, we get some very obscure lifetime errors that I honestly don't
fully understand. This is either a bug in Rust itself, or some *very*
obscure interaction with
https://github.com/sgrif/diesel/blob/8a33029/diesel/src/types/impls/mod.rs#L23-L29
that I do not understand (I believe it is a bug in Rust, given that it
only manifests in `where` clauses. I have been unable to isolate it in a
playground link, but I will follow up with the core team tomorrow on
this.)

The changes that were required to the `migrations` module are
unfortunate, but necessary. Even though SQLite is the *only* backend
which would not implement `SupportsDefaultKeyword`, I have no way to
represent this in the type system. As such, I can provide no evidence
that a tuple of `ColumnInsertValue`s always implements
`InsertValues<DB>` for all possible `DB`. This can go away once
specialization lands in stable.

Ultimately we can likely re-implement batch inserts for SQLite in the
future by having it execute one query per row under the hood. As has
been pointed out to me numerous times in other places, since SQLite is
an embedded databbase, there is no downside to doing 10 queries instead
of 1 (assuming we're in a transaction so there is no IO cost).

Fixes #157
3d32b97
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment