[
https://issues.apache.org/jira/browse/LUCENE-1188?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Chandan Raj Rupakheti updated LUCENE-1188:
------------------------------------------
Description:
I would like to talk about the implementation of equals and hashCode method in
org.apache.lucene.search.* package.
Example One:
org.apache.lucene.search.spans.SpanTermQuery (Super Class)
<- org.apache.lucene.search.payloads.BoostingTermQuery (Sub Class)
Observation:
* BoostingTermQuery defines equals but inherits hashCode from SpanTermQuery.
Definition of equals is a code clone of SpanTermQuery with a change in class
name.
Intention:
I believe the intention of equals redefinition in BoostingTermQuery is not to
make the objects of SpanTermQuery and BoostingTermQuery comparable. ie.
spanTermQuery.equals(boostingTermQuery) == false &&
boostingTermQuery.equals(spanTermQuery) == false.
Problem:
With current implementation, the intention might not be respected as a result
of symmetric property violation of equals contract i.e.
spanTermQuery.equals(boostingTermQuery) == true (can be) &&
boostingTermQuery.equals(spanTermQuery) == false. (always)
(Note: Provided their state variables are equal)
Solution:
Change implementation of equals in SpanTermQuery from:
{code:title=SpanTermQuery.java|borderStyle=solid}
public boolean equals(Object o) {
if (!(o instanceof SpanTermQuery))
return false;
SpanTermQuery other = (SpanTermQuery)o;
return (this.getBoost() == other.getBoost())
&& this.term.equals(other.term);
}
{code}
To:
{code:title=SpanTermQuery.java|borderStyle=solid}
public boolean equals(Object o) {
if(o == this) return true;
if(o == null || o.getClass() != this.getClass()) return false;
// if (!(o instanceof SpanTermQuery))
// return false;
SpanTermQuery other = (SpanTermQuery)o;
return (this.getBoost() == other.getBoost())
&& this.term.equals(other.term);
}
{code}
Advantage:
* BoostingTermQuery.equals and BoostingTermQuery.hashCode is not needed while
still preserving the same intention as before.
* Any further subclassing that does not add new state variables in the extended
classes of SpanTermQuery, does not have to redefine equals and hashCode.
* Even if a new state variable is added in a subclass, the symmetric property
of equals contract will still be respected irrespective of implementation (i.e.
instanceof / getClass) of equals and hashCode in the subclasses.
Example Two:
org.apache.lucene.search.CachingWrapperFilter (Super Class)
<- org.apache.lucene.search.CachingWrapperFilterHelper (Sub Class)
Observation:
Same as Example One.
Problem:
Same as Example one.
Solution:
Change equals in CachingWrapperFilter from:
{code:title=CachingWrapperFilter.java|borderStyle=solid}
public boolean equals(Object o) {
if (!(o instanceof CachingWrapperFilter)) return false;
return this.filter.equals(((CachingWrapperFilter)o).filter);
}
{code}
To:
{code:title=CachingWrapperFilter.java|borderStyle=solid}
public boolean equals(Object o) {
// if (!(o instanceof CachingWrapperFilter)) return false;
if(o == this) return true;
if(o == null || o.getClass() != this.getClass()) return false;
return this.filter.equals(((CachingWrapperFilter)o).filter);
}
{code}
Advantage:
Same as Example One. Here, CachingWrapperFilterHelper.equals and
CachingWrapperFilterHelper.hashCode is not needed.
Example Three:
org.apache.lucene.search.MultiTermQuery (Abstract Parent)
<- org.apache.lucene.search.FuzzyQuery (Concrete Sub)
<- org.apache.lucene.search.WildcardQuery (Concrete Sub)
Observation (Not a problem):
* WildcardQuery defines equals but inherits hashCode from MultiTermQuery.
Definition of equals contains just super.equals invocation.
* FuzzyQuery has few state variables added that are referenced in its equals
and hashCode.
Intention:
I believe the intention here is not to make objects of FuzzyQuery and
WildcardQuery comparable. ie. fuzzyQuery.equals(wildCardQuery) == false &&
wildCardQuery.equals(fuzzyQuery) == false.
Proposed Implementation:
How about changing the implementation of equals in MultiTermQuery from:
{code:title=MultiTermQuery.java|borderStyle=solid}
public boolean equals(Object o) {
if (this == o) return true;
if (!(o instanceof MultiTermQuery)) return false;
final MultiTermQuery multiTermQuery = (MultiTermQuery) o;
if (!term.equals(multiTermQuery.term)) return false;
return getBoost() == multiTermQuery.getBoost();
}
{code}
To:
{code:title=MultiTermQuery.java|borderStyle=solid}
public boolean equals(Object o) {
if (this == o) return true;
// if (!(o instanceof MultiTermQuery)) return false;
if(o == null || o.getClass() != this.getClass()) return false;
final MultiTermQuery multiTermQuery = (MultiTermQuery) o;
if (!term.equals(multiTermQuery.term)) return false;
return getBoost() == multiTermQuery.getBoost();
}
{code}
Advantage:
Same as above. Here, WildcardQuery.equals is not needed as it does not define
any new state. (FuzzyQuery.equals is still needed because FuzzyQuery defines
new state.)
was:
I would like to talk about the implementation of equals and hashCode method in
org.apache.lucene.search.* package.
Example One:
org.apache.lucene.search.spans.SpanTermQuery (Super Class)
<- org.apache.lucene.search.payloads.BoostingTermQuery (Sub Class)
Observation:
* BoostingTermQuery defines equals but inherits hashCode from SpanTermQuery.
Definition of equals is a code clone of SpanTermQuery with a change in class
name.
Intention:
I believe the intention of equals redefinition in BoostingTermQuery is not to
make the objects of SpanTermQuery and BoostingTermQuery comparable. ie.
spanTermQuery.equals(boostingTermQuery) == false &&
boostingTermQuery.equals(spanTermQuery) == false.
Problem:
With current implementation, the intention might not be respected as a result
of symmetric property violation of equals contract i.e.
spanTermQuery.equals(boostingTermQuery) == true (can be) &&
boostingTermQuery.equals(spanTermQuery) == false. (always)
(Note: Provided their state variables are equal)
Solution:
Change implementation of equals in SpanTermQuery from:
{code:title=SpanTermQuery.java|borderStyle=solid}
public boolean equals(Object o) {
if (!(o instanceof SpanTermQuery))
return false;
SpanTermQuery other = (SpanTermQuery)o;
return (this.getBoost() == other.getBoost())
&& this.term.equals(other.term);
}
{code}
To:
{code:title=SpanTermQuery.java|borderStyle=solid}
public boolean equals(Object o) {
if(o == this) return true;
if(o == null || o.getClass() != this.getClass()) return false;
// if (!(o instanceof SpanTermQuery))
// return false;
SpanTermQuery other = (SpanTermQuery)o;
return (this.getBoost() == other.getBoost())
&& this.term.equals(other.term);
}
{code}
Advantage:
* Do not have to redefine equals and hashCode in BoostingTermQuery while
preserving the same intention as before.
* Any further subclassing that does not add new state variables in the extended
classes of SpanTermQuery, does not have to redefine equals and hashCode.
* Even if a new state variable is added in a subclass, the symmetric property
of equals contract will still be respected irrespective of implementation (i.e.
instanceof / getClass) of equals and hashCode in the subclasses.
Example Two:
org.apache.lucene.search.CachingWrapperFilter (Super Class)
<- org.apache.lucene.search.CachingWrapperFilterHelper (Sub Class)
Observation:
Same as Example One.
Problem:
Same as Example one.
Solution:
Change equals in CachingWrapperFilter from:
{code:title=CachingWrapperFilter.java|borderStyle=solid}
public boolean equals(Object o) {
if (!(o instanceof CachingWrapperFilter)) return false;
return this.filter.equals(((CachingWrapperFilter)o).filter);
}
{code}
To:
{code:title=CachingWrapperFilter.java|borderStyle=solid}
public boolean equals(Object o) {
// if (!(o instanceof CachingWrapperFilter)) return false;
if(o == this) return true;
if(o == null || o.getClass() != this.getClass()) return false;
return this.filter.equals(((CachingWrapperFilter)o).filter);
}
{code}
Advantage:
Same as Example One.
Example Three:
org.apache.lucene.search.MultiTermQuery (Abstract Parent)
<- org.apache.lucene.search.FuzzyQuery (Concrete Sub)
<- org.apache.lucene.search.WildcardQuery (Concrete Sub)
Observation (Not a problem):
* WildcardQuery defines equals but inherits hashCode from MultiTermQuery.
Definition of equals contains just super.equals invocation.
* FuzzyQuery has few state variables added that are referenced in its equals
and hashCode.
Intention:
I believe the intention here is not to make objects of FuzzyQuery and
WildcardQuery comparable. ie. fuzzyQuery.equals(wildCardQuery) == false &&
wildCardQuery.equals(fuzzyQuery) == false.
Proposed Implementation:
How about changing the implementation of equals in MultiTermQuery from:
{code:title=MultiTermQuery.java|borderStyle=solid}
public boolean equals(Object o) {
if (this == o) return true;
if (!(o instanceof MultiTermQuery)) return false;
final MultiTermQuery multiTermQuery = (MultiTermQuery) o;
if (!term.equals(multiTermQuery.term)) return false;
return getBoost() == multiTermQuery.getBoost();
}
{code}
To:
{code:title=MultiTermQuery.java|borderStyle=solid}
public boolean equals(Object o) {
if (this == o) return true;
// if (!(o instanceof MultiTermQuery)) return false;
if(o == null || o.getClass() != this.getClass()) return false;
final MultiTermQuery multiTermQuery = (MultiTermQuery) o;
if (!term.equals(multiTermQuery.term)) return false;
return getBoost() == multiTermQuery.getBoost();
}
{code}
Advantage:
Same as above.
Updated the advantage of the proposed solution to make it more clear.
> equals and hashCode implementation in org.apache.lucene.search.* package
> ------------------------------------------------------------------------
>
> Key: LUCENE-1188
> URL: https://issues.apache.org/jira/browse/LUCENE-1188
> Project: Lucene - Java
> Issue Type: Improvement
> Components: Search
> Affects Versions: 2.2, 2.3, 2.3.1
> Environment: All
> Reporter: Chandan Raj Rupakheti
> Fix For: 2.2, 2.3, 2.3.1
>
>
> I would like to talk about the implementation of equals and hashCode method
> in org.apache.lucene.search.* package.
> Example One:
> org.apache.lucene.search.spans.SpanTermQuery (Super Class)
> <- org.apache.lucene.search.payloads.BoostingTermQuery (Sub Class)
> Observation:
> * BoostingTermQuery defines equals but inherits hashCode from SpanTermQuery.
> Definition of equals is a code clone of SpanTermQuery with a change in class
> name.
> Intention:
> I believe the intention of equals redefinition in BoostingTermQuery is not to
> make the objects of SpanTermQuery and BoostingTermQuery comparable. ie.
> spanTermQuery.equals(boostingTermQuery) == false &&
> boostingTermQuery.equals(spanTermQuery) == false.
> Problem:
> With current implementation, the intention might not be respected as a result
> of symmetric property violation of equals contract i.e.
> spanTermQuery.equals(boostingTermQuery) == true (can be) &&
> boostingTermQuery.equals(spanTermQuery) == false. (always)
> (Note: Provided their state variables are equal)
> Solution:
> Change implementation of equals in SpanTermQuery from:
> {code:title=SpanTermQuery.java|borderStyle=solid}
> public boolean equals(Object o) {
> if (!(o instanceof SpanTermQuery))
> return false;
> SpanTermQuery other = (SpanTermQuery)o;
> return (this.getBoost() == other.getBoost())
> && this.term.equals(other.term);
> }
> {code}
> To:
> {code:title=SpanTermQuery.java|borderStyle=solid}
> public boolean equals(Object o) {
> if(o == this) return true;
> if(o == null || o.getClass() != this.getClass()) return false;
> // if (!(o instanceof SpanTermQuery))
> // return false;
> SpanTermQuery other = (SpanTermQuery)o;
> return (this.getBoost() == other.getBoost())
> && this.term.equals(other.term);
> }
> {code}
> Advantage:
> * BoostingTermQuery.equals and BoostingTermQuery.hashCode is not needed while
> still preserving the same intention as before.
>
> * Any further subclassing that does not add new state variables in the
> extended classes of SpanTermQuery, does not have to redefine equals and
> hashCode.
> * Even if a new state variable is added in a subclass, the symmetric property
> of equals contract will still be respected irrespective of implementation
> (i.e. instanceof / getClass) of equals and hashCode in the subclasses.
> Example Two:
> org.apache.lucene.search.CachingWrapperFilter (Super Class)
> <- org.apache.lucene.search.CachingWrapperFilterHelper (Sub Class)
> Observation:
> Same as Example One.
> Problem:
> Same as Example one.
> Solution:
> Change equals in CachingWrapperFilter from:
> {code:title=CachingWrapperFilter.java|borderStyle=solid}
> public boolean equals(Object o) {
> if (!(o instanceof CachingWrapperFilter)) return false;
> return this.filter.equals(((CachingWrapperFilter)o).filter);
> }
> {code}
> To:
> {code:title=CachingWrapperFilter.java|borderStyle=solid}
> public boolean equals(Object o) {
> // if (!(o instanceof CachingWrapperFilter)) return false;
> if(o == this) return true;
> if(o == null || o.getClass() != this.getClass()) return false;
> return this.filter.equals(((CachingWrapperFilter)o).filter);
> }
> {code}
> Advantage:
> Same as Example One. Here, CachingWrapperFilterHelper.equals and
> CachingWrapperFilterHelper.hashCode is not needed.
> Example Three:
> org.apache.lucene.search.MultiTermQuery (Abstract Parent)
> <- org.apache.lucene.search.FuzzyQuery (Concrete Sub)
> <- org.apache.lucene.search.WildcardQuery (Concrete Sub)
> Observation (Not a problem):
> * WildcardQuery defines equals but inherits hashCode from MultiTermQuery.
> Definition of equals contains just super.equals invocation.
> * FuzzyQuery has few state variables added that are referenced in its equals
> and hashCode.
> Intention:
> I believe the intention here is not to make objects of FuzzyQuery and
> WildcardQuery comparable. ie. fuzzyQuery.equals(wildCardQuery) == false &&
> wildCardQuery.equals(fuzzyQuery) == false.
> Proposed Implementation:
> How about changing the implementation of equals in MultiTermQuery from:
> {code:title=MultiTermQuery.java|borderStyle=solid}
> public boolean equals(Object o) {
> if (this == o) return true;
> if (!(o instanceof MultiTermQuery)) return false;
> final MultiTermQuery multiTermQuery = (MultiTermQuery) o;
> if (!term.equals(multiTermQuery.term)) return false;
> return getBoost() == multiTermQuery.getBoost();
> }
> {code}
> To:
> {code:title=MultiTermQuery.java|borderStyle=solid}
> public boolean equals(Object o) {
> if (this == o) return true;
> // if (!(o instanceof MultiTermQuery)) return false;
> if(o == null || o.getClass() != this.getClass()) return false;
> final MultiTermQuery multiTermQuery = (MultiTermQuery) o;
> if (!term.equals(multiTermQuery.term)) return false;
> return getBoost() == multiTermQuery.getBoost();
> }
> {code}
> Advantage:
> Same as above. Here, WildcardQuery.equals is not needed as it does not define
> any new state. (FuzzyQuery.equals is still needed because FuzzyQuery defines
> new state.)
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
