Lucene - Core
  1. Lucene - Core
  2. LUCENE-6372

Simplify hashCode/equals for SpanQuery subclasses

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      Spin off from LUCENE-6308, see the comments there from around 23 March 2015.

      1. LUCENE-6372.patch
        17 kB
        Paul Elschot
      2. LUCENE-6372.patch
        15 kB
        Paul Elschot

        Activity

        Hide
        Paul Elschot added a comment -

        Patch of 4 May 2015.
        Simplifies hashCode/equals for all subclasses of SpanQuery.
        Removes this == other checks in equals(), this might affect performance.
        Adds a few Objects.requireNonNull calls in constructors.
        Leaves various getBoost calls in hashCode implementations to super.
        Removes hashCode/equals from SpanFirstQuery, not needed anymore.
        Uses new collectPayloads attribute in SpanNearQuery hashCode/equals.

        Show
        Paul Elschot added a comment - Patch of 4 May 2015. Simplifies hashCode/equals for all subclasses of SpanQuery. Removes this == other checks in equals(), this might affect performance. Adds a few Objects.requireNonNull calls in constructors. Leaves various getBoost calls in hashCode implementations to super. Removes hashCode/equals from SpanFirstQuery, not needed anymore. Uses new collectPayloads attribute in SpanNearQuery hashCode/equals.
        Hide
        Paul Elschot added a comment -

        See also LUCENE-6333

        Show
        Paul Elschot added a comment - See also LUCENE-6333
        Hide
        Adrien Grand added a comment -

        I really like the additional checks and how hashcode/equals properly call super.hashcode and only care about members of the class – not of its parent.

        I think my only concern would be that some bit mixes that these queries have in their hashcode do not have the intended effect now that you changed the order in which members are incorporated into the hash code (referring to this comment in particular: Mix bits before folding in things like boost). It would probably be safer to move to a multiplicative hash.

        Show
        Adrien Grand added a comment - I really like the additional checks and how hashcode/equals properly call super.hashcode and only care about members of the class – not of its parent. I think my only concern would be that some bit mixes that these queries have in their hashcode do not have the intended effect now that you changed the order in which members are incorporated into the hash code (referring to this comment in particular: Mix bits before folding in things like boost ). It would probably be safer to move to a multiplicative hash.
        Hide
        Paul Elschot added a comment -

        I'll remove the comment and use a multiplicative hash instead of rotations.

        Query has a this == other test in equals().
        When this passes in the Query superclass, the subclass still has to check its members.
        Wouldn't it be better to do that test only in non abstract classes as the first thing to be checked, if at all?
        This is actually another issue, but anyway.

        Show
        Paul Elschot added a comment - I'll remove the comment and use a multiplicative hash instead of rotations. Query has a this == other test in equals(). When this passes in the Query superclass, the subclass still has to check its members. Wouldn't it be better to do that test only in non abstract classes as the first thing to be checked, if at all? This is actually another issue, but anyway.
        Hide
        Adrien Grand added a comment -

        Query has a this == other test in equals().

        I'm not sure how much it helps and I wouldn't mind seeing it removed, but to answer your question I think it still kicks in for queries that do not need to override equals/hashcode like MatchAllDocsQuery?

        Show
        Adrien Grand added a comment - Query has a this == other test in equals(). I'm not sure how much it helps and I wouldn't mind seeing it removed, but to answer your question I think it still kicks in for queries that do not need to override equals/hashcode like MatchAllDocsQuery?
        Hide
        Paul Elschot added a comment -

        Patch of 6 May 2015.
        Compared to previous patch this changes hash code rotations to multiplicative hashes, and this includes an implementation for SpanPayloadCheckQuery.

        Show
        Paul Elschot added a comment - Patch of 6 May 2015. Compared to previous patch this changes hash code rotations to multiplicative hashes, and this includes an implementation for SpanPayloadCheckQuery.
        Hide
        Paul Elschot added a comment -

        One typical case for equals() is a newly constructed query compared against a query as a key in a cache.
        In that case the this == other test in Query.equals() will fail, so it can just as well be removed, and never done.

        Are there other typical cases for Query.equals()?
        For example one could cache the result of a query parser by its input string, and reuse a query from that cache to check a cache with query results. In that case the this == other test might help in equals() for non abstract Query subclasses.

        Show
        Paul Elschot added a comment - One typical case for equals() is a newly constructed query compared against a query as a key in a cache. In that case the this == other test in Query.equals() will fail, so it can just as well be removed, and never done. Are there other typical cases for Query.equals()? For example one could cache the result of a query parser by its input string, and reuse a query from that cache to check a cache with query results. In that case the this == other test might help in equals() for non abstract Query subclasses.
        Hide
        Adrien Grand added a comment -

        Indeed, I don't think these == checks are useful for anything but corner cases, especially given that query rewriting often involves cloning.

        Show
        Adrien Grand added a comment - Indeed, I don't think these == checks are useful for anything but corner cases, especially given that query rewriting often involves cloning.
        Hide
        Adrien Grand added a comment -

        The patch looks good to me! I'll commit shortly.

        Show
        Adrien Grand added a comment - The patch looks good to me! I'll commit shortly.
        Hide
        ASF subversion and git services added a comment -

        Commit 1677963 from Adrien Grand in branch 'dev/trunk'
        [ https://svn.apache.org/r1677963 ]

        LUCENE-6372: Simplified and improved equals/hashcode of span queries.

        Show
        ASF subversion and git services added a comment - Commit 1677963 from Adrien Grand in branch 'dev/trunk' [ https://svn.apache.org/r1677963 ] LUCENE-6372 : Simplified and improved equals/hashcode of span queries.
        Hide
        ASF subversion and git services added a comment -

        Commit 1678104 from Adrien Grand in branch 'dev/branches/branch_5x'
        [ https://svn.apache.org/r1678104 ]

        LUCENE-6372: Simplified and improved equals/hashcode of span queries.

        Show
        ASF subversion and git services added a comment - Commit 1678104 from Adrien Grand in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1678104 ] LUCENE-6372 : Simplified and improved equals/hashcode of span queries.

          People

          • Assignee:
            Adrien Grand
            Reporter:
            Paul Elschot
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development