Solr
  1. Solr
  2. SOLR-781

Change facet.sort from boolean to string and specify sort method explicitely

    Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.3
    • Fix Version/s: 1.4
    • Component/s: None
    • Labels:
      None

      Description

      The facet.sort parameter is currently a boolean, which has some downsides as explained in http://www.nabble.com/facet.sort-parameter-td19570834.html

      Changing it to a string which explicitely states the sort method would address these issues.

      1. SOLR-781.patch
        2 kB
        Lars Kotthoff
      2. SOLR-781.patch
        46 kB
        Lars Kotthoff
      3. SOLR-781.patch
        45 kB
        Lars Kotthoff
      4. SOLR-781.patch
        22 kB
        Lars Kotthoff

        Activity

        Hide
        Lars Kotthoff added a comment -

        Attaching patch which makes the necessary changes.

        This patch does not break backwards compatibility; the "strings" "true" and "false" passed in URLs are handled as before and SolrJ adds methods returning/taking strings instead of booleans while keeping (and deprecating) the old ones.

        Show
        Lars Kotthoff added a comment - Attaching patch which makes the necessary changes. This patch does not break backwards compatibility; the "strings" "true" and "false" passed in URLs are handled as before and SolrJ adds methods returning/taking strings instead of booleans while keeping (and deprecating) the old ones.
        Hide
        Hoss Man added a comment -

        Note the comment i made in the thread after Lars opend this issue ... we need to make sure there's at least one test verifying that <bool name="facet.sort">true</bool> still works if people are using it as a default/appends/invariant in their configs.

        also: a quick skim a the patch suggests all of the old tests were changed to use "count" and "lex" ... there should probably still be at least one or two using "true" and "false" to prove that the back-compat works.

        lastly: an enum would probably make sense here rather then a lot of `sort.equals("count") || sort.equals("true")}` sprinkled throughout the code.

        Show
        Hoss Man added a comment - Note the comment i made in the thread after Lars opend this issue ... we need to make sure there's at least one test verifying that <bool name="facet.sort">true</bool> still works if people are using it as a default/appends/invariant in their configs. also: a quick skim a the patch suggests all of the old tests were changed to use "count" and "lex" ... there should probably still be at least one or two using "true" and "false" to prove that the back-compat works. lastly: an enum would probably make sense here rather then a lot of `sort.equals("count") || sort.equals("true")}` sprinkled throughout the code.
        Hide
        Lars Kotthoff added a comment -

        Attaching new patch which addresses the issues raised by Hoss Man.

        • added legacy tests for facet.sort=true|false parameters and <boolean name="facet.sort"> configuration directives
        • added deprecation warning when <boolean name="facet.sort"> is found in solrconfig
        • replaced hardcoded "count", "lex", "true", and "false" with constants in FacetParams
        Show
        Lars Kotthoff added a comment - Attaching new patch which addresses the issues raised by Hoss Man. added legacy tests for facet.sort=true|false parameters and <boolean name="facet.sort"> configuration directives added deprecation warning when <boolean name="facet.sort"> is found in solrconfig replaced hardcoded "count", "lex", "true", and "false" with constants in FacetParams
        Hide
        Lars Kotthoff added a comment -

        Syncing patch with trunk

        Show
        Lars Kotthoff added a comment - Syncing patch with trunk
        Hide
        Yonik Seeley added a comment -

        Thank you for your persistence Lars, I'll try and take a look at this soon.

        Show
        Yonik Seeley added a comment - Thank you for your persistence Lars, I'll try and take a look at this soon.
        Hide
        Yonik Seeley added a comment -

        Committed - thanks Lars!

        Show
        Yonik Seeley added a comment - Committed - thanks Lars!
        Hide
        Yonik Seeley added a comment -

        I've also turned off over-requesting (increasing face.limit on shards) when facet.sort=lex

        Show
        Yonik Seeley added a comment - I've also turned off over-requesting (increasing face.limit on shards) when facet.sort=lex
        Hide
        Lars Kotthoff added a comment -

        Thanks Yonik! On the topic of issues which have been around for a long time... do you have some time to take a look at SOLR-540 as well

        Show
        Lars Kotthoff added a comment - Thanks Yonik! On the topic of issues which have been around for a long time... do you have some time to take a look at SOLR-540 as well
        Hide
        Lars Kotthoff added a comment -

        I've taken a look at the non-overrequesting stuff – we also need to consider the legacy case (i.e. true/false instead of sort/lex). Attaching patch which addresses this and replaces some string constants which escaped me before with the constants from FacetParams.

        Show
        Lars Kotthoff added a comment - I've taken a look at the non-overrequesting stuff – we also need to consider the legacy case (i.e. true/false instead of sort/lex). Attaching patch which addresses this and replaces some string constants which escaped me before with the constants from FacetParams.
        Hide
        Yonik Seeley added a comment -

        Lars, I already committed some code to normalize on the new facet.sort values so we don't need to check for both new and legacy everywhere.

        Also, you didn't miss those string constants - I removed those two cases since they were being used for the wrong variable (facet=true/false instead of facet.sort=true/false). Wrong context makes the code more confusing.

        Show
        Yonik Seeley added a comment - Lars, I already committed some code to normalize on the new facet.sort values so we don't need to check for both new and legacy everywhere. Also, you didn't miss those string constants - I removed those two cases since they were being used for the wrong variable (facet=true/false instead of facet.sort=true/false). Wrong context makes the code more confusing.
        Hide
        Lars Kotthoff added a comment -

        Lars, I already committed some code to normalize on the new facet.sort values so we don't need to check for both new and legacy everywhere.

        Ah sorry, missed that. Must have been in the gigantic diff

        Also, you didn't miss those string constants - I removed those two cases since they were being used for the wrong variable (facet=true/false instead of facet.sort=true/false). Wrong context makes the code more confusing.

        Argh, the dangers of search and replace...

        Show
        Lars Kotthoff added a comment - Lars, I already committed some code to normalize on the new facet.sort values so we don't need to check for both new and legacy everywhere. Ah sorry, missed that. Must have been in the gigantic diff Also, you didn't miss those string constants - I removed those two cases since they were being used for the wrong variable (facet=true/false instead of facet.sort=true/false). Wrong context makes the code more confusing. Argh, the dangers of search and replace...
        Hide
        Grant Ingersoll added a comment -

        Bulk close for Solr 1.4

        Show
        Grant Ingersoll added a comment - Bulk close for Solr 1.4

          People

          • Assignee:
            Erik Hatcher
            Reporter:
            Lars Kotthoff
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development