HSEARCH-4577 @ProjectionConstructor: aggregating multi-valued field/object projections in Set, SortedSet, etc. instead of List

[marko-bekhta]

https://hibernate.atlassian.net/browse/HSEARCH-4577

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license
and can be relicensed under the terms of the LGPL v2.1 license in the future at the maintainers' discretion.
For more information on licensing, please check here.

---

[yrodiere]

Just noticed this was still pending -- did you encounter any problem? Did I forget a review? :x

[marko-bekhta]

Just noticed this was still pending -- did you encounter any problem? Did I forget a review? :x

Problems ? no no problems, only opportunities 😄.
From my notes I only have: take another look at the docs.

I suppose ProjectionCollector is a better name, but then people might wonder why we didn't just use java.util.stream.Collector. From the javadoc it's mainly because of unnecessary features in Collector and because of the transform stuff.

mm ok 😃 I can rename it 👍🏻 (it was good I left the PR hanging here 😄) So if we go with ProjectionCollector then corresponding .accumulator(...) methods become .collector(...) ? ooor ...

[yrodiere]

mm ok 😃 I can rename it 👍🏻 (it was good I left the PR hanging here 😄) So if we go with ProjectionCollector then corresponding .accumulator(...) methods become .collector(...) ? ooor ...

I guess that's what makes most sense, yes. collect(collector) would be closer to java.util.stream, but confusing since our method doesn't actually perform the collecting, which happens later.

BTW I wonder if we could just use java.util.stream.Collector :D Probably something to investigate in a different ticket, though -- if we even investigate it.

[marko-bekhta]

I guess that's what makes most sense, yes. collect(collector) would be closer to java.util.stream, but confusing since our method doesn't actually perform the collecting, which happens later.

ok 😃, yeah that's what I thought too ... that we are not collecting in that particular place ...

BTW I wonder if we could just use java.util.stream.Collector :D Probably something to investigate in a different ticket, though -- if we even investigate it.

hah I was just looking at the collector interface 😄, but yeah I agree that it's for the other PR. But if we want to explore that option, should I mark ProjectionCollector as incubating for now? (if we end up replacing it with collector)

[yrodiere]

should I mark ProjectionCollector as incubating for now? (if we end up replacing it with collector)

That's probably a good idea regardless of our plans, yes.

[sonarqubecloud]

Quality Gate passed

Issues
29 New issues
0 Accepted issues

Measures
0 Security Hotspots
87.8% Coverage on New Code
1.4% Duplication on New Code

See analysis details on SonarQube Cloud

[marko-bekhta]

Soooo following the discussion at https://hibernate.zulipchat.com/#narrow/channel/132094-hibernate-orm-dev/topic/StandardStack.20and.20reflection
I guess this won't fly well with Quarkus too 😖 😃....

I can change this to use IntFunction<T[]> and simply use accumulated.toList( ... ) but then..... it won't work fine here:

hibernate-search/mapper/pojo-base/src/main/java/org/hibernate/search/mapper/pojo/search/definition/binding/impl/ProjectionBindingContextImpl.java

Lines 417 to 419 in 12cd6cb

	else if ( containerType.isArray() ) {
	reference = ProjectionCollector.array( containerElementType );
	}

so we could do something like for sorted sets and throw an exception. Or give two ways to create array collector with class and with function. Use class in the ProjectionBindingContextImpl and it'll fail for users so they'd need to register their arrays themselves .... @yrodiere WDYT ? 🙈

[yrodiere]

I think it's fine-ish, as this only happens when a user explicitly requests an array, so it's reasonable to expect them to register that array type for reflection.

For ProjectionBindingContextImpl, I suppose we can add something to Quarkus that would detect projection constructors and register for reflection any array type used in their parameters. But only if we consider this important enough -- I personally don't. I wouldn't even have provided the feature TBH, because using arrays in domain models is a big no-no in my books. But now that it's there, I guess we might as well keep it.