Compiler: Add Defaults support in bindings #3375

hfhbd · 2022-07-15T16:48:41Z

AlecKazakova · 2022-07-15T17:41:14Z

Is this a SQLDelight feature? Like would writing

INSERT INTO dog
VALUES (?, ?, DEFAULT);

be valid HSQL?

hfhbd · 2022-07-15T17:41:51Z

Yes it is valid and normal HSql

AlecKazakova · 2022-07-15T20:05:41Z

im confused then, isn't this replacing it with ? in the actual executed SQL?

hfhbd · 2022-07-15T23:00:02Z

Before the PR, all binding parameter are replaced with ?, regardless the provided string, eg DEFAULT. With my changes, in the executeBlock the binding parameter is "replaced" with the string defined in the dialect function replaceBind.
So yes, the original statement INSERT INTO dog VALUES (?, ?, DEFAULT); is now 1:1 executed in the sqldelight query INSERT INTO dog VALUES (?, ?, DEFAULT);. But the function of the query still has the unused parameter is_good, because I did only changed the executeBlock, not the actual parameter of the insert stmt. So how can I improve the implementation?

sqldelight-compiler/src/main/kotlin/app/cash/sqldelight/core/compiler/QueryGenerator.kt

hfhbd · 2022-07-17T11:20:38Z

This PR needs a mixin, specified in sql-psi. I will try to add the mixin to sqldelight.
Update: Added to sqldelight :)

hfhbd · 2022-07-17T11:47:42Z

im confused then, isn't this replacing it with ? in the actual executed SQL?

Yes, this was the problem

AlecKazakova · 2022-07-18T12:54:01Z

Oh I see, you're doing the opposite of what I was saying: before it was replacing DEFAULT with ?, this PR keeps it as DEFAULT

hfhbd · 2022-07-18T12:57:44Z

Exactly, this is the reason for the PR.

...ler/dialect/src/main/kotlin/app/cash/sqldelight/dialect/grammar/mixins/BindParameterMixin.kt

...in/src/test/integration-hsql/src/main/sqldelight/app/cash/sqldelight/hsql/integration/Dog.sq

dialects/hsql/src/main/kotlin/app/cash/sqldelight/dialects/hsql/grammar/hsql.bnf

...ts/postgresql/src/main/kotlin/app/cash/sqldelight/dialects/postgresql/grammar/PostgreSql.bnf

AlecKazakova · 2022-08-03T14:10:51Z

I will review this at some point it just requires more brain power than the easy PRs and I am short on brain power atm

AlecKazakova

I'm also okay exploring the suggested change myself later if you've spun off this

sqldelight-compiler/src/main/kotlin/app/cash/sqldelight/core/compiler/QueryGenerator.kt

Add test for PostgreSQL Spotless Move mixin to sqldelight Use mixin instead Don't add DEFAULT bindings to query Fix DEFAULT in binding

hfhbd · 2022-09-22T16:53:52Z

To test the replacement with the async code generator I also needed to fix R2DBC driver... So this PR contains some commits of #3525 and #3524, which should be merged first.

Fix docker test dependencies

Add test for PostgreSQL Spotless Move mixin to sqldelight Use mixin instead Don't add DEFAULT bindings to query Fix DEFAULT in binding

Fix docker test dependencies

AlecKazakova · 2022-09-29T14:57:59Z

...src/main/kotlin/app/cash/sqldelight/dialects/postgresql/grammar/mixins/BindParameterMixin.kt

+abstract class BindParameterMixin(node: ASTNode) : BindParameterMixin(node) {
+  override fun replaceWith(isAsync: Boolean, index: Int): String = when {
+    text == "DEFAULT" -> text
+    isAsync -> "$$index"


this feels like a driver implementation detail, that specifically for R2DBC we need indexed parameters.

We're getting closer and I'm very happy to have functioning tests for this stuff now, so this is really the only thing I'm still hung up on

Not only r2dbc. Postgres native needs indexed parameters too: https://github.com/hfhbd/postgres-native-sqldelight/blob/1a82ac52d6ab6246644fe8216932fd348afd1a9a/postgres-native-sqldelight-driver/src/commonMain/kotlin/app/softwork/sqldelight/postgresdriver/PostgresNativeDriver.kt#L165

I kinda like that approach more, doing it at the driver level

But at driver level you only have strings. So "SELECT '?'" or "LIKE 'foo?%'" does not work. (Except using sql-psi at driver level during runtime again before executing each stmt)

alright, I buy that

last suggestion then: could we just always replace arguments for the postgres driver with their indexed argument, even if its not the async driver? So that parameter can be removed and we're just saying for this dialect this is the expected parameter format

What parameter do you want to drop?
index => How do you want to do this? Hardcode a check in sqldelight-core to replace the parameter wildcard with a specific string for postgresql only?
isAsync => I have to check it

As expected, using $1 does not work on JDBC and results in an org.postgresql.util.PSQLException, because you try to bind a parameter to an unknown one:
In JDBC, the question mark (?) is the placeholder for the positional parameters of a PreparedStatement. There are, however, a number of PostgreSQL® operators that contain a question mark. To keep such question marks in an SQL statement from being interpreted as positional parameters, use two question marks ( ?? ) as escape sequence. You can also use this escape sequence in a Statement , but that is not required. Specifically only in a Statement a single ( ? ) can be used as an operator.
https://jdbc.postgresql.org/documentation/query/#using-the-statement-or-preparedstatement-interface

But I removed some useless check/binding implementations because I already check for DEFAULT

sounds good. I'm still wishing that this logic was in the driver somewhere instead of the dialect since it looks like a driver constraint, not a postgres one, but lets just merge to fix the underlying issue and then if it becomes a problem later we can revisit.

This reverts commit 4c0f582.

hfhbd commented Jul 17, 2022

View reviewed changes

sqldelight-compiler/src/main/kotlin/app/cash/sqldelight/core/compiler/QueryGenerator.kt Show resolved Hide resolved

hfhbd marked this pull request as ready for review July 17, 2022 11:19

hfhbd mentioned this pull request Jul 17, 2022

Add Mixing for BindingParameter to use it in sqldelight AlecKazakova/sql-psi#410

Closed

hfhbd marked this pull request as draft July 17, 2022 11:30

hfhbd marked this pull request as ready for review July 17, 2022 11:45

hfhbd mentioned this pull request Jul 17, 2022

Remove workaround with $1 hfhbd/postgres-native-sqldelight#50

Closed

hfhbd mentioned this pull request Jul 19, 2022

Fix AS hfhbd/sqldelight-db2-dialect#4

Closed

AlecKazakova reviewed Jul 21, 2022

View reviewed changes

AlecKazakova reviewed Jul 22, 2022

View reviewed changes

...ts/postgresql/src/main/kotlin/app/cash/sqldelight/dialects/postgresql/grammar/PostgreSql.bnf Outdated Show resolved Hide resolved

AlecKazakova approved these changes Sep 21, 2022

View reviewed changes

sqldelight-compiler/src/main/kotlin/app/cash/sqldelight/core/compiler/QueryGenerator.kt Outdated Show resolved Hide resolved

hfhbd added 4 commits September 22, 2022 11:48

Fix DEFAULT in binding

46838fb

Add test for PostgreSQL Spotless Move mixin to sqldelight Use mixin instead Don't add DEFAULT bindings to query Fix DEFAULT in binding

Remove isDefault api

293db31

Fix grammar

5c9670a

Add mysql async test

ad350f2

hfhbd mentioned this pull request Sep 22, 2022

Adopt R2DBC changes which do not always send a value #3525

Merged

Add postgresql async test

bf605c0

hfhbd added 2 commits September 22, 2022 19:02

Fix wildards

b583718

Fix code style

bf3de38

dellisd mentioned this pull request Sep 22, 2022

java.lang.UnsupportedOperationException: Binding parameters is not supported for the statement #3526

Closed

Merge branch 'master' into defaults

980d85b

hfhbd enabled auto-merge (squash) September 23, 2022 05:58

hfhbd disabled auto-merge September 23, 2022 06:03

Add default to mysql too

b2593f7

hfhbd requested a review from AlecKazakova September 23, 2022 06:11

hfhbd added 10 commits September 23, 2022 11:35

Downgrade r2dbc spi due binary incompatible with drivers

f5619f6

Fix docker test dependencies

Fix DEFAULT in binding

466a934

Add test for PostgreSQL Spotless Move mixin to sqldelight Use mixin instead Don't add DEFAULT bindings to query Fix DEFAULT in binding

Remove isDefault api

053327b

Fix grammar

19b3723

Add mysql async test

c3169c1

Add postgresql async test

cd163e9

Fix wildards

a280a97

Fix code style

18caef1

Add default to mysql too

115b2ff

Downgrade r2dbc spi due binary incompatible with drivers

e3b9eba

Fix docker test dependencies

AlecKazakova reviewed Sep 29, 2022

View reviewed changes

hfhbd and others added 4 commits October 1, 2022 09:55

Merge remote-tracking branch 'old/defaults' into defaults

e218001

Merge branch 'cashapp:master' into defaults

c909a2f

Merge remote-tracking branch 'old/defaults' into defaults

727c911

Remove useless check for default

8dcc05f

AlecKazakova approved these changes Oct 3, 2022

View reviewed changes

AlecKazakova merged commit 4c0f582 into cashapp:master Oct 3, 2022

hfhbd deleted the defaults branch October 3, 2022 21:09

AlecKazakova pushed a commit that referenced this pull request Oct 3, 2022

Revert "Compiler: Add Defaults support in bindings (#3375)"

df3fdf7

This reverts commit 4c0f582.

hfhbd restored the defaults branch October 3, 2022 22:51

hfhbd mentioned this pull request Oct 4, 2022

Replace BindParameter ? with $1 does not work in INSERT VALUES ? #3564

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compiler: Add Defaults support in bindings #3375

Compiler: Add Defaults support in bindings #3375

hfhbd commented Jul 15, 2022 •

edited

AlecKazakova commented Jul 15, 2022

hfhbd commented Jul 15, 2022 •

edited

AlecKazakova commented Jul 15, 2022

hfhbd commented Jul 15, 2022 •

edited

hfhbd commented Jul 17, 2022 •

edited

hfhbd commented Jul 17, 2022

AlecKazakova commented Jul 18, 2022

hfhbd commented Jul 18, 2022

AlecKazakova commented Aug 3, 2022

AlecKazakova left a comment

hfhbd commented Sep 22, 2022 •

edited

AlecKazakova Sep 29, 2022

hfhbd Sep 29, 2022

AlecKazakova Sep 29, 2022

hfhbd Sep 29, 2022

AlecKazakova Sep 29, 2022

hfhbd Oct 1, 2022 •

edited

hfhbd Oct 1, 2022 •

edited

AlecKazakova Oct 3, 2022

Compiler: Add Defaults support in bindings #3375

Compiler: Add Defaults support in bindings #3375

Conversation

hfhbd commented Jul 15, 2022 • edited

AlecKazakova commented Jul 15, 2022

hfhbd commented Jul 15, 2022 • edited

AlecKazakova commented Jul 15, 2022

hfhbd commented Jul 15, 2022 • edited

hfhbd commented Jul 17, 2022 • edited

hfhbd commented Jul 17, 2022

AlecKazakova commented Jul 18, 2022

hfhbd commented Jul 18, 2022

AlecKazakova commented Aug 3, 2022

AlecKazakova left a comment

Choose a reason for hiding this comment

hfhbd commented Sep 22, 2022 • edited

AlecKazakova Sep 29, 2022

Choose a reason for hiding this comment

hfhbd Sep 29, 2022

Choose a reason for hiding this comment

AlecKazakova Sep 29, 2022

Choose a reason for hiding this comment

hfhbd Sep 29, 2022

Choose a reason for hiding this comment

AlecKazakova Sep 29, 2022

Choose a reason for hiding this comment

hfhbd Oct 1, 2022 • edited

Choose a reason for hiding this comment

hfhbd Oct 1, 2022 • edited

Choose a reason for hiding this comment

AlecKazakova Oct 3, 2022

Choose a reason for hiding this comment

hfhbd commented Jul 15, 2022 •

edited

hfhbd commented Jul 15, 2022 •

edited

hfhbd commented Jul 15, 2022 •

edited

hfhbd commented Jul 17, 2022 •

edited

hfhbd commented Sep 22, 2022 •

edited

hfhbd Oct 1, 2022 •

edited

hfhbd Oct 1, 2022 •

edited