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.

Sign up for GitHub

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

Jump to bottom

Add stream() method for convenient result traversal #24525

Merged
merged 4 commits into from
Jul 4, 2023
Merged

Add stream() method for convenient result traversal #24525

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8a2a399
Add stream() method for convenient result traversal
TomaszGaweda May 11, 2023
c1e6527
Add Nonnull
TomaszGaweda May 11, 2023
142aef7
Better JavaDoc
TomaszGaweda May 12, 2023
aa645ff
Update version
k-jamroz Jun 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 2 additions & 5 deletions hazelcast-sql/src/test/java/com/hazelcast/jet/sql/SqlQueryResultTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import java.util.List;

import static java.util.Collections.emptyList;
import static java.util.stream.Collectors.toList;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.fail;

Expand Down Expand Up @@ -79,13 +80,9 @@ private void check(boolean client) {
}

private List<SqlRow> execute(String query) {
List<SqlRow> rows = new ArrayList<>();
try (SqlResult result = instance().getSql().execute(query)) {
for (SqlRow row : result) {
rows.add(row);
}
return result.stream().collect(toList());
k-jamroz marked this conversation as resolved.
Show resolved Hide resolved
}
return rows;
}

private void checkSuccess(
Expand Down
23 changes: 23 additions & 0 deletions hazelcast/src/main/java/com/hazelcast/sql/SqlResult.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@

import javax.annotation.Nonnull;
import java.util.Iterator;
import java.util.stream.Stream;
import java.util.stream.StreamSupport;

/**
* SQL query result. Depending on the statement type it represents a stream of
Expand Down Expand Up @@ -82,6 +84,27 @@ default boolean isRowSet() {
@Override
Iterator<SqlRow> iterator();

/**
* Returns a stream of result rows.
* <p>It uses internally {@link #iterator()} method, so it cannot be called twice.</p>
*
* <p>You should still call {@link #close()} method after the stream is used (or use this method inside
* {@code try-with-resources} block. You should not pass the {@link Stream} from this method outside
* {@code try-with-resources} block, if it's used.</p>
*
* @throws IllegalStateException if the method is invoked more than once or
* if this result doesn't have rows
* @throws HazelcastSqlException in case of an SQL-related error condition
*
* @return Stream of result rows
*
* @since 5.4
*/
@Nonnull
default Stream<SqlRow> stream() {
return StreamSupport.stream(spliterator(), false);
}
Comment on lines +104 to +106
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why this default method doesn't exist in the Iterable interface.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LMGTFY: https://stackoverflow.com/a/23177907

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

argument with specialised iterator types does not apply to us. I wonder if adding this method would promote less efficient (but easier) usage. Also users will not be able to use this method in a fully fluent way because SqlResult has to be closed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Java architects' defenses on these type of poor design choices are really funny. They are reluctant to get rid of primitive/boxed duality (by burying auto-(un)boxing deeper) and they use this as a counter-argument in order not to implement some convenience methods. By the way, I upvoted this and this.

To get rid of try-with-resources, we can return StreamSupport.stream(spliterator(), false).onClose(this::close), but try-with-resources is the only reliable way to release resources in case a RuntimeException is thrown. Closing SqlResult in the end of iterator(), i.e. when hasNext() returns false, is also a bad idea since the iterator doesn't have to be used to the end.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to say "Java is not perfect" would be a huge understatement, but we have to live with it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I know it's not perfect - still needs try-with-resources - but it's simpler for queries where you expect not many result rows (e.g. with limit), similar to Mongo's .into() function


/**
* Returns the number of rows updated by the statement or -1 if this result
* is a row set. In case the result doesn't contain rows but the update
Expand Down