Lucene - Core
  1. Lucene - Core
  2. LUCENE-3501

random sampler is not random (and so facet SamplingWrapperTest occasionally fails)

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: modules/facet
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      RandomSample is not random at all:
      It does not even import java.util.Random, and its behavior is deterministic.

      in addition, the test testCountUsingSamping() never retries as it was supposed to (for taking care of the hoped-for randomness).

        Activity

        Hide
        Doron Cohen added a comment -

        The error (from Jenkins) was:

        junit.framework.AssertionFailedError: Results are not the same!
        	at org.apache.lucene.util.LuceneTestCaseRunner.runChild(LuceneTestCaseRunner.java:149)
        	at org.apache.lucene.util.LuceneTestCaseRunner.runChild(LuceneTestCaseRunner.java:51)
        	at org.apache.lucene.facet.FacetTestBase.assertSameResults(FacetTestBase.java:316)
        	at org.apache.lucene.facet.search.sampling.BaseSampleTestTopK.assertSampling(BaseSampleTestTopK.java:93)
        	at org.apache.lucene.facet.search.sampling.BaseSampleTestTopK.testCountUsingSamping(BaseSampleTestTopK.java:76)
        	at org.apache.lucene.util.LuceneTestCase$2$1.evaluate(LuceneTestCase.java:610)
        
        reproduce with: ant test -Dtestcase=SamplingWrapperTest -Dtestmethod=testCountUsingSamping -Dtests.seed=39c6b88dcada2192:-cf936a4278714b1:-770b2814b4a6acd7
        
        Show
        Doron Cohen added a comment - The error (from Jenkins) was: junit.framework.AssertionFailedError: Results are not the same! at org.apache.lucene.util.LuceneTestCaseRunner.runChild(LuceneTestCaseRunner.java:149) at org.apache.lucene.util.LuceneTestCaseRunner.runChild(LuceneTestCaseRunner.java:51) at org.apache.lucene.facet.FacetTestBase.assertSameResults(FacetTestBase.java:316) at org.apache.lucene.facet.search.sampling.BaseSampleTestTopK.assertSampling(BaseSampleTestTopK.java:93) at org.apache.lucene.facet.search.sampling.BaseSampleTestTopK.testCountUsingSamping(BaseSampleTestTopK.java:76) at org.apache.lucene.util.LuceneTestCase$2$1.evaluate(LuceneTestCase.java:610) reproduce with: ant test -Dtestcase=SamplingWrapperTest -Dtestmethod=testCountUsingSamping -Dtests.seed=39c6b88dcada2192:-cf936a4278714b1:-770b2814b4a6acd7
        Hide
        Doron Cohen added a comment - - edited

        Before applying this patch should do:

        svn mv modules/facet/src/java/org/apache/lucene/facet/util/RandomSample.java modules/facet/src/java/org/apache/lucene/facet/search/sampling/RepeatableSampler.java
        

        I looked at this, and also discussed with Gilad, and it seems that:

        • The test is broken as it claims to do N trials in case of failure but it does not, because its try/catch does not catch AssertionError, and so only one trial is attempted. (Few trials make sense because with sampling, there is always a possibility that the selected sample set of docs would not contain the "correct" best facets even with a high over sampling ratio (over sampling means that for the selected set of docs more best facets are collected).
        • Even after fixing the test to actually try more than once, it still fails, because there is no randomness in RandomSample... surprising but true.

        In this patch:

        • Sampler made an abstract class.
        • RandomSample renamed to RepeatableSampler which extends Sampler.
        • RandomSampler was added - it too extends Sampler - this is a simple random implementation, which is now the default (used by default in SamplingWrapperAccumulator).
        • The test randomly selects between the two sampler implementations. If you want to see the behavior that created the bug, remove that latter randomness by setting to false the variable useRandomSampler of BaseSampleTestTopK.testCountUsingSamping().

        I think this is ready to commit.
        Wasn't sure though, where should the Changes entry go?

        Show
        Doron Cohen added a comment - - edited Before applying this patch should do: svn mv modules/facet/src/java/org/apache/lucene/facet/util/RandomSample.java modules/facet/src/java/org/apache/lucene/facet/search/sampling/RepeatableSampler.java I looked at this, and also discussed with Gilad, and it seems that: The test is broken as it claims to do N trials in case of failure but it does not, because its try/catch does not catch AssertionError, and so only one trial is attempted. (Few trials make sense because with sampling, there is always a possibility that the selected sample set of docs would not contain the "correct" best facets even with a high over sampling ratio (over sampling means that for the selected set of docs more best facets are collected). Even after fixing the test to actually try more than once, it still fails, because there is no randomness in RandomSample... surprising but true. In this patch: Sampler made an abstract class. RandomSample renamed to RepeatableSampler which extends Sampler. RandomSampler was added - it too extends Sampler - this is a simple random implementation, which is now the default (used by default in SamplingWrapperAccumulator). The test randomly selects between the two sampler implementations. If you want to see the behavior that created the bug, remove that latter randomness by setting to false the variable useRandomSampler of BaseSampleTestTopK.testCountUsingSamping() . I think this is ready to commit. Wasn't sure though, where should the Changes entry go?
        Hide
        Shai Erera added a comment -

        I briefly went through the patch:

        • In the test, I prefer to still catch Exception, or if you want to be on the safe side, Throwable. And have assertSameResult throw RuntimeException. Calling fail() from there, forcing you to handle Errors, seem too low-level to me ...
        • In AdaptiveFacetsAccumulator you have this line: //private Sampler sampler = new RepeatableSampler();. Is it a leftover?
        • Maybe add a line or two to RandomSample.createSample() (internal comments), such as:
          • "Skip over 'step' documents" before the for-loop
          • "Add all leftover documents to the sampled set" before last 'while'.
            (Please also confirm my understanding)
        • CHANGES - in this case you only need a CHANGES entry for 3x (since the change is applied to there as well), and it is under contrib.

        Otherwise, it looks very good !

        Show
        Shai Erera added a comment - I briefly went through the patch: In the test, I prefer to still catch Exception, or if you want to be on the safe side, Throwable. And have assertSameResult throw RuntimeException. Calling fail() from there, forcing you to handle Errors, seem too low-level to me ... In AdaptiveFacetsAccumulator you have this line: //private Sampler sampler = new RepeatableSampler();. Is it a leftover? Maybe add a line or two to RandomSample.createSample() (internal comments), such as: "Skip over 'step' documents" before the for-loop "Add all leftover documents to the sampled set" before last 'while'. (Please also confirm my understanding) CHANGES - in this case you only need a CHANGES entry for 3x (since the change is applied to there as well), and it is under contrib. Otherwise, it looks very good !
        Hide
        Doron Cohen added a comment -

        Thanks for reviewing Shai!
        I'll change as you propose (confirming your understanding) and commit tomorrow.

        Show
        Doron Cohen added a comment - Thanks for reviewing Shai! I'll change as you propose (confirming your understanding) and commit tomorrow.
        Hide
        Doron Cohen added a comment -

        Fixed in trunk: r1181760

        Shai's comment on catching AssertionError made me search for other cases of catching this error in Lucene. Few such cases exist, and they all seem wrong, as they call fail when failing fail due to assert not enabled but fail to detect that failure since they then silently ignore AssertionError thrown by fail(). Opened LUCENE-3506 for this.

        Show
        Doron Cohen added a comment - Fixed in trunk: r1181760 Shai's comment on catching AssertionError made me search for other cases of catching this error in Lucene. Few such cases exist, and they all seem wrong, as they call fail when failing fail due to assert not enabled but fail to detect that failure since they then silently ignore AssertionError thrown by fail(). Opened LUCENE-3506 for this.
        Hide
        Doron Cohen added a comment -

        Fix merged to 3x: 1188129.
        Thanks Gilad and Shai for helping to fix this.

        Show
        Doron Cohen added a comment - Fix merged to 3x: 1188129. Thanks Gilad and Shai for helping to fix this.

          People

          • Assignee:
            Doron Cohen
            Reporter:
            Doron Cohen
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development