Lucene - Core
  1. Lucene - Core
  2. LUCENE-5155

Add OrdinalValueResolver in favor of FacetRequest.getValueOf

    Details

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

      Description

      FacetRequest.getValueOf is responsible for resolving an ordinal's value. It is given FacetArrays, and typically does something like arrays.getIntArray()[ord] – for every ordinal! The purpose of this method is to allow special requests, e.g. average, to do some post processing on the values, that couldn't be done during aggregation.

      I feel that getValueOf is in the wrong place – the calls to getInt/FloatArray are really redundant. Also, if an aggregator maintains some statistics by which it needs to "correct" the aggregated values, it's not trivial to pass it from the aggregator to the request.

      Therefore I would like to make the following changes:

      • Remove FacetRequest.getValueOf and .getFacetArraysSource
      • Add FacetsAggregator.createOrdinalValueResolver which takes the FacetArrays and has a simple API .valueOf(ordinal).
      • Modify the FacetResultHandlers to use OrdValResolver.

      This allows an OVR to initialize the right array instance(s) in the ctor, and return the value of the requested ordinal, without doing arrays.getArray() calls.

      Will post a patch shortly.

        Activity

        Hide
        Shai Erera added a comment -

        Patch addresses the proposed changes. All tests pass.

        Show
        Shai Erera added a comment - Patch addresses the proposed changes. All tests pass.
        Hide
        Gilad Barkai added a comment -

        Patch looks good.
        +1 for commit.

        Perhaps also document that FRNode is now comparable?

        Show
        Gilad Barkai added a comment - Patch looks good. +1 for commit. Perhaps also document that FRNode is now comparable?
        Hide
        ASF subversion and git services added a comment -

        Commit 1509152 from Shai Erera in branch 'dev/trunk'
        [ https://svn.apache.org/r1509152 ]

        LUCENE-5155: add OrdinalValueResolver

        Show
        ASF subversion and git services added a comment - Commit 1509152 from Shai Erera in branch 'dev/trunk' [ https://svn.apache.org/r1509152 ] LUCENE-5155 : add OrdinalValueResolver
        Hide
        ASF subversion and git services added a comment -

        Commit 1509154 from Shai Erera in branch 'dev/branches/branch_4x'
        [ https://svn.apache.org/r1509154 ]

        LUCENE-5155: add OrdinalValueResolver

        Show
        ASF subversion and git services added a comment - Commit 1509154 from Shai Erera in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1509154 ] LUCENE-5155 : add OrdinalValueResolver
        Hide
        Shai Erera added a comment -

        Thanks Gilad, added a comment and committed.

        Show
        Shai Erera added a comment - Thanks Gilad, added a comment and committed.
        Hide
        Adrien Grand added a comment -

        4.5 release -> bulk close

        Show
        Adrien Grand added a comment - 4.5 release -> bulk close

          People

          • Assignee:
            Shai Erera
            Reporter:
            Shai Erera
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development