Lucene - Core
  1. Lucene - Core
  2. LUCENE-4904

Sorter API: Make NumericDocValuesSorter able to sort in reverse order

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Trivial Trivial
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.3, 5.0
    • Component/s: None
    • Labels:
    • Lucene Fields:
      New, Patch Available

      Description

      Today it is only able to sort in ascending order.

      1. LUCENE-4904.patch
        6 kB
        Shai Erera
      2. LUCENE-4904.patch
        2 kB
        Shai Erera
      3. LUCENE-4904.patch
        2 kB
        Shai Erera
      4. LUCENE-4904.patch
        2 kB
        Shai Erera

        Activity

        Hide
        Uwe Schindler added a comment -

        Closed after release.

        Show
        Uwe Schindler added a comment - Closed after release.
        Hide
        Shai Erera added a comment -

        Committed to trunk and 4x.

        Show
        Shai Erera added a comment - Committed to trunk and 4x.
        Hide
        Adrien Grand added a comment -

        It is OK for me.

        Show
        Adrien Grand added a comment - It is OK for me.
        Hide
        Shai Erera added a comment -

        I am also wondering if I should keep ReverseOrderSorter, because implementing reverse in numeric is so trivial, that ReverseOrderSorter becomes just another sorter to maintain... I think I'll remove it

        Show
        Shai Erera added a comment - I am also wondering if I should keep ReverseOrderSorter, because implementing reverse in numeric is so trivial, that ReverseOrderSorter becomes just another sorter to maintain... I think I'll remove it
        Hide
        Adrien Grand added a comment -

        This got me thinking if ascending/descending should be on the Sorter.sort API

        I think it shouldn't for the reasons you mentioned.

        The patch looks good to me, +1 to commit!

        Show
        Adrien Grand added a comment - This got me thinking if ascending/descending should be on the Sorter.sort API I think it shouldn't for the reasons you mentioned. The patch looks good to me, +1 to commit!
        Hide
        Shai Erera added a comment -

        Fair enough. It was quite easy to modify NumericDVSorter. Patch adds another ctor which takes a boolean ascending, and integrates it with the test.

        This got me thinking if ascending/descending should be on the Sorter.sort API, but I think it shouldn't because someone can dangerously sort some segments in ascending and others in descending order. Better, I think, if it's a consistent decision for all segments. What do you think?

        Show
        Shai Erera added a comment - Fair enough. It was quite easy to modify NumericDVSorter. Patch adds another ctor which takes a boolean ascending , and integrates it with the test. This got me thinking if ascending/descending should be on the Sorter.sort API, but I think it shouldn't because someone can dangerously sort some segments in ascending and others in descending order. Better, I think, if it's a consistent decision for all segments. What do you think?
        Hide
        Adrien Grand added a comment -

        We can add this ReverseOrderSorter, but as far as NumericDocValuesSorter is concerned, I would rather have the abstraction at the level of the DocComparator rather than the Sorter. This would allow Sorter.sort(int,DocComparator) to quickly return null without allocating (potentially lots of) memory for the doc maps if the reader is already sorted. Additionally, this would allow for more readable diagnostics (such as "DocValues(fieldName,desc)" instead of "Reverse(DocValues(fieldName,asc))".

        Show
        Adrien Grand added a comment - We can add this ReverseOrderSorter, but as far as NumericDocValuesSorter is concerned, I would rather have the abstraction at the level of the DocComparator rather than the Sorter. This would allow Sorter.sort(int,DocComparator) to quickly return null without allocating (potentially lots of) memory for the doc maps if the reader is already sorted. Additionally, this would allow for more readable diagnostics (such as "DocValues(fieldName,desc)" instead of "Reverse(DocValues(fieldName,asc))".
        Hide
        Shai Erera added a comment -

        Patch on latest trunk (previous one had issues applying).

        Show
        Shai Erera added a comment - Patch on latest trunk (previous one had issues applying).
        Hide
        Shai Erera added a comment -

        Added ReverseOrderSorter to IndexSortingTest (was after all very easy), which uncovered a bug in my original implementation. It's now working and tests are happy.

        I basically think this is ready, would appreciate some review.

        Show
        Shai Erera added a comment - Added ReverseOrderSorter to IndexSortingTest (was after all very easy), which uncovered a bug in my original implementation. It's now working and tests are happy. I basically think this is ready, would appreciate some review.
        Hide
        Shai Erera added a comment -

        I hacked this up real quickly, so I could be missing something. Patch adds a ReverseOrderSorter which wraps a Sorter and on sort() returns a DocMap that reverses whatever the wrapped Sorter DocMap returned.

        I still didn't figure out how to plug that sorter with existing tests, so it could be this approach doesn't work. Will look at it later.

        Show
        Shai Erera added a comment - I hacked this up real quickly, so I could be missing something. Patch adds a ReverseOrderSorter which wraps a Sorter and on sort() returns a DocMap that reverses whatever the wrapped Sorter DocMap returned. I still didn't figure out how to plug that sorter with existing tests, so it could be this approach doesn't work. Will look at it later.
        Hide
        Adrien Grand added a comment -

        That's another option. I have no strong feeling towards any of them.

        Show
        Adrien Grand added a comment - That's another option. I have no strong feeling towards any of them.
        Hide
        Shai Erera added a comment -

        Maybe instead of fixing NumericDVSorter, we can have a ReverseSortSorter (must have a better name!) which wraps any Sorter and reverses the DocMap?

        Show
        Shai Erera added a comment - Maybe instead of fixing NumericDVSorter, we can have a ReverseSortSorter (must have a better name!) which wraps any Sorter and reverses the DocMap?

          People

          • Assignee:
            Shai Erera
            Reporter:
            Adrien Grand
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development