Details

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

      Description

      The new DirectDocValuesFormat is nearly identical to Facet42DVF, except that it stores the addresses in direct int[] rather than PackedInts. On LUCENE-5296 we measured the performance of DirectDVF vs Facet42DVF and it improves perf for some queries and have negligible effect for others, as well as RAM consumption isn't much worse. We should remove Facet42DVF and use DirectDVF instead.

      I also want to rename Facet46Codec to FacetCodec. There's no need to refactor the class whenever the default codec changes (e.g. from 45 to 46) since it doesn't care about the actual Codec version underneath, it only overrides the DVF used for the facet fields. FacetCodec should take the DVF from the app (so e.g. the facet/ module doesn't depend on codecs/) and be exposed more as a utility Codec rather than a real, versioned, Codec.

        Activity

        Hide
        Shai Erera added a comment -

        I ended up removing everything under o.a.l.facet.codecs/, including Facet46Codec. It seemed redundant as all it does is use the app's DVF with the facet fields that are returned by FacetIndexingParams.getAllCategoryListParams(). It's a waste of time and resources to maintain such a Codec.

        I also removed some tests which tested Facet42DVF.

        Show
        Shai Erera added a comment - I ended up removing everything under o.a.l.facet.codecs/, including Facet46Codec. It seemed redundant as all it does is use the app's DVF with the facet fields that are returned by FacetIndexingParams.getAllCategoryListParams(). It's a waste of time and resources to maintain such a Codec. I also removed some tests which tested Facet42DVF.
        Hide
        Michael McCandless added a comment -

        +1, but maybe we can keep that test case if we just change it to an assumeTrue(_TestUtil.fieldSupportsHugeBinaryValues)?

        Show
        Michael McCandless added a comment - +1, but maybe we can keep that test case if we just change it to an assumeTrue(_TestUtil.fieldSupportsHugeBinaryValues)?
        Hide
        Michael McCandless added a comment -

        Also, maybe somewhere in javadocs we could show how the app could do what Facet46Codec is doing today? Ie, how to gather up all facet fields and then override getDocValuesFormatForField w/ the default codec?

        Show
        Michael McCandless added a comment - Also, maybe somewhere in javadocs we could show how the app could do what Facet46Codec is doing today? Ie, how to gather up all facet fields and then override getDocValuesFormatForField w/ the default codec?
        Hide
        Shai Erera added a comment -

        I'll add the test back with the assumeTrue. I'm not sure where to document this FacetCodec example ... it doesn't seem to belong in any of the classes' javadocs, and package.html aren't too verbose (point to blogs). So maybe we can just write a blog about it, though really, this isn't too complicated to figure out. I'll attach a patch shortly, want to make sure this test + assume really work!

        Show
        Shai Erera added a comment - I'll add the test back with the assumeTrue. I'm not sure where to document this FacetCodec example ... it doesn't seem to belong in any of the classes' javadocs, and package.html aren't too verbose (point to blogs). So maybe we can just write a blog about it, though really, this isn't too complicated to figure out. I'll attach a patch shortly, want to make sure this test + assume really work!
        Hide
        ASF subversion and git services added a comment -

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

        LUCENE-5321: remove Facet42DocValuesFormat and FacetCodec

        Show
        ASF subversion and git services added a comment - Commit 1538245 from Shai Erera in branch 'dev/trunk' [ https://svn.apache.org/r1538245 ] LUCENE-5321 : remove Facet42DocValuesFormat and FacetCodec
        Hide
        ASF subversion and git services added a comment -

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

        LUCENE-5321: remove Facet42DocValuesFormat and FacetCodec

        Show
        ASF subversion and git services added a comment - Commit 1538249 from Shai Erera in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1538249 ] LUCENE-5321 : remove Facet42DocValuesFormat and FacetCodec
        Hide
        Shai Erera added a comment -

        Committed to trunk and 4x. If it turns out FacetCodec is too tricky to write, we can either add it back under facet/ or maybe under demo/. For the moment, I think it's not that important to keep it and maintain it.

        Show
        Shai Erera added a comment - Committed to trunk and 4x. If it turns out FacetCodec is too tricky to write, we can either add it back under facet/ or maybe under demo/. For the moment, I think it's not that important to keep it and maintain it.

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development