Returning Record #1489

deusaquilus · 2019-06-21T07:49:41Z

Trying out build similar to #1383 (fix for #572).

Unit test all changes
Update README.md if applicable
Add [WIP] to the pull request title if it's work in progress
Squash commits that aren't meaningful changes
Run sbt scalariformFormat test:scalariformFormat to make sure that the source files are formatted

@getquill/maintainers

mosyp

At the middle of PR I've lost on various changes, it really makes sense to me to split this PR (more in a comment below).
Also I've notice that in a lot of places returning itself is changed to returningGenerated, but the first one is not deleted from codebase, so it means that both will be in use? A short summary of a returning design in PR as well as in readme would be nice

mosyp · 2019-07-04T10:56:15Z

quill-cassandra/src/main/scala/io/getquill/context/cassandra/CqlIdiom.scala

@@ -175,6 +181,9 @@ trait CqlIdiom extends Idiom {
      case _: Returning =>


minor: _: ReturningGenerated | _: Returning

mosyp · 2019-07-04T10:57:33Z

quill-core/src/main/scala/io/getquill/ast/Ast.scala

@@ -69,6 +69,10 @@ case class Function(params: List[Ident], body: Ast) extends Ast

 case class Ident(name: String) extends Ast

+// Like identity but is but defined in a clause external to the query. Currently this is used


I'm suffering from "The Curse of Knowledge" here. What part do you mean?

mosyp · 2019-07-04T11:03:01Z

quill-core/src/main/scala/io/getquill/dsl/ValueComputation.scala

@@ -0,0 +1,139 @@
+package io.getquill.dsl


I'd make a separate commit for such refactor before introducing returning record feature. So it'd be easy to track

This makes sense. I'll try to separate it out.

Okay. There are 4 commits now.

mosyp · 2019-07-04T11:11:25Z

quill-core/src/main/scala/io/getquill/context/ActionMacro.scala

+        case ret: io.getquill.ast.ReturningAction =>
+            io.getquill.norm.ExpandReturning.applyMap(ret)(
+              (ast, statement) => io.getquill.context.Expand(${c.prefix}, ast, statement, idiom, naming).string
+            )(idiom, naming)
        case ast =>


missing case io.getquill.ast.Returning ?

So ReturningAction is a de-composer that matches Returning or ReturningGenerated, I defined it in Ast.scala. I decide to write it since their structure is identical. I'm not sure I like the name "ReturningAction" though. Maybe "ReturningAst"? Or maybe it shouldn't start with a capital letter. Any thoughts?

deusaquilus · 2019-07-05T03:15:11Z

Im sorry that I forgot to mention something important. The insert(entity).returning(_.id) API used to not only add id SQL RETURNING clause but also removed It from the Insert clause as well. After a few conversations with Flavio and others, we concluded that if we want to support returning tuples etc.. in the returning clause eg .returning(r => (r.foo, r.bar)) it is very unclear that the behavior of returning is to exclude foo and bar from the initial insertion. Also, we wanted to have an API that does not exclude records at all. Therefore, we decided to make .returning do exactly that and .returningGenerated return generated columns I.e. columns that are auto-generated therefore should be excluded from the Insert.

deusaquilus · 2019-07-05T08:02:22Z

The other thing that we realized was that whether a single column (MySQL, Sqlite etc...), multiple columns (Oracle) or multiple columns with an actual RETURNING clause that supports sql operations (Postgres) is supported has to be controlled by the idiom as opposed to the context. MirrorContext for instance has to generate RETURNING clauses for only Postgres but... allow multiple column tuples/case-classes for Oracle and allow only one return variable for other databases the same way in MirrorContext as well as JdbcContext. So the dialect has to control these things. Also,
we need to know all of this during compile time during. For the reason we introduced the RuturnCapability types. These live in the dialects but can be inspected from anything that has access to the context including the Parser, the Metas etc...

mosyp · 2019-07-05T15:40:06Z

@deusaquilus Okay, now it's clear, great work! Except missing readme update, everything else lgtm.

This was referenced Jun 21, 2019

[WIP] return inserted record #1455

Closed

[WIP] inserted record can be returned from query #1383

Closed

Add SQL Server Support for returning via OUTPUT #1497

Closed

deusaquilus force-pushed the returning_record_redo branch 11 times, most recently from 838eb41 to 7219984 Compare July 4, 2019 06:56

deusaquilus changed the title ~~[WIP] Returning Record Redo #2~~ Returning Record Redo #2 Jul 4, 2019

deusaquilus changed the title ~~Returning Record Redo #2~~ Returning Record RC Jul 4, 2019

deusaquilus requested review from fwbrasil, mxl and jilen July 4, 2019 06:57

mosyp reviewed Jul 4, 2019

View reviewed changes

deusaquilus force-pushed the returning_record_redo branch 2 times, most recently from 7d31c72 to f58be32 Compare July 5, 2019 06:50

deusaquilus added 3 commits July 5, 2019 02:51

Additions for Bloop, Metals, and Intellij Root Dir

07c6729

Fix intermittent issues from out-of-order elements.

42389c3

Extract out reuseable parts of Meta Macro

49e83d6

deusaquilus force-pushed the returning_record_redo branch 2 times, most recently from da165c8 to 341016f Compare July 5, 2019 07:44

returning support for case-classes/tuples with multiple columns.

f1c8eb4

deusaquilus force-pushed the returning_record_redo branch from 341016f to f1c8eb4 Compare July 5, 2019 14:41

deusaquilus merged commit ca23d3c into zio:master Jul 5, 2019

deusaquilus changed the title ~~Returning Record RC~~ Returning Record Jul 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Returning Record #1489

Returning Record #1489

deusaquilus commented Jun 21, 2019 •

edited

mosyp left a comment •

edited

mosyp Jul 4, 2019

deusaquilus Jul 5, 2019 •

edited

mosyp Jul 4, 2019

deusaquilus Jul 4, 2019

mosyp Jul 4, 2019

deusaquilus Jul 4, 2019

deusaquilus Jul 5, 2019

mosyp Jul 4, 2019

deusaquilus Jul 4, 2019

deusaquilus commented Jul 5, 2019

deusaquilus commented Jul 5, 2019

mosyp commented Jul 5, 2019

		@@ -175,6 +181,9 @@ trait CqlIdiom extends Idiom {
		case _: Returning =>

		@@ -69,6 +69,10 @@ case class Function(params: List[Ident], body: Ast) extends Ast

		case class Ident(name: String) extends Ast

		// Like identity but is but defined in a clause external to the query. Currently this is used

Returning Record #1489

Returning Record #1489

Conversation

deusaquilus commented Jun 21, 2019 • edited

mosyp left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deusaquilus Jul 5, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deusaquilus commented Jul 5, 2019

deusaquilus commented Jul 5, 2019

mosyp commented Jul 5, 2019

deusaquilus commented Jun 21, 2019 •

edited

mosyp left a comment •

edited

deusaquilus Jul 5, 2019 •

edited