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

Add getters to PointInSetQuery and PointRangeQuery classes

    Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Trivial
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None
    • Lucene Fields:
      New
    1. LUCENE_7307.patch
      5 kB
      Martijn van Groningen
    2. LUCENE_7307.patch
      5 kB
      Martijn van Groningen
    3. LUCENE_7307.patch
      3 kB
      Martijn van Groningen
    4. LUCENE_7307.patch
      2 kB
      Martijn van Groningen
    5. LUCENE-7307
      6 kB
      Martijn van Groningen

      Activity

      Hide
      mikemccand Michael McCandless added a comment -

      +1

      Show
      mikemccand Michael McCandless added a comment - +1
      Hide
      jpountz Adrien Grand added a comment -

      Should we be defensive and copy the low/high points before returning them? Also I don't think sortedPackedPointsHashCode should have a getter, and maybe sortedPackedPoints should be returned as a Collection<byte[]> rather exposing PrefixCodedTerms (which should remain internal to Lucene).

      Show
      jpountz Adrien Grand added a comment - Should we be defensive and copy the low/high points before returning them? Also I don't think sortedPackedPointsHashCode should have a getter, and maybe sortedPackedPoints should be returned as a Collection<byte[]> rather exposing PrefixCodedTerms (which should remain internal to Lucene).
      Hide
      martijn.v.groningen Martijn van Groningen added a comment -

      Also I don't think sortedPackedPointsHashCode should have a getter,

      Oops, totally agree.

      I updated the patch. The low/high points are cloned the sortedPackedPoints are exposed as Collection<byte[]>.

      Show
      martijn.v.groningen Martijn van Groningen added a comment - Also I don't think sortedPackedPointsHashCode should have a getter, Oops, totally agree. I updated the patch. The low/high points are cloned the sortedPackedPoints are exposed as Collection<byte[]>.
      Hide
      jpountz Adrien Grand added a comment -

      Maybe the collection should be a view rather than a copy? For instance this would allow knowing how many points there are in canstant time (by calling size()) and without duplicating all points in memory?

      Show
      jpountz Adrien Grand added a comment - Maybe the collection should be a view rather than a copy? For instance this would allow knowing how many points there are in canstant time (by calling size()) and without duplicating all points in memory?
      Hide
      martijn.v.groningen Martijn van Groningen added a comment -

      I've updated the patch and the returned collection is now a view.
      Also I changed the return type from Collection<byte[]> to Collection<BytesRef> because the sortedPackedPoints iterator already makes a copy and returns that as BytesRef.

      Show
      martijn.v.groningen Martijn van Groningen added a comment - I've updated the patch and the returned collection is now a view. Also I changed the return type from Collection<byte[]> to Collection<BytesRef> because the sortedPackedPoints iterator already makes a copy and returns that as BytesRef.
      Hide
      martijn.v.groningen Martijn van Groningen added a comment -

      Forgot to add an assertion in the test.

      Show
      martijn.v.groningen Martijn van Groningen added a comment - Forgot to add an assertion in the test.
      Hide
      jpountz Adrien Grand added a comment -

      Also I changed the return type from Collection<byte[]> to Collection<BytesRef> because the sortedPackedPoints iterator already makes a copy and returns that as BytesRef.

      I think the byte[] version was better since it is consitent with PointPangeQuery which exposes the low/high bounds as a byte[]? Also I don't think it is true that the sortedPackedPoints iterator makes a copy: looking at PrefixCodedTerms, it seems to be reusing the same BytesRef object?

      Something else we should do is throwing a NoSuchElementException in the Iterator whin upTo==size to comply with the Iterator API.

      Show
      jpountz Adrien Grand added a comment - Also I changed the return type from Collection<byte[]> to Collection<BytesRef> because the sortedPackedPoints iterator already makes a copy and returns that as BytesRef. I think the byte[] version was better since it is consitent with PointPangeQuery which exposes the low/high bounds as a byte[]? Also I don't think it is true that the sortedPackedPoints iterator makes a copy: looking at PrefixCodedTerms, it seems to be reusing the same BytesRef object? Something else we should do is throwing a NoSuchElementException in the Iterator whin upTo==size to comply with the Iterator API.
      Hide
      martijn.v.groningen Martijn van Groningen added a comment -

      I think the byte[] version was better since it is consitent with PointPangeQuery which exposes the low/high bounds as a byte[]?

      Good point. I'll change that.

      Also I don't think it is true that the sortedPackedPoints iterator makes a copy: looking at PrefixCodedTerms, it seems to be reusing the same BytesRef object?

      True... we need to make a copy. I guess what I was confused with is the copy that PrefixCodedTerms makes from the input, but if it is reusing the BytesRef it copies into then a copy is required if we return the points in a collection.

      Something else we should do is throwing a NoSuchElementException in the Iterator whin upTo==size to comply with the Iterator API.

      +1!

      Show
      martijn.v.groningen Martijn van Groningen added a comment - I think the byte[] version was better since it is consitent with PointPangeQuery which exposes the low/high bounds as a byte[]? Good point. I'll change that. Also I don't think it is true that the sortedPackedPoints iterator makes a copy: looking at PrefixCodedTerms, it seems to be reusing the same BytesRef object? True... we need to make a copy. I guess what I was confused with is the copy that PrefixCodedTerms makes from the input, but if it is reusing the BytesRef it copies into then a copy is required if we return the points in a collection. Something else we should do is throwing a NoSuchElementException in the Iterator whin upTo==size to comply with the Iterator API. +1!
      Hide
      martijn.v.groningen Martijn van Groningen added a comment -

      I've updated the patch.

      Show
      martijn.v.groningen Martijn van Groningen added a comment - I've updated the patch.
      Hide
      jpountz Adrien Grand added a comment -

      Thanks Martijn, +1

      Show
      jpountz Adrien Grand added a comment - Thanks Martijn, +1
      Hide
      jira-bot ASF subversion and git services added a comment -

      Commit 50feae82af5d297b720a98db07ab43e093e788b8 in lucene-solr's branch refs/heads/branch_6x from Martijn van Groningen
      [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=50feae8 ]

      LUCENE-7307: Add getters to the PointInSetQuery and PointRangeQuery queries.

      Show
      jira-bot ASF subversion and git services added a comment - Commit 50feae82af5d297b720a98db07ab43e093e788b8 in lucene-solr's branch refs/heads/branch_6x from Martijn van Groningen [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=50feae8 ] LUCENE-7307 : Add getters to the PointInSetQuery and PointRangeQuery queries.
      Hide
      jira-bot ASF subversion and git services added a comment -

      Commit b977275185a111fad55da2bd993c1afb73a00b51 in lucene-solr's branch refs/heads/master from Martijn van Groningen
      [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=b977275 ]

      LUCENE-7307: Add getters to the PointInSetQuery and PointRangeQuery queries.

      Show
      jira-bot ASF subversion and git services added a comment - Commit b977275185a111fad55da2bd993c1afb73a00b51 in lucene-solr's branch refs/heads/master from Martijn van Groningen [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=b977275 ] LUCENE-7307 : Add getters to the PointInSetQuery and PointRangeQuery queries.
      Hide
      mikemccand Michael McCandless added a comment -

      Martijn van Groningen can this be closed now?

      Show
      mikemccand Michael McCandless added a comment - Martijn van Groningen can this be closed now?
      Hide
      martijn.v.groningen Martijn van Groningen added a comment -

      Michael McCandless yes! Thanks for reminding me.

      Show
      martijn.v.groningen Martijn van Groningen added a comment - Michael McCandless yes! Thanks for reminding me.

        People

        • Assignee:
          Unassigned
          Reporter:
          martijn.v.groningen Martijn van Groningen
        • Votes:
          0 Vote for this issue
          Watchers:
          4 Start watching this issue

          Dates

          • Created:
            Updated:
            Resolved:

            Development