You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@lucene.apache.org by "david.w.smiley@gmail.com" <da...@gmail.com> on 2015/04/01 21:25:52 UTC
Re: [DISCUSS] Change Query API to make queries immutable in 6.0
I’m +1 to going all the way (fully immutable) but the proposal stops short
by skipping the boost. I agree with Terry’s comments — what a shame to
make Queries “more immutable” but not really quite immutable. It kinda
misses the point? Otherwise why bother? If this is about progress not
perfection, then okay, but if we don’t ultimately go all the way then there
isn’t the benefit we’re after and we’ve both changed the API and made it a
bit more awkward to use. I like the idea of a method like cloneWithBoost()
or some-such. A no-arg clone() could be final and call that one with the
current boost.
While we’re at it, BooleanQuery & other variable aggregates could cache the
hashCode at construction.
~ David Smiley
Freelance Apache Lucene/Solr Search Consultant/Developer
http://www.linkedin.com/in/davidwsmiley
On Tue, Mar 31, 2015 at 11:06 AM, Adrien Grand <jp...@gmail.com> wrote:
> On Tue, Mar 31, 2015 at 4:32 PM, Terry Smith <sh...@gmail.com> wrote:
> > Thanks for the explanation. It seems a pity to make queries just nearly
> > immutable. Do you have any interest in adding a boost parameter to
> clone()
> > so they really could be immutable?
>
> We could have a single method, but if we do it I would rather do it in
> a different change since it would affect all queries as opposed to
> only a handful of them.
>
> Also there is some benefit in having clone() and setBoost() in that
> cloning and setters are things that are familiar to everyone. If we
> replace them with a new method, we would need to specify its
> semantics. (Not a blocker, just wanted to mention what the pros/cons
> are in my opinion.)
>
> --
> Adrien
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
> For additional commands, e-mail: dev-help@lucene.apache.org
>
>
Re: [DISCUSS] Change Query API to make queries immutable in 6.0
Posted by "david.w.smiley@gmail.com" <da...@gmail.com>.
On Thu, Apr 2, 2015 at 9:45 AM, Robert Muir <rc...@gmail.com> wrote:
> They are also only relevant when scores are needed:
> so we can prevent nasty filter caching bugs as a step, by making
> everything else immutable.
>
That’s a good point.
+1 to your progress Adrien!
~ David Smiley
Freelance Apache Lucene/Solr Search Consultant/Developer
http://www.linkedin.com/in/davidwsmiley
Re: [DISCUSS] Change Query API to make queries immutable in 6.0
Posted by Robert Muir <rc...@gmail.com>.
On Thu, Apr 2, 2015 at 6:23 PM, Adrien Grand <jp...@gmail.com> wrote:
> On Fri, Apr 3, 2015 at 12:10 AM, Reitzel, Charles
> <Ch...@tiaa-cref.org> wrote:
>> Unfortunately, since boost is used in hashCode() and equals() calculations, changing the boost will still make the queries "trappy". You will do all that work to make everything-but-boost immutable and still not fix the problem.
>
> This is why the query cache clones queries before putting them into
> the cache and sets the boost to 1. The issue is that this clone method
> is only shallow since its purpose is to change the boost. So it works
> fine for the boost parameter but not for eg. boolean clauses.
>
> Again, I don't dismiss advantages of going fully immutable
I will. I do not think it is worth it.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
Re: [DISCUSS] Change Query API to make queries immutable in 6.0
Posted by Adrien Grand <jp...@gmail.com>.
On Fri, Apr 3, 2015 at 12:10 AM, Reitzel, Charles
<Ch...@tiaa-cref.org> wrote:
> Unfortunately, since boost is used in hashCode() and equals() calculations, changing the boost will still make the queries "trappy". You will do all that work to make everything-but-boost immutable and still not fix the problem.
This is why the query cache clones queries before putting them into
the cache and sets the boost to 1. The issue is that this clone method
is only shallow since its purpose is to change the boost. So it works
fine for the boost parameter but not for eg. boolean clauses.
Again, I don't dismiss advantages of going fully immutable, I'm just
arguing that making all queries immutable up to the boost would
already be a good improvement like Robert described. We can still
discuss about going fully immutable in a different issue, it would
benefit from the proposed change.
--
Adrien
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
RE: [DISCUSS] Change Query API to make queries immutable in 6.0
Posted by "Reitzel, Charles" <Ch...@tiaa-cref.org>.
Unfortunately, since boost is used in hashCode() and equals() calculations, changing the boost will still make the queries "trappy". You will do all that work to make everything-but-boost immutable and still not fix the problem.
You can prove it to yourself like so (this test fails!):
public void testMapOrphan() {
Map<Query, Integer> map = new HashMap<>();
BooleanQuery booleanAB = new BooleanQuery();
booleanAB.add(new TermQuery(new Term("contents", "a")), BooleanClause.Occur.SHOULD);
booleanAB.add(new TermQuery(new Term("contents", "b")), BooleanClause.Occur.SHOULD);
map.put( booleanAB, 1 );
booleanAB.setBoost( 33.3f ); // Set boost after map.put()
assertTrue( map.containsKey(booleanAB) );
}
Seems like the quick way to the coast is to write a failing test - before making changes.
I realize this is easier said than done. Based on your testing that led you to start this discussion, can you narrow it down to a single Query class and/or IndexSearcher use case? Not there will be only one case. But, at least, it will be a starting point. Once the first failing test has been written, it should be relatively easy to write test variations to cover the remaining "mutuable" Query classes.
With the scale of the changes you are proposing, "test first" seems like a reasonable approach.
Another compromise approach might be to sub-class the mutable Query classes like so:
class ImmutableBooleanQuery extends BooleanQuery {
public void add(BooleanClause clause) {
throw new UnsupportedOperationException( "ImmutableBooleanQuery.add(BooleanClause)" );
}
public void setBoost( int boost ) {
throw new UnsupportedOperationException( "ImmutableBooleanQuery.add(BooleanClause)" );
}
// etc.
public static ImmutableBooleanQuery cloneFrom(BooleanQuery original) {
// Use field level access to by-pass mutator methods.
}
// Do NOT override rewrite(IndexReader)!
}
In theory, such a proxy class could be generated at runtime to force immutability:
https://github.com/verhas/immutator
Which could make a lot of sense in JUnit tests, if not production runtime.
An immutable Query would be cloned from the original and place on the cache instead. Any attempt to modify the cache entry should fail quickly.
To me, a less invasive approach seems like a faster and easier way to actually find and fix this bug.
Once that is done, then it might make sense to perform the exhaustive updates to prevent a "relapse" in the future.
-----Original Message-----
From: Robert Muir [mailto:rcmuir@gmail.com]
Sent: Thursday, April 02, 2015 9:46 AM
To: dev@lucene.apache.org
Subject: Re: [DISCUSS] Change Query API to make queries immutable in 6.0
Boosts might not make sense to become immutable, it might make the code too complex. Who is to say until the other stuff is fixed first.
The downsides might outweight the upsides.
So yeah, if you want to say "if anyone disagrees with what the future might look like i'm gonna -1 your progress", then i will bite right now.
Fixing the rest of Query to be immutable, so filter caching isn't trappy, we should really do that. And we have been doing it already. I remember Uwe suggested this approach when adding automaton and related queries a long time ago. It made things simpler and avoided bugs, we ultimately made as much of it immutable as we could.
Queries have to be well-behaved, they need a good hashcode/equals, thread safety, good error checking, etc. It is easier to do this when things are immutable. Someone today can make a patch for FooQuery that nukes setBar and moves it to a ctor parameter named 'bar' and chances are a lot of the time, it probably fixes bugs in FooQuery somehow.
Thats just what it is.
Boosts are the 'long tail'. they are simple primitive floating point values, so susceptible to less problems. The base class incorporates boosts into equals/hashcode already, which prevents the most common bugs with them. They are trickier because internal things like
rewrite() might "shuffle them around" in conjunction with clone(), to do optimizations. They are also only relevant when scores are needed:
so we can prevent nasty filter caching bugs as a step, by making everything else immutable.
On Thu, Apr 2, 2015 at 9:27 AM, david.w.smiley@gmail.com <da...@gmail.com> wrote:
> On Thu, Apr 2, 2015 at 3:40 AM, Adrien Grand <jp...@gmail.com> wrote:
>>
>> first make queries immutable up to the boost and then discuss
>> if/how/when we should go fully immutable with a new API to change
>> boosts?
>
>
> The “if” part concerns me; I don’t mind it being a separate issue to
> make the changes more manageable (progress not perfection, and all
> that). I’m all for the whole shebang. But if others think “no”
> then…. will it have been worthwhile to do this big change and not go all the way? I think not.
> Does anyone feel the answer is “no” to make boosts immutable? And if so why?
>
> If nobody comes up with a dissenting opinion to make boosts immutable
> within a couple days then count me as “+1” to your plans, else “-1”
> pending that discussion.
>
> ~ David Smiley
> Freelance Apache Lucene/Solr Search Consultant/Developer
> http://www.linkedin.com/in/davidwsmiley
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org For additional commands, e-mail: dev-help@lucene.apache.org
*************************************************************************
This e-mail may contain confidential or privileged information.
If you are not the intended recipient, please notify the sender immediately and then delete it.
TIAA-CREF
*************************************************************************
Re: [DISCUSS] Change Query API to make queries immutable in 6.0
Posted by Robert Muir <rc...@gmail.com>.
Boosts might not make sense to become immutable, it might make the
code too complex. Who is to say until the other stuff is fixed first.
The downsides might outweight the upsides.
So yeah, if you want to say "if anyone disagrees with what the future
might look like i'm gonna -1 your progress", then i will bite right
now.
Fixing the rest of Query to be immutable, so filter caching isn't
trappy, we should really do that. And we have been doing it already. I
remember Uwe suggested this approach when adding automaton and related
queries a long time ago. It made things simpler and avoided bugs, we
ultimately made as much of it immutable as we could.
Queries have to be well-behaved, they need a good hashcode/equals,
thread safety, good error checking, etc. It is easier to do this when
things are immutable. Someone today can make a patch for FooQuery that
nukes setBar and moves it to a ctor parameter named 'bar' and chances
are a lot of the time, it probably fixes bugs in FooQuery somehow.
Thats just what it is.
Boosts are the 'long tail'. they are simple primitive floating point
values, so susceptible to less problems. The base class incorporates
boosts into equals/hashcode already, which prevents the most common
bugs with them. They are trickier because internal things like
rewrite() might "shuffle them around" in conjunction with clone(), to
do optimizations. They are also only relevant when scores are needed:
so we can prevent nasty filter caching bugs as a step, by making
everything else immutable.
On Thu, Apr 2, 2015 at 9:27 AM, david.w.smiley@gmail.com
<da...@gmail.com> wrote:
> On Thu, Apr 2, 2015 at 3:40 AM, Adrien Grand <jp...@gmail.com> wrote:
>>
>> first make queries immutable up to the boost and then discuss
>> if/how/when we should go fully immutable with a new API to change
>> boosts?
>
>
> The “if” part concerns me; I don’t mind it being a separate issue to make
> the changes more manageable (progress not perfection, and all that). I’m
> all for the whole shebang. But if others think “no” then…. will it have
> been worthwhile to do this big change and not go all the way? I think not.
> Does anyone feel the answer is “no” to make boosts immutable? And if so why?
>
> If nobody comes up with a dissenting opinion to make boosts immutable within
> a couple days then count me as “+1” to your plans, else “-1” pending that
> discussion.
>
> ~ David Smiley
> Freelance Apache Lucene/Solr Search Consultant/Developer
> http://www.linkedin.com/in/davidwsmiley
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
Re: [DISCUSS] Change Query API to make queries immutable in 6.0
Posted by Yonik Seeley <ys...@gmail.com>.
If we were designing things from scratch again, would boost really be
on Query, or would it be on BooleanClause?
Just throwing that out there... although it may make it easier to
implement immutable queries (and perhaps make more sense too), it may
also be too big of a change to be worth while.
-Yonik
On Thu, Apr 2, 2015 at 9:27 AM, david.w.smiley@gmail.com
<da...@gmail.com> wrote:
> On Thu, Apr 2, 2015 at 3:40 AM, Adrien Grand <jp...@gmail.com> wrote:
>>
>> first make queries immutable up to the boost and then discuss
>> if/how/when we should go fully immutable with a new API to change
>> boosts?
>
>
> The “if” part concerns me; I don’t mind it being a separate issue to make
> the changes more manageable (progress not perfection, and all that). I’m
> all for the whole shebang. But if others think “no” then…. will it have
> been worthwhile to do this big change and not go all the way? I think not.
> Does anyone feel the answer is “no” to make boosts immutable? And if so why?
>
> If nobody comes up with a dissenting opinion to make boosts immutable within
> a couple days then count me as “+1” to your plans, else “-1” pending that
> discussion.
>
> ~ David Smiley
> Freelance Apache Lucene/Solr Search Consultant/Developer
> http://www.linkedin.com/in/davidwsmiley
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
Re: [DISCUSS] Change Query API to make queries immutable in 6.0
Posted by "david.w.smiley@gmail.com" <da...@gmail.com>.
On Thu, Apr 2, 2015 at 3:40 AM, Adrien Grand <jp...@gmail.com> wrote:
> first make queries immutable up to the boost and then discuss
> if/how/when we should go fully immutable with a new API to change
> boosts?
>
The “if” part concerns me; I don’t mind it being a separate issue to make
the changes more manageable (progress not perfection, and all that). I’m
all for the whole shebang. But if others think “no” then…. will it have
been worthwhile to do this big change and not go all the way? I think
not. Does anyone feel the answer is “no” to make boosts immutable? And if
so why?
If nobody comes up with a dissenting opinion to make boosts immutable
within a couple days then count me as “+1” to your plans, else “-1” pending
that discussion.
~ David Smiley
Freelance Apache Lucene/Solr Search Consultant/Developer
http://www.linkedin.com/in/davidwsmiley
Re: [DISCUSS] Change Query API to make queries immutable in 6.0
Posted by Michael McCandless <lu...@mikemccandless.com>.
On Thu, Apr 2, 2015 at 3:40 AM, Adrien Grand <jp...@gmail.com> wrote:
> Would would be
> ok to first make queries immutable up to the boost and then discuss
> if/how/when we should go fully immutable with a new API to change
> boosts?
+1 ... progress not perfection.
Mike McCandless
http://blog.mikemccandless.com
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
Re: [DISCUSS] Change Query API to make queries immutable in 6.0
Posted by Adrien Grand <jp...@gmail.com>.
I did not close this door, I agree this is something that should be
considered and tried to list the pros/cons that I could think about.
However I would like it to be dealt with in a different issue as it
will already be a big change to change those 4 queries. Would would be
ok to first make queries immutable up to the boost and then discuss
if/how/when we should go fully immutable with a new API to change
boosts?
On Wed, Apr 1, 2015 at 9:25 PM, david.w.smiley@gmail.com
<da...@gmail.com> wrote:
> I’m +1 to going all the way (fully immutable) but the proposal stops short
> by skipping the boost. I agree with Terry’s comments — what a shame to make
> Queries “more immutable” but not really quite immutable. It kinda misses
> the point? Otherwise why bother? If this is about progress not perfection,
> then okay, but if we don’t ultimately go all the way then there isn’t the
> benefit we’re after and we’ve both changed the API and made it a bit more
> awkward to use. I like the idea of a method like cloneWithBoost() or
> some-such. A no-arg clone() could be final and call that one with the
> current boost.
>
> While we’re at it, BooleanQuery & other variable aggregates could cache the
> hashCode at construction.
>
> ~ David Smiley
> Freelance Apache Lucene/Solr Search Consultant/Developer
> http://www.linkedin.com/in/davidwsmiley
>
> On Tue, Mar 31, 2015 at 11:06 AM, Adrien Grand <jp...@gmail.com> wrote:
>>
>> On Tue, Mar 31, 2015 at 4:32 PM, Terry Smith <sh...@gmail.com> wrote:
>> > Thanks for the explanation. It seems a pity to make queries just nearly
>> > immutable. Do you have any interest in adding a boost parameter to
>> > clone()
>> > so they really could be immutable?
>>
>> We could have a single method, but if we do it I would rather do it in
>> a different change since it would affect all queries as opposed to
>> only a handful of them.
>>
>> Also there is some benefit in having clone() and setBoost() in that
>> cloning and setters are things that are familiar to everyone. If we
>> replace them with a new method, we would need to specify its
>> semantics. (Not a blocker, just wanted to mention what the pros/cons
>> are in my opinion.)
>>
>> --
>> Adrien
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
>> For additional commands, e-mail: dev-help@lucene.apache.org
>>
>
--
Adrien
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org