Solr
  1. Solr
  2. SOLR-6507

various bugs using localparams with stats.field

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 5.0, 6.0
    • Component/s: None
    • Labels:
      None

      Description

      StatsComponent has two different code paths for dealing with param parsing depending on wether it's a single node request of a distributed request, which results in two very differnet looking bugs (but in my opinion have the same root cause: bogus local param parsing):

      • the documented local params for stats.field don't work on distributed stats requests at all
      • per field "calcdistinct" doesn't work if localparms are used on single node request
      1. SOLR-6507.patch
        25 kB
        Hoss Man
      2. SOLR-6507.patch
        21 kB
        Hoss Man

        Issue Links

          Activity

          Hide
          Hoss Man added a comment -

          The attached patch has some quick and dirty tests demonstraitng these problems as well as my porposed fix.

          Rather then slap a bandaid on this i did a pretty serious overhaul of the way the code in StatsComponent is laid out. This not only fixes the bug by consolidating the param parsing logic into a single place, it helps ensure param descrepencies like won't pop up as we add features (by consolidating the param parsing logic into a single place), and it moves the direction i want to move for features like SOLR-6354 & SOLR-6351.

          • StatsInfo is now used for both for both distributed & local requests
            • still one instance per request
          • SimpleFacets has been renamed/refactored into "StatsField"
            • now only models a single "stats.field" param (no more "iterating" local state variables)
            • API is now a lot more locked down and easier to understand (in my opinion)
            • code that dealt with multiple "stats.field" params bubbled up to either StatsInfo or the process() method of StatsComponent

          There's still plenty of nocommits – mostly dealing with javadocs and more robust tests, but i think this is a good start in the right direction.

          Show
          Hoss Man added a comment - The attached patch has some quick and dirty tests demonstraitng these problems as well as my porposed fix. Rather then slap a bandaid on this i did a pretty serious overhaul of the way the code in StatsComponent is laid out. This not only fixes the bug by consolidating the param parsing logic into a single place, it helps ensure param descrepencies like won't pop up as we add features (by consolidating the param parsing logic into a single place), and it moves the direction i want to move for features like SOLR-6354 & SOLR-6351 . StatsInfo is now used for both for both distributed & local requests still one instance per request SimpleFacets has been renamed/refactored into "StatsField" now only models a single "stats.field" param (no more "iterating" local state variables) API is now a lot more locked down and easier to understand (in my opinion) code that dealt with multiple "stats.field" params bubbled up to either StatsInfo or the process() method of StatsComponent There's still plenty of nocommits – mostly dealing with javadocs and more robust tests, but i think this is a good start in the right direction.
          Hide
          Hoss Man added a comment -

          Not as many remaining test & code changes as i anticipated – My recollection of how per-field overrides worked in facets when the "key" local param is used was totally wrong ... which simplified the logic & test permutations needed.

          Updated pach for trunk with javadocs. Hoping to commit & backport later today (or early tomorow) – the backport changes should be fairly straight forward since this change didn't require any modifications to the DovValuesStast class (nor should any changes be required to UnInvertedField on 4x)

          Show
          Hoss Man added a comment - Not as many remaining test & code changes as i anticipated – My recollection of how per-field overrides worked in facets when the "key" local param is used was totally wrong ... which simplified the logic & test permutations needed. Updated pach for trunk with javadocs. Hoping to commit & backport later today (or early tomorow) – the backport changes should be fairly straight forward since this change didn't require any modifications to the DovValuesStast class (nor should any changes be required to UnInvertedField on 4x)
          Hide
          ASF subversion and git services added a comment -

          Commit 1625163 from hossman@apache.org in branch 'dev/trunk'
          [ https://svn.apache.org/r1625163 ]

          SOLR-6507: Fixed several bugs involving stats.field used with local params

          Show
          ASF subversion and git services added a comment - Commit 1625163 from hossman@apache.org in branch 'dev/trunk' [ https://svn.apache.org/r1625163 ] SOLR-6507 : Fixed several bugs involving stats.field used with local params
          Hide
          ASF subversion and git services added a comment -

          Commit 1625172 from hossman@apache.org in branch 'dev/branches/branch_4x'
          [ https://svn.apache.org/r1625172 ]

          SOLR-6507: Fixed several bugs involving stats.field used with local params (merge r1625163)

          Show
          ASF subversion and git services added a comment - Commit 1625172 from hossman@apache.org in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1625172 ] SOLR-6507 : Fixed several bugs involving stats.field used with local params (merge r1625163)
          Hide
          Hoss Man added a comment -

          committed to trunk & backported to 4.x

          given the amount of code churn involved here, i'm not really comfortable backporting to branch_4_10 ... not until this code gets smoked out a bit more (we'll see what happens with the timing of a 4.10.1 release - but for now i'm erring on the side of caution)

          Show
          Hoss Man added a comment - committed to trunk & backported to 4.x given the amount of code churn involved here, i'm not really comfortable backporting to branch_4_10 ... not until this code gets smoked out a bit more (we'll see what happens with the timing of a 4.10.1 release - but for now i'm erring on the side of caution)
          Hide
          Anshum Gupta added a comment -

          Bulk close after 5.0 release.

          Show
          Anshum Gupta added a comment - Bulk close after 5.0 release.

            People

            • Assignee:
              Hoss Man
              Reporter:
              Hoss Man
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development