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

[hfhbd]

SQLDelight Version

2.0.0-alpha04

Operating System

macOS

Gradle Version

7.5.1

Kotlin Version

1.7.20

Dialect

Postgresql

AGP Version

No response

Describe the Bug

While #3375 adds support to replace ? to $1 in code generating, #3375 (comment), this does not work when sqldelight replaces a single ? with multiple ? in an insert statement:

insert:
INSERT INTO foo VALUES ?;

Results into: INSERT INTO foo VALUES (?, ?, ?), but should be: INSERT INTO foo VALUES ($1, $2, $3) too.

Stacktrace

No response

Gradle Build Script

No response

[AlecKazakova]

yea okay this further makes me believe the driver should be more replacing these instead of the codegen

[hfhbd]

This makes only sense if sqldelight provides the range of ? (bindExpr) to prevent replacing wrong ?s. Parsing the sql statement (again) during runtime is a huge performance problem.

[AlecKazakova]

yea I'm imagining something where we go from

driver.executeQuery("SELECT * FROM foo WHERE bar = ?")

to

driver.executeQuery("SELECT * FROM foo WHERE bar = ${driver.indexedParameter(1)}")

my biggest concern right now is if this effects the performance dramatically

[hfhbd]

Or driver.executeQuery("SELECT * FROM foo WHERE bar = ?", listOf(32)) so almost every driver would still use ? without any changes and only some drivers would replace the binding parameters. Then only some drivers would need to change the hardcoded string and other drivers would not have a negative performance impact.

[sachera]

Or driver.executeQuery("SELECT * FROM foo WHERE bar = ?", listOf(32)) so almost every driver would still use ? without any changes and only some drivers would replace the binding parameters. Then only some drivers would need to change the hardcoded string and other drivers would not have a negative performance impact.

I have started implementing this proposal on a branch in my fork. The changes are mostly untested, the tests have not been adjusted and there is most likely potential for optimisation in the generated code. It does already handle array arguments and index changes due to runtime replacements (when treatNullAsUnknownForEquality is false and the variable is nullable).

Before continuing I would like your opinion, If you want to I can open a pull request and we can discuss the changes there.

[hfhbd]

I am fine with the changes in the driver api.
Nice work! The part in query generator is hard to read through, but the old code too. Maybe we can refactor it later, and use more readable names/classes, like refactoring Pair with a named class for example.

Before continuing please also wait for feedback from @AlecStrong

[sachera]

@AlecStrong did you have time to take a (short) look over these changes? If so i would very much like to know if the direction of these changes are ok for you or if you want some assurance (benchmarks?) before considering these changes.

[hfhbd]

@sachera I would suggest creating the PR and we can discuss it there.

[sachera]

Done: #3662