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 converting Sort to Comparator [DATACMNS-1678] #2099

Closed
spring-projects-issues opened this issue Mar 10, 2020 · 7 comments
Closed

Support converting Sort to Comparator [DATACMNS-1678] #2099

spring-projects-issues opened this issue Mar 10, 2020 · 7 comments
Assignees
Labels
in: mapping status: declined type: enhancement

Comments

@spring-projects-issues
Copy link

spring-projects-issues commented Mar 10, 2020

Martin Konštiak opened DATACMNS-1678 and commented

I'd like to apply Sort to Collection or Stream. Thanks to reflection it's possible to convert each Sort.Order to Comparator and then chain them into one. And then use this Comparator as parameter for Stream.sorted method


No further details from DATACMNS-1678

@spring-projects-issues
Copy link
Author

spring-projects-issues commented Mar 11, 2020

Mark Paluch commented

Thanks for the ticket. That is an interesting thought. We never evaluated really in-memory processing of data and having a Comparator built from Sort could nicely fit into our Streamable efforts. I'm going to take this ticket to our team to get other opinions

@spring-projects-issues
Copy link
Author

spring-projects-issues commented Mar 11, 2020

Martin Konštiak commented

There's already PR with first proposal: #433
 

@spring-projects-issues
Copy link
Author

spring-projects-issues commented Mar 11, 2020

Oliver Drotbohm commented

As I might not be properly available in the team call this afternoon, some thoughts on this. I fundamentally like the idea of a Sort being able to be used sorting a collection. It's also in proximity of what Querydsl already allows with it's collection querying capabilities. In case of use with TypedSort we might even be able to directly use the method handles given to perform the invocation although it currently resolves the referred to properties to strings eagerly, which would have to change for that.

I also see a couple of other risks:

  • Especially to beginners it creates incentives to query all data and apply sorting later that it would actually make sense (probable, but niche part of the audience, something we might want to accept)
  • The suggested implementation uses PropertyDescriptor instances and reflectively invokes the read methods. That's basically a parallel implementation of what we have in PersistentEntity with PersistentPropertyAccessor etc. It's a parallel implementation that works slightly different in a subtle way which probably creates confusion and suffers from repeated reflection overhead. Unfortunately, using the mapping information from our MappingContext is rather involved, is not something a user usually interacts with.

Especially the latter makes me think that we're looking at something that looks easy on the surface but bears quite a bit of unanticipated complexity.

Edit: looking at the PR, it looks like that helper class to create Comparator instances could easily just live outside Spring Data in a user's project in case one would need this for now

@spring-projects-issues
Copy link
Author

spring-projects-issues commented Mar 11, 2020

Mark Paluch commented

I second Ollie's opinion here. At first sight, it's a neat extension. I also share the main concern regarding property accessors. This utility would require to be tied to MappingContext which comes with its own rules. E.g. @Transient properties would not be included. Adding another approach to property access asks for trouble.

To stick with a consistent way of retrieving properties, obtaining a Comparator would require an association with the MappingContext and therefore less convenient to use (e.g. MappingContext.getComparator(Sort))

@spring-projects-issues
Copy link
Author

spring-projects-issues commented Mar 12, 2020

Martin Konštiak commented

Thanks for suggestions. I agree with your points for reusing existing property accessing functionality. I tried to orientate in PersistentEntity/PersistentPropertyAccessor/MappingContext but I've got lost there. Is it possible to explain it on more specific examples? I tried to use org.springframework.beans.BeanWrapperImpl but it looks it wasn't designed for such purpose (NullValueInNestedPathException)

@spring-projects-issues
Copy link
Author

spring-projects-issues commented Mar 12, 2020

Oliver Drotbohm commented

There's hardly any way to make this work using the MappingContext abstraction as the mapping package already depends on the domain package and we'd either create a cyclic dependency if the new code lived in the domain package as well. Also, setting up a MappingContext is a store specific undertaking that requires inclusion of user configuration etc. I.e. it's not code that should live anywhere near user domain code in the first place.

We could probably find a solution but that would require significant rework of abstractions and packages and would probably still end up creating more complexity for all other use cases due to additional indirections

@spring-projects-issues
Copy link
Author

spring-projects-issues commented Dec 1, 2020

Mark Paluch commented

After reiterating here, we couldn't convince ourselves to add your contribution. Probably there should be a separate place outside of Spring Data where this extension could live in.

@spring-projects-issues spring-projects-issues added status: declined type: enhancement in: mapping labels Dec 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: mapping status: declined type: enhancement
Projects
None yet
Development

No branches or pull requests

2 participants