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 support to JdbcAggregateOperations #1963

Closed
wants to merge 1 commit into from

Conversation

serezakorotaev
Copy link
Contributor

@serezakorotaev serezakorotaev commented Dec 26, 2024

Hi, everyone!
I wanted to contribute for you and started with this PR :)
I have added Stream support to JdbcAggregateOperations. These methods use other methods at the db level with the return value type stream (or cursor for MyBatis).
I also read that findAll** is a reserved naming convention, for the sake of similarity I added findAllStreamable or something like that.
Added tests for these methods.

Closes #1714

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Dec 26, 2024
@mp911de mp911de linked an issue Jan 13, 2025 that may be closed by this pull request
@mp911de mp911de added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Jan 13, 2025
Copy link
Contributor

@schauder schauder left a comment

Choose a reason for hiding this comment

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

That looks pretty good.

I'd prefer methods where we use stream instead of find so the streaming variant of findByIds would be streamByIds.

Could you rename the methods accordingly?

@@ -15,14 +15,9 @@
*/
package org.springframework.data.jdbc.mybatis;

import static java.util.Arrays.*;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please avoid reformatting the code.
See https://github.com/spring-projects/spring-data-build/tree/main/etc/ide for how to setup the formatter of your IDE.

@@ -149,7 +149,7 @@ protected RelationalPersistentProperty createPersistentProperty(Property propert

/**
* @since 3.2
* @return iff single query loading is enabled.
Copy link
Contributor

Choose a reason for hiding this comment

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

The double f is actually intentionally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ops, sorry :) I'll fix it

@serezakorotaev
Copy link
Contributor Author

Hi!
I fixed and already squashed it

@schauder schauder linked an issue Jan 16, 2025 that may be closed by this pull request
schauder pushed a commit that referenced this pull request Jan 16, 2025
See #1714
Original pull request #1963

Signed-off-by: Sergey Korotaev <[email protected]>
schauder added a commit that referenced this pull request Jan 16, 2025
Minor formatting and references to GH issues.

See #1714
Original pull request #1963
@schauder
Copy link
Contributor

Thanks, that's merged.

@schauder schauder closed this Jan 16, 2025
@schauder schauder added this to the 3.5 M1 (2025.0.0) milestone Jan 16, 2025
@serezakorotaev
Copy link
Contributor Author

Super, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Stream support to JdbcAggregateOperations
4 participants