Uploaded image for project: 'Solr'
  1. Solr
  2. SOLR-9176

Legacy Faceting Term Enum Method Regression

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 5.2, 5.2.1, 5.3, 5.3.1, 5.3.2, 5.4, 5.4.1, 5.5, 5.5.1, 6.0, 6.0.1
    • Fix Version/s: 5.5.2, 5.6, 6.0.2, 6.1
    • Component/s: faceting
    • Labels:
      None
    • Flags:
      Important

      Description

      Starting from this commit :

      LUCENE-5666: get solr started
      git-svn-id: https://svn.apache.org/repos/asf/lucene/dev/branches/lucene5666@1594254 13f79535-47bb-0310-9956-ffa450edef68

      https://github.com/apache/lucene-solr/commit/1489085807cb10981a7ea5b5663ada4e3f85953e#diff-5ac9dc7b128b4dd99b764060759222b2

      It is not possible to use Term Enum as a faceting method, for numeric and single valued fields ( org.apache.solr.request.SimpleFacets ) .

      We personally verified that there are use cases when this is bringing a quite big performance regression ( even with DocValues enabled) .
      In the mailing list from time to time people complain about a regression happening with the term enum method, but actually it is more likely to be the automatic forcing of FCS.

      Forcing FCS in co-op with the famous regression that happened in SOLR-8096 could be confused as a term Enum regression.

      1. SOLR-9176.patch
        10 kB
        Alessandro Benedetti
      2. SOLR-9176.patch
        1 kB
        Alessandro Benedetti

        Issue Links

          Activity

          Hide
          alessandro.benedetti Alessandro Benedetti added a comment - - edited

          The same commit seems to be involved Yonik Seeley

          Show
          alessandro.benedetti Alessandro Benedetti added a comment - - edited The same commit seems to be involved Yonik Seeley
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user alessandrobenedetti commented on the pull request:

          https://github.com/apache/lucene-solr/commit/1489085807cb10981a7ea5b5663ada4e3f85953e#commitcomment-17682489

          In solr/core/src/java/org/apache/solr/request/SimpleFacets.java:
          In solr/core/src/java/org/apache/solr/request/SimpleFacets.java on line 392:
          https://issues.apache.org/jira/browse/SOLR-9176

          Show
          githubbot ASF GitHub Bot added a comment - Github user alessandrobenedetti commented on the pull request: https://github.com/apache/lucene-solr/commit/1489085807cb10981a7ea5b5663ada4e3f85953e#commitcomment-17682489 In solr/core/src/java/org/apache/solr/request/SimpleFacets.java: In solr/core/src/java/org/apache/solr/request/SimpleFacets.java on line 392: https://issues.apache.org/jira/browse/SOLR-9176
          Hide
          alessandro.benedetti Alessandro Benedetti added a comment -

          Robert Muir I know it is an old commit, but do you have any idea about the reason of that change?

          org/apache/solr/request/SimpleFacets.java:431

          Show
          alessandro.benedetti Alessandro Benedetti added a comment - Robert Muir I know it is an old commit, but do you have any idea about the reason of that change? org/apache/solr/request/SimpleFacets.java:431
          Hide
          nzjess Jesse McLaughlin added a comment -

          Here's a representative, minimal patch that restores the old 4.x behaviour:

          https://github.com/nzjess/lucene-solr/commit/512298fc153738cc0d323dfc7aadbde241085d5b

          Haven't re-run the unit tests on this, but you get the idea...

          Show
          nzjess Jesse McLaughlin added a comment - Here's a representative, minimal patch that restores the old 4.x behaviour: https://github.com/nzjess/lucene-solr/commit/512298fc153738cc0d323dfc7aadbde241085d5b Haven't re-run the unit tests on this, but you get the idea...
          Hide
          alessandro.benedetti Alessandro Benedetti added a comment -

          First minimal patch :

          1) first modification is to bring back the possibility of explicit choice if you want to use Term Enum method

          2) second modification is to avoid a transparent switch to FC if you have docValues ( you can have enabled docValues to speed up sorting for example, this shouldn't prevent you to use the term Enum approach for faceting)

          Let's start the discussion !

          Show
          alessandro.benedetti Alessandro Benedetti added a comment - First minimal patch : 1) first modification is to bring back the possibility of explicit choice if you want to use Term Enum method 2) second modification is to avoid a transparent switch to FC if you have docValues ( you can have enabled docValues to speed up sorting for example, this shouldn't prevent you to use the term Enum approach for faceting) Let's start the discussion !
          Hide
          romseygeek Alan Woodward added a comment -

          Patch looks good, I'm running tests now.

          I think we could also add a test that runs faceting on a bunch of different field types with various methods added, and checks the debug output to see which method was actually used.

          Show
          romseygeek Alan Woodward added a comment - Patch looks good, I'm running tests now. I think we could also add a test that runs faceting on a bunch of different field types with various methods added, and checks the debug output to see which method was actually used.
          Hide
          romseygeek Alan Woodward added a comment -

          Hm, testRandomFaceting fails with this patch:

          ant test -Dtestcase=TestRandomDVFaceting -Dtests.method=testRandomFaceting -Dtests.seed=A22BD61DCB4A6569 -Dtests.slow=true -Dtests.locale=sv -Dtests.timezone=Indian/Antananarivo -Dtests.asserts=true -Dtests.file.encoding=UTF-8

          Show
          romseygeek Alan Woodward added a comment - Hm, testRandomFaceting fails with this patch: ant test -Dtestcase=TestRandomDVFaceting -Dtests.method=testRandomFaceting -Dtests.seed=A22BD61DCB4A6569 -Dtests.slow=true -Dtests.locale=sv -Dtests.timezone=Indian/Antananarivo -Dtests.asserts=true -Dtests.file.encoding=UTF-8
          Hide
          alessandro.benedetti Alessandro Benedetti added a comment -

          Hi Alan,
          you are right!
          As i was not in the office today I quickly just executed the SimpleFacetTest.
          Tomorrow I will verify the RandomFaceting tests and understand what's going on.
          I keep the Jira up to date.

          Cheers

          Show
          alessandro.benedetti Alessandro Benedetti added a comment - Hi Alan, you are right! As i was not in the office today I quickly just executed the SimpleFacetTest. Tomorrow I will verify the RandomFaceting tests and understand what's going on. I keep the Jira up to date. Cheers
          Hide
          alessandro.benedetti Alessandro Benedetti added a comment -
          • Select Facet method refactored to be more readable according Jesse McLaughlin suggestion
          • added to the facet debug both the requested facet method and the applied facet metod according Jesse McLaughlin suggestion ( it is true the requested facet method is already in the requestParam, but in this way it will be easier to spot undesired situations
          • found some additional bug that at the moment are covered by workarounds ( we need to create appropriate Jira issues for them) :

          /* FC without docValues does not support single valued numeric facets */
          if (method == FacetMethod.FC
          && type.getNumericType() != null && !field.multiValued())

          { method = FacetMethod.FCS; }

          /* UIF without DocValues can't deal with mincount=0, the reason is because
          we create the buckets based on the values present in the result set.
          So we are not going to see facet values which are not in the result set */
          if (method == FacetMethod.UIF
          && !field.hasDocValues() && mincount == 0)

          { method = field.multiValued() ? FacetMethod.FC : FacetMethod.FCS; }
          • added a test to cover the uif bug
          • run all the tests in request and the Random DV ones and they are succeeding
          • it is required a full test execution on core . We don't have time this week end, if necessary we proceed next week only to double check
          Show
          alessandro.benedetti Alessandro Benedetti added a comment - Select Facet method refactored to be more readable according Jesse McLaughlin suggestion added to the facet debug both the requested facet method and the applied facet metod according Jesse McLaughlin suggestion ( it is true the requested facet method is already in the requestParam, but in this way it will be easier to spot undesired situations found some additional bug that at the moment are covered by workarounds ( we need to create appropriate Jira issues for them) : /* FC without docValues does not support single valued numeric facets */ if (method == FacetMethod.FC && type.getNumericType() != null && !field.multiValued()) { method = FacetMethod.FCS; } /* UIF without DocValues can't deal with mincount=0, the reason is because we create the buckets based on the values present in the result set. So we are not going to see facet values which are not in the result set */ if (method == FacetMethod.UIF && !field.hasDocValues() && mincount == 0) { method = field.multiValued() ? FacetMethod.FC : FacetMethod.FCS; } added a test to cover the uif bug run all the tests in request and the Random DV ones and they are succeeding it is required a full test execution on core . We don't have time this week end, if necessary we proceed next week only to double check
          Hide
          tomasflobbe Tomás Fernández Löbbe added a comment -

          Patch looks good to me and I like the refactor of selectFacetMethod. One suggestion, maybe you can make it package private and do unit tests for it?

          Show
          tomasflobbe Tomás Fernández Löbbe added a comment - Patch looks good to me and I like the refactor of selectFacetMethod. One suggestion, maybe you can make it package private and do unit tests for it?
          Hide
          romseygeek Alan Woodward added a comment -

          Echoing Tomás, can you add some specific tests for selectFacetMethod()? I think this is ready to commit other than that.

          Show
          romseygeek Alan Woodward added a comment - Echoing Tomás, can you add some specific tests for selectFacetMethod()? I think this is ready to commit other than that.
          Hide
          jpountz Adrien Grand added a comment -

          This looks to me like something that would be nice to fix for 6.1?

          Show
          jpountz Adrien Grand added a comment - This looks to me like something that would be nice to fix for 6.1?
          Hide
          alessandro.benedetti Alessandro Benedetti added a comment -

          i agree for the unit test ( even if some clause in the selectFacet method are workaround to avoid bugs or incompatibility in some legacy facet method, and they don't deserve a test but actually a bug) .
          Unfortunately I will be away until monday, I can try to contribute the tests next week. If you want to contribute them before, feel free to do that!
          Cheers

          Show
          alessandro.benedetti Alessandro Benedetti added a comment - i agree for the unit test ( even if some clause in the selectFacet method are workaround to avoid bugs or incompatibility in some legacy facet method, and they don't deserve a test but actually a bug) . Unfortunately I will be away until monday, I can try to contribute the tests next week. If you want to contribute them before, feel free to do that! Cheers
          Hide
          romseygeek Alan Woodward added a comment -

          This looks to me like something that would be nice to fix for 6.1?

          It would! I'll write some tests and get this committed tomorrow.

          Show
          romseygeek Alan Woodward added a comment - This looks to me like something that would be nice to fix for 6.1? It would! I'll write some tests and get this committed tomorrow.
          Hide
          jpountz Adrien Grand added a comment -

          Thanks Alan.

          Show
          jpountz Adrien Grand added a comment - Thanks Alan.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 0eacfe87c28bb8725ee70626eab762fe596ff79e in lucene-solr's branch refs/heads/master from Alan Woodward
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=0eacfe8 ]

          SOLR-9176: Fix facet method fallback selection

          Show
          jira-bot ASF subversion and git services added a comment - Commit 0eacfe87c28bb8725ee70626eab762fe596ff79e in lucene-solr's branch refs/heads/master from Alan Woodward [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=0eacfe8 ] SOLR-9176 : Fix facet method fallback selection
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 4a22b0ff82fe3a891a3b5137369b68afd5e48e98 in lucene-solr's branch refs/heads/branch_6x from Alan Woodward
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=4a22b0f ]

          SOLR-9176: Fix facet method fallback selection

          Show
          jira-bot ASF subversion and git services added a comment - Commit 4a22b0ff82fe3a891a3b5137369b68afd5e48e98 in lucene-solr's branch refs/heads/branch_6x from Alan Woodward [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=4a22b0f ] SOLR-9176 : Fix facet method fallback selection
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit f77afe120590c2fde866e773ca296b8c0ad8a9a5 in lucene-solr's branch refs/heads/branch_6_1 from Alan Woodward
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=f77afe1 ]

          SOLR-9176: Fix facet method fallback selection

          Show
          jira-bot ASF subversion and git services added a comment - Commit f77afe120590c2fde866e773ca296b8c0ad8a9a5 in lucene-solr's branch refs/heads/branch_6_1 from Alan Woodward [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=f77afe1 ] SOLR-9176 : Fix facet method fallback selection
          Hide
          romseygeek Alan Woodward added a comment -

          Thanks all!

          Show
          romseygeek Alan Woodward added a comment - Thanks all!
          Hide
          alessandro.benedetti Alessandro Benedetti added a comment -

          Thanks Alan for the test cases !
          Good to know is going to be in 6.1

          Show
          alessandro.benedetti Alessandro Benedetti added a comment - Thanks Alan for the test cases ! Good to know is going to be in 6.1
          Hide
          steve_rowe Steve Rowe added a comment -

          Reopening to backport to 6.0.2, 5.6 and 5.5.2.

          Show
          steve_rowe Steve Rowe added a comment - Reopening to backport to 6.0.2, 5.6 and 5.5.2.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 3ad19fecee2f4b0280208787e29e2b73db2dbe49 in lucene-solr's branch refs/heads/branch_5_5 from Alan Woodward
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=3ad19fe ]

          SOLR-9176: Fix facet method fallback selection

          Show
          jira-bot ASF subversion and git services added a comment - Commit 3ad19fecee2f4b0280208787e29e2b73db2dbe49 in lucene-solr's branch refs/heads/branch_5_5 from Alan Woodward [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=3ad19fe ] SOLR-9176 : Fix facet method fallback selection
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 3344d21dc812d1890c935c800e0715355f993975 in lucene-solr's branch refs/heads/branch_5x from Alan Woodward
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=3344d21 ]

          SOLR-9176: Fix facet method fallback selection

          Show
          jira-bot ASF subversion and git services added a comment - Commit 3344d21dc812d1890c935c800e0715355f993975 in lucene-solr's branch refs/heads/branch_5x from Alan Woodward [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=3344d21 ] SOLR-9176 : Fix facet method fallback selection
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit eb28958e1275bef7b5dd8c1a8b7268bc29dc5663 in lucene-solr's branch refs/heads/branch_6_0 from Alan Woodward
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=eb28958 ]

          SOLR-9176: Fix facet method fallback selection

          Show
          jira-bot ASF subversion and git services added a comment - Commit eb28958e1275bef7b5dd8c1a8b7268bc29dc5663 in lucene-solr's branch refs/heads/branch_6_0 from Alan Woodward [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=eb28958 ] SOLR-9176 : Fix facet method fallback selection
          Hide
          steve_rowe Steve Rowe added a comment -

          Bulk close issues released with 5.5.2.

          Show
          steve_rowe Steve Rowe added a comment - Bulk close issues released with 5.5.2.

            People

            • Assignee:
              romseygeek Alan Woodward
              Reporter:
              alessandro.benedetti Alessandro Benedetti
            • Votes:
              0 Vote for this issue
              Watchers:
              16 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development