9:43 p.m.
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
12:14 a.m.
Hi Hardy,
I'd avoid a new configuration element.
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.
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.
Now the complexity is I guess to define what it means to "explicitly
flag the need for it"; I think @IndexedEmbedded should not include it
by default, unless explicitly listed via the _includePaths_ attribute.
Question: would we still interpret the literal "id" as a keyword
pointing to whatever getter is the primary key of the embedded object?
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.
Sanne
On 28 April 2014 20:43, Hardy Ferentschik <hardy(a)hibernate.org> wrote:
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
12:28 p.m.
On 29 Jan 2014, at 00:14, Sanne Grinovero <sanne(a)hibernate.org> 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
1:03 p.m.
On 29 April 2014 11:28, Hardy Ferentschik <hardy(a)hibernate.org> wrote:
On 29 Jan 2014, at 00:14, Sanne Grinovero <sanne(a)hibernate.org> 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.
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.
Remember _includePath_ is additive to fields otherwise included via
@IndexedEmbedded _depth_, so it fits nicely for this role:
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_.
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.
Cheers,
Sanne
—Hardy
1:45 p.m.
>> 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
2:49 p.m.
Hi,
On Tue, Apr 29, 2014 at 1:45 PM, Hardy Ferentschik <hardy(a)hibernate.org> wrote:
> 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.
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.
--
Guillaume
3:09 p.m.
On 29 April 2014 12:45, Hardy Ferentschik <hardy(a)hibernate.org> wrote:
>>> 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.
No it's additive. depth=n will add "all" @Field until depth=n, and it
*also* will add what is explicitly listed in includePath.
So asserting that "all" doesn't include fields which are missing
Hibernate Search indexing annotations is a quite natural implication.
The problem if any is that the default depth value is different
depending if you use includePath or not, but also the alternative
Integer.MAX_VALUE doesn't make much sense in practice as noone will
define queries on an unbounded path.
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.
> 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).
right. I guess we'll have to try.
>> 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
3:24 p.m.
On 29 April 2014 13:49, Guillaume Smet <guillaume.smet(a)gmail.com> wrote:
Hi,
On Tue, Apr 29, 2014 at 1:45 PM, Hardy Ferentschik <hardy(a)hibernate.org> wrote:
>> 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.
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.
I'm sure there are some good use cases for it, but here are my reasons
to prefer the field to be removed by default:
- 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, but embedded
collections would violate the uniqueness of the field instance in the
Document, so force us to actually encode this case in a different
way.. I guess we could explore it but encoding consistency becomes
puzzling.
- Index size matters a lot on query performance (especially on native
IO buffers), and what most affects index size is stored Fields.. like
ID. I think most people will have no use for this id field and will
probably not notice this additional optimisation opportunity.
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
Sanne
--
Guillaume
8:27 p.m.
>> 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.
No it's additive. depth=n will add "all" @Field until depth=n, and it
*also* will add what is explicitly listed in includePath.
Sure. What I meant that just adding a single include path changes the depth value.
I understand how it a depth > 0 and include path work in combination.
So asserting that "all" doesn't include fields which
are missing
Hibernate Search indexing annotations is a quite natural implication.
?
The problem if any is that the default depth value is different
depending if you use includePath or not
exactly
but also the alternative
Integer.MAX_VALUE doesn't make much sense in practice as noone will
define queries on an unbounded path.
hmmm, I guess you have a point here. At least at query time you need to know which
fields you are targeting. Unless of course we would support an empty prefix for index
embedded.
That’s requested in HSEARCH-183, but at the same time our documentation has a note
saying: "The prefix cannot be set to the empty string.”
I added a quick test and there seems to be verification that the prefix is not the empty
string.
If we allowed an empty prefix then unbounded inclusion makes sense. Either way, probably a
good
time to address HSEARCH-183 as well. One way or the other.
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.
It would be a shame in this case that the default annotation basically is an “invalid”
configuration.
If we go down this path where we either require a depth > 0 or at least one include
path, I would
go further than just logging a warning. I think we should throw an exception in case. This
is probably
also better for people who upgrade from Search 4 to 5. Messages can be ignored or
depending on the
log level even won’t show up and suddenly we get everyone asking why their app does not
work anymore.
Throwing an exception would force people to make an active choice.
> 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.
I think I lost this one. I have no reason to believe that this has changed. I would have
been easy on the names, but
dedicated annotation was what I was after.
—Hardy
8:43 p.m.
On 29 Jan 2014, at 15:24, Sanne Grinovero <sanne(a)hibernate.org> wrote:
>> 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.
Nope, the code is quite explicit. It contains a if/else statement checking whether
we are processing the root entity and in the else part the comment says:
“// component should index their document id”
This is also not just an outcome of the metadata refactoring, since this if statement
and comment also has been there in the DocumentBuilder code [1]
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.
- 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.
Interesting. Do you have a pointer to this functionality?
We should aim at supporting it
Sounds good.
Am I missing some strong use case which needs this, for which
explicitly listing it on includePath is too annoying?
As said, it needs to make sense as a whole.
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
I want to address HSEARCH-183 as well, just not sure your proposal is a
requirement for that.
I was hoping HSEARCH-1494 would be an easy one, but it seems this goes much deeper ;-)
—Hardy
[1]
https://github.com/hibernate/hibernate-search/blob/4.1/hibernate-search-e...
8:19 p.m.
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
9:43 p.m.
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.
Thoughts?
—Hardy
On 30 Jan 2014, at 20:19, Hardy Ferentschik <hardy(a)hibernate.org> wrote:
> 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
9:10 a.m.
Hi,
On Tue, May 6, 2014 at 9:43 PM, Hardy Ferentschik <hardy(a)hibernate.org> wrote:
I think I favour #2 atm, since it seems more symmetric.
I'm also in favour of #2.
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.
Same here. Otherwise, it will break (mostly) silently a lot of
applications out there.
--
Guillaume
12:01 p.m.
On 06 May 2014, at 21:43, Hardy Ferentschik <hardy(a)hibernate.org> 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.
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.
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.
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.
Emmanuel
12:19 p.m.
On 29 Apr 2014, at 20:43, Hardy Ferentschik <hardy(a)hibernate.org> wrote:
On 29 Jan 2014, at 15:24, Sanne Grinovero <sanne(a)hibernate.org>
wrote:
>>> 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.
Nope, the code is quite explicit. It contains a if/else statement checking whether
we are processing the root entity and in the else part the comment says:
“// component should index their document id”
This is also not just an outcome of the metadata refactoring, since this if statement
and comment also has been there in the DocumentBuilder code [1]
So it has been an explicit choice at some point of time.
Correct.
12:29 p.m.
Thanks for the input. Comments inline.
On 27 Jan 2014, at 12:01, Emmanuel Bernard <emmanuel(a)hibernate.org> wrote:
On 06 May 2014, at 21:43, Hardy Ferentschik <hardy(a)hibernate.org> 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.
—Hardy
1:53 p.m.
On 29 Apr 2014, at 15:09, Sanne Grinovero <sanne(a)hibernate.org> wrote:
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.
Right, splitting the annotation in two does not address the mix of both concepts.
5:29 p.m.
On 27 May 2014, at 12:29, Hardy Ferentschik <hardy(a)hibernate.org> wrote:
Thanks for the input. Comments inline.
On 27 Jan 2014, at 12:01, Emmanuel Bernard <emmanuel(a)hibernate.org> wrote:
>
> On 06 May 2014, at 21:43, Hardy Ferentschik <hardy(a)hibernate.org> 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.
#3 then #2 for me. I really like #3 better though for the reason I explained.
We should bet at horse races together ;)
9:55 p.m.
> 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?
—Hardy
12:46 p.m.
On 27 May 2014 20:55, Hardy Ferentschik <hardy(a)hibernate.org> wrote:
>> 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?
My thought is that this new attribute is redundant: it seems we all
agree that the ids should not be included by default, and as a power
user I can opt to use includePath to force inclusion.
So the @IndexEmbedded(includeEmbeddedObjectId=[boolean]) is not
strictly needed. Why not remove it?
I don't feel strongly about it, and since you all seemed to agree on
wanting this I'm ok to merge the current proposal, but I think it's
worthwile to return on this as a re-wrap since we seemed to agree on
the basic needs of "slim index by default", the only doubt I have is
if we really need two different ways to override the defaults.
Sanne
3600
days inactive
3650
days old
19 comments
4 participants
participants (4)
-
Emmanuel Bernard -
Guillaume Smet -
Hardy Ferentschik -
Sanne Grinovero