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

Add stream() method for convenient result traversal #24525

Merged

Conversation

TomaszGaweda
Copy link
Contributor

Checklist:

  • Labels (Team:, Type:, Source:, Module:) and Milestone set
  • Label Add to Release Notes or Not Release Notes content set
  • Request reviewers if possible
  • Send backports/forwardports if fix needs to be applied to past/future releases
  • New public APIs have @Nonnull/@Nullable annotations
  • New public APIs have @since tags in Javadoc

@TomaszGaweda TomaszGaweda added Team: SQL Add to Release Notes SQL-only Use when changes are only in hazelcast-sql module Module: SQL labels May 11, 2023
@TomaszGaweda
Copy link
Contributor Author

I wonder what do you think of such thing, IMHO it makes API more user-friendly without much of a code (underlying interator is the same).
@k-jamroz @Fly-Style @ivanthescientist @burakgok

@TomaszGaweda
Copy link
Contributor Author

It would close #22166 I think

@TomaszGaweda TomaszGaweda added SQL-only Use when changes are only in hazelcast-sql module and removed SQL-only Use when changes are only in hazelcast-sql module labels May 11, 2023
Comment on lines +100 to +102
default Stream<SqlRow> stream() {
return StreamSupport.stream(spliterator(), false);
}
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
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator

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
Collaborator

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

@TomaszGaweda TomaszGaweda removed the SQL-only Use when changes are only in hazelcast-sql module label May 11, 2023
@hazelcast hazelcast deleted a comment from hz-devops-test May 11, 2023
@Patras3
Copy link
Contributor

Patras3 commented May 11, 2023

run-lab-run

@hazelcast hazelcast deleted a comment from Patras3 May 11, 2023
@hazelcast hazelcast deleted a comment from hz-devops-test May 11, 2023
@TomaszGaweda
Copy link
Contributor Author

run-lab-run

@k-jamroz k-jamroz added this to the 5.4.0 milestone May 12, 2023
@k-jamroz
Copy link
Collaborator

Because of https://github.com/hazelcast/hazelcast-enterprise/issues/5966 and time constraints postponing decision to 5.4 release.

@k-jamroz k-jamroz added the Source: Internal PR or issue was opened by an employee label Jun 30, 2023
@TomaszGaweda TomaszGaweda enabled auto-merge (squash) June 30, 2023 12:14
@hz-devops-test
Copy link

The job Hazelcast-pr-builder of your PR failed. (Hazelcast internal details: build log, artifacts).
Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log file
--------------------------
-------TEST FAILURE-------
--------------------------
[INFO] Results:
[INFO] 
[ERROR] Failures: 
[ERROR]   StreamKafkaPTest.when_processingGuaranteeNone_then_continueFromBeginningAfterJobRestart:249->testWithJobRestart:315->testWithJobRestart:350->HazelcastTestSupport.assertTrueEventually:1184->HazelcastTestSupport.assertTrueEventually:1165->lambda$testWithJobRestart$4:350 expected:<300> but was:<389>
[INFO] 
[ERROR] Tests run: 51, Failures: 1, Errors: 0, Skipped: 1
[INFO] 

[ERROR] There are test failures.

@TomaszGaweda
Copy link
Contributor Author

run-lab-run

@TomaszGaweda TomaszGaweda merged commit 69d15f6 into hazelcast:master Jul 4, 2023
7 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants