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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bind arguments with SqlBinaryExpr #4604

Merged
merged 7 commits into from
Nov 17, 2023
Merged

Conversation

griffio
Copy link
Contributor

@griffio griffio commented Sep 6, 2023

fixes #4504

馃ぞ This PR is just to try out and see if any quick fixes

Try and lookup the parent SqlColumnExpr to get the type of the column

Only the parent column can tell us what the type actually is declared as e.g Instant

Currently the tests pass and local snapshot works with cases

import java.time.Instant;

CREATE TABLE session (
  id UUID PRIMARY KEY,
  created_at TIMESTAMP AS Instant NOT NULL,
  updated_at TIMESTAMP AS Instant NOT NULL
);

selectSession1:
SELECT *
FROM session
WHERE created_at = :createdAt - INTERVAL '2 days' OR updated_at = :updatedAt + INTERVAL '2 days';

selectSession2:
SELECT *
FROM session
WHERE created_at BETWEEN :createdAt - INTERVAL '2 days' AND :createdAt + INTERVAL '2 days';

馃摀 The user will be required to implement and provide a ColumnAdapter for Instant to LocalDateTime, so the generated code would look like this e.g

 override fun <R> execute(mapper: (SqlCursor) -> QueryResult<R>): QueryResult<R> =
        driver.executeQuery(681_848_824, """
    |SELECT *
    |FROM session
    |WHERE created_at BETWEEN ? - INTERVAL '2 days' AND ? + INTERVAL '2 days'
    """.trimMargin(), mapper, 2) {
      check(this is JdbcPreparedStatement)
      val createdAt_ = sessionAdapter.created_atAdapter.encode(createdAt)
      bindObject(0, createdAt_)
      bindObject(1, createdAt_)
    }

CAST doesn't work with SqlBinaryExpr

In this contrived example, CAST doesn't force factor to be an INTEGER - the inferred type is REAL as the literal expression is chosen for the bind parameter type. Of course, the column result type is REAL

selectData:
    SELECT CAST(:factor AS INTEGER) - 10.5
    FROM data;

Should generate bindLong not bindDouble

override fun <R> execute(mapper: (SqlCursor) -> QueryResult<R>): QueryResult<R> =
        driver.executeQuery(-1_455_721_480, """
    |SELECT CAST(? AS INTEGER) - 10.5
    |FROM session
    """.trimMargin(), mapper, 1) {
      check(this is JdbcPreparedStatement)
      bindDouble(0, factor)
    }

Also cases of multiple CAST with bindArgs of different types

SELECT CAST(:factor1 AS NUMERIC(10, 2)) * CAST(:factor2 AS INTEGER) - 10.5

@griffio griffio force-pushed the fix-4504-arguments branch 2 times, most recently from 9efa0b5 to 120ba4f Compare September 10, 2023 17:39
@griffio griffio marked this pull request as ready for review September 18, 2023 12:53
@griffio griffio force-pushed the fix-4504-arguments branch 2 times, most recently from 811d0d1 to 39aa044 Compare September 21, 2023 15:51
@griffio griffio force-pushed the fix-4504-arguments branch 2 times, most recently from 373eaf0 to cee71c7 Compare October 2, 2023 13:56
@hfhbd
Copy link
Collaborator

hfhbd commented Nov 15, 2023

Can you please also add a test with custom Kotlin types, as you mentioned in your summary, session?

fixes cashapp#4504

Try and lookup the parent SqlColumnExpr to get the type of the column
e.g there are two different parameter argument types

SELECT CAST(:factor1 AS NUMERIC(10, 2)) * CAST(:factor2 AS INTEGER) - 10.5
Use SqlCastExpr tyoe or use SqlColumExpr to get column type

Add Tests for Bind argument when used with CAST and Binary Expression
to streamline checks for CAST,
Arithmetic test case
BindArgTest for Instant using binary expressions
@griffio
Copy link
Contributor Author

griffio commented Nov 15, 2023

馃ゼ Added tests for custom type Instant

also see other 馃エ #4657 that adds temporal support to binary expressions

@hfhbd
Copy link
Collaborator

hfhbd commented Nov 16, 2023

Nice, but sorry, I mean integration tests, can you just copy the existing tests to the integration tests too? Thank you!

@griffio
Copy link
Contributor Author

griffio commented Nov 16, 2023

@hfhbd 鉂揇o o you mean add atleast some docker integration-postgrsql tests or in the sqdelight-compiler integration tests.
Though the issue was bind args in binary statements the use-site was for PostgreSql

@hfhbd
Copy link
Collaborator

hfhbd commented Nov 16, 2023

some docker integration-postgrsql tests

Yes, just in case.

Test with adapter implementation and Instant types
@griffio
Copy link
Contributor Author

griffio commented Nov 16, 2023

Ok - have added integration tests

@hfhbd hfhbd merged commit 371f1b8 into cashapp:master Nov 17, 2023
7 checks passed
@griffio griffio deleted the fix-4504-arguments branch November 17, 2023 07:29
hfhbd pushed a commit that referenced this pull request Apr 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants