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

Support batches using @SqlUpdate #844

Open
electrum opened this issue Jun 30, 2017 · 7 comments
Open

Support batches using @SqlUpdate #844

electrum opened this issue Jun 30, 2017 · 7 comments
Labels
Milestone

Comments

@electrum
Copy link
Contributor

Using @SqlBatch can be annoying because it uses a different interface from @SqlUpdate and requires the user to do a lot of additional work (doesn't degrade gracefully). For example, let's say we start with a method to insert a row, which calls a DAO method that uses @SqlUpdate:

void insertColumn(MasterWriterDao dao, long commitId, long tableId, ColumnInfo column)
{
    dao.insertColumn(
            commitId,
            column.getColumnId(),
            column.getColumnName().getBytes(UTF_8),
            column.getType().getTypeSignature().toString().getBytes(UTF_8),
            tableId,
            column.getOrdinal(),
            boxedInt(column.getSortOrdinal()),
            boxedInt(column.getBucketOrdinal()),
            utf8Bytes(column.getComment()));
}

This method has non-trivial work that can't be done (easily) using BeanMapper or similar. I'm using some real code rather than a contrived example.

We call the above method to insert single rows. Works great. Sometime later, we add another usage that calls this in a loop:

void insertColumns(MasterWriterDao dao, long commitId, long tableId, List<ColumnInfo> columns)
{
    for (ColumnInfo column : columns) {
        insertColumn(dao, commitId, tableId, column);
    }
}

Later, we find out performance is bad if the number of items is large. How do we fix this?

We can convert the DAO to use @SqlBatch, but that requires the caller to either build seven separate collections for each variable parameter or to create a new bean object just for this. Both of these violate the encapsulation of the insertColumn() method by moving the work into insertColumns(), which previously didn't have any details of the actual insert operation. It also breaks callers that were happily inserting a single row.

One idea is to add a couple of methods to SqlObject:

void insertColumns(MasterWriterDao dao, long commitId, long tableId, List<ColumnInfo> columns)
{
    dao.beginBatch();
    for (ColumnInfo column : columns) {
        insertColumn(dao, commitId, tableId, column);
    }
    dao.endBatch();
}

This doesn't handle returning generated keys. I was thinking users could add a separate method annotated with something like @BatchFinisher that would be used in place of endBatch() and would return generated keys. Though we could do the simple version first and tackle generated keys later.

@electrum
Copy link
Contributor Author

If there is agreement, I can prototype the simple version.

@stevenschlansker
Copy link
Member

I think that's an interesting idea! Definitely would give a prototype a spin.

@electrum
Copy link
Contributor Author

I started hacking on this and it turns out to be rather complicated since it doesn't fit the existing model at all.

@qualidafial
Copy link
Member

@electrum Could you elaborate?

@qualidafial qualidafial added this to the JDBI 3 Post-release (non-blocker) milestone Aug 1, 2017
@electrum
Copy link
Contributor Author

This is the API and test I started with: https://gist.github.com/electrum/5b08813442be8b2d348293ce0ddd2423

The extension model is based on a method annotation mapping to a Handler. This is different because it needs to add additional methods to the SQL object which then change the state of the object and perform different behavior when existing annotated methods are invoked. Specifically, when in a batch, we need @SqlUpdate methods to add to a batch rather than executing directly.

I need to play around with this more. It might turn out easier than I thought. Though I won't have time to look at this for a while.

@qualidafial
Copy link
Member

@electrum Does #950 help?

@hgschmie
Copy link
Contributor

Closing, still tracked in #2390.

@hgschmie hgschmie reopened this Jun 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

4 participants