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, Trunk
    • 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

        Adrien Grand created issue -
        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?
        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 -

        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.
        Shai Erera made changes -
        Field Original Value New Value
        Attachment LUCENE-4904.patch [ 12577811 ]
        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.
        Shai Erera made changes -
        Attachment LUCENE-4904.patch [ 12577843 ]
        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).
        Shai Erera made changes -
        Attachment LUCENE-4904.patch [ 12577846 ]
        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 -

        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?
        Shai Erera made changes -
        Attachment LUCENE-4904.patch [ 12577944 ]
        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 -

        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 -

        It is OK for me.

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

        Committed to trunk and 4x.

        Show
        Shai Erera added a comment - Committed to trunk and 4x.
        Shai Erera made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Lucene Fields New [ 10121 ] New,Patch Available [ 10121, 10120 ]
        Assignee Shai Erera [ shaie ]
        Fix Version/s 5.0 [ 12321663 ]
        Resolution Fixed [ 1 ]
        Hide
        Uwe Schindler added a comment -

        Closed after release.

        Show
        Uwe Schindler added a comment - Closed after release.
        Uwe Schindler made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Transition Time In Source Status Execution Times Last Executer Last Execution Date
        Open Open Resolved Resolved
        5d 18h 14m 1 Shai Erera 10/Apr/13 11:56
        Resolved Resolved Closed Closed
        29d 23h 37m 1 Uwe Schindler 10/May/13 11:33

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development