Join GitHub today
GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.
Sign upAdd support for `INSERT INTO table (...) SELECT ...` #1496
Conversation
|
This looks really good |
| fn rows_to_insert(&self) -> usize; | ||
| /// How many rows will this query insert? | ||
| /// | ||
| /// This function should only return `None` when the query is valid on all |
This comment has been minimized.
This comment has been minimized.
weiznich
Jan 19, 2018
Member
Maybe we should better use some self documenting type here instead of Option?
| @@ -43,17 +101,17 @@ impl<'a, T, Tab, DB> CanInsertInSingleQuery<DB> for BatchInsert<'a, T, Tab> | |||
| where | |||
This comment has been minimized.
This comment has been minimized.
weiznich
Jan 19, 2018
Member
I think those two implementations should be moved down to the actual structs needing them
This comment has been minimized.
This comment has been minimized.
| @@ -861,6 +862,30 @@ macro_rules! table_body { | |||
| } | |||
| } | |||
|
|
|||
| // This impl should be able to live in Diesel, | |||
| // but Rust tries to recurse for no reason | |||
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
sgrif
Jan 20, 2018
Author
Member
There are a bunch of open issues for this, one of them being rust-lang/rust#34260. I don't think it warrants a link, it's not getting fixed in the near future.
| /// | ||
| /// When inserting from a select statement, | ||
| /// the column list can be specified with [`.into_columns`]. | ||
| /// (See also [`SelectStatement::insert_into`], which generally |
This comment has been minimized.
This comment has been minimized.
weiznich
Jan 19, 2018
Member
Just to clarify this: This links to Insertable::insert_into but tells the user that this is SelectStatement::insert_into (Yes it is technical correct that SelectStatement implements Insertable but maybe this should be formulated in a other way)
This comment has been minimized.
This comment has been minimized.
sgrif
Jan 20, 2018
Author
Member
Yes, that was intentional. Even though it is on Insertable, it is designed primarily for use with select statements.
| @@ -2,9 +2,11 @@ use associations::HasTable; | |||
| use backend::Backend; | |||
| use dsl::AsExprOf; | |||
| use expression::*; | |||
| use insertable::*; | |||
This comment has been minimized.
This comment has been minimized.
This feature has been in the works for a very long time, and has a lot
of context... I've added headers so if you already know about the
iteration of the API and the evolution of `InsertStatement` internally,
skip to the third section.
Getting to this API
===
I'd like to give a bit of context on the APIs that have been considered,
and how I landed on this one.
To preface, all of the iterations around this have been trying to
carefully balance three things:
- Easy to discover in the API
- Being syntactically close to the generated SQL
- Avoiding rightward drift
For most of Diesel's life, our API was `insert(values).into(table)`.
That API was originally introduced in 0.2 "to mirror `update` and
`delete` (it didn't mirror them. It was backwards. It's always been
backwards).
My main concern with the old API actually was related to this feature.
I couldn't come up with a decent API that had you specify the column
list (you basically always need to specify the column list for this
feature).
So in 0.99 we changed it to what we have now, and I had toyed around
with `insert_into(table).columns(columns).values(select)`, as well as
`insert_into(table).from_select(columns, select)`. I was leaning towards
the second one for a while (I didn't realize at the time that it was
exactly SQLAlchemy's API). I hated the `columns` method because it was
unclear what it was doing until you saw you were inserting from a select
statement. It also implied an interaction with tuples that didn't exist.
However, another thing that happened in 0.99 was that we deprecated our
old upsert API. The `from_select` form reminded me far too much of the
old `on_conflict` API, which we had just removed. In practice what that
API would give you was something like this:
```rust
insert_into(posts::table)
.from_select(
(posts::user_id, posts::title),
users::table
.select((
users::id,
users::name.concat("'s First Post"),
)),
)
```
That's just far too much rightward drift for me. Yes, you can assign the
args to local variables, but they're awkward to name and now your code
reads weird. I thought moving the columns list to another method call
would help, but it doesn't.
```rust
insert_into(posts::table)
.from_select(
users::table
.select((
users::id,
users::name.concat("'s First Post"),
)),
)
.into_columns((posts::user_id, posts::title))
```
Eventually a member of the Diesel team had the idea of making this an
`insert_into` method on select statements. This solves all of my
concerns around rightward drift, though at the cost of the other two
priorities. The API now looked like this:
```
users::table
.select((
users::id,
users::name.concat("'s First Post"),
))
.insert_into(posts::table)
.into_columns((posts::user_id, posts::title))
```
I liked the way the code flowed, but I had concerns around
discoverability, and didn't like how backwards it was from SQL. But I
could live with it and started implementing.
Up until this point I had been assuming that we would have an
`InsertFromSelectStatement` struct, which was distinct from
`InsertStatement` and necessitated the differently named methods. I
realized when I started digging into it though, that we really just want
to re-use `InsertStatement` for this. It seems obvious in hindsight.
And if we were going to use that structure, that meant that it wouldn't
be much harder to just make passing `SelectStatement` to `values` work.
This automatically solves most of my concerns around discoverability,
since it now just works exactly like every other form of insert.
That said, I really don't like the rightward drift. I liked the
`.insert_into` form for being able to avoid that. But for the final
implementation, I just generalized that feature. *Anything* that can be
written as `insert_into(table).values(values)` can now be written as
`values.insert_into(table)`.
Context around InsertStatement
===
This file has churned more than just about any other part of Diesel. I
feel like I've re-written it nearly every release at this point. I think
the reason it's churned so much is for two reasons. The first is that
it's kept a fundamental design flaw through 1.1 (which I'll get to), and
we've been constantly working around it. The second is that `INSERT` is
actually one of the most complex queries in SQL. It has less variations
than `SELECT`, but far more of its variations are backend specific, or
have different syntaxes between backends.
`InsertStatement` was originally added for 0.2 in c9894b3 which has a
very helpful commit message "WIP" (good job past Sean). At the time we
only supported PG, so we actually had two versions -- `InsertStatement`
and `InsertQuery`, the latter having a returning clause. I'm honestly
not sure why I didn't do the `ReturningClause<T>` `NoReturningClause`
dance we do now and did back then in `SelectStatement`. Maybe I thought
it'd be easier?
Anyway this file had to go through some pretty major changes in 0.5 when
we added SQLite support. We needed to disallow batch insert and
returning clauses on that backend. Additionally, the way we handle
default values had to work differently on SQLite since it doesn't
support the `DEFAULT` keyword.
At this point though, it still a few things. The query starts with
`INSERT INTO`, had a columns list, then the word `VALUES` and then some
values. It also managed all parenthesis. (Yes it was trivial to make it
generate invalid SQL at that point).
Fast forward to 0.9, I decided to support batch insert on SQLite. This
needs to do one query per row, and I felt that meant we needed a
separate struct. I didn't want to have `BatchInsertStatement` and
`BatchInsertQuery` and `InsertStatement` and `InsertQuery`, so we got
the `NoReturningClause` struct, and got a little closer to how literally
every other part of Diesel works.
In 0.14 we added `insert_default_values` which left us with
`InsertStatement`, `BatchInsertStatement`, and `DefaultInsertStatement`,
which eventually went back down to the two.
The last time the file went through a big rewrite was in 0.99 when I
finally unified it down to the one struct we have today.
However, it still had the fatal flaw I mentioned earlier. It was trying
to handle too much logic, and was too disjoint from the rest of the
query builder. It assumed that the two forms were `DEFAULT VALUES` or
`(columns) VALUES ...`, and also handled parens.
We've gone through a lot of refactoring to get rid of that. I finally
think this struct is at a point where it will stop churning, mostly
because it looks like the rest of Diesel now. It doesn't do anything at
all, and the only thing it assumes is that the word `INTO` appears in
the query (which I'm pretty sure actually is true).
The actual implementation of this commit
====
With all that said, this commit is relatively simple. The main addition
is the `InsertFromSelect` struct. It's definitely a Diesel struct, in
that it basically does nothing, and all the logic is in its `where`
clauses.
I tried to make `Insertable` work roughly everywhere that methods like
`filter` work. I couldn't actually do a blanket impl for tables in Rust
itself, because it made rustc vomit on recursion with our impls on
`Option`. That shouldn't be happening, but I suspect it's a lot of work
to fix that in the language.
I've also implemented it for references, since most of the time that you
pass values to `.values`, you pass a reference. That should "just work"
here unless we have a good reason not to.
The majority of the additions here are tests. This is a feature that
fundamentally interacts with many of our most complex features, and I've
tried to be exhaustive. Theres ~3 lines of tests for every line of code
added. I've done at least a minimal test of this feature's interaction
with every other feature on inserts that I could think of. I am not so
much worried about whether they work, but I was worried about if there
was a syntactic edge case I didn't know about. There weren't. Same with
the compile tests, I've tried to think of every way the feature could be
accidentally misused (bad arguments to `into_columns` basically), as
well as all the nonsense things we should make sure don't work (putting
it in a tuple or vec).
I didn't add any compile tests on making sure that the select statement
itself is valid. The fact that the `Query` bound only matches valid
complete select statements is already very well tested, and we don't
need to re-test that here. I also did not explicitly disallow selecting
from the same table as the insert, as this appears to be one of the few
places where the table can appear twice with no ambiguity.
Fixes #1106
sgrif commentedJan 18, 2018
This feature has been in the works for a very long time, and has a lot
of context... I've added headers so if you already know about the
iteration of the API and the evolution of
InsertStatementinternally,skip to the third section.
Getting to this API
I'd like to give a bit of context on the APIs that have been considered,
and how I landed on this one.
To preface, all of the iterations around this have been trying to
carefully balance three things:
For most of Diesel's life, our API was
insert(values).into(table).That API was originally introduced in 0.2 "to mirror
updateanddelete(it didn't mirror them. It was backwards. It's always beenbackwards).
My main concern with the old API actually was related to this feature.
I couldn't come up with a decent API that had you specify the column
list (you basically always need to specify the column list for this
feature).
So in 0.99 we changed it to what we have now, and I had toyed around
with
insert_into(table).columns(columns).values(select), as well asinsert_into(table).from_select(columns, select). I was leaning towardsthe second one for a while (I didn't realize at the time that it was
exactly SQLAlchemy's API). I hated the
columnsmethod because it wasunclear what it was doing until you saw you were inserting from a select
statement. It also implied an interaction with tuples that didn't exist.
However, another thing that happened in 0.99 was that we deprecated our
old upsert API. The
from_selectform reminded me far too much of theold
on_conflictAPI, which we had just removed. In practice what thatAPI would give you was something like this:
That's just far too much rightward drift for me. Yes, you can assign the
args to local variables, but they're awkward to name and now your code
reads weird. I thought moving the columns list to another method call
would help, but it doesn't.
Eventually a member of the Diesel team had the idea of making this an
insert_intomethod on select statements. This solves all of myconcerns around rightward drift, though at the cost of the other two
priorities. The API now looked like this:
I liked the way the code flowed, but I had concerns around
discoverability, and didn't like how backwards it was from SQL. But I
could live with it and started implementing.
Up until this point I had been assuming that we would have an
InsertFromSelectStatementstruct, which was distinct fromInsertStatementand necessitated the differently named methods. Irealized when I started digging into it though, that we really just want
to re-use
InsertStatementfor this. It seems obvious in hindsight.And if we were going to use that structure, that meant that it wouldn't
be much harder to just make passing
SelectStatementtovalueswork.This automatically solves most of my concerns around discoverability,
since it now just works exactly like every other form of insert.
That said, I really don't like the rightward drift. I liked the
.insert_intoform for being able to avoid that. But for the finalimplementation, I just generalized that feature. Anything that can be
written as
insert_into(table).values(values)can now be written asvalues.insert_into(table).Context around InsertStatement
This file has churned more than just about any other part of Diesel. I
feel like I've re-written it nearly every release at this point. I think
the reason it's churned so much is for two reasons. The first is that
it's kept a fundamental design flaw through 1.1 (which I'll get to), and
we've been constantly working around it. The second is that
INSERTisactually one of the most complex queries in SQL. It has less variations
than
SELECT, but far more of its variations are backend specific, orhave different syntaxes between backends.
InsertStatementwas originally added for 0.2 in c9894b3 which has avery helpful commit message "WIP" (good job past Sean). At the time we
only supported PG, so we actually had two versions --
InsertStatementand
InsertQuery, the latter having a returning clause. I'm honestlynot sure why I didn't do the
ReturningClause<T>NoReturningClausedance we do now and did back then in
SelectStatement. Maybe I thoughtit'd be easier?
Anyway this file had to go through some pretty major changes in 0.5 when
we added SQLite support. We needed to disallow batch insert and
returning clauses on that backend. Additionally, the way we handle
default values had to work differently on SQLite since it doesn't
support the
DEFAULTkeyword.At this point though, it still a few things. The query starts with
INSERT INTO, had a columns list, then the wordVALUESand then somevalues. It also managed all parenthesis. (Yes it was trivial to make it
generate invalid SQL at that point).
Fast forward to 0.9, I decided to support batch insert on SQLite. This
needs to do one query per row, and I felt that meant we needed a
separate struct. I didn't want to have
BatchInsertStatementandBatchInsertQueryandInsertStatementandInsertQuery, so we gotthe
NoReturningClausestruct, and got a little closer to how literallyevery other part of Diesel works.
In 0.14 we added
insert_default_valueswhich left us withInsertStatement,BatchInsertStatement, andDefaultInsertStatement,which eventually went back down to the two.
The last time the file went through a big rewrite was in 0.99 when I
finally unified it down to the one struct we have today.
However, it still had the fatal flaw I mentioned earlier. It was trying
to handle too much logic, and was too disjoint from the rest of the
query builder. It assumed that the two forms were
DEFAULT VALUESor(columns) VALUES ..., and also handled parens.We've gone through a lot of refactoring to get rid of that. I finally
think this struct is at a point where it will stop churning, mostly
because it looks like the rest of Diesel now. It doesn't do anything at
all, and the only thing it assumes is that the word
INTOappears inthe query (which I'm pretty sure actually is true).
The actual implementation of this commit
With all that said, this commit is relatively simple. The main addition
is the
InsertFromSelectstruct. It's definitely a Diesel struct, inthat it basically does nothing, and all the logic is in its
whereclauses.
I tried to make
Insertablework roughly everywhere that methods likefilterwork. I couldn't actually do a blanket impl for tables in Rustitself, because it made rustc vomit on recursion with our impls on
Option. That shouldn't be happening, but I suspect it's a lot of workto fix that in the language.
I've also implemented it for references, since most of the time that you
pass values to
.values, you pass a reference. That should "just work"here unless we have a good reason not to.
The majority of the additions here are tests. This is a feature that
fundamentally interacts with many of our most complex features, and I've
tried to be exhaustive. Theres ~3 lines of tests for every line of code
added. I've done at least a minimal test of this feature's interaction
with every other feature on inserts that I could think of. I am not so
much worried about whether they work, but I was worried about if there
was a syntactic edge case I didn't know about. There weren't. Same with
the compile tests, I've tried to think of every way the feature could be
accidentally misused (bad arguments to
into_columnsbasically), aswell as all the nonsense things we should make sure don't work (putting
it in a tuple or vec).
I didn't add any compile tests on making sure that the select statement
itself is valid. The fact that the
Querybound only matches validcomplete select statements is already very well tested, and we don't
need to re-test that here. I also did not explicitly disallow selecting
from the same table as the insert, as this appears to be one of the few
places where the table can appear twice with no ambiguity.
Fixes #1106