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

Cannot use bindList with prepareBatch when lists have varied size. #2335

Closed
PatrickNausha opened this issue Apr 26, 2023 · 7 comments
Closed
Assignees

Comments

@PatrickNausha
Copy link

PatrickNausha commented Apr 26, 2023

JDBI version: 3.37.1

Database: Postgresql

I have a kotlin project where I'm trying to use prepareBatch in combination with bindList, but it is failing.

transactionHandle.prepareBatch("SELECT foo FROM bar WHERE id IN (<array>)")
    .bindList("array", listOf(1))
    .add()
    .bindList("array", listOf(1, 2))
    .add()
    .execute()

Expected

I should be able to execute the above code with an error being thrown.

Note: Above is a minimal repro. Actual use case is an UPDATE with many more add() calls in a loop, but the result is the same.

Actual

Instead I see the following exception.

org.jdbi.v3.core.statement.UnableToCreateStatementException: Missing named parameter '__array_1' in binding:{positional:{}, named:{__array_0:1}, finder:[]}
[statement:"SELECT foo FROM bar WHERE id IN (:__array_0,:__array_1)", arguments:{positional:{}, named:{__array_0:1}, finder:[]}]

It looks like the prepared statement with IN (:__array_0,:__array_1) is generated for the larger list 1, 2 so the smaller list of just 1 fails to match on :__array_1.

Follow up

Is this a bug? Or is bindList incompatible with prepareBatch by design? If the latter, should this be clarified in the docs?

@hgschmie
Copy link
Contributor

So I put together this test:

class Issue2335Test {

    static final Something ONE = new Something(3, "foo");
    static final Something TWO = new Something(4, "bar");
    static final Something THREE = new Something(5, "baz");

    @RegisterExtension
    public static EmbeddedPgExtension pg = MultiDatabaseBuilder.instanceWithDefaults().build();

    @RegisterExtension
    PgDatabaseExtension h2Extension = PgDatabaseExtension.instance(pg);

    Handle handle;
    Jdbi jdbi;

    @BeforeEach
    void setUp() {
        jdbi = h2Extension.getJdbi();
        handle = h2Extension.getSharedHandle();
        handle.execute("CREATE TABLE arr_table (id INT PRIMARY KEY, data varchar array)");
        handle.execute("INSERT INTO arr_table (id, data) values (1, ARRAY[1, 2, 3])");
        handle.execute("INSERT INTO arr_table (id, data) values (2, ARRAY[4, 5, 6])");
    }

    @Test
    void testBatchQuery() {

        try (PreparedBatch batch = handle.prepareBatch("INSERT INTO arr_table (id, data) VALUES (:id, ARRAY[<data>])")) {
            batch.bind("id", 3).bindList("data", Arrays.asList(3, 4, 5)).add();
            batch.bind("id", 4).bindList("data", Arrays.asList(6, 7, 8)).add();
            batch.bind("id", 5).bindList("data", Arrays.asList(10, 11, 12)).add();
            int [] result = batch.execute();

            assertThat(result).hasSize(3)
                .containsExactly(1, 1, 1);
        }
    }
}

and that works fine. I would need to see more of your specific use case. Are you binding more parameters?

@hgschmie
Copy link
Contributor

ok, I figured out how to reproduce it. It depends on the numbers of list elements begin different from row to row.

@hgschmie hgschmie added the bug label Apr 27, 2023
@PatrickNausha PatrickNausha changed the title Cannot use bindList with prepareBatch Cannot use bindList with prepareBatch when arrays are of varied size. Apr 27, 2023
@PatrickNausha PatrickNausha changed the title Cannot use bindList with prepareBatch when arrays are of varied size. Cannot use bindList with prepareBatch when lists have varied size. Apr 27, 2023
@hgschmie hgschmie self-assigned this Apr 27, 2023
@hgschmie
Copy link
Contributor

The batch pieces work by creating a prepared statement and then executing those repeatedly with different values. This is where the performance of batch comes from. The bindList is an odd duck, because it it actually changes the statement by using templating.

Depending on the database that you use, you can do this (this is for postgres):

    @Test
    void testUpdateOperation() {

        try (PreparedBatch batch = handle.prepareBatch(
            "UPDATE something SET name = :name WHERE id = ANY(:ids)")) {
            batch.bind("name", "foo").bindArray("ids", Integer.class, Arrays.asList(1, 2)).add();
            batch.bind("name", "bar").bindArray("ids", Integer.class, Arrays.asList(3)).add();
            batch.bind("name", "baz").bindArray("ids", Integer.class, Arrays.asList(1, 2, 4)).add();
            int[] result = batch.execute();

            assertThat(result).hasSize(3)
                .containsExactly(2, 1, 3);
        }
    }

=> Bind the list as a typed SQL array (integer array) and use the id = ANY ( :ids) clause instead of an IN (...) clause. This does not work for all use cases but if you are really struggling with an IN clause, this may help.

This is more a documentation issue (you are correct: bindList() is incompatible with the way the batches work) than a real but, but it nevertheless causes customer issues.

@PatrickNausha
Copy link
Author

Thanks for investigating and providing the workaround.

A nice-to-have would be if this case was detected and a more specific error was thrown. Having one might've prevented me from spending time questioning what I might be doing wrong.

hgschmie added a commit to hgschmie/jdbi that referenced this issue Apr 29, 2023
document that bindList is not working well with batches. addresses jdbi#2335
@hgschmie
Copy link
Contributor

Understood. Detecting this problem is not simple (Jdbi is not a fully fledged ORM; it is a convenience library); we do not actually track the number of placeholders in batch statements. Maybe we should.

I added documentation for this issue.

hgschmie added a commit to hgschmie/jdbi that referenced this issue Apr 30, 2023
document that bindList is not working well with batches. addresses jdbi#2335
@hgschmie
Copy link
Contributor

Closing, still tracked in #2390.

@hgschmie hgschmie reopened this Jun 15, 2023
@stevenschlansker
Copy link
Member

I think we can close this one - as you say, batching works by re-using the same sql, so you can't change the template.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

3 participants