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

Insert or Update #782

Merged
merged 3 commits into from May 13, 2014
Merged

Insert or Update #782

merged 3 commits into from May 13, 2014

Conversation

szeiger
Copy link
Member

@szeiger szeiger commented Apr 29, 2014

A ton of work to close our longest-standing feature request. Includes lots of refactoring of the insert-related code, including a new way to compile mappings from InsertCompiler which allows client-side transformations and gets rid of the requirement to use all columns in the order in which they appear in the lifted value. We should be able to reuse that later for all queries (might be useful for nested options and nested collections).

Fixes #6 and #746. Review by @cvogt.

- Introduce a new CompiledInsert abstraction in BasicProfile. Drivers
  can implement it with a compiled AST or a more abstract object that
  can compile to different kinds of insert statements and cache them.

- JdbcProfile provides its own JdbcCompiledInsert class caches
  statements on first use.

- Introduce StreamableCompiled (extends RunnableCompiled) and
  StreamingExecutable (extends Executable) to distinguish compiled
  and runnable collection-valued queries from other (record-valued
  and scalar) queries, and provide the element types. StreamableCompiled
  is needed here for inserts (you can only insert into collections) and
  can also be used later for a streaming Executor that can fully replace
  Invoker.

- Provide a StreamingExecutable for TableQuery and BaseJoinQuery (#746)

- Allow inserts from StreamableCompiled queries. Since the actual cached
  statement is contained in the JdbcCompiledInsert produced on demand
  by the StreamableCompiled, this removes the need to cache the
  InsertInvoker for efficient reuse of insert statements, thus providing
  the same kind of abstraction for cached inserts that was already used
  for query, update and delete operations since Slick 2.0.

Tests in TemplateTest.testCompiled (inserting into a Compiled query,
compiling TableQuery and BaseJoinQuery). Fixes issue #746.
- Split the code generator and mapping compiler off from InsertCompiler,
  using a standard CodeGen phase instead. CodeGen also gains a
  skeleton implementation that can be used by all existing profiles.
  (There is no need to inherit from CodeGen at all if it doesn't fit.)

- Allow efficient building of insert statements from source queries
  in InsertBuilderResult.

- Fix a bug in FullInsertInvoker.insert(query) that caused the query to
  be compiled twice.

- Add an extra `baseIdentity` TableIdentitySymbol to TableNode which is
  initialized with the same value as `identity` but keeps that value
  throughout the compilation. This TableIdentitySymbol can be used to
  check if two TableNodes point to the same database table. So far we
  used the unqualified table name for that (which was wrong).

- Clean up type-checking and the use of symbols in Insert nodes.

- Allow inserts from pre-compiled queries (as the data source) in
  addition to the ability to use pre-compiled queries for the target
  (as added in the previous commit).

- InsertColumns can now have zero or more paths. For skipped columns,
  an empty InsertColumn is generated, indicating that the value is
  part of the result type (and thus the ResultConverter) but is gets
  discarded. InsertColumns with zero paths or more than one path are
  compiled to a CompoundResultConverter.

- Remove special handling for setting full and skipping data from
  ResultSetConverter and compile different converters instead.
  Ironically, that's what I tried first a couple of months ago and then
  aborted because it seemed like a dead end. Now the approach I took
  instead turned out to be the dead end.

Test in InsertTest.testSimple (inserting from a compiled query).
- Refactor InsertInvokers: Split them off from InvokerComponent into a
  separate InsertInvokerComponent because in the JDBC implementations
  they contain a lot more code than all other Invokers combined. A
  separation into traits and protected implementation classes makes
  them more readable and allows InsertInvokers for "return ... into"
  to always work out of the box without extra customization in drivers.

- Remove the unnecessary "forced" parameter from ResultConverter.set.

- Add an UpsertBuilder to JdbcStatementBuilderComponent which builds a
  standard SQL MERGE statement by default.

- Add CheckInsertBuilder and UpdateInsertBuilder for use in emulated
  client-side insertOrUpdate operations.

- Add JdbcProfile.capabilities.insertOrUpdate to indicate native
  insertOrUpdate support in a driver.

- Move scalarFrom out of QueryBuilder to JdbcProfile (UpsertBuilder
  needs it, too).

- Make ResultConverterCompiler stateless and use the index from the
  path into the ResultSetMapping instead of enumerating columns there.
  In the future, this could also be used for query operations to allow
  transformations of client-side mappings.

Driver support (which makes you wonder why we even bother with native
MERGE support, given how limited it is):

- Hsqldb uses the standard MERGE syntax but cannot return generated
  keys from a MERGE, so we have to use the client-side emulation when
  returning keys.

- MySQL requires a custom upsert command with good semantics. We can
  use it in all cases.

- H2 and SQLite require custom upsert commands and can only perform
  upserts with forced insert semantics, so we have to use the
  client-side emulation for tables with AutoInc columns.

- PostgreSQL uses a multi-statement server-side emulation.

- The client-side emulation is used for Access and Derby.

Fixes issue #6. Tests in InsertTest.testInsertOrUpdatePlain and
InsertTest.testInsertOrUpdateAutoInc.
@szeiger
Copy link
Member Author

szeiger commented Apr 30, 2014

rebased on master

szeiger added a commit that referenced this pull request Apr 30, 2014
Backported from #782, fixes #746. Test in TemplateTest.testCompiled.
@szeiger szeiger added this to the 2.1-M2 milestone May 1, 2014
@szeiger szeiger self-assigned this May 1, 2014
@rfranco
Copy link

rfranco commented May 7, 2014

What happy if pass None to a option id and that is AutoInc?

@szeiger
Copy link
Member Author

szeiger commented May 7, 2014

That should work the same way as the AutoInc test case. It'll compare to null instead of 0, which will also fail every time. The rest is the same (since there is no difference between Option[T] and T in the generated code)

@cvogt
Copy link
Member

cvogt commented May 13, 2014

LGTM

szeiger added a commit that referenced this pull request May 13, 2014
@szeiger szeiger merged commit ba2c47a into master May 13, 2014
@szeiger szeiger deleted the tmp/insert-or-update branch May 13, 2014 16:24
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.

Add insert-or-update feature
3 participants