Hi, I started to look at HSEARCH-1494 [1] which deals with an unexpected exception is thrown when @IndexedEmbedded is used. Easiest to explain with an example: @Entity @Indexed public class A { @Id @GeneratedValue private long id; @OneToOne @IndexedEmbedded private B b; } @Entity public class B { @Id @GeneratedValue private Timestamp id; @Field private String foo; public Timestamp getId() { return id; } public String getFoo() { return foo; } } A includes B via @IndexedEmbedded and is only interested in including the ‘foo’ field. However, atm we implicitly index B.id as well. In this particular case an exception is thrown, because we don’t know which field bridge to use for B.id. This also relates to HSEARCH-1092 [2], where the include path feature of @IndexedEmbedded is used. Even though the configured paths don’t include the ids, they are added which increases the index size unnecessarily (not really sure whether it really matters in practice). If we skip the implicit inclusion of id properties, the user will need to add an explicit @Field in case he wants to include an id property via indexed embedded. If the embedded entity itself is not indexed, I think this makes sense. But what if the embedded entity is indexed itself? Does it seem wrong in this case to specify an additional @Field? Do we need some additional configuration element? Thoughts? —Hardy [1] https://hibernate.atlassian.net/browse/HSEARCH-1494 [2] https://hibernate.atlassian.net/browse/HSEARCH-1092 _______________________________________________ hibernate-dev mailing list hibernate-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/hibernate-dev

On 29 Jan 2014, at 00:14, Sanne Grinovero <sanne(a)hibernate.org&gt; wrote: > I'd avoid a new configuration element. +1 Sure, that would be best. > I think we should strive at keeping the index small by default unless > there is a strong usability penalty, and I don't think this is a big > one. I agree. I don’t think there is a good reason for including the id of the embedded entity by default, especially since it leads to potential problems as described in HSEARCH-1494. > Some power users pointed out that there are use cases in which having > the _id_ field of embedded relations is useful, so we should still > allow to add it when someone explicitly flags the need for it. Sure. My questions was basically about how we allow this type of id inclusion. > Now the complexity is I guess to define what it means to "explicitly > flag the need for it”; exactly :-) > I think @IndexedEmbedded should not include it > by default, unless explicitly listed via the _includePaths_ attribute. include paths is one use case, but there is also the default @IndexedEmbedded behaviour which includes all indexed field (including the id atm). > Question: would we still interpret the literal "id" as a keyword > pointing to whatever getter is the primary key of the embedded object? Why still? We don’t do this at the moment afaict. Our examples and tests just use ‘id’ all the time.

> I'm inclined to seek for a property which has the name as listed in > _includePaths_, not giving the "id" literal a special treatment in > this case. I am not following you here. What do you mean? Also you seem to just target the includePath case.

Another thought would be to make this a global configuration option. Something like hibernate.search.embeddedindexing.include_id_property. Per default the flag would be false, but could be set to true. Obviously this is a quite coarse solution.

—Hardy

> Yes I mean we could never include it by default, and allow the > "includePath" to be the (only) way to include things. But this forces you to list all fields to include explicitly in case you want the id added, but otherwise are happy to just use the default @IndexedEmbedded.

>> But this forces you to list all fields to include explicitly in case you want the id added, but otherwise are happy >> to just use the default @IndexedEmbedded. > > I think we should keep the id with the default @IndexedEmbedded. It > was weird to include the id when the includePath didn't ask for it but > it would be weird to not include the id with the default > @IndexedEmbedded. It's not an indexed field, I don't think it was ever meant to be in the index but rather a side effect of our recursion.. as we obviously need the id of the root element.

- API wise it's much more complex to express the request-for-removal than to simply add it to includePath when needed

- Lucene4 includes a special payload encoding for "id like fields" which has lots of benefits, with the one drawback that it has to be unique per document.

We should aim at supporting it

Am I missing some strong use case which needs this, for which explicitly listing it on includePath is too annoying?

Also by simply removing it I hope to give some more control back to user (as we'll obviously honour any bridge/@Field annotation), as this id-indexing behaviour has caused other related discussions: HSEARCH-183

> So it has been an explicit choice at some point of time. > >> - API wise it's much more complex to express the request-for-removal >> than to simply add it to includePath when needed > > The question is also about a consistent behaviour across all use cases. Sure, > we can add the id via the include path, but what I want to make sure that our implementation > makes sense in all cases and we have a comprehensible solution for users. > So, I guess you are saying here, that the document id is just not a regular field. To clarify what bugs me is, that you are basically suggesting that the id can be included via include path, but can never be used via the depth approach. One option we have not discussed yet, is an additional method on @indexedEmbedded. Something like includeEmbeddedId. Default would be false. If you set it to true, the id is included. It makes id inclusion orthogonal to depth and includePath. —Hardy _______________________________________________ hibernate-dev mailing list hibernate-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/hibernate-dev

Where are we at in this discussion? I think we basically have to main proposals. #1 Don’t include the embedded id per default. If @IndexEmbedded is used via the depth parameter there is no way to include the embedded id. In order to include the id you would need to use includePaths. depth and includePaths can be used in combination, so something like this is possible @IndexedEmbedded( depth = 1, includePaths = “foo.id” ). This will include all configured fields of the embedded entity, plus its id. #2 Don’t include the embedded id per default, but have an additional flag on @IndexedEmbedded to include the embedded id. The default would be false. To include the id would be something like @IndexedEmbedded( depth = 1, includeEmbeddedId = true ) or @IndexedEmbedded( includePaths = {“foo.a”, “foo.b”, “foo.c”} , includeEmbeddedId = true ).

I think I favour #2 atm, since it seems more symmetric. On top of this we seem to agree that it is a good idea to set the default depth value of @IndexedEmbedded to 0. This avoids the change in default values, when includePaths is used. As a side effect the default @IndexedEmbedded annotation becomes a no-op and should be treated as an invalid configuration. The choice here is between: #1 Log warning and move on #2 Throw an exception due to invalid configuration My choice would be #2 again.

Thanks for the input. Comments inline. On 27 Jan 2014, at 12:01, Emmanuel Bernard <emmanuel(a)hibernate.org&gt; wrote: > > On 06 May 2014, at 21:43, Hardy Ferentschik <hardy(a)hibernate.org&gt; wrote: > >> Where are we at in this discussion? >> >> I think we basically have to main proposals. >> >> #1 Don’t include the embedded id per default. If @IndexEmbedded is used via the depth parameter there is no way to include the embedded id. >> In order to include the id you would need to use includePaths. depth and includePaths can be used in combination, so something like this >> is possible @IndexedEmbedded( depth = 1, includePaths = “foo.id” ). This will include all configured fields of the embedded entity, plus its id. >> #2 Don’t include the embedded id per default, but have an additional flag on @IndexedEmbedded to include the embedded id. The default would be false. >> To include the id would be something like @IndexedEmbedded( depth = 1, includeEmbeddedId = true ) or >> @IndexedEmbedded( includePaths = {“foo.a”, “foo.b”, “foo.c”} , includeEmbeddedId = true ). > > I like #2 much better. It is too complex to ask of the user to understand how the combination of depth and includePaths work based on fairly arbitrary rules. > I thing the property should be named includeEmbeddedObjectsIds because embeddedId has a precise meaning in ORM. Cool. Seems we have now several votes for this approach. I will start implementing it then. +1 also for ‘includeEmbeddedObjectsIds' > One question in that case, what does this one do > > @IndexedEmbedded( includePath = { “foo.id” }, includeEmbeddedId=false //default ) > > I think we should honour includePath according to the additive rule mentioned earlier and the path of least surprise. Sounds reasonable. >> On top of this we seem to agree that it is a good idea to set the default depth value of @IndexedEmbedded to 0. This avoids the change in default values, >> when includePaths is used. As a side effect the default @IndexedEmbedded annotation becomes a no-op and should be treated as an invalid configuration. >> The choice here is between: >> >> #1 Log warning and move on >> #2 Throw an exception due to invalid configuration >> >> My choice would be #2 again. > > Is that really a good idea? As you guys said, @IndexedEmbedded becomes a (silently or not) broken mapping :( > It also means that the simplest @IE usage requires to understand depth and / or includePath. > I don’t like that at all. It makes things more complex than they should be for the simple case. So what is your take on this then? Leave as is and keep the fact that the default depth value changes its default value depending on whether or not includePaths is used? That would be option #3 Keep status quo for value of depth parameter I raised the concern that the simple @IndexEmbedded is now not valid anymore as well before. I guess the question is what you give more importance, ability to use the annotation with its default values or have consistent default values which don’t change. I still think consistency is more important and #2 is the better approach. However, before going to #1, I would rather join you and keep the status quo with #3. Btw, enforcing a depth or includePath value might have the advantage of creating smaller (more targeted) indexes, since we less likely include fields which are need targeted by a query.

>> So what is your take on this then? Leave as is and keep the fact that the default depth value changes its default value depending >> on whether or not includePaths is used? That would be option >> >> #3 Keep status quo for value of depth parameter >> >> I raised the concern that the simple @IndexEmbedded is now not valid anymore as well before. I guess the question is what you give more importance, >> ability to use the annotation with its default values or have consistent default values which don’t change. >> >> I still think consistency is more important and #2 is the better approach. However, before going to #1, I would rather join you and keep the status quo >> with #3. >> >> Btw, enforcing a depth or includePath value might have the advantage of creating smaller (more targeted) indexes, since we less likely include >> fields which are need targeted by a query. > > #3 then #2 for me. I really like #3 better though for the reason I explained. > We should bet at horse races together ;) Tough negotiations. @Sanne, you brought this depth default of 0 up. WDYT?

>>> Question: would we still interpret the literal "id" as a keyword >>> pointing to whatever getter is the primary key of the embedded object? >> >> Why still? We don’t do this at the moment afaict. Our examples and tests just use ‘id’ >> all the time. > > Right I got confused. I thought for a moment that we would encode the > primary identifier as a specific keyword, but there's no reason. > Forget what I said :) :-) >>> I'm inclined to seek for a property which has the name as listed in >>> _includePaths_, not giving the "id" literal a special treatment in >>> this case. >> >> I am not following you here. What do you mean? Also you seem to just target the includePath case. > > Yes I mean we could never include it by default, and allow the > "includePath" to be the (only) way to include things. But this forces you to list all fields to include explicitly in case you want the id added, but otherwise are happy to just use the default @IndexedEmbedded. > Remember _includePath_ is additive to fields otherwise included via > @IndexedEmbedded _depth_, so it fits nicely for this role: Is that really true? AFAIK the includePath attribute changes the behaviour of the depth attribute. Depth is ‘0’ per default if ‘includePath’ is used. So you would have to include the id via includePath and also set the depth explicitly to something > 0.

I also find it not intuitive that this means: "index all embedded fields up to the given depth, plus the specified paths”. I never liked how we baked depth and includePath into the same annotation. A dedicated annotation would have been more appropriate.

> for non-indedex embedded elements it's of course nonsesense (as usual) > to specify an attribute which is lacking a @Field or similar option, > while for indexed types we implicitly apply one on the primary > identifier, this just needs to be selectable via _includePath_. I am just not sure, whether we easily can tell when processing the embedded entity whether it is indexed or not. Obviously that is an implementation detail, but we might have to jump through some hoops here (might be wrong).

>> Another thought would be to make this a global configuration option. Something like >> hibernate.search.embeddedindexing.include_id_property. Per default the flag would be false, but >> could be set to true. Obviously this is a quite coarse solution. > > I wouldn't do that now, but we can certainly leave this option open > for future evolution. Ok. —Hardy

I'd actually like to propose to change the depth default to zero, and since includePath also defaults to an empty list, we'd be logging a warning as the @IndexedEmbedded annotation would have no effect. > I also find it not intuitive that this means: > "index all embedded fields up to the given depth, plus the specified paths”. > I never liked how we baked depth and includePath into the same annotation. A dedicated annotation would > have been more appropriate. We've been there and couldn't agree on a better proposal. Feel free to reopen the case on a new thread, if you have a nice name in mind. But ultimately remember the goal is to allow queries on a well-known list of field names, and it would be great to validate for these queries, so to have a clear definitions of which indexing and analysis options are applied to each Lucene field, and how to apply a bi-directional projection.. so I'm still skeptical on leaving too much freedoom, or have multiple ways to achieve the same thing.