HSEARCH-2535 @Facet on container (Iterable, Array, ...) properties indexes the container's toString() #1273
Conversation
yrodiere
changed the title
|
Jenkins, test this please |
| @@ -444,7 +444,7 @@ private void buildDocumentFields(Object instance, | |||
| ConversionContext conversionContext, | |||
| InstanceInitializer objectInitializer, | |||
| final float inheritedBoost, | |||
| boolean multiValued, | |||
| boolean multiValuedParent, | |||
There was a problem hiding this comment.
I'm not really convinced it's clearer. What is "Parent"?
There was a problem hiding this comment.
Feel free to suggest a better name. I just meant to mention that it's not the property we're processing that is multi-valued, but its parent property.
What's important is that even though the parent is not multi-valued, the property we're processing still can be.
| @@ -661,10 +661,33 @@ else if ( currentFieldValue instanceof Map ) { | |||
| if ( fieldMetadata.hasFacets() ) { | |||
| faceting.enableFacetProcessing(); | |||
| for ( FacetMetadata facetMetadata : fieldMetadata.getFacetMetadata() ) { | |||
| if ( multiValuedParent ) { | |||
There was a problem hiding this comment.
s/facetting/faceting/ in the commit comment
| @@ -0,0 +1,354 @@ | |||
| /* | |||
There was a problem hiding this comment.
s/facetting/faceting/ in the commit comment
| } | ||
|
|
||
| @Indexed | ||
| static class StringArrayFacetEntity { |
There was a problem hiding this comment.
Any reason why they are not private? I wouldn't expect them to be used elsewhere in the package.
There was a problem hiding this comment.
I originally used Hibernate ORM in the test, and Hibernate ORM cannot instantiate private classes.
But you're right, since the test doesn't use ORM anymore, there's no reason for making the class package-scoped.
Fixed.
| @@ -124,6 +125,7 @@ public void stringMap() throws Exception { | |||
| } | |||
|
|
|||
| @Test | |||
| @Ignore // HSEARCH-1927 : Range faceting on multiple numeric values does not work | |||
There was a problem hiding this comment.
Yeah, it's not a good idea to commit tests not working then ignore them. It's especially painful when you try to bisect something.
There was a problem hiding this comment.
Ok. I'll squash that into the previous commit.
| } | ||
| else if ( member.isArray() ) { | ||
| multivalued = true; | ||
| for ( Object element : (Object[]) currentFieldValue ) { |
There was a problem hiding this comment.
So we don't expect array of primitives here?
There was a problem hiding this comment.
See my other PR: #1274
@Facet does not work if we don't also have a @Field. Which means supporting arrays of primitives for facets makes little sense if we have no support for arrays of primitives in the field bridges.
Plus, I wanted to discuss the whole solution of Array.* in a single PR, because it's likely to raise some objections.
yrodiere
dismissed
gsmet’s stale review
Remarks should have been addressed. Thanks.
…y means 'the property holder might have multiple values'
This test is not related to the facetting of Collection properties, but to the facetting inside an embedded collection.
…, maps) Some tests are disabled because they do not pass due to HSEARCH-1927.
|
I fixed a variable name for consistency and merged (s/multivalued/multiValued/). Thanks. |
https://hibernate.atlassian.net/browse/HSEARCH-2535
I added code to support both collections of Strings and collections of numeric values, but only containers of Strings work right now, due to HSEARCH-1927. This one will have to be fixed in another PR.
I think it's safe to merge this PR during the CR period, because the previous behavior was clearly suboptimal:
toString()on arrays/collections/maps for multi-valued string fields, which may work in some cases but clearly isn't a good solution