You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@lucene.apache.org by Paul Cowan <co...@aconex.com> on 2009/08/18 01:25:43 UTC
Gentle Prod: LUCENE-1626 (flexible getPositionIncrementGap)
Hi all,
Just wondering if anyone out there would be interested in taking a quick
peek at https://issues.apache.org/jira/browse/LUCENE-1626 -- this lets
the position increment gap in an analyzer depend on the current term
position, which lets you do some cool things with 'one-to-many'
relationships (see https://issues.apache.org/jira/browse/LUCENE-1494 for
the background and explanation).
It's a very low-impact patch: all existing Analyzer implementations
should work fine, as the default implementation of the new method just
delegates to the old one.
The only breaking change is that the original
getPositionIncrementGap(String) becomes protected, as now it's merely
the default implementation. I think this is unlikely to be a big impact
though: the only place this is called inside Lucene outside another
analyzer class is in DocInverterPerField, and it seems unlikely to me
that any user code would be calling this method directly (happy to be
corrected here).
I'm asking for purely selfish reasons, if 2.9 is getting very close then
if this makes it in, it's one less change to 2.9 we have to maintain in
our own repo. Not saying it SHOULD get in, but hey, worth a crack.
That patch is slightly old (done against 2.4, I think), but is very
similar to what's required against trunk. If people seem amenable to
landing this (either pre- or post-2.9) then I'll be sure produce a new
patch which applies cleanly.
Let me know what you think!
Cheers,
Paul
---------------------------------------------------------------------
To unsubscribe, e-mail: java-dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: java-dev-help@lucene.apache.org
Re: Gentle Prod: LUCENE-1626 (flexible getPositionIncrementGap)
Posted by Paul Cowan <co...@aconex.com>.
Mark Miller wrote:
> Thats a break in back compat - but I guess your saying even if its
> public its a break.
Agreed, it's a trade-off. Make it protected, and compile-time break any
custom inverters (if any), or leave it public and (potentially) break
the inverters anyway (by having them call the wrong method on
newly-written analyzers) -- but the second one happens silently, at runtime.
> Thats a tough one to push through in my mind then. Though the rarity of
> it affecting anyone might lure someone else to take it on.
Yep, it's an interesting one. I mean, I think it's actually very
low-risk because I can't see anyone having written their own inverter,
and going to 3.0 (effectively 2.9 is an early 3.0, without removing
deprecations, yeah?) is the perfect opportunity to do so because that's
when people expect back-compat to break, even if only in a minor way in
this case.,
That said, it's almost an impossible situation -- if you've got an API
exposed publicly that is a final method, you can work around it. If it's
NOT public, but can be overridden (non-final), you can work around THAT.
But when it's public to users of the class and non-final (indeed,
intended to be overridden!) there's no way to change the API in any
substantial way (certainly not by adding params) without breaking one or
the other use case. Presuming the intent is to allow SOME changes to the
API where required, I think this one is low risk but interested to see
what others think.
Cheers,
Paul
---------------------------------------------------------------------
To unsubscribe, e-mail: java-dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: java-dev-help@lucene.apache.org
Re: Gentle Prod: LUCENE-1626 (flexible getPositionIncrementGap)
Posted by Mark Miller <ma...@gmail.com>.
Sorry - not taking the time to properly think.
I got stuck on overriding - was thinking a lot on it on an earlier issue.
Scratch that.
{quote}Not sure how making it protected breaks backcompat. If people are
calling it directly (in their own inverter? Not sure if that would ever
happen) they'd be getting the wrong result after the change, because the
(String, int) version actually becomes the correct public API to find
the gap.{quote}
Thats a break in back compat - but I guess your saying even if its
public its a break.
Thats a tough one to push through in my mind then. Though the rarity of
it affecting anyone might lure someone else to take it on.
--
- Mark
http://www.lucidimagination.com
Paul Cowan wrote:
> Mark Miller wrote:
>> Why wouldn't people overriding the method be declaring it as
>> protected as well?
>
> I'm talking about _existing_ code... i.e. if someone already has:
>
> public class CrazyAnalyzer ... {
> public int getPositionIncrementGap(String field) {
> return random.nextInt(100);
> }
> }
>
> then my change (making gPIG(String) protected) won't break it. It will
> also continue to behave as expected due to the default impl of
> gPIG(String, int).
>
> Presumably people who are implementing new analyzers after this would
> make it protected, but it won't break back-compat of existing public
> overrides.
>
> Paul
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: java-dev-unsubscribe@lucene.apache.org
> For additional commands, e-mail: java-dev-help@lucene.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: java-dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: java-dev-help@lucene.apache.org
Re: Gentle Prod: LUCENE-1626 (flexible getPositionIncrementGap)
Posted by Paul Cowan <co...@aconex.com>.
Mark Miller wrote:
> Why wouldn't people overriding the method be declaring it as protected
> as well?
I'm talking about _existing_ code... i.e. if someone already has:
public class CrazyAnalyzer ... {
public int getPositionIncrementGap(String field) {
return random.nextInt(100);
}
}
then my change (making gPIG(String) protected) won't break it. It will
also continue to behave as expected due to the default impl of
gPIG(String, int).
Presumably people who are implementing new analyzers after this would
make it protected, but it won't break back-compat of existing public
overrides.
Paul
---------------------------------------------------------------------
To unsubscribe, e-mail: java-dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: java-dev-help@lucene.apache.org
Re: Gentle Prod: LUCENE-1626 (flexible getPositionIncrementGap)
Posted by Mark Miller <ma...@gmail.com>.
Paul Cowan wrote:
>
> If people are overriding the method in their own analyzers (which is
> presumably happening... that's what it's for) making it protected
> won't break compat -- people who are overriding the method must be
> declaring their overridden ones as public now, and overriding a
> protected method with a public one is perfectly acceptable (just not
> vice-versa).
Why wouldn't people overriding the method be declaring it as protected
as well?
--
- Mark
http://www.lucidimagination.com
---------------------------------------------------------------------
To unsubscribe, e-mail: java-dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: java-dev-help@lucene.apache.org
Re: Gentle Prod: LUCENE-1626 (flexible getPositionIncrementGap)
Posted by Paul Cowan <co...@aconex.com>.
Mark Miller wrote:
> Hmm ... I don't think it should become protected - is there a good
> reason for this? - it should probably remain public to preserve back
> compat.
Not sure how making it protected breaks backcompat. If people are
calling it directly (in their own inverter? Not sure if that would ever
happen) they'd be getting the wrong result after the change, because the
(String, int) version actually becomes the correct public API to find
the gap. Otherwise people might write their own analyzer just overriding
the new method, and one particular inverter implementation would ignore
their change.
If people are overriding the method in their own analyzers (which is
presumably happening... that's what it's for) making it protected won't
break compat -- people who are overriding the method must be declaring
their overridden ones as public now, and overriding a protected method
with a public one is perfectly acceptable (just not vice-versa).
Cheers,
Paul
---------------------------------------------------------------------
To unsubscribe, e-mail: java-dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: java-dev-help@lucene.apache.org
Re: Gentle Prod: LUCENE-1626 (flexible getPositionIncrementGap)
Posted by Mark Miller <ma...@gmail.com>.
Hmm ... I don't think it should become protected - is there a good
reason for this? - it should probably remain public to preserve back compat.
- Mark
Paul Cowan wrote:
> Hi all,
>
> Just wondering if anyone out there would be interested in taking a
> quick peek at https://issues.apache.org/jira/browse/LUCENE-1626 --
> this lets the position increment gap in an analyzer depend on the
> current term position, which lets you do some cool things with
> 'one-to-many' relationships (see
> https://issues.apache.org/jira/browse/LUCENE-1494 for the background
> and explanation).
>
> It's a very low-impact patch: all existing Analyzer implementations
> should work fine, as the default implementation of the new method just
> delegates to the old one.
>
> The only breaking change is that the original
> getPositionIncrementGap(String) becomes protected, as now it's merely
> the default implementation. I think this is unlikely to be a big
> impact though: the only place this is called inside Lucene outside
> another analyzer class is in DocInverterPerField, and it seems
> unlikely to me that any user code would be calling this method
> directly (happy to be corrected here).
>
> I'm asking for purely selfish reasons, if 2.9 is getting very close
> then if this makes it in, it's one less change to 2.9 we have to
> maintain in our own repo. Not saying it SHOULD get in, but hey, worth
> a crack.
>
> That patch is slightly old (done against 2.4, I think), but is very
> similar to what's required against trunk. If people seem amenable to
> landing this (either pre- or post-2.9) then I'll be sure produce a new
> patch which applies cleanly.
>
> Let me know what you think!
>
> Cheers,
>
> Paul
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: java-dev-unsubscribe@lucene.apache.org
> For additional commands, e-mail: java-dev-help@lucene.apache.org
>
--
- Mark
http://www.lucidimagination.com
---------------------------------------------------------------------
To unsubscribe, e-mail: java-dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: java-dev-help@lucene.apache.org