Lucene - Core
  1. Lucene - Core
  2. LUCENE-5961

FunctionValues.exist(int) isn't returning false in cases where it should for many "math" based value sources

    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
    • Lucene Fields:
      New

      Description

      The FunctionValues class contains an exist(int doc) method with a default implementation that returns true - field based DocValues override this method as appropriate, but most of the "function" based subclasses in the code (typically anonymous subclasses of "FloatDocValues") don't override this method when wrapping other ValueSources.

      So for example: the FunctionValues returned by ProductFloatFunction.getValues() will say that a value exists for any doc, even if that ProductFloatFunction wraps two FloatFieldSources that don't exist for any docs

      1. LUCENE-5961.patch
        37 kB
        Hoss Man
      2. LUCENE-5961.patch
        35 kB
        Hoss Man
      3. LUCENE-5961.patch
        2 kB
        Hoss Man

        Issue Links

          Activity

          Hide
          Hoss Man added a comment -

          Minimal test showing the problem.

          I haven't looked in depth, but i think the same basic problem exists in, and can easily be fixed in, MultiFloatFunction, DualFloatFunction, MultiBoolFunction & DualFloatFunction ... may be more.

          I'm happy to dig into this more when i get back from vacation (write more tests, etc...) but before i bother: does anyone disagree with this statement:

          Expected Behavior

          FunctionValues returned by ValueSources that model math functions, and which wrap other sub-FunctionValues, should generally implement exists(doc) by returning "true" if and only if the exists method of all of the sub-FunctionValues it wraps returns "true" for that document

          Show
          Hoss Man added a comment - Minimal test showing the problem. I haven't looked in depth, but i think the same basic problem exists in, and can easily be fixed in, MultiFloatFunction, DualFloatFunction, MultiBoolFunction & DualFloatFunction ... may be more. I'm happy to dig into this more when i get back from vacation (write more tests, etc...) but before i bother: does anyone disagree with this statement: Expected Behavior FunctionValues returned by ValueSources that model math functions, and which wrap other sub-FunctionValues, should generally implement exists(doc) by returning "true" if and only if the exists method of all of the sub-FunctionValues it wraps returns "true" for that document
          Hide
          Hoss Man added a comment -

          discovered this while working on tests for SOLR-6354.

          As part of that issue i'm updating StatsComponentTest.java with some commented out assertions that can be re-enabled once this issue is resolved (grep for "SOLR-6354")

          Show
          Hoss Man added a comment - discovered this while working on tests for SOLR-6354 . As part of that issue i'm updating StatsComponentTest.java with some commented out assertions that can be re-enabled once this issue is resolved (grep for " SOLR-6354 ")
          Hide
          Hoss Man added a comment -

          A whole lot of new tests, and fixed behavior for several ValueSources.

          I added a few TODOs in the test related to a couple of ValueSource classes where i wasn't sure what the "correct" behavior should even be - but i'm not planning on trying to tackle those here. i just wanted to note in the test that the expected behavior is unclear & untested.

          Show
          Hoss Man added a comment - A whole lot of new tests, and fixed behavior for several ValueSources. I added a few TODOs in the test related to a couple of ValueSource classes where i wasn't sure what the "correct" behavior should even be - but i'm not planning on trying to tackle those here. i just wanted to note in the test that the expected behavior is unclear & untested.
          Hide
          Hoss Man added a comment -

          added a couple more solr tests & fixed a few minor issues found by precommit.

          Planning to commit later today after another full test run

          Show
          Hoss Man added a comment - added a couple more solr tests & fixed a few minor issues found by precommit. Planning to commit later today after another full test run
          Hide
          ASF subversion and git services added a comment -

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

          LUCENE-5961: Fix the exists() method for FunctionValues returned by many ValueSoures to behave properly when wrapping other ValueSources which do not exist for the specified document

          Show
          ASF subversion and git services added a comment - Commit 1632414 from hossman@apache.org in branch 'dev/trunk' [ https://svn.apache.org/r1632414 ] LUCENE-5961 : Fix the exists() method for FunctionValues returned by many ValueSoures to behave properly when wrapping other ValueSources which do not exist for the specified document
          Hide
          ASF subversion and git services added a comment -

          Commit 1632432 from hossman@apache.org in branch 'dev/branches/branch_5x'
          [ https://svn.apache.org/r1632432 ]

          LUCENE-5961: Fix the exists() method for FunctionValues returned by many ValueSoures to behave properly when wrapping other ValueSources which do not exist for the specified document (merge r1632414)

          Show
          ASF subversion and git services added a comment - Commit 1632432 from hossman@apache.org in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1632432 ] LUCENE-5961 : Fix the exists() method for FunctionValues returned by many ValueSoures to behave properly when wrapping other ValueSources which do not exist for the specified document (merge r1632414)
          Hide
          ASF subversion and git services added a comment -

          Commit 1633069 from Adrien Grand in branch 'dev/branches/branch_5x'
          [ https://svn.apache.org/r1633069 ]

          LUCENE-5961: Fix test bug in TestValueSources.

          Show
          ASF subversion and git services added a comment - Commit 1633069 from Adrien Grand in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1633069 ] LUCENE-5961 : Fix test bug in TestValueSources.
          Hide
          ASF subversion and git services added a comment -

          Commit 1633073 from Adrien Grand in branch 'dev/trunk'
          [ https://svn.apache.org/r1633073 ]

          LUCENE-5961: Fix test bug in TestValueSources.

          Show
          ASF subversion and git services added a comment - Commit 1633073 from Adrien Grand in branch 'dev/trunk' [ https://svn.apache.org/r1633073 ] LUCENE-5961 : Fix test bug in TestValueSources.
          Hide
          Robert Muir added a comment -

          reopen for backport

          Show
          Robert Muir added a comment - reopen for backport
          Hide
          Hoss Man added a comment -

          I'm -0 to backporting this to 4.10.x...

          I'm not convinced the benefits of the "fixed" behavior out-weigh the risk that this will cause problems for existing users who have code that depends on the current behavior, and will expect 4.10.3 to be a drop in replacement w/o needing to modify any of their lucene client code or solr queries/configs.

          I'd rather let this fix wait for 5.0 (or a 4.11 if there was going to be one), when affected users are more likely to pay attention to MIGRATE.txt and the Solr upgrade instructions and take the time to fix their code/configs/queries if they really want the existing broken behavior...

          https://svn.apache.org/viewvc/lucene/dev/trunk/lucene/MIGRATE.txt?r1=1632414&r2=1632413&pathrev=1632414
          https://svn.apache.org/viewvc/lucene/dev/trunk/solr/CHANGES.txt?r1=1632414&r2=1632413&pathrev=1632414

          Show
          Hoss Man added a comment - I'm -0 to backporting this to 4.10.x... I'm not convinced the benefits of the "fixed" behavior out-weigh the risk that this will cause problems for existing users who have code that depends on the current behavior, and will expect 4.10.3 to be a drop in replacement w/o needing to modify any of their lucene client code or solr queries/configs. I'd rather let this fix wait for 5.0 (or a 4.11 if there was going to be one), when affected users are more likely to pay attention to MIGRATE.txt and the Solr upgrade instructions and take the time to fix their code/configs/queries if they really want the existing broken behavior... https://svn.apache.org/viewvc/lucene/dev/trunk/lucene/MIGRATE.txt?r1=1632414&r2=1632413&pathrev=1632414 https://svn.apache.org/viewvc/lucene/dev/trunk/solr/CHANGES.txt?r1=1632414&r2=1632413&pathrev=1632414
          Hide
          Robert Muir added a comment -

          remove 4.10.3, too risky

          Show
          Robert Muir added a comment - remove 4.10.3, too risky
          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