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 for Stream API #38

Open
mme-private opened this issue Sep 19, 2017 · 1 comment
Open

Support for Stream API #38

mme-private opened this issue Sep 19, 2017 · 1 comment

Comments

@mme-private
Copy link

mme-private commented Sep 19, 2017

Is there any thought about adding stream support as return option? I understand that for stream to work, Statement and ResultSet must be open and there would be needed some special cleanup. Probably it would work only for existing transactions and not for implicitly created.

First cleanup could happen in Stream.onClose handler, but if Stream is not closed, then it could happen just before connection release in DefaultTransaction (statements and result sets would be registered there for cleanup).

So if someone would use stream like

SqlQuery query = query("select id, name from department where update_timestamp > ?", date);
try (Stream<Department> stream = db.streamAll(Department.class, query)) {
    stream....
}

then all resources would be released after stream is used up, otherwise resources will be released when connection is released.

In RowMapper interfaces would be something like code below, but executed not from executeQuery method which closes PreparedStatement

     default ResultSetProcessor<Stream<T>> stream() {

		return resultSet -> StreamSupport
				.stream(new Spliterators.AbstractSpliterator<T>(Long.MAX_VALUE, Spliterator.ORDERED) {
					@Override
					public boolean tryAdvance(Consumer<? super T> action) {
						try {
							if (!resultSet.next())
								return false;
							action.accept(mapRow(resultSet));
							return true;
						} catch (SQLException ex) {
							throw new RuntimeException(ex);
						}
					}
				}, false).onClose(() -> {
					try {
						resultSet.close();
					} catch (SQLException e) {
						throw new DatabaseSQLException(e);
					}
				});
	}
@komu
Copy link
Member

komu commented Sep 20, 2017

Thanks for the suggestion.

I've thought about this along the same lines, so I'm open to the idea.

I have some concerns though: nobody reads documentation and I'm afraid people would just end up calling streamAll without bothering to check if they need to release the resources. I'd guess that most people don't even know that you can close Streams or at least think about such concerns upon seeing a stream returned. Perhaps I shouldn't worry about that: ResultSet would be closed when the transaction is finished and shame on the programmer for not reading the Javadoc and releasing resources earlier.

Other annoying thing is that this would not work with implicit transactions, unlike other methods in Database.

(One kind of solution to both of these problems would be to implement a method that would allow a callback Function<Stream,T>, similar to withTransaction. But I guess the API ergonomics would be so bad that the solution is worse than the problem of possibly leaking resources.)

So I have some reservations (which is why I haven't implemented this so far), but of course I want to provide useful functionality, if it's needed. So I will merge a reasonable PR and provide help if necessary. I might possibly even implement this myself, but don't hold your breath for that because I have quite a lot of other things to do as well. 😄

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

No branches or pull requests

2 participants