Uploaded image for project: 'Lucene - Core'
  1. Lucene - Core
  2. LUCENE-7277

Make Query.hashCode and Query.equals abstract

    Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Trivial
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 6.x, 7.0
    • Component/s: None
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      Custom subclasses of the Query class have the default implementation of hashCode/equals that make all instances of the subclass equal. If somebody doesn't know this it can be pretty tricky to debug with IndexSearcher's query cache on.

      Is there any rationale for declaring it this way instead of making those methods abstract (and enforcing their proper implementation in a subclass)?

        public int hashCode() {
          return getClass().hashCode();
        }
      
        public boolean equals(Object obj) {
          if (obj == null)
            return false;
          return getClass() == obj.getClass();
        }
      
      1. LUCENE-7277.patch
        205 kB
        Dawid Weiss
      2. LUCENE-7277.patch
        9 kB
        Dawid Weiss
      3. LUCENE-7277-20160518.patch
        91 kB
        Paul Elschot

        Issue Links

          Activity

          Hide
          jpountz Adrien Grand added a comment -

          +1 on making these abstract, especially now that the boost is gone from the base class

          Show
          jpountz Adrien Grand added a comment - +1 on making these abstract, especially now that the boost is gone from the base class
          Hide
          paul.elschot@xs4all.nl Paul Elschot added a comment -

          The rationale is that this code is common to some subclasses of Query, see also the patch at LUCENE-6372.

          There is nothing specific in this code for the Query class, any abstract class without attributes might have these definitions.
          Is there a way to enforce overriding a non abstract method in a subclass?

          Aside: somehow this reminds me of the TermQuery constructors, see LUCENE-6821 and LUCENE-4483.
          It's probably better to be on the safe side here too, and make these methods abstract.

          Otherwise we could add a remark in the javadocs of these methods that these methods should normally be overridden and called via super.

          A reminder in the javadocs of IndexSearcher would also help.

          Show
          paul.elschot@xs4all.nl Paul Elschot added a comment - The rationale is that this code is common to some subclasses of Query, see also the patch at LUCENE-6372 . There is nothing specific in this code for the Query class, any abstract class without attributes might have these definitions. Is there a way to enforce overriding a non abstract method in a subclass? Aside: somehow this reminds me of the TermQuery constructors, see LUCENE-6821 and LUCENE-4483 . It's probably better to be on the safe side here too, and make these methods abstract. Otherwise we could add a remark in the javadocs of these methods that these methods should normally be overridden and called via super. A reminder in the javadocs of IndexSearcher would also help.
          Hide
          dweiss Dawid Weiss added a comment -

          I fail see how an equivalence relationship over all objects of a class could be useful for caching reason (perhaps with the exception of MatchAllQuery)? I know for a fact it's misleading because a seasoned developer made the mistake of not overriding it (assuming the defaults are instance-equivalence, inherited from Object).

          Is there a way to enforce overriding a non abstract method in a subclass?

          Yes, you redeclare an overriden method as an abstract one. Then all subclasses have to reimplement it properly.

          Show
          dweiss Dawid Weiss added a comment - I fail see how an equivalence relationship over all objects of a class could be useful for caching reason (perhaps with the exception of MatchAllQuery)? I know for a fact it's misleading because a seasoned developer made the mistake of not overriding it (assuming the defaults are instance-equivalence, inherited from Object). Is there a way to enforce overriding a non abstract method in a subclass? Yes, you redeclare an overriden method as an abstract one. Then all subclasses have to reimplement it properly.
          Hide
          paul.elschot@xs4all.nl Paul Elschot added a comment -

          I fail see how an equivalence relationship over all objects of a class could be useful for caching reason

          The idea is to make super.equals() determine equality as far as the (object of the) superclass is concerned, and when it returns true to test the local class attributes.

          it's misleading ... assuming defaults ... inherited from Object

          So at least javadocs are needed to indicate that the current implementation only determines class equivalence.

          redeclare an overridden method as an abstract one

          Thanks for that.

          So there are some solutions here:

          • javadocs only (not preferred),
          • javadocs + redeclaration of overridden method as abstract
          • abstract methods only

          I'm fine with the latter two.
          Redeclaration is probably a smaller change because it would keep the common code in the superclass.

          Show
          paul.elschot@xs4all.nl Paul Elschot added a comment - I fail see how an equivalence relationship over all objects of a class could be useful for caching reason The idea is to make super.equals() determine equality as far as the (object of the) superclass is concerned, and when it returns true to test the local class attributes. it's misleading ... assuming defaults ... inherited from Object So at least javadocs are needed to indicate that the current implementation only determines class equivalence. redeclare an overridden method as an abstract one Thanks for that. So there are some solutions here: javadocs only (not preferred), javadocs + redeclaration of overridden method as abstract abstract methods only I'm fine with the latter two. Redeclaration is probably a smaller change because it would keep the common code in the superclass.
          Hide
          dsmiley David Smiley added a comment -

          +1 to make abstract only.

          Show
          dsmiley David Smiley added a comment - +1 to make abstract only.
          Hide
          dweiss Dawid Weiss added a comment -

          While it may sound like a good idea to put some of the shared "equivalence checks" in the superclass, it's actually the misleading part because, if left as-is and not overridden in a subclass, it results in an incorrect behavior (the query object is cached for all instances of the subclass).

          If query hash/equals is so important (and it is), I'd make it an abstract method to enforce proper override in all subclasses.

          Show
          dweiss Dawid Weiss added a comment - While it may sound like a good idea to put some of the shared "equivalence checks" in the superclass, it's actually the misleading part because, if left as-is and not overridden in a subclass, it results in an incorrect behavior (the query object is cached for all instances of the subclass). If query hash/equals is so important (and it is), I'd make it an abstract method to enforce proper override in all subclasses.
          Hide
          dweiss Dawid Weiss added a comment -

          So I'm thinking about something like this (it's not a complete patch, but given an idea what the pattern looks like).

          Looking at the current implementation only convinces me that the delegation pattern to superclass makes little sense – every query subclass is typically different, so it really should implement these correctly for its own case.

          I also opted to remove Class.hashCode() from hashCode's mixing functions, even if there are collisions they should be infrequent and easily solvable with equals(), so I don't see the point of computing it. But it's a matter of taste.

          Show
          dweiss Dawid Weiss added a comment - So I'm thinking about something like this (it's not a complete patch, but given an idea what the pattern looks like). Looking at the current implementation only convinces me that the delegation pattern to superclass makes little sense – every query subclass is typically different, so it really should implement these correctly for its own case. I also opted to remove Class.hashCode() from hashCode's mixing functions, even if there are collisions they should be infrequent and easily solvable with equals(), so I don't see the point of computing it. But it's a matter of taste.
          Hide
          dweiss Dawid Weiss added a comment -

          Corrected patch (wrong import).

          Show
          dweiss Dawid Weiss added a comment - Corrected patch (wrong import).
          Hide
          paul.elschot@xs4all.nl Paul Elschot added a comment -

          LGTM, shall I try this on the span queries?

          Show
          paul.elschot@xs4all.nl Paul Elschot added a comment - LGTM, shall I try this on the span queries?
          Hide
          dweiss Dawid Weiss added a comment -

          Yes, absolutely, if you have time, do it, go ahead! I noticed some endpoints of inheritance hierarchy (leaf classes) actually do not override equals and rely on superclasses – this seems incorrect to me, but perhaps there's a valid reason why this is the case.

          Show
          dweiss Dawid Weiss added a comment - Yes, absolutely, if you have time, do it, go ahead! I noticed some endpoints of inheritance hierarchy (leaf classes) actually do not override equals and rely on superclasses – this seems incorrect to me, but perhaps there's a valid reason why this is the case.
          Hide
          paul.elschot@xs4all.nl Paul Elschot added a comment -

          For leaf classes without overriding this used to work because of the getClass() in Query.equals() and Query.hashCode().
          Would it be ok use that in the intermediate span classes?

          Show
          paul.elschot@xs4all.nl Paul Elschot added a comment - For leaf classes without overriding this used to work because of the getClass() in Query.equals() and Query.hashCode(). Would it be ok use that in the intermediate span classes?
          Hide
          dweiss Dawid Weiss added a comment -

          What I meant was: if they're different Query classes then the equivalence should be object-specific, not class-specific, right? Otherwise what's the point of having those different classes – are all of their objects really equal?

          Show
          dweiss Dawid Weiss added a comment - What I meant was: if they're different Query classes then the equivalence should be object-specific, not class-specific, right? Otherwise what's the point of having those different classes – are all of their objects really equal?
          Hide
          paul.elschot@xs4all.nl Paul Elschot added a comment - - edited

          An intermediate class with attributes uses these attributes in its equals()/hashCode(), and a leaf class without attributes currently still has its class used by the default implementation that is called via super.

          The same can be done with sameClassAs() from the patch for equals(), combined with a getClass() in an intermediate hashCode() implementation. I think I'll give that a try.

          Show
          paul.elschot@xs4all.nl Paul Elschot added a comment - - edited An intermediate class with attributes uses these attributes in its equals()/hashCode(), and a leaf class without attributes currently still has its class used by the default implementation that is called via super. The same can be done with sameClassAs() from the patch for equals(), combined with a getClass() in an intermediate hashCode() implementation. I think I'll give that a try.
          Hide
          paul.elschot@xs4all.nl Paul Elschot added a comment -

          While trying this, I found that CommonTermsQuery is still mutable, and that is a bug iirc.

          Show
          paul.elschot@xs4all.nl Paul Elschot added a comment - While trying this, I found that CommonTermsQuery is still mutable, and that is a bug iirc.
          Hide
          paul.elschot@xs4all.nl Paul Elschot added a comment -

          Patch of 18 May 2016.

          This is for lucene core and for the other lucene modules, solr fails to compile.

          I added Objects.requireNonNull in a few constructors to simplify the hashCode/equals implementations.

          In the suggest module the queries throw UOE in hashCode/equals, that is probably correct, but I'm not sure.

          Show
          paul.elschot@xs4all.nl Paul Elschot added a comment - Patch of 18 May 2016. This is for lucene core and for the other lucene modules, solr fails to compile. I added Objects.requireNonNull in a few constructors to simplify the hashCode/equals implementations. In the suggest module the queries throw UOE in hashCode/equals, that is probably correct, but I'm not sure.
          Hide
          dweiss Dawid Weiss added a comment -

          Paul, which git commit is this patch against? Because, oddly, it doesn't apply for me now (master) and the commit sha's inside the patch I can't find in my local checkout... odd.

          In any case, my notes from manual review:

          • one of the reasons I don't like getClass().hashCode() is that it changes with every execution and runtime (classes delegate up to object identity which is ephemeral). While I'm a big fan of randomized testing, I'd like to keep things predictable and with a changing hashCode the layout of hashmaps (iteration order, collisions) will be different every time – something that may or may not drive you crazy when debugging... For this reason I'd opt to integrate something constant (a hashcode of the name of the class?) rather than the hashcode of the class object itself.
          • I'd throw an AssertionError (explicitly via new AssertionError) rather than UnsupportedOperationException in places where we think these methods should never be called (because of rewriting or other reasons).

          I can make these changes, but let me know what revision this patch was made against (so that I can apply it first and then rebase against master).

          Show
          dweiss Dawid Weiss added a comment - Paul, which git commit is this patch against? Because, oddly, it doesn't apply for me now (master) and the commit sha's inside the patch I can't find in my local checkout... odd. In any case, my notes from manual review: one of the reasons I don't like getClass().hashCode() is that it changes with every execution and runtime (classes delegate up to object identity which is ephemeral). While I'm a big fan of randomized testing, I'd like to keep things predictable and with a changing hashCode the layout of hashmaps (iteration order, collisions) will be different every time – something that may or may not drive you crazy when debugging... For this reason I'd opt to integrate something constant (a hashcode of the name of the class?) rather than the hashcode of the class object itself. I'd throw an AssertionError (explicitly via new AssertionError) rather than UnsupportedOperationException in places where we think these methods should never be called (because of rewriting or other reasons). I can make these changes, but let me know what revision this patch was made against (so that I can apply it first and then rebase against master).
          Hide
          paul.elschot@xs4all.nl Paul Elschot added a comment -

          The patch is against commit 4bc3477fbf9be688c14bbb07f5982d5f4b615591 on master of yesterday.
          It was made with git diff -w master from a local lucene7277 branch, which has commits roughly for each lucene module.
          I don't know which sha's git puts in a patch.

          I would not mind rebasing the local branch to update the patch, but I'll have very little time the coming days.

          In case replacing getClass().hashCode() by something like getClass().getName().hashCode() is preferred for easier testing, it would probably be better to add that in a companion method to sameClassAs(), for example classHash().

          Show
          paul.elschot@xs4all.nl Paul Elschot added a comment - The patch is against commit 4bc3477fbf9be688c14bbb07f5982d5f4b615591 on master of yesterday. It was made with git diff -w master from a local lucene7277 branch, which has commits roughly for each lucene module. I don't know which sha's git puts in a patch. I would not mind rebasing the local branch to update the patch, but I'll have very little time the coming days. In case replacing getClass().hashCode() by something like getClass().getName().hashCode() is preferred for easier testing, it would probably be better to add that in a companion method to sameClassAs(), for example classHash().
          Hide
          paul.elschot@xs4all.nl Paul Elschot added a comment -

          As to AssertionError or UOE, do we have a standard for that?
          I have seen UOE in a few places, so I used that.

          Show
          paul.elschot@xs4all.nl Paul Elschot added a comment - As to AssertionError or UOE, do we have a standard for that? I have seen UOE in a few places, so I used that.
          Hide
          dweiss Dawid Weiss added a comment -

          Hmm..., try it:

          git co 4bc3477fbf9be688c14bbb07f5982d5f4b615591 
          git apply LUCENE-7277-20160518.patch
          

          results in errors for me:

          error: patch failed: lucene/core/src/java/org/apache/lucene/search/BooleanQuery.java:421
          error: lucene/core/src/java/org/apache/lucene/search/BooleanQuery.java: patch does not apply
          error: patch failed: lucene/core/src/java/org/apache/lucene/search/MatchNoDocsQuery.java:25
          error: lucene/core/src/java/org/apache/lucene/search/MatchNoDocsQuery.java: patch does not apply
          error: patch failed: lucene/core/src/java/org/apache/lucene/search/PointInSetQuery.java:315
          error: lucene/core/src/java/org/apache/lucene/search/PointInSetQuery.java: patch does not apply
          error: patch failed: lucene/core/src/java/org/apache/lucene/search/PointRangeQuery.java:230
          error: lucene/core/src/java/org/apache/lucene/search/PointRangeQuery.java: patch does not apply
          error: patch failed: lucene/core/src/java/org/apache/lucene/search/Query.java:74
          error: lucene/core/src/java/org/apache/lucene/search/Query.java: patch does not apply
          error: patch failed: lucene/core/src/test/org/apache/lucene/search/JustCompileSearch.java:153
          error: lucene/core/src/test/org/apache/lucene/search/JustCompileSearch.java: patch does not apply
          error: patch failed: lucene/core/src/test/org/apache/lucene/search/spans/JustCompileSearchSpans.java:102
          error: lucene/core/src/test/org/apache/lucene/search/spans/JustCompileSearchSpans.java: patch does not apply
          error: patch failed: lucene/join/src/java/org/apache/lucene/search/join/PointInSetIncludingScoreQuery.java:299
          error: lucene/join/src/java/org/apache/lucene/search/join/PointInSetIncludingScoreQuery.java: patch does not apply
          error: patch failed: lucene/join/src/java/org/apache/lucene/search/join/ToParentBlockJoinQuery.java:475
          error: lucene/join/src/java/org/apache/lucene/search/join/ToParentBlockJoinQuery.java: patch does not apply
          error: patch failed: lucene/queryparser/src/java/org/apache/lucene/queryparser/complexPhrase/ComplexPhraseQueryParser.java:420
          error: lucene/queryparser/src/java/org/apache/lucene/queryparser/complexPhrase/ComplexPhraseQueryParser.java: patch does not apply
          error: patch failed: lucene/test-framework/src/java/org/apache/lucene/search/QueryUtils.java:67
          error: lucene/test-framework/src/java/org/apache/lucene/search/QueryUtils.java: patch does not apply
          error: patch failed: solr/core/src/java/org/apache/solr/search/QueryWrapperFilter.java:89
          error: solr/core/src/java/org/apache/solr/search/QueryWrapperFilter.java: patch does not apply
          

          In case replacing getClass().hashCode() by something like getClass().getName().hashCode() is preferred for easier testing, it would probably be better to add that in a companion method to sameClassAs(), for example classHash().

          Yep, I agree. Could even be lazily initialized once, much like the hash in Boolean query.

          As to AssertionError or UOE, do we have a standard for that? I have seen UOE in a few places, so I used that.

          I don't think there's a standard for such things – IllegalStateException, UOE, RuntimeException, InternalError... all these are used to signal unreachable code (or code that "theoretically" shouldn't execute). Not anything specific to Lucene, the inconsistency is everywhere, in the JDK too. I prefer new AssertionError("Reason") because this is an error (not an exception) and it cannot be confused with legitimate exceptions thrown by the java linker (unlike UOE)... but it's really my personal call, not a rule

          I would not mind rebasing the local branch to update the patch, but I'll have very little time the coming days.

          I can take over if you make that branch available somewhere (github?).

          Show
          dweiss Dawid Weiss added a comment - Hmm..., try it: git co 4bc3477fbf9be688c14bbb07f5982d5f4b615591 git apply LUCENE-7277-20160518.patch results in errors for me: error: patch failed: lucene/core/src/java/org/apache/lucene/search/BooleanQuery.java:421 error: lucene/core/src/java/org/apache/lucene/search/BooleanQuery.java: patch does not apply error: patch failed: lucene/core/src/java/org/apache/lucene/search/MatchNoDocsQuery.java:25 error: lucene/core/src/java/org/apache/lucene/search/MatchNoDocsQuery.java: patch does not apply error: patch failed: lucene/core/src/java/org/apache/lucene/search/PointInSetQuery.java:315 error: lucene/core/src/java/org/apache/lucene/search/PointInSetQuery.java: patch does not apply error: patch failed: lucene/core/src/java/org/apache/lucene/search/PointRangeQuery.java:230 error: lucene/core/src/java/org/apache/lucene/search/PointRangeQuery.java: patch does not apply error: patch failed: lucene/core/src/java/org/apache/lucene/search/Query.java:74 error: lucene/core/src/java/org/apache/lucene/search/Query.java: patch does not apply error: patch failed: lucene/core/src/test/org/apache/lucene/search/JustCompileSearch.java:153 error: lucene/core/src/test/org/apache/lucene/search/JustCompileSearch.java: patch does not apply error: patch failed: lucene/core/src/test/org/apache/lucene/search/spans/JustCompileSearchSpans.java:102 error: lucene/core/src/test/org/apache/lucene/search/spans/JustCompileSearchSpans.java: patch does not apply error: patch failed: lucene/join/src/java/org/apache/lucene/search/join/PointInSetIncludingScoreQuery.java:299 error: lucene/join/src/java/org/apache/lucene/search/join/PointInSetIncludingScoreQuery.java: patch does not apply error: patch failed: lucene/join/src/java/org/apache/lucene/search/join/ToParentBlockJoinQuery.java:475 error: lucene/join/src/java/org/apache/lucene/search/join/ToParentBlockJoinQuery.java: patch does not apply error: patch failed: lucene/queryparser/src/java/org/apache/lucene/queryparser/complexPhrase/ComplexPhraseQueryParser.java:420 error: lucene/queryparser/src/java/org/apache/lucene/queryparser/complexPhrase/ComplexPhraseQueryParser.java: patch does not apply error: patch failed: lucene/test-framework/src/java/org/apache/lucene/search/QueryUtils.java:67 error: lucene/test-framework/src/java/org/apache/lucene/search/QueryUtils.java: patch does not apply error: patch failed: solr/core/src/java/org/apache/solr/search/QueryWrapperFilter.java:89 error: solr/core/src/java/org/apache/solr/search/QueryWrapperFilter.java: patch does not apply In case replacing getClass().hashCode() by something like getClass().getName().hashCode() is preferred for easier testing, it would probably be better to add that in a companion method to sameClassAs(), for example classHash(). Yep, I agree. Could even be lazily initialized once, much like the hash in Boolean query. As to AssertionError or UOE, do we have a standard for that? I have seen UOE in a few places, so I used that. I don't think there's a standard for such things – IllegalStateException, UOE, RuntimeException, InternalError... all these are used to signal unreachable code (or code that "theoretically" shouldn't execute). Not anything specific to Lucene, the inconsistency is everywhere, in the JDK too. I prefer new AssertionError("Reason") because this is an error (not an exception) and it cannot be confused with legitimate exceptions thrown by the java linker (unlike UOE)... but it's really my personal call, not a rule I would not mind rebasing the local branch to update the patch, but I'll have very little time the coming days. I can take over if you make that branch available somewhere (github?).
          Hide
          paul.elschot@xs4all.nl Paul Elschot added a comment -

          I rebased locally to a more recent master. This went without conflicts, so I wonder why the patch did not apply cleanly.
          I then added one more commit with more use of sameClassAs in the lucene test code, so now ant compile-test passes for lucene.
          In this last commit I used AssertionError in few places.

          I did not change getClass().hashCode() into getClass().getName().hashCode().

          Then I pushed the local branch to github, with an added date in the branch name in order to avoid confusion with other rebases (just treat it as a tag):
          https://github.com/PaulElschot/lucene-solr/tree/lucene7277-20160519
          In case this git branch is not usable, please holler, I'd gladly post another patch here.

          Show
          paul.elschot@xs4all.nl Paul Elschot added a comment - I rebased locally to a more recent master. This went without conflicts, so I wonder why the patch did not apply cleanly. I then added one more commit with more use of sameClassAs in the lucene test code, so now ant compile-test passes for lucene. In this last commit I used AssertionError in few places. I did not change getClass().hashCode() into getClass().getName().hashCode(). Then I pushed the local branch to github, with an added date in the branch name in order to avoid confusion with other rebases (just treat it as a tag): https://github.com/PaulElschot/lucene-solr/tree/lucene7277-20160519 In case this git branch is not usable, please holler, I'd gladly post another patch here.
          Hide
          paul.elschot@xs4all.nl Paul Elschot added a comment -

          One thing to look out for: my editor deletes white space at all ends of lines (even otherwise unchanged lines), so when squashing the commits try and ignore white space, or at the end use a patch generated with git diff -w to ignore whitespace differences.

          This might also be the reason why the patch did not apply cleanly.

          Show
          paul.elschot@xs4all.nl Paul Elschot added a comment - One thing to look out for: my editor deletes white space at all ends of lines (even otherwise unchanged lines), so when squashing the commits try and ignore white space, or at the end use a patch generated with git diff -w to ignore whitespace differences. This might also be the reason why the patch did not apply cleanly.
          Hide
          dweiss Dawid Weiss added a comment -

          Whitespace wasn't an issue, but I didn't have time to dig deeper. github tree works much better – out of curiosity I made a diff from the branch to your master and re-applied it without problems, go figure.

          I'll take it from there, thanks Paul!

          Show
          dweiss Dawid Weiss added a comment - Whitespace wasn't an issue, but I didn't have time to dig deeper. github tree works much better – out of curiosity I made a diff from the branch to your master and re-applied it without problems, go figure. I'll take it from there, thanks Paul!
          Hide
          dweiss Dawid Weiss added a comment -

          I continue the work here:
          https://github.com/dweiss/lucene-solr/tree/work

          Some of the query subclasses (and filters) didn't do anything with regard to overriding equals/hashCode, which makes me wonder if they ever worked correctly with query caches enabled... I'll work on the remaining glitches through the weekend, run tests, etc.

          Show
          dweiss Dawid Weiss added a comment - I continue the work here: https://github.com/dweiss/lucene-solr/tree/work Some of the query subclasses (and filters) didn't do anything with regard to overriding equals/hashCode, which makes me wonder if they ever worked correctly with query caches enabled... I'll work on the remaining glitches through the weekend, run tests, etc.
          Hide
          dweiss Dawid Weiss added a comment -

          Updated patch. Tests pass.

          This is a rather large patch already, so I didn't want to make it any larger, leaving some hashCode implementation the way they were implemented (although some of them look rather odd).

          I think we should put this in as soon as possible, it really is a nice cleanup over various forms and shapes of the hashCode/equals implementation encountered in the code.

          As a separate issue, I'd ban the use of getClass().hashCode() entirely (it is still used in a number of places) as it changes from execution to execution and can cause problems with debugging.

          Show
          dweiss Dawid Weiss added a comment - Updated patch. Tests pass. This is a rather large patch already, so I didn't want to make it any larger, leaving some hashCode implementation the way they were implemented (although some of them look rather odd). I think we should put this in as soon as possible, it really is a nice cleanup over various forms and shapes of the hashCode/equals implementation encountered in the code. As a separate issue, I'd ban the use of getClass().hashCode() entirely (it is still used in a number of places) as it changes from execution to execution and can cause problems with debugging.
          Hide
          paul.elschot@xs4all.nl Paul Elschot added a comment -

          LGTM.

          In case the whitespace line changes cause conflicts later the workaround would be to ignore whitespace.

          The caching of the hash value in the Query class could also be left to places where the hash value is needed multiple times, for example in a query cache entry.

          Show
          paul.elschot@xs4all.nl Paul Elschot added a comment - LGTM. In case the whitespace line changes cause conflicts later the workaround would be to ignore whitespace. The caching of the hash value in the Query class could also be left to places where the hash value is needed multiple times, for example in a query cache entry.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit e44509f2dfc22f95f0a13372461d6db58b66611c in lucene-solr's branch refs/heads/master from Dawid Weiss
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=e44509f ]

          LUCENE-7277: Make Query.hashCode and Query.equals abstract.

          Show
          jira-bot ASF subversion and git services added a comment - Commit e44509f2dfc22f95f0a13372461d6db58b66611c in lucene-solr's branch refs/heads/master from Dawid Weiss [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=e44509f ] LUCENE-7277 : Make Query.hashCode and Query.equals abstract.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit d6264eb4756013fcfd9f6c2d9a224851c331767a in lucene-solr's branch refs/heads/branch_6x from Dawid Weiss
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=d6264eb ]

          LUCENE-7277: Make Query.hashCode and Query.equals abstract.

          Show
          jira-bot ASF subversion and git services added a comment - Commit d6264eb4756013fcfd9f6c2d9a224851c331767a in lucene-solr's branch refs/heads/branch_6x from Dawid Weiss [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=d6264eb ] LUCENE-7277 : Make Query.hashCode and Query.equals abstract.
          Hide
          dweiss Dawid Weiss added a comment -

          Pushed to master and branch_6x. Thanks for helping out, Paul!

          Show
          dweiss Dawid Weiss added a comment - Pushed to master and branch_6x. Thanks for helping out, Paul!

            People

            • Assignee:
              dweiss Dawid Weiss
              Reporter:
              dweiss Dawid Weiss
            • Votes:
              1 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development