Lucene - Core
  1. Lucene - Core
  2. LUCENE-1536

if a filter can support random access API, we should use it

    Details

    • Lucene Fields:
      New

      Description

      I ran some performance tests, comparing applying a filter via
      random-access API instead of current trunk's iterator API.

      This was inspired by LUCENE-1476, where we realized deletions should
      really be implemented just like a filter, but then in testing found
      that switching deletions to iterator was a very sizable performance
      hit.

      Some notes on the test:

      • Index is first 2M docs of Wikipedia. Test machine is Mac OS X
        10.5.6, quad core Intel CPU, 6 GB RAM, java 1.6.0_07-b06-153.
      • I test across multiple queries. 1-X means an OR query, eg 1-4
        means 1 OR 2 OR 3 OR 4, whereas +1-4 is an AND query, ie 1 AND 2
        AND 3 AND 4. "u s" means "united states" (phrase search).
      • I test with multiple filter densities (0, 1, 2, 5, 10, 25, 75, 90,
        95, 98, 99, 99.99999 (filter is non-null but all bits are set),
        100 (filter=null, control)).
      • Method high means I use random-access filter API in
        IndexSearcher's main loop. Method low means I use random-access
        filter API down in SegmentTermDocs (just like deleted docs
        today).
      • Baseline (QPS) is current trunk, where filter is applied as iterator up
        "high" (ie in IndexSearcher's search loop).
      1. luceneutil.patch
        3 kB
        Robert Muir
      2. LUCENE-1536-rewrite.patch
        52 kB
        Uwe Schindler
      3. LUCENE-1536-rewrite.patch
        71 kB
        Uwe Schindler
      4. LUCENE-1536-rewrite.patch
        84 kB
        Uwe Schindler
      5. LUCENE-1536-rewrite.patch
        88 kB
        Uwe Schindler
      6. LUCENE-1536-rewrite.patch
        115 kB
        Uwe Schindler
      7. LUCENE-1536-rewrite.patch
        128 kB
        Uwe Schindler
      8. LUCENE-1536-rewrite.patch
        137 kB
        Uwe Schindler
      9. LUCENE-1536-rewrite.patch
        0.1 kB
        Uwe Schindler
      10. LUCENE-1536.patch
        11 kB
        Michael McCandless
      11. LUCENE-1536.patch
        19 kB
        Jason Rutherglen
      12. LUCENE-1536.patch
        26 kB
        Jason Rutherglen
      13. LUCENE-1536.patch
        51 kB
        Jason Rutherglen
      14. LUCENE-1536.patch
        55 kB
        Jason Rutherglen
      15. LUCENE-1536.patch
        199 kB
        Michael McCandless
      16. LUCENE-1536.patch
        63 kB
        Michael McCandless
      17. LUCENE-1536.patch
        69 kB
        Chris Male
      18. LUCENE-1536.patch
        67 kB
        Chris Male
      19. LUCENE-1536.patch
        68 kB
        Chris Male
      20. LUCENE-1536.patch
        71 kB
        Michael McCandless
      21. LUCENE-1536.patch
        75 kB
        Michael McCandless
      22. LUCENE-1536.patch
        24 kB
        Chris Male
      23. LUCENE-1536.patch
        20 kB
        Robert Muir
      24. LUCENE-1536.patch
        24 kB
        Robert Muir
      25. LUCENE-1536.patch
        24 kB
        Chris Male
      26. LUCENE-1536.patch
        28 kB
        Uwe Schindler
      27. LUCENE-1536.patch
        28 kB
        Uwe Schindler
      28. LUCENE-1536.patch
        29 kB
        Uwe Schindler
      29. LUCENE-1536.patch
        52 kB
        Robert Muir
      30. LUCENE-1536.patch
        123 kB
        Robert Muir
      31. LUCENE-1536.patch
        141 kB
        Yonik Seeley
      32. LUCENE-1536.patch
        141 kB
        Uwe Schindler
      33. LUCENE-1536.patch
        147 kB
        Robert Muir
      34. LUCENE-1536.patch
        147 kB
        Uwe Schindler
      35. LUCENE-1536.patch
        148 kB
        Uwe Schindler
      36. LUCENE-1536.patch
        148 kB
        Uwe Schindler
      37. LUCENE-1536.patch
        148 kB
        Uwe Schindler
      38. LUCENE-1536.patch
        52 kB
        Uwe Schindler
      39. LUCENE-1536_hack.patch
        150 kB
        Robert Muir
      40. changes-yonik-uwe.patch
        3 kB
        Uwe Schindler
      41. CachedFilterIndexReader.java
        4 kB
        Michael McCandless

        Issue Links

          Activity

          Hide
          Michael McCandless added a comment -

          Test results:

          %tg Filter Query Method Hits QPS QPSNew %tg change
          0% 1 low 0 18992.3 142.6 -99.2%
          0% 1 high 0 18992.3 109.6 -99.4%
          1% 1 low 3863 133.7 135.3 1.2%
          1% 1 high 3863 133.7 99.7 -25.4%
          2% 1 low 7714 108.2 133.7 23.6%
          2% 1 high 7714 108.2 100.5 -7.1%
          5% 1 low 19333 76.9 128.6 67.2%
          5% 1 high 19333 76.9 97.2 26.4%
          10% 1 low 38673 62.5 119.1 90.6%
          10% 1 high 38673 62.5 92.0 47.2%
          25% 1 low 96670 47.3 102.3 116.3%
          25% 1 high 96670 47.3 90.5 91.3%
          50% 1 low 193098 40.0 85.6 114.0%
          50% 1 high 193098 40.0 79.6 99.0%
          75% 1 low 289765 38.0 82.8 117.9%
          75% 1 high 289765 38.0 79.0 107.9%
          90% 1 low 347762 37.2 82.7 122.3%
          90% 1 high 347762 37.2 72.8 95.7%
          95% 1 low 367102 36.5 82.9 127.1%
          95% 1 high 367102 36.5 73.1 100.3%
          98% 1 low 378721 37.3 81.8 119.3%
          98% 1 high 378721 37.3 73.0 95.7%
          99% 1 low 382572 36.6 83.3 127.6%
          99% 1 high 382572 36.6 71.8 96.2%
          99.99999% 1 low 386435 38.0 83.8 120.5%
          99.99999% 1 high 386435 38.0 70.9 86.6%
          100% 1 low 386435 88.0 89.1 1.2%
          100% 1 high 386435 88.0 89.5 1.7%
          0% 1-2 low 0 18808.1 71.5 -99.6%
          0% 1-2 high 0 18808.1 22.4 -99.9%
          1% 1-2 low 5363 46.8 65.2 39.3%
          1% 1-2 high 5363 46.8 22.5 -51.9%
          2% 1-2 low 10675 37.6 61.4 63.3%
          2% 1-2 high 10675 37.6 22.4 -40.4%
          5% 1-2 low 26880 28.8 53.4 85.4%
          5% 1-2 high 26880 28.8 22.3 -22.6%
          10% 1-2 low 53673 23.7 48.1 103.0%
          10% 1-2 high 53673 23.7 21.8 -8.0%
          25% 1-2 low 133988 19.9 37.2 86.9%
          25% 1-2 high 133988 19.9 21.0 5.5%
          50% 1-2 low 267757 17.2 27.4 59.3%
          50% 1-2 high 267757 17.2 20.3 18.0%
          75% 1-2 low 401596 16.9 23.1 36.7%
          75% 1-2 high 401596 16.9 20.3 20.1%
          90% 1-2 low 481911 17.0 21.2 24.7%
          90% 1-2 high 481911 17.0 20.6 21.2%
          95% 1-2 low 508704 17.1 20.7 21.1%
          95% 1-2 high 508704 17.1 20.7 21.1%
          98% 1-2 low 524909 17.3 20.7 19.7%
          98% 1-2 high 524909 17.3 20.7 19.7%
          99% 1-2 low 530221 17.4 20.5 17.8%
          99% 1-2 high 530221 17.4 20.8 19.5%
          99.99999% 1-2 low 535584 17.1 20.3 18.7%
          99.99999% 1-2 high 535584 17.1 20.3 18.7%
          100% 1-2 low 535584 21.0 20.7 -1.4%
          100% 1-2 high 535584 21.0 20.8 -1.0%
          0% 1-4 low 0 17961.7 42.2 -99.8%
          0% 1-4 high 0 17961.7 11.9 -99.9%
          1% 1-4 low 6544 27.1 38.4 41.7%
          1% 1-4 high 6544 27.1 12.0 -55.7%
          2% 1-4 low 13062 21.4 36.0 68.2%
          2% 1-4 high 13062 21.4 11.9 -44.4%
          5% 1-4 low 32815 16.1 31.3 94.4%
          5% 1-4 high 32815 16.1 11.8 -26.7%
          10% 1-4 low 65491 13.3 27.8 109.0%
          10% 1-4 high 65491 13.3 11.7 -12.0%
          25% 1-4 low 163600 10.5 21.0 100.0%
          25% 1-4 high 163600 10.5 11.5 9.5%
          50% 1-4 low 327302 9.9 15.3 54.5%
          50% 1-4 high 327302 9.9 11.2 13.1%
          75% 1-4 low 490881 9.7 12.6 29.9%
          75% 1-4 high 490881 9.7 11.1 14.4%
          90% 1-4 low 588990 9.6 11.6 20.8%
          90% 1-4 high 588990 9.6 11.1 15.6%
          95% 1-4 low 621666 9.6 11.3 17.7%
          95% 1-4 high 621666 9.6 11.2 16.7%
          98% 1-4 low 641419 9.4 11.1 18.1%
          98% 1-4 high 641419 9.4 11.2 19.1%
          99% 1-4 low 647937 9.8 11.1 13.3%
          99% 1-4 high 647937 9.8 11.2 14.3%
          99.99999% 1-4 low 654481 10.0 11.1 11.0%
          99.99999% 1-4 high 654481 10.0 11.2 12.0%
          100% 1-4 low 654481 11.3 11.3 0.0%
          100% 1-4 high 654481 11.3 11.2 -0.9%
          0% 1-10 low 0 15990.0 22.5 -99.9%
          0% 1-10 high 0 15990.0 5.8 -100.0%
          1% 1-10 low 8406 13.1 20.2 54.2%
          1% 1-10 high 8406 13.1 5.8 -55.7%
          2% 1-10 low 16756 10.2 18.9 85.3%
          2% 1-10 high 16756 10.2 5.8 -43.1%
          5% 1-10 low 41937 7.7 16.5 114.3%
          5% 1-10 high 41937 7.7 5.7 -26.0%
          10% 1-10 low 83828 6.3 14.5 130.2%
          10% 1-10 high 83828 6.3 5.7 -9.5%
          25% 1-10 low 209328 5.3 10.9 105.7%
          25% 1-10 high 209328 5.3 5.7 7.5%
          50% 1-10 low 418668 4.9 7.8 59.2%
          50% 1-10 high 418668 4.9 5.5 12.2%
          75% 1-10 low 628338 4.9 6.4 30.6%
          75% 1-10 high 628338 4.9 5.5 12.2%
          90% 1-10 low 753838 4.9 5.9 20.4%
          90% 1-10 high 753838 4.9 5.5 12.2%
          95% 1-10 low 795729 5.0 5.7 14.0%
          95% 1-10 high 795729 5.0 5.6 12.0%
          98% 1-10 low 820910 5.1 5.6 9.8%
          98% 1-10 high 820910 5.1 5.6 9.8%
          99% 1-10 low 829260 5.1 5.6 9.8%
          99% 1-10 high 829260 5.1 5.6 9.8%
          99.99999% 1-10 low 837666 5.2 5.6 7.7%
          99.99999% 1-10 high 837666 5.2 5.6 7.7%
          100% 1-10 low 837666 5.7 5.7 0.0%
          100% 1-10 high 837666 5.7 5.6 -1.8%
          0% +1-2 low 0 18848.4 138.4 -99.3%
          0% +1-2 high 0 18848.4 27.4 -99.9%
          1% +1-2 low 2308 63.1 77.0 22.0%
          1% +1-2 high 2308 63.1 27.1 -57.1%
          2% +1-2 low 4621 50.3 69.7 38.6%
          2% +1-2 high 4621 50.3 26.8 -46.7%
          5% +1-2 low 11706 36.1 56.9 57.6%
          5% +1-2 high 11706 36.1 26.5 -26.6%
          10% +1-2 low 23272 28.4 48.6 71.1%
          10% +1-2 high 23272 28.4 26.2 -7.7%
          25% +1-2 low 58401 23.7 36.4 53.6%
          25% +1-2 high 58401 23.7 24.9 5.1%
          50% +1-2 low 117083 20.9 28.2 34.9%
          50% +1-2 high 117083 20.9 23.9 14.4%
          75% +1-2 low 176233 19.3 24.4 26.4%
          75% +1-2 high 176233 19.3 22.8 18.1%
          90% +1-2 low 211362 18.6 22.9 23.1%
          90% +1-2 high 211362 18.6 22.5 21.0%
          95% +1-2 low 222928 18.5 22.5 21.6%
          95% +1-2 high 222928 18.5 22.5 21.6%
          98% +1-2 low 230013 18.3 22.0 20.2%
          98% +1-2 high 230013 18.3 22.4 22.4%
          99% +1-2 low 232326 18.3 22.1 20.8%
          99% +1-2 high 232326 18.3 22.3 21.9%
          99.99999% +1-2 low 234634 17.8 21.9 23.0%
          99.99999% +1-2 high 234634 17.8 22.2 24.7%
          100% +1-2 low 234634 22.9 22.7 -0.9%
          100% +1-2 high 234634 22.9 22.6 -1.3%
          0% +1-4 low 0 17987.0 137.9 -99.2%
          0% +1-4 high 0 17987.0 18.0 -99.9%
          1% +1-4 low 923 34.5 58.4 69.3%
          1% +1-4 high 923 34.5 17.9 -48.1%
          2% +1-4 low 1849 28.7 51.9 80.8%
          2% +1-4 high 1849 28.7 17.9 -37.6%
          5% +1-4 low 4794 22.1 39.0 76.5%
          5% +1-4 high 4794 22.1 17.8 -19.5%
          10% +1-4 low 9595 19.9 35.3 77.4%
          10% +1-4 high 9595 19.9 17.5 -12.1%
          25% +1-4 low 24136 17.3 25.7 48.6%
          25% +1-4 high 24136 17.3 17.2 -0.6%
          50% +1-4 low 48328 16.1 19.5 21.1%
          50% +1-4 high 48328 16.1 16.8 4.3%
          75% +1-4 low 72718 15.8 17.0 7.6%
          75% +1-4 high 72718 15.8 16.6 5.1%
          90% +1-4 low 87259 15.3 16.3 6.5%
          90% +1-4 high 87259 15.3 16.4 7.2%
          95% +1-4 low 92060 15.4 16.0 3.9%
          95% +1-4 high 92060 15.4 16.4 6.5%
          98% +1-4 low 95005 15.4 15.8 2.6%
          98% +1-4 high 95005 15.4 16.4 6.5%
          99% +1-4 low 95931 15.1 15.7 4.0%
          99% +1-4 high 95931 15.1 16.4 8.6%
          99.99999% +1-4 low 96854 14.3 15.9 11.2%
          99.99999% +1-4 high 96854 14.3 16.2 13.3%
          100% +1-4 low 96854 16.6 16.6 0.0%
          100% +1-4 high 96854 16.6 16.6 0.0%
          0% "u s" low 0 19123.0 124.9 -99.3%
          0% "u s" high 0 19123.0 7.0 -100.0%
          1% "u s" low 3192 23.5 27.3 16.2%
          1% "u s" high 3192 23.5 7.1 -69.8%
          2% "u s" low 6179 17.8 24.3 36.5%
          2% "u s" high 6179 17.8 7.0 -60.7%
          5% "u s" low 15446 12.7 20.3 59.8%
          5% "u s" high 15446 12.7 7.0 -44.9%
          10% "u s" low 30858 10.1 16.1 59.4%
          10% "u s" high 30858 10.1 6.8 -32.7%
          25% "u s" low 77138 7.7 13.0 68.8%
          25% "u s" high 77138 7.7 6.8 -11.7%
          50% "u s" low 154331 6.7 9.9 47.8%
          50% "u s" high 154331 6.7 7.0 4.5%
          75% "u s" low 231412 6.3 8.4 33.3%
          75% "u s" high 231412 6.3 7.0 11.1%
          90% "u s" low 277692 5.8 7.2 24.1%
          90% "u s" high 277692 5.8 7.0 20.7%
          95% "u s" low 293104 5.8 7.1 22.4%
          95% "u s" high 293104 5.8 7.0 20.7%
          98% "u s" low 302371 5.8 7.0 20.7%
          98% "u s" high 302371 5.8 6.9 19.0%
          99% "u s" low 305358 5.8 6.9 19.0%
          99% "u s" high 305358 5.8 6.9 19.0%
          99.99999% "u s" low 308550 5.8 6.8 17.2%
          99.99999% "u s" high 308550 5.8 6.9 19.0%
          100% "u s" low 308550 7.0 6.9 -1.4%
          100% "u s" high 308550 7.0 6.9 -1.4%
          Show
          Michael McCandless added a comment - Test results: %tg Filter Query Method Hits QPS QPSNew %tg change 0% 1 low 0 18992.3 142.6 -99.2% 0% 1 high 0 18992.3 109.6 -99.4% 1% 1 low 3863 133.7 135.3 1.2% 1% 1 high 3863 133.7 99.7 -25.4% 2% 1 low 7714 108.2 133.7 23.6% 2% 1 high 7714 108.2 100.5 -7.1% 5% 1 low 19333 76.9 128.6 67.2% 5% 1 high 19333 76.9 97.2 26.4% 10% 1 low 38673 62.5 119.1 90.6% 10% 1 high 38673 62.5 92.0 47.2% 25% 1 low 96670 47.3 102.3 116.3% 25% 1 high 96670 47.3 90.5 91.3% 50% 1 low 193098 40.0 85.6 114.0% 50% 1 high 193098 40.0 79.6 99.0% 75% 1 low 289765 38.0 82.8 117.9% 75% 1 high 289765 38.0 79.0 107.9% 90% 1 low 347762 37.2 82.7 122.3% 90% 1 high 347762 37.2 72.8 95.7% 95% 1 low 367102 36.5 82.9 127.1% 95% 1 high 367102 36.5 73.1 100.3% 98% 1 low 378721 37.3 81.8 119.3% 98% 1 high 378721 37.3 73.0 95.7% 99% 1 low 382572 36.6 83.3 127.6% 99% 1 high 382572 36.6 71.8 96.2% 99.99999% 1 low 386435 38.0 83.8 120.5% 99.99999% 1 high 386435 38.0 70.9 86.6% 100% 1 low 386435 88.0 89.1 1.2% 100% 1 high 386435 88.0 89.5 1.7% 0% 1-2 low 0 18808.1 71.5 -99.6% 0% 1-2 high 0 18808.1 22.4 -99.9% 1% 1-2 low 5363 46.8 65.2 39.3% 1% 1-2 high 5363 46.8 22.5 -51.9% 2% 1-2 low 10675 37.6 61.4 63.3% 2% 1-2 high 10675 37.6 22.4 -40.4% 5% 1-2 low 26880 28.8 53.4 85.4% 5% 1-2 high 26880 28.8 22.3 -22.6% 10% 1-2 low 53673 23.7 48.1 103.0% 10% 1-2 high 53673 23.7 21.8 -8.0% 25% 1-2 low 133988 19.9 37.2 86.9% 25% 1-2 high 133988 19.9 21.0 5.5% 50% 1-2 low 267757 17.2 27.4 59.3% 50% 1-2 high 267757 17.2 20.3 18.0% 75% 1-2 low 401596 16.9 23.1 36.7% 75% 1-2 high 401596 16.9 20.3 20.1% 90% 1-2 low 481911 17.0 21.2 24.7% 90% 1-2 high 481911 17.0 20.6 21.2% 95% 1-2 low 508704 17.1 20.7 21.1% 95% 1-2 high 508704 17.1 20.7 21.1% 98% 1-2 low 524909 17.3 20.7 19.7% 98% 1-2 high 524909 17.3 20.7 19.7% 99% 1-2 low 530221 17.4 20.5 17.8% 99% 1-2 high 530221 17.4 20.8 19.5% 99.99999% 1-2 low 535584 17.1 20.3 18.7% 99.99999% 1-2 high 535584 17.1 20.3 18.7% 100% 1-2 low 535584 21.0 20.7 -1.4% 100% 1-2 high 535584 21.0 20.8 -1.0% 0% 1-4 low 0 17961.7 42.2 -99.8% 0% 1-4 high 0 17961.7 11.9 -99.9% 1% 1-4 low 6544 27.1 38.4 41.7% 1% 1-4 high 6544 27.1 12.0 -55.7% 2% 1-4 low 13062 21.4 36.0 68.2% 2% 1-4 high 13062 21.4 11.9 -44.4% 5% 1-4 low 32815 16.1 31.3 94.4% 5% 1-4 high 32815 16.1 11.8 -26.7% 10% 1-4 low 65491 13.3 27.8 109.0% 10% 1-4 high 65491 13.3 11.7 -12.0% 25% 1-4 low 163600 10.5 21.0 100.0% 25% 1-4 high 163600 10.5 11.5 9.5% 50% 1-4 low 327302 9.9 15.3 54.5% 50% 1-4 high 327302 9.9 11.2 13.1% 75% 1-4 low 490881 9.7 12.6 29.9% 75% 1-4 high 490881 9.7 11.1 14.4% 90% 1-4 low 588990 9.6 11.6 20.8% 90% 1-4 high 588990 9.6 11.1 15.6% 95% 1-4 low 621666 9.6 11.3 17.7% 95% 1-4 high 621666 9.6 11.2 16.7% 98% 1-4 low 641419 9.4 11.1 18.1% 98% 1-4 high 641419 9.4 11.2 19.1% 99% 1-4 low 647937 9.8 11.1 13.3% 99% 1-4 high 647937 9.8 11.2 14.3% 99.99999% 1-4 low 654481 10.0 11.1 11.0% 99.99999% 1-4 high 654481 10.0 11.2 12.0% 100% 1-4 low 654481 11.3 11.3 0.0% 100% 1-4 high 654481 11.3 11.2 -0.9% 0% 1-10 low 0 15990.0 22.5 -99.9% 0% 1-10 high 0 15990.0 5.8 -100.0% 1% 1-10 low 8406 13.1 20.2 54.2% 1% 1-10 high 8406 13.1 5.8 -55.7% 2% 1-10 low 16756 10.2 18.9 85.3% 2% 1-10 high 16756 10.2 5.8 -43.1% 5% 1-10 low 41937 7.7 16.5 114.3% 5% 1-10 high 41937 7.7 5.7 -26.0% 10% 1-10 low 83828 6.3 14.5 130.2% 10% 1-10 high 83828 6.3 5.7 -9.5% 25% 1-10 low 209328 5.3 10.9 105.7% 25% 1-10 high 209328 5.3 5.7 7.5% 50% 1-10 low 418668 4.9 7.8 59.2% 50% 1-10 high 418668 4.9 5.5 12.2% 75% 1-10 low 628338 4.9 6.4 30.6% 75% 1-10 high 628338 4.9 5.5 12.2% 90% 1-10 low 753838 4.9 5.9 20.4% 90% 1-10 high 753838 4.9 5.5 12.2% 95% 1-10 low 795729 5.0 5.7 14.0% 95% 1-10 high 795729 5.0 5.6 12.0% 98% 1-10 low 820910 5.1 5.6 9.8% 98% 1-10 high 820910 5.1 5.6 9.8% 99% 1-10 low 829260 5.1 5.6 9.8% 99% 1-10 high 829260 5.1 5.6 9.8% 99.99999% 1-10 low 837666 5.2 5.6 7.7% 99.99999% 1-10 high 837666 5.2 5.6 7.7% 100% 1-10 low 837666 5.7 5.7 0.0% 100% 1-10 high 837666 5.7 5.6 -1.8% 0% +1-2 low 0 18848.4 138.4 -99.3% 0% +1-2 high 0 18848.4 27.4 -99.9% 1% +1-2 low 2308 63.1 77.0 22.0% 1% +1-2 high 2308 63.1 27.1 -57.1% 2% +1-2 low 4621 50.3 69.7 38.6% 2% +1-2 high 4621 50.3 26.8 -46.7% 5% +1-2 low 11706 36.1 56.9 57.6% 5% +1-2 high 11706 36.1 26.5 -26.6% 10% +1-2 low 23272 28.4 48.6 71.1% 10% +1-2 high 23272 28.4 26.2 -7.7% 25% +1-2 low 58401 23.7 36.4 53.6% 25% +1-2 high 58401 23.7 24.9 5.1% 50% +1-2 low 117083 20.9 28.2 34.9% 50% +1-2 high 117083 20.9 23.9 14.4% 75% +1-2 low 176233 19.3 24.4 26.4% 75% +1-2 high 176233 19.3 22.8 18.1% 90% +1-2 low 211362 18.6 22.9 23.1% 90% +1-2 high 211362 18.6 22.5 21.0% 95% +1-2 low 222928 18.5 22.5 21.6% 95% +1-2 high 222928 18.5 22.5 21.6% 98% +1-2 low 230013 18.3 22.0 20.2% 98% +1-2 high 230013 18.3 22.4 22.4% 99% +1-2 low 232326 18.3 22.1 20.8% 99% +1-2 high 232326 18.3 22.3 21.9% 99.99999% +1-2 low 234634 17.8 21.9 23.0% 99.99999% +1-2 high 234634 17.8 22.2 24.7% 100% +1-2 low 234634 22.9 22.7 -0.9% 100% +1-2 high 234634 22.9 22.6 -1.3% 0% +1-4 low 0 17987.0 137.9 -99.2% 0% +1-4 high 0 17987.0 18.0 -99.9% 1% +1-4 low 923 34.5 58.4 69.3% 1% +1-4 high 923 34.5 17.9 -48.1% 2% +1-4 low 1849 28.7 51.9 80.8% 2% +1-4 high 1849 28.7 17.9 -37.6% 5% +1-4 low 4794 22.1 39.0 76.5% 5% +1-4 high 4794 22.1 17.8 -19.5% 10% +1-4 low 9595 19.9 35.3 77.4% 10% +1-4 high 9595 19.9 17.5 -12.1% 25% +1-4 low 24136 17.3 25.7 48.6% 25% +1-4 high 24136 17.3 17.2 -0.6% 50% +1-4 low 48328 16.1 19.5 21.1% 50% +1-4 high 48328 16.1 16.8 4.3% 75% +1-4 low 72718 15.8 17.0 7.6% 75% +1-4 high 72718 15.8 16.6 5.1% 90% +1-4 low 87259 15.3 16.3 6.5% 90% +1-4 high 87259 15.3 16.4 7.2% 95% +1-4 low 92060 15.4 16.0 3.9% 95% +1-4 high 92060 15.4 16.4 6.5% 98% +1-4 low 95005 15.4 15.8 2.6% 98% +1-4 high 95005 15.4 16.4 6.5% 99% +1-4 low 95931 15.1 15.7 4.0% 99% +1-4 high 95931 15.1 16.4 8.6% 99.99999% +1-4 low 96854 14.3 15.9 11.2% 99.99999% +1-4 high 96854 14.3 16.2 13.3% 100% +1-4 low 96854 16.6 16.6 0.0% 100% +1-4 high 96854 16.6 16.6 0.0% 0% "u s" low 0 19123.0 124.9 -99.3% 0% "u s" high 0 19123.0 7.0 -100.0% 1% "u s" low 3192 23.5 27.3 16.2% 1% "u s" high 3192 23.5 7.1 -69.8% 2% "u s" low 6179 17.8 24.3 36.5% 2% "u s" high 6179 17.8 7.0 -60.7% 5% "u s" low 15446 12.7 20.3 59.8% 5% "u s" high 15446 12.7 7.0 -44.9% 10% "u s" low 30858 10.1 16.1 59.4% 10% "u s" high 30858 10.1 6.8 -32.7% 25% "u s" low 77138 7.7 13.0 68.8% 25% "u s" high 77138 7.7 6.8 -11.7% 50% "u s" low 154331 6.7 9.9 47.8% 50% "u s" high 154331 6.7 7.0 4.5% 75% "u s" low 231412 6.3 8.4 33.3% 75% "u s" high 231412 6.3 7.0 11.1% 90% "u s" low 277692 5.8 7.2 24.1% 90% "u s" high 277692 5.8 7.0 20.7% 95% "u s" low 293104 5.8 7.1 22.4% 95% "u s" high 293104 5.8 7.0 20.7% 98% "u s" low 302371 5.8 7.0 20.7% 98% "u s" high 302371 5.8 6.9 19.0% 99% "u s" low 305358 5.8 6.9 19.0% 99% "u s" high 305358 5.8 6.9 19.0% 99.99999% "u s" low 308550 5.8 6.8 17.2% 99.99999% "u s" high 308550 5.8 6.9 19.0% 100% "u s" low 308550 7.0 6.9 -1.4% 100% "u s" high 308550 7.0 6.9 -1.4%
          Hide
          Michael McCandless added a comment -

          Attaching patch I'm using to run tests. NOTE: this is nowhere near
          committable. It's just a hack to allow testing the different ways of
          applying filters.

          Show
          Michael McCandless added a comment - Attaching patch I'm using to run tests. NOTE: this is nowhere near committable. It's just a hack to allow testing the different ways of applying filters.
          Hide
          Michael McCandless added a comment -

          It's a ridiculous amount of data to digest, but here are some initial
          observations/thoughts:

          • There are very sizable gains here by switching to random-access
            low (ie, handling top-level filter the way we now handle deletes).
            I'm especially interested in gains in the slowest queries. EG the
            phrase query "united states" sees QPS gains from 16%-69%. The
            10-clause OR query 1-10 sees QPS gains between 8% and 130%.
          • Results are consistent with LUCENE-1476: random-access low gives
            the best performance when filter density is >= 1%.
          • High is worse than trunk up until ~25% density, which makes sense
            since we are asking Scorer to do alot of work producing docIDs
            that we then nix with the filter.
          • Low is consistently better than high, though as filter density
            gets higher the gap between them narrows. I'll drop high from
            future tests.
          • The gains are generally strongest in the "moderate" density range,
            5-25%.
          • The degenerate 0% case is clearly far far worse, which is expected
            since the iterator scans the bits, finds none set, and quickly
            ends the search. For very low density filters we should continue
            to use iterator.
          • The "control" 100% case (where filter is null) is about the same,
            which is expected.
          Show
          Michael McCandless added a comment - It's a ridiculous amount of data to digest, but here are some initial observations/thoughts: There are very sizable gains here by switching to random-access low (ie, handling top-level filter the way we now handle deletes). I'm especially interested in gains in the slowest queries. EG the phrase query "united states" sees QPS gains from 16%-69%. The 10-clause OR query 1-10 sees QPS gains between 8% and 130%. Results are consistent with LUCENE-1476 : random-access low gives the best performance when filter density is >= 1%. High is worse than trunk up until ~25% density, which makes sense since we are asking Scorer to do alot of work producing docIDs that we then nix with the filter. Low is consistently better than high, though as filter density gets higher the gap between them narrows. I'll drop high from future tests. The gains are generally strongest in the "moderate" density range, 5-25%. The degenerate 0% case is clearly far far worse, which is expected since the iterator scans the bits, finds none set, and quickly ends the search. For very low density filters we should continue to use iterator. The "control" 100% case (where filter is null) is about the same, which is expected.
          Hide
          Michael McCandless added a comment -

          Some ideas / further things to explore:

          • Deletions, top-level filters, and BooleanQuery that "factors" to a
            toplevel AND really should be handled by the same code.
          • Even an AND'd filter on a sub-clause of a BooleanQuery can be
            pushed down to the TermDocs under that tree.
          • That common code should send a top-level filter down to the lowest
            level, used by random access API, if the filter supports random
            access (not all do) and it's not super sparse.
          • I think one thing slowing down trunk is the lack of a
            Scorer.skipToButNotNext API. We now ask the filter for its
            next(), which gives us a filterDocID. Then we call
            Scorer.skipTo(filterDocID). If the scorer does not match that
            filterDocID, it internally does next(), which for an expensive
            scorer is alot of likely wasted work: it advances to a docID that
            the filter may not accept. If we had a "skipToButNotNext" API we
            could avoid that wasted work. I'm curious what gains this change
            alone would provide.
          • I'm thinking (but haven't tested this) if the filter is relatively
            sparse compared to the other iterators, it'd be better to convert
            it to a sparse repr (eg SortedVIntList) and drive the search by
            iteration through the filter, after fixing the above skipTo issue.
            Maybe a "low iterator" access.
          • We may need a "filter optimizer" utility class somewhere, somehow.
            For filters you do not plan to re-use, you would not bother with
            this. But for filters that will be re-used, you should 1) convert
            them to sparse or non-sparse repr depending on their density, 2)
            maybe invert them and make sparse if they are close to 100%
            density, 3) maybe factor in deletions to the filter so there is
            only a single top-level filter to apply.
          • I'm not yet sure how to make this change cleanly to the APIs...
          Show
          Michael McCandless added a comment - Some ideas / further things to explore: Deletions, top-level filters, and BooleanQuery that "factors" to a toplevel AND really should be handled by the same code. Even an AND'd filter on a sub-clause of a BooleanQuery can be pushed down to the TermDocs under that tree. That common code should send a top-level filter down to the lowest level, used by random access API, if the filter supports random access (not all do) and it's not super sparse. I think one thing slowing down trunk is the lack of a Scorer.skipToButNotNext API. We now ask the filter for its next(), which gives us a filterDocID. Then we call Scorer.skipTo(filterDocID). If the scorer does not match that filterDocID, it internally does next(), which for an expensive scorer is alot of likely wasted work: it advances to a docID that the filter may not accept. If we had a "skipToButNotNext" API we could avoid that wasted work. I'm curious what gains this change alone would provide. I'm thinking (but haven't tested this) if the filter is relatively sparse compared to the other iterators, it'd be better to convert it to a sparse repr (eg SortedVIntList) and drive the search by iteration through the filter, after fixing the above skipTo issue. Maybe a "low iterator" access. We may need a "filter optimizer" utility class somewhere, somehow. For filters you do not plan to re-use, you would not bother with this. But for filters that will be re-used, you should 1) convert them to sparse or non-sparse repr depending on their density, 2) maybe invert them and make sparse if they are close to 100% density, 3) maybe factor in deletions to the filter so there is only a single top-level filter to apply. I'm not yet sure how to make this change cleanly to the APIs...
          Hide
          Michael McCandless added a comment -

          OK I tested a different approach for matching when the filter is
          relatively sparse filter (<= 10% of index size), by implementing a
          simplistic prototype "random access" scorer API (JumpScorer), which
          only exposes the method "boolean jump(int docID)" that returns true if
          that doc matches, else false (and no next() under the hood).

          I only implemented JumpScorer for pure AND/OR queries (ie no excluded
          terms), so I only test for these queries.

          I convert the filter to SortedVIntList up front, so iteration is fast.
          Then during matching I iterate through each doc in the filter, and ask
          the random-access scoring API to test whether it accepts the doc, and
          collect it if so.

          This only performs better when the filter is sparse relative to the
          query. Once we merge Query/Filter, I think this optimization can more
          generally be used whenever one sub-query is very restrictive compared
          to the rest of the sub-queries.

          This is basically the reverse of what I first tested, which was to
          take a filter that can support random access API and "distribute" it
          down to each TermQuery, which gives very good gains especially when
          filter is in the middle of the sparse/dense range. Whereas this test
          keeps the iterator API on the filter, but switches to a random access
          API on the scorer.

          Results:

          %tg Filter Query Hits QPS QPSNew %tg change
          1% 1-2 5363 47.4 66.7 40.7%
          2% 1-2 10675 37.6 50.6 34.6%
          5% 1-2 26880 28.6 37.0 29.4%
          10% 1-2 53673 23.8 26.2 10.1%
          1% 1-4 6544 26.9 37.2 38.3%
          2% 1-4 13062 21.2 29.2 37.7%
          5% 1-4 32815 16.1 21.2 31.7%
          10% 1-4 65491 13.2 15.2 15.2%
          1% 1-10 8406 13.0 17.6 35.4%
          2% 1-10 16756 10.2 14.2 39.2%
          5% 1-10 41937 7.7 10.3 33.8%
          10% 1-10 83828 6.3 7.8 23.8%
          1% +1-2 2308 63.6 82.9 30.3%
          2% +1-2 4621 49.9 60.7 21.6%
          5% +1-2 11706 35.8 47.6 33.0%
          10% +1-2 23272 28.3 35.5 25.4%
          1% +1-4 923 34.4 58.0 68.6%
          2% +1-4 1849 28.5 44.9 57.5%
          5% +1-4 4794 22.0 33.7 53.2%
          10% +1-4 9595 19.8 25.4 28.3%
          1% +1-10 292 17.0 36.6 115.3%
          2% +1-10 579 15.2 30.2 98.7%
          5% +1-10 1517 13.5 22.2 64.4%
          10% +1-10 2999 12.4 17.4 40.3%
          Show
          Michael McCandless added a comment - OK I tested a different approach for matching when the filter is relatively sparse filter (<= 10% of index size), by implementing a simplistic prototype "random access" scorer API (JumpScorer), which only exposes the method "boolean jump(int docID)" that returns true if that doc matches, else false (and no next() under the hood). I only implemented JumpScorer for pure AND/OR queries (ie no excluded terms), so I only test for these queries. I convert the filter to SortedVIntList up front, so iteration is fast. Then during matching I iterate through each doc in the filter, and ask the random-access scoring API to test whether it accepts the doc, and collect it if so. This only performs better when the filter is sparse relative to the query. Once we merge Query/Filter, I think this optimization can more generally be used whenever one sub-query is very restrictive compared to the rest of the sub-queries. This is basically the reverse of what I first tested, which was to take a filter that can support random access API and "distribute" it down to each TermQuery, which gives very good gains especially when filter is in the middle of the sparse/dense range. Whereas this test keeps the iterator API on the filter, but switches to a random access API on the scorer. Results: %tg Filter Query Hits QPS QPSNew %tg change 1% 1-2 5363 47.4 66.7 40.7% 2% 1-2 10675 37.6 50.6 34.6% 5% 1-2 26880 28.6 37.0 29.4% 10% 1-2 53673 23.8 26.2 10.1% 1% 1-4 6544 26.9 37.2 38.3% 2% 1-4 13062 21.2 29.2 37.7% 5% 1-4 32815 16.1 21.2 31.7% 10% 1-4 65491 13.2 15.2 15.2% 1% 1-10 8406 13.0 17.6 35.4% 2% 1-10 16756 10.2 14.2 39.2% 5% 1-10 41937 7.7 10.3 33.8% 10% 1-10 83828 6.3 7.8 23.8% 1% +1-2 2308 63.6 82.9 30.3% 2% +1-2 4621 49.9 60.7 21.6% 5% +1-2 11706 35.8 47.6 33.0% 10% +1-2 23272 28.3 35.5 25.4% 1% +1-4 923 34.4 58.0 68.6% 2% +1-4 1849 28.5 44.9 57.5% 5% +1-4 4794 22.0 33.7 53.2% 10% +1-4 9595 19.8 25.4 28.3% 1% +1-10 292 17.0 36.6 115.3% 2% +1-10 579 15.2 30.2 98.7% 5% +1-10 1517 13.5 22.2 64.4% 10% +1-10 2999 12.4 17.4 40.3%
          Hide
          Paul Elschot added a comment -

          For the skipToButNotNext() did you mean sth like LUCENE-1252 ?

          Show
          Paul Elschot added a comment - For the skipToButNotNext() did you mean sth like LUCENE-1252 ?
          Hide
          Michael McCandless added a comment -

          Ahh, that's actually something different but also a neat idea to
          explore.

          I want a way to skipTo(docID) without having it then internally do a
          next() if the docID was not accepted. Basically a random-access
          "accepts(int docID)" API, that's called only on increasing docIDs.
          Implementing "accepts" for queries is often alot simpler than
          implementing next/skipTo.

          LUCENE-1252 wants a way to expose access to the two constraints within
          a single query separately. EG a phrase search 1) must have all N
          terms, and 2) must have them in the right positions. But if you could
          check only 1), and if it passes next check the filter on the search,
          and if it still passes go back and check 2), then that could give
          better search performance.

          I think there's decent room for improving search performance of
          complex queries.

          Show
          Michael McCandless added a comment - Ahh, that's actually something different but also a neat idea to explore. I want a way to skipTo(docID) without having it then internally do a next() if the docID was not accepted. Basically a random-access "accepts(int docID)" API, that's called only on increasing docIDs. Implementing "accepts" for queries is often alot simpler than implementing next/skipTo. LUCENE-1252 wants a way to expose access to the two constraints within a single query separately. EG a phrase search 1) must have all N terms, and 2) must have them in the right positions. But if you could check only 1), and if it passes next check the filter on the search, and if it still passes go back and check 2), then that could give better search performance. I think there's decent room for improving search performance of complex queries.
          Hide
          Michael McCandless added a comment -

          Given the sizable performance gains, I think we should try to do this for 2.9, if possible.

          Show
          Michael McCandless added a comment - Given the sizable performance gains, I think we should try to do this for 2.9, if possible.
          Hide
          Uwe Schindler added a comment -

          How about DocIdSet adds a

          boolean isRandomAccess() { return false; }
          

          That is implemented to return false in the default abstract class for backwards compatibility.
          If a DocIdSet is random access (backed by OpenBitSet or is the empty iterator), isRandomAccess() is overridden to return true and an additional method in DocIdSet is implemented, the default would be:

          boolean acceptDoc(int docid) { throw new UnsupportedOperationException(); }
          

          Both changes are backwards compatible, but filters using OpenBitSet would automatically be random access and support acceptDoc().

          Show
          Uwe Schindler added a comment - How about DocIdSet adds a boolean isRandomAccess() { return false ; } That is implemented to return false in the default abstract class for backwards compatibility. If a DocIdSet is random access (backed by OpenBitSet or is the empty iterator), isRandomAccess() is overridden to return true and an additional method in DocIdSet is implemented, the default would be: boolean acceptDoc( int docid) { throw new UnsupportedOperationException(); } Both changes are backwards compatible, but filters using OpenBitSet would automatically be random access and support acceptDoc().
          Hide
          Uwe Schindler added a comment - - edited

          The empty docidset instance should not be random access , so the only change would affect OpenBitSet to overwrite these two new methods from the default abstract class:

          boolean isRandomAccess() { return true; }
          boolean acceptDoc(int docid) { return fastGet(docid); /* possibly inlined */ }
          
          Show
          Uwe Schindler added a comment - - edited The empty docidset instance should not be random access , so the only change would affect OpenBitSet to overwrite these two new methods from the default abstract class: boolean isRandomAccess() { return true ; } boolean acceptDoc( int docid) { return fastGet(docid); /* possibly inlined */ }
          Hide
          Uwe Schindler added a comment -

          And the switch for different densities:
          OpenBitSet could calculate its density in isRandomAccess() and return true or false depending on the density factors above. The search code then would only check initially isRandomAccess() (before starting filtering) and then switch between iterator or random acess api.

          Show
          Uwe Schindler added a comment - And the switch for different densities: OpenBitSet could calculate its density in isRandomAccess() and return true or false depending on the density factors above. The search code then would only check initially isRandomAccess() (before starting filtering) and then switch between iterator or random acess api.
          Hide
          Michael McCandless added a comment -

          I like this approach!

          But should we somehow decouple the density check vs the is random access check? Ie, isRandomAccess should return true or false based on the underlying datastructure. Then, somehow, I think the search code should determine whether a given docIdSet should be randomly accessed vs iterated? (I'm not sure how yet!)

          Also, we somehow need the mechanism to "denormalize" the application of the filter from top to bottom, ie, each leaf TermQuery involved in the full query needs to know to apply the random access filter just like it applies deletes.

          Show
          Michael McCandless added a comment - I like this approach! But should we somehow decouple the density check vs the is random access check? Ie, isRandomAccess should return true or false based on the underlying datastructure. Then, somehow, I think the search code should determine whether a given docIdSet should be randomly accessed vs iterated? (I'm not sure how yet!) Also, we somehow need the mechanism to "denormalize" the application of the filter from top to bottom, ie, each leaf TermQuery involved in the full query needs to know to apply the random access filter just like it applies deletes.
          Hide
          Uwe Schindler added a comment -

          I coupled the density check inside the OpenBitSet, because the internals of OpenBitset are responsible for determining how fast a sequential vs. random approach is. Maybe someone invents an new hyper-bitset that can faster do sequential accesses even in sparingly filled bitsets (e.g. fragmented bitset, bitset with RDBMS-like "index"). In this case, it has the responsibility to say: if density is between this and this i would use sequential.

          Show
          Uwe Schindler added a comment - I coupled the density check inside the OpenBitSet, because the internals of OpenBitset are responsible for determining how fast a sequential vs. random approach is. Maybe someone invents an new hyper-bitset that can faster do sequential accesses even in sparingly filled bitsets (e.g. fragmented bitset, bitset with RDBMS-like "index"). In this case, it has the responsibility to say: if density is between this and this i would use sequential.
          Hide
          Michael McCandless added a comment -

          OK, if we do choose to couple, maybe we should name it "useRandomAccess()"?

          Another filter optimization that'd be nice to get in is to somehow "know" that a filter has pre-incorporated deleted documents. This way, once we have a solution for the "push filter down to all TermScorers", we could have it only check the filter and not also deleted docs. (This is one of the optimizations in LUCENE-1594).

          We might eventually want/need some sort of external FilterManager that would handle this (ie, convert a filter to sparse vs random-access as appropriate, multiply in deleted docs, handle caching, etc).

          Show
          Michael McCandless added a comment - OK, if we do choose to couple, maybe we should name it "useRandomAccess()"? Another filter optimization that'd be nice to get in is to somehow "know" that a filter has pre-incorporated deleted documents. This way, once we have a solution for the "push filter down to all TermScorers", we could have it only check the filter and not also deleted docs. (This is one of the optimizations in LUCENE-1594 ). We might eventually want/need some sort of external FilterManager that would handle this (ie, convert a filter to sparse vs random-access as appropriate, multiply in deleted docs, handle caching, etc).
          Hide
          Jason Rutherglen added a comment -

          I thought we are going to get LUCENE-1518 working to compare the performance against passing the filter into TermDocs?

          Show
          Jason Rutherglen added a comment - I thought we are going to get LUCENE-1518 working to compare the performance against passing the filter into TermDocs?
          Hide
          Michael McCandless added a comment -

          Ahh right, we should re-test performance of this after LUCENE-1518 is done.

          Show
          Michael McCandless added a comment - Ahh right, we should re-test performance of this after LUCENE-1518 is done.
          Hide
          Jason Rutherglen added a comment - - edited

          Perhaps we can go ahead with this patch given we're not sure how
          to do an optimized version of LUCENE-1518 yet. This patch
          entails passing the RandomAccessFilter to TermScorer, what's a
          good way to do this without rewriting too much of the Lucene API?

          • TermQuery.createWeight -> TermWeight.scorer instantiates the
            TermScorer which is where we need to pass in the filter? So we
            could somehow pass the filter in via multiple constructors? I
            didn't see a clean API way though.
          • Or we can add a new method to Scorer, something like
            getSequentialSubScorers? Which we then iterate over and if one
            is a TermScorer set the filter(s). This setting of the RAF would
            happen in IndexSearcher.doSearch.
          Show
          Jason Rutherglen added a comment - - edited Perhaps we can go ahead with this patch given we're not sure how to do an optimized version of LUCENE-1518 yet. This patch entails passing the RandomAccessFilter to TermScorer, what's a good way to do this without rewriting too much of the Lucene API? TermQuery.createWeight -> TermWeight.scorer instantiates the TermScorer which is where we need to pass in the filter? So we could somehow pass the filter in via multiple constructors? I didn't see a clean API way though. Or we can add a new method to Scorer, something like getSequentialSubScorers? Which we then iterate over and if one is a TermScorer set the filter(s). This setting of the RAF would happen in IndexSearcher.doSearch.
          Hide
          Jason Rutherglen added a comment -

          The patch is a start at something that will work within the API
          (I think).

          • Created a RandomAccessDocIdSet class that BitVector and
            OpenBitSet implement.
          • SegmentTermDocs has a setter for RandomAccessDocIdSet with the
            option of including the deletedDocs
          • IndexSearcher.doSearch does the main work of setting the
            RandomAccessDocIdSet on the TermScorers
          • BooleanScorer2 and other classes implement
            getSequentialSubScorers
          • AndRandomAccessDocIdSet iterates over an array of
            RandomAccessDocIdSets
          Show
          Jason Rutherglen added a comment - The patch is a start at something that will work within the API (I think). Created a RandomAccessDocIdSet class that BitVector and OpenBitSet implement. SegmentTermDocs has a setter for RandomAccessDocIdSet with the option of including the deletedDocs IndexSearcher.doSearch does the main work of setting the RandomAccessDocIdSet on the TermScorers BooleanScorer2 and other classes implement getSequentialSubScorers AndRandomAccessDocIdSet iterates over an array of RandomAccessDocIdSets
          Hide
          Jason Rutherglen added a comment -
          • Filter has a parameter for included deletes. IndexSearcher
            uses it for setting the RandomAccessDocIdSet on the Scorers
          • MatchAllDocsScorer in addition to TermScorer properly supports
            setting a RandomAccessDocIdSet
          • Added more test cases
          • AndRandomAccessDocIdSet.iterator() and BitVector.iterator()
            needs to be implemented
          Show
          Jason Rutherglen added a comment - Filter has a parameter for included deletes. IndexSearcher uses it for setting the RandomAccessDocIdSet on the Scorers MatchAllDocsScorer in addition to TermScorer properly supports setting a RandomAccessDocIdSet Added more test cases AndRandomAccessDocIdSet.iterator() and BitVector.iterator() needs to be implemented
          Hide
          Yonik Seeley added a comment -

          Interesting stuff!
          Has anyone tested if this results in a performance degradation on SegmentTermDocs?
          This is very inner loop stuff, and it's replacing a "non virtual" BitVector.get() which can be easily inlined with two dispatches through base classes. Hopefully hotspot could handle it, but it's tough to figure out, esp in a real system where sometimes a user RAF will be used and sometimes not.

          Show
          Yonik Seeley added a comment - Interesting stuff! Has anyone tested if this results in a performance degradation on SegmentTermDocs? This is very inner loop stuff, and it's replacing a "non virtual" BitVector.get() which can be easily inlined with two dispatches through base classes. Hopefully hotspot could handle it, but it's tough to figure out, esp in a real system where sometimes a user RAF will be used and sometimes not.
          Hide
          Michael McCandless added a comment -

          Has anyone tested if this results in a performance degradation on SegmentTermDocs?

          I haven't, and I'm also nervous.

          One thing we could do is require a concrete impl (eg OpenBitSet) in order to use this optimization?

          I also think we should require pre-multiplication of the deleted docs, and possibly inversion of the filter, rather than doing it on the fly per docID. Also, we shouldn't negate the deleted docs per doc when there's no RAF. We may want/need to switch to OpenBitSet for deleted docs, in order to not make 2 copies of the STD code.

          Show
          Michael McCandless added a comment - Has anyone tested if this results in a performance degradation on SegmentTermDocs? I haven't, and I'm also nervous. One thing we could do is require a concrete impl (eg OpenBitSet) in order to use this optimization? I also think we should require pre-multiplication of the deleted docs, and possibly inversion of the filter, rather than doing it on the fly per docID. Also, we shouldn't negate the deleted docs per doc when there's no RAF. We may want/need to switch to OpenBitSet for deleted docs, in order to not make 2 copies of the STD code.
          Hide
          Michael McCandless added a comment -

          I don't think it should be the caller's job to getSequentialSubScorers
          and push down a RAF? Rather, I think when requesting a scorer we
          should pass in a RAF, requiring that the returned scorer factors it
          in (passing on to its own sub-scorers if needed).

          Show
          Michael McCandless added a comment - I don't think it should be the caller's job to getSequentialSubScorers and push down a RAF? Rather, I think when requesting a scorer we should pass in a RAF, requiring that the returned scorer factors it in (passing on to its own sub-scorers if needed).
          Hide
          Michael McCandless added a comment -

          I don't think it should be the caller's job to getSequentialSubScorers
          and push down a RAF? Rather, I think when requesting a scorer we
          should pass in a RAF, requiring that the returned scorer factors it
          in (passing on to its own sub-scorers if needed).

          One thing we could do is require a concrete impl (eg OpenBitSet) in order to use this optimization?

          This may be too restrictive, because another use case (touched on
          already in LUCENE-1593, but actually much more similar to this issue)
          is when sorting by field.

          EG say we are sorting by int ascending, and from the
          FieldValueHitQueue we know the bottom value is 17. Then, within
          TermScorer if we see a docID we should check if its value is greater
          than 17 and skip it if so.

          Very likely this will be a sizable performance gain, but it would be a
          major change because you can only do this if you do not need the
          precise totalHits back.

          So... maybe we need to allow an abstract RandomAccesDocIdSet, to allow
          this use case. But perhaps we should negate its API? Ie it exposes
          "boolean reject(int docID)".

          Show
          Michael McCandless added a comment - I don't think it should be the caller's job to getSequentialSubScorers and push down a RAF? Rather, I think when requesting a scorer we should pass in a RAF, requiring that the returned scorer factors it in (passing on to its own sub-scorers if needed). One thing we could do is require a concrete impl (eg OpenBitSet) in order to use this optimization? This may be too restrictive, because another use case (touched on already in LUCENE-1593 , but actually much more similar to this issue) is when sorting by field. EG say we are sorting by int ascending, and from the FieldValueHitQueue we know the bottom value is 17. Then, within TermScorer if we see a docID we should check if its value is greater than 17 and skip it if so. Very likely this will be a sizable performance gain, but it would be a major change because you can only do this if you do not need the precise totalHits back. So... maybe we need to allow an abstract RandomAccesDocIdSet, to allow this use case. But perhaps we should negate its API? Ie it exposes "boolean reject(int docID)".
          Hide
          Marvin Humphrey added a comment -

          > it would be amajor change because you can only do this if
          > you do not need the precise totalHits back.

          Early termination (pruning) also messes with totalHits. I think
          it would be good to get away from the idea that totalHits is
          reliable side effect.

          Show
          Marvin Humphrey added a comment - > it would be amajor change because you can only do this if > you do not need the precise totalHits back. Early termination (pruning) also messes with totalHits. I think it would be good to get away from the idea that totalHits is reliable side effect.
          Hide
          Michael McCandless added a comment -

          Early termination (pruning) also messes with totalHits. I think
          it would be good to get away from the idea that totalHits is
          reliable side effect.

          Agreed. Does KS/Lucy not guarantee accurate totalHits returned? Lucene today always returns the precise total hits.

          If we can move away from that (optionally) then we can gain sizable performance.

          But: we'd then presumably need to return an approx hit count.

          Show
          Michael McCandless added a comment - Early termination (pruning) also messes with totalHits. I think it would be good to get away from the idea that totalHits is reliable side effect. Agreed. Does KS/Lucy not guarantee accurate totalHits returned? Lucene today always returns the precise total hits. If we can move away from that (optionally) then we can gain sizable performance. But: we'd then presumably need to return an approx hit count.
          Hide
          Michael McCandless added a comment -

          Moving out.

          Show
          Michael McCandless added a comment - Moving out.
          Hide
          Jason Rutherglen added a comment -

          I don't think it should be the caller's job to
          getSequentialSubScorers and push down a RAF? Rather, I think
          when requesting a scorer we should pass in a RAF, requiring that
          the returned scorer factors it in (passing on to its own
          sub-scorers if needed).

          True, however that requires adding a scorer(IndexReader, RAF)
          method to the Weight interface? Which means adding it to I think
          20 classes or so. Which is fine, however is that definitely what
          we want to do?

          Also I forget if we created a patch or benchmarked AND NOT
          deleteDocs?

          Show
          Jason Rutherglen added a comment - I don't think it should be the caller's job to getSequentialSubScorers and push down a RAF? Rather, I think when requesting a scorer we should pass in a RAF, requiring that the returned scorer factors it in (passing on to its own sub-scorers if needed). True, however that requires adding a scorer(IndexReader, RAF) method to the Weight interface? Which means adding it to I think 20 classes or so. Which is fine, however is that definitely what we want to do? Also I forget if we created a patch or benchmarked AND NOT deleteDocs?
          Hide
          Michael McCandless added a comment -

          that requires adding a scorer(IndexReader, RAF) method to the Weight interface?

          Well, to QueryWeight (abstract class, not interface) that we are migrating to with LUCENE-1630. We'd make a default impl, probably a static method somewhere, so that any external QueryWeight's "out there" would just work.

          Though, since QueryWeight is new in 2.9, we are free to make this an abstract method, and put the default only in QueryWeightWrapper, if we could get this issue done for 2.9.

          I think we'd also want control on whether the Scorer should apply deletes or not, so that for filters that are often re-used, we have the option to pre-fold deletes in.

          Show
          Michael McCandless added a comment - that requires adding a scorer(IndexReader, RAF) method to the Weight interface? Well, to QueryWeight (abstract class, not interface) that we are migrating to with LUCENE-1630 . We'd make a default impl, probably a static method somewhere, so that any external QueryWeight's "out there" would just work. Though, since QueryWeight is new in 2.9, we are free to make this an abstract method, and put the default only in QueryWeightWrapper, if we could get this issue done for 2.9. I think we'd also want control on whether the Scorer should apply deletes or not, so that for filters that are often re-used, we have the option to pre-fold deletes in.
          Hide
          Jason Rutherglen added a comment -

          since QueryWeight is new in 2.9, we are free to make
          this an abstract method, and put the default only in
          QueryWeightWrapper,

          Sounds good.

          we'd also want control on whether the Scorer should
          apply deletes or not

          Filter.hasDeletes can be used know whether to apply deletes or
          not.

          migrating to with LUCENE-1630

          Looks this this will be committed soon, so I'll wait for that
          then make the changes, then benchmark.

          Show
          Jason Rutherglen added a comment - since QueryWeight is new in 2.9, we are free to make this an abstract method, and put the default only in QueryWeightWrapper, Sounds good. we'd also want control on whether the Scorer should apply deletes or not Filter.hasDeletes can be used know whether to apply deletes or not. migrating to with LUCENE-1630 Looks this this will be committed soon, so I'll wait for that then make the changes, then benchmark.
          Hide
          Jason Rutherglen added a comment -
          • Added a IndexReader.termDocs(term, filter) method which
            culminates in passing a RandomAccessDocIdSet to the scorers.
            ConstantScoreQuery and FilterQuery need to AND the filters
            together.
          • Does explain need the filter?
          • Spans aren't supported yet
          Show
          Jason Rutherglen added a comment - Added a IndexReader.termDocs(term, filter) method which culminates in passing a RandomAccessDocIdSet to the scorers. ConstantScoreQuery and FilterQuery need to AND the filters together. Does explain need the filter? Spans aren't supported yet
          Hide
          Jason Rutherglen added a comment -
          • The test case includes testConstantCoreQuery
          • If LUCENE-1632 were updated to use the new DocIdSetIterator API,
            it could be useful for the iterators of AndRandomAccessDocIdSet
            and NotRandomAccessDocIdSet.
          Show
          Jason Rutherglen added a comment - The test case includes testConstantCoreQuery If LUCENE-1632 were updated to use the new DocIdSetIterator API, it could be useful for the iterators of AndRandomAccessDocIdSet and NotRandomAccessDocIdSet.
          Hide
          Michael McCandless added a comment -

          With flex, you can now get the deleted docs from a reader (returns a Bits, yet another interface for bitsets), and, Docs/AndPositionsEnums require that you pass in the skipDocs. Ie they no longer skip deleted docs by default.

          I think this makes this issue quite a bit simpler? EG OpenBitSets implements Bits (as of flex landing), so... we just need a way to pass this Bits down to the low level scorers that actually pull a postings list. But, we should only do this if the filter is not sparse.

          Also: the filter must be inverted, and, ORd with the deleted docs.

          This can result in enormous perf gains for searches doing filtering where the filter is relatively static (can be cached & shared across searches).

          Show
          Michael McCandless added a comment - With flex, you can now get the deleted docs from a reader (returns a Bits, yet another interface for bitsets), and, Docs/AndPositionsEnums require that you pass in the skipDocs. Ie they no longer skip deleted docs by default. I think this makes this issue quite a bit simpler? EG OpenBitSets implements Bits (as of flex landing), so... we just need a way to pass this Bits down to the low level scorers that actually pull a postings list. But, we should only do this if the filter is not sparse. Also: the filter must be inverted, and, ORd with the deleted docs. This can result in enormous perf gains for searches doing filtering where the filter is relatively static (can be cached & shared across searches).
          Hide
          Michael McCandless added a comment -

          By subclassing FilterIndexReader, and taking advantage of how the flex
          APIs now let you pass a custom skipDocs when pulling the postings, I
          created a prototype class (attached, named CachedFilterIndexReader) that
          up-front compiles the deleted docs for each segment with the negation
          of a Filter (that you provide), and returns a reader that applies that
          filter.

          This is nice because it's fully external to Lucene, and it gives
          awesome gains in many cases (see
          http://chbits.blogspot.com/2010/09/fast-search-filters-using-flex.html).

          I don't think we should commit this class – we should instead fix
          Filters correctly! But it's a nice workaround until we do that.

          Show
          Michael McCandless added a comment - By subclassing FilterIndexReader, and taking advantage of how the flex APIs now let you pass a custom skipDocs when pulling the postings, I created a prototype class (attached, named CachedFilterIndexReader) that up-front compiles the deleted docs for each segment with the negation of a Filter (that you provide), and returns a reader that applies that filter. This is nice because it's fully external to Lucene, and it gives awesome gains in many cases (see http://chbits.blogspot.com/2010/09/fast-search-filters-using-flex.html ). I don't think we should commit this class – we should instead fix Filters correctly! But it's a nice workaround until we do that.
          Hide
          Shay Banon added a comment -

          Hi Mike,

          Wondering what are your thoughts on fixing filters correctly are? I think that the initial thought of getting filters all the way down to postings enumeration if they support random access is a great one. A random access doc id set can be added (interface), and if a filter returns it (can be checked using instanceof), then the that doc set can be passed all the way to the enumeration (and intersected per doc with the deleted docs).

          I think that any type of solution should support the great feature of Lucene queries, for example, FilteredQuery should use that, allowing to build complex query expressions without having the mentioned optimization only applied on the top level search.

          As most filters results do support random access, either because they use OpenBitSet, or because they are built on top of FieldCache functionality, I think this feature will give great speed improvements to the query execution time.

          Show
          Shay Banon added a comment - Hi Mike, Wondering what are your thoughts on fixing filters correctly are? I think that the initial thought of getting filters all the way down to postings enumeration if they support random access is a great one. A random access doc id set can be added (interface), and if a filter returns it (can be checked using instanceof), then the that doc set can be passed all the way to the enumeration (and intersected per doc with the deleted docs). I think that any type of solution should support the great feature of Lucene queries, for example, FilteredQuery should use that, allowing to build complex query expressions without having the mentioned optimization only applied on the top level search. As most filters results do support random access, either because they use OpenBitSet, or because they are built on top of FieldCache functionality, I think this feature will give great speed improvements to the query execution time.
          Hide
          Michael McCandless added a comment -

          Wondering what are your thoughts on fixing filters correctly are?

          I think the approach you outlined is the right one!

          We already have the APIs in flex (Bits interface for random access, postings APIs take a Bits skipDocs); in backporting to 3.x I think we'd just port Bits back.

          There are some challenges though:

          • We should add a method to Filter to ask it if its already folded in deleted docs or not. So eg if a Filter is random access but doesn't factor in del docs we'd have to wrap it so that every random access check also checks del docs ("AND NOT deleted.get(docID)").
          • We need a coarse heuristic in IndexSearcher to decide when a filter "merits" down low application. Ie, even if a filter is random access, if it's rather sparse (< 1% or 2% or something) it's better to apply it the way we do today ("up high"). In the current patch it's too coarse (it's either globally on or off); it should be based on the filter instead, or maybe the filter provides a method and that method defaults to the 1/2% threshold check.
          • I suspect we should invert the "Bits skipDocs" now passed to the flex APIs, to be "Bits acceptDocs" instead, so that we don't have to invert every filter. This'd also mean changing IndexReader.getDeletedDocs to IndexReader.getNotDeleteDocs.

          Then I think we simply pass the Bits filter into the Weight.scorer API.

          I think that any type of solution should support the great feature of Lucene queries, for example, FilteredQuery should use that, allowing to build complex query expressions without having the mentioned optimization only applied on the top level search.

          Good point – FilteredQuery should use this same low level API if its filter is random access and "dense enough".

          As most filters results do support random access, either because they use OpenBitSet, or because they are built on top of FieldCache functionality, I think this feature will give great speed improvements to the query execution time.

          Right, the speed gains are often awesome!

          Show
          Michael McCandless added a comment - Wondering what are your thoughts on fixing filters correctly are? I think the approach you outlined is the right one! We already have the APIs in flex (Bits interface for random access, postings APIs take a Bits skipDocs); in backporting to 3.x I think we'd just port Bits back. There are some challenges though: We should add a method to Filter to ask it if its already folded in deleted docs or not. So eg if a Filter is random access but doesn't factor in del docs we'd have to wrap it so that every random access check also checks del docs ("AND NOT deleted.get(docID)"). We need a coarse heuristic in IndexSearcher to decide when a filter "merits" down low application. Ie, even if a filter is random access, if it's rather sparse (< 1% or 2% or something) it's better to apply it the way we do today ("up high"). In the current patch it's too coarse (it's either globally on or off); it should be based on the filter instead, or maybe the filter provides a method and that method defaults to the 1/2% threshold check. I suspect we should invert the "Bits skipDocs" now passed to the flex APIs, to be "Bits acceptDocs" instead, so that we don't have to invert every filter. This'd also mean changing IndexReader.getDeletedDocs to IndexReader.getNotDeleteDocs. Then I think we simply pass the Bits filter into the Weight.scorer API. I think that any type of solution should support the great feature of Lucene queries, for example, FilteredQuery should use that, allowing to build complex query expressions without having the mentioned optimization only applied on the top level search. Good point – FilteredQuery should use this same low level API if its filter is random access and "dense enough". As most filters results do support random access, either because they use OpenBitSet, or because they are built on top of FieldCache functionality, I think this feature will give great speed improvements to the query execution time. Right, the speed gains are often awesome!
          Hide
          Michael McCandless added a comment -

          Initial patch for trunk... lots of nocommits, but tests all pass and I
          think this is [roughly] the approach we should take to get fast(er)
          Filter perf.

          Conceptually, this change is fairly easy, because the flex APIs all
          accept a Bits to apply low-level filtering. However, this Bits is
          inverted vs the Filter that callers pass to IndexSearcher (skipDocs vs
          keepDocs), so, my patch inverts 1) the meaning of this first arg to
          the Docs/AndPositions enums (it becomes an acceptDocs instead of
          skipDocs), and 2) deleted docs coming back from IndexReaders (renames
          IR.getDeletedDocs -> IR.getNotDeletedDocs).

          That change (inverting the Bits to be keepDocs not skipDocs) is the
          vast majority of the patch.

          The "real" change is to add DocIdSet.getRandomAccessBits and
          bitsIncludesDeletedDocs, which IndexSearcher then consults to figure
          out whether to push the filter "low" instead of "high". I then fixed
          OpenBitSet to return this from getRandomAccessBits, and fixed
          CachingWrapperFilter to turn this on/off as well as state whether
          deleted docs were folded into the filter.

          This means filters cached with CachingWrapperFilter will apply "low",
          and if it's DeletesMode.RECACHE then it's a single filter that's
          applied (else I wrap with an AND NOT deleted check per docID), but
          custom filters are also free to impl these methods to have their
          filters applied "low".

          Show
          Michael McCandless added a comment - Initial patch for trunk... lots of nocommits, but tests all pass and I think this is [roughly] the approach we should take to get fast(er) Filter perf. Conceptually, this change is fairly easy, because the flex APIs all accept a Bits to apply low-level filtering. However, this Bits is inverted vs the Filter that callers pass to IndexSearcher (skipDocs vs keepDocs), so, my patch inverts 1) the meaning of this first arg to the Docs/AndPositions enums (it becomes an acceptDocs instead of skipDocs), and 2) deleted docs coming back from IndexReaders (renames IR.getDeletedDocs -> IR.getNotDeletedDocs). That change (inverting the Bits to be keepDocs not skipDocs) is the vast majority of the patch. The "real" change is to add DocIdSet.getRandomAccessBits and bitsIncludesDeletedDocs, which IndexSearcher then consults to figure out whether to push the filter "low" instead of "high". I then fixed OpenBitSet to return this from getRandomAccessBits, and fixed CachingWrapperFilter to turn this on/off as well as state whether deleted docs were folded into the filter. This means filters cached with CachingWrapperFilter will apply "low", and if it's DeletesMode.RECACHE then it's a single filter that's applied (else I wrap with an AND NOT deleted check per docID), but custom filters are also free to impl these methods to have their filters applied "low".
          Hide
          Uwe Schindler added a comment -

          Hi Mike,
          nicae patch, only little bit big. I reviewed the essential parts like applying the filter in IndexSearcher, real cool. Also CachingWrapperFilter looks fine (not closely reviewed).

          My question: Do we really need to make the delDocs inverse in this issue? The IndexSearcher impl can also be done using a simple OrNotBits(delDocs, filterDocs) wrapper (instead AndBits) implementation and NotBits (if no delDocs available)? The patch is unreadable because of that. In general, reversing the delDocs might be a good idea, but we should do it separate and hard (not allow both variants implemented by IndexReader & Co.). The method name getNotDeletedDocs() should also be getVisibleDocs() or similar [I don't like double negation].

          About the filters: I like the new API (it is as discussed before), so the DocIdSet is extended by an optional getBits() method, defaulting to null.

          About the impls: FieldCacheRangeFilter can also implement getBits() directly as FieldCache is random access. It should just return an own Bits impl for the DocIdSet that checks the filtering in get(index).

          Show
          Uwe Schindler added a comment - Hi Mike, nicae patch, only little bit big. I reviewed the essential parts like applying the filter in IndexSearcher, real cool. Also CachingWrapperFilter looks fine (not closely reviewed). My question: Do we really need to make the delDocs inverse in this issue? The IndexSearcher impl can also be done using a simple OrNotBits(delDocs, filterDocs) wrapper (instead AndBits) implementation and NotBits (if no delDocs available)? The patch is unreadable because of that. In general, reversing the delDocs might be a good idea, but we should do it separate and hard (not allow both variants implemented by IndexReader & Co.). The method name getNotDeletedDocs() should also be getVisibleDocs() or similar [I don't like double negation] . About the filters: I like the new API (it is as discussed before), so the DocIdSet is extended by an optional getBits() method, defaulting to null. About the impls: FieldCacheRangeFilter can also implement getBits() directly as FieldCache is random access. It should just return an own Bits impl for the DocIdSet that checks the filtering in get(index).
          Hide
          Uwe Schindler added a comment -

          One more comment about DocIdSet.bitsIncludesDeletedDocs(). I think the default in DocIdSet and of course OpenBitSet should be true, because current filters always respect deleted docs (this was a requirement: MTQ uses deleted docs, FCRF explicitely ands it in). So the default is fine here. Of course CachingWrapperFilter sets this to false if the SegmentReader got new deletes.

          Show
          Uwe Schindler added a comment - One more comment about DocIdSet.bitsIncludesDeletedDocs(). I think the default in DocIdSet and of course OpenBitSet should be true, because current filters always respect deleted docs (this was a requirement: MTQ uses deleted docs, FCRF explicitely ands it in). So the default is fine here. Of course CachingWrapperFilter sets this to false if the SegmentReader got new deletes.
          Hide
          Michael McCandless added a comment -

          My question: Do we really need to make the delDocs inverse in this issue?

          I agree, let's break this (inverting delDocs/skipDocs) into a new issue and do it first, then come back to this issue. There's still more work to do here, eg the bits should be stored inverted too (and the sparse encoding "flipped").

          The method name getNotDeletedDocs() should also be getVisibleDocs() or similar [I don't like double negation].

          +1 for getVisibleDocs – I also don't like double negation!

          In general, reversing the delDocs might be a good idea, but we should do it separate and hard (not allow both variants implemented by IndexReader & Co.).

          I agree it must be hard cutover – no more getDelDocs, and getVisibleDocs is abstract in IR.

          About the impls: FieldCacheRangeFilter can also implement getBits() directly as FieldCache is random access. It should just return an own Bits impl for the DocIdSet that checks the filtering in get(index).

          Ahh, right: FCRF has no trouble being random access, and it can re-use the already created matchDoc in the subclasses.

          Show
          Michael McCandless added a comment - My question: Do we really need to make the delDocs inverse in this issue? I agree, let's break this (inverting delDocs/skipDocs) into a new issue and do it first, then come back to this issue. There's still more work to do here, eg the bits should be stored inverted too (and the sparse encoding "flipped"). The method name getNotDeletedDocs() should also be getVisibleDocs() or similar [I don't like double negation] . +1 for getVisibleDocs – I also don't like double negation! In general, reversing the delDocs might be a good idea, but we should do it separate and hard (not allow both variants implemented by IndexReader & Co.). I agree it must be hard cutover – no more getDelDocs, and getVisibleDocs is abstract in IR. About the impls: FieldCacheRangeFilter can also implement getBits() directly as FieldCache is random access. It should just return an own Bits impl for the DocIdSet that checks the filtering in get(index). Ahh, right: FCRF has no trouble being random access, and it can re-use the already created matchDoc in the subclasses.
          Hide
          Robert Muir added a comment -

          +1 for getVisibleDocs – I also don't like double negation!

          I agree... getVisibleDocs() or another alternative would be getLiveDocs()

          Show
          Robert Muir added a comment - +1 for getVisibleDocs – I also don't like double negation! I agree... getVisibleDocs() or another alternative would be getLiveDocs()
          Hide
          Michael McCandless added a comment -

          Patch. Lots of nocommits still but tests pass.

          Show
          Michael McCandless added a comment - Patch. Lots of nocommits still but tests pass.
          Hide
          Chris Male added a comment -

          I think I have managed to update this to trunk. The only dated aspect is the use of OpenBitSet, which if I understand correctly, has been replaced by FixedBitSet for most cases.

          Show
          Chris Male added a comment - I think I have managed to update this to trunk. The only dated aspect is the use of OpenBitSet, which if I understand correctly, has been replaced by FixedBitSet for most cases.
          Hide
          Uwe Schindler added a comment -

          The only dated aspect is the use of OpenBitSet, which if I understand correctly, has been replaced by FixedBitSet for most cases.

          YES!

          Show
          Uwe Schindler added a comment - The only dated aspect is the use of OpenBitSet, which if I understand correctly, has been replaced by FixedBitSet for most cases. YES!
          Hide
          Michael McCandless added a comment -

          Looks good Chris; thanks for bringing up to date, and feel free to fix the nocommits too

          I think a couple trunk changes got lost in your merging – eg MultiPhraseQuery.rewrite lost the "if (termsArray.isEmpty())" case? (Also PhraseQuery.rewrite: I didn't mean to remove that; I think that was committed after my last patch?). And PayloadTermQuery lost an "if (includeSpanScore)"?

          We should certainly cutover to FixedBitSet!

          Show
          Michael McCandless added a comment - Looks good Chris; thanks for bringing up to date, and feel free to fix the nocommits too I think a couple trunk changes got lost in your merging – eg MultiPhraseQuery.rewrite lost the "if (termsArray.isEmpty())" case? (Also PhraseQuery.rewrite: I didn't mean to remove that; I think that was committed after my last patch?). And PayloadTermQuery lost an "if (includeSpanScore)"? We should certainly cutover to FixedBitSet!
          Hide
          Chris Male added a comment -

          New patch which includes the missed merging, some small tidy-ups and converting over to FixedBitSet.

          Show
          Chris Male added a comment - New patch which includes the missed merging, some small tidy-ups and converting over to FixedBitSet.
          Hide
          Chris Male added a comment - - edited

          Few questions:

          • Would it make more sense to use the 'LiveDocs' notion in this API too? So rather than it being bitsIncludeDeletedDocs, it would be liveDocsOnly.
          • In what situations would a DocIdSet return a different Bits implementation for getRandomAccessBits? If the DocIdSet impl doesn't support random access, then converting to a random-access Bits implementation could be intensive, right? which would probably out-weigh the benefits. Therefore could we simply check if the DocIdSet is also a Bits impl? If it isn't, then we do the traditional filtering approach.
          • When would a Bits implementation not want to support random-access? It seems its interface is implicitly random-access. If so, could we cut the setter from FixedBitSet (I'm moved it from OpenBitSet).
          Show
          Chris Male added a comment - - edited Few questions: Would it make more sense to use the 'LiveDocs' notion in this API too? So rather than it being bitsIncludeDeletedDocs, it would be liveDocsOnly. In what situations would a DocIdSet return a different Bits implementation for getRandomAccessBits? If the DocIdSet impl doesn't support random access, then converting to a random-access Bits implementation could be intensive, right? which would probably out-weigh the benefits. Therefore could we simply check if the DocIdSet is also a Bits impl? If it isn't, then we do the traditional filtering approach. When would a Bits implementation not want to support random-access? It seems its interface is implicitly random-access. If so, could we cut the setter from FixedBitSet (I'm moved it from OpenBitSet).
          Hide
          Michael McCandless added a comment -

          Would it make more sense to use the 'LiveDocs' notion in this API too? So rather than it being bitsIncludeDeletedDocs, it would be liveDocsOnly.

          +1

          In what situations would a DocIdSet return a different Bits implementation for getRandomAccessBits? If the DocIdSet impl doesn't support random access, then converting to a random-access Bits implementation could be intensive, right? which would probably out-weigh the benefits. Therefore could we simply check if the DocIdSet is also a Bits impl? If it isn't, then we do the traditional filtering approach.

          You mean instanceof check instead of getRandomAccessBits? I think
          that's good?

          When would a Bits implementation not want to support random-access? It seems its interface is implicitly random-access. If so, could we cut the setter from FixedBitSet (I'm moved it from OpenBitSet).

          Random access is actually slower when the number of docs that match
          the filter is tiny (like less than 1% of the index), so I think if
          it's a Bits instance we still need a way to force it to use the
          old way?

          Show
          Michael McCandless added a comment - Would it make more sense to use the 'LiveDocs' notion in this API too? So rather than it being bitsIncludeDeletedDocs, it would be liveDocsOnly. +1 In what situations would a DocIdSet return a different Bits implementation for getRandomAccessBits? If the DocIdSet impl doesn't support random access, then converting to a random-access Bits implementation could be intensive, right? which would probably out-weigh the benefits. Therefore could we simply check if the DocIdSet is also a Bits impl? If it isn't, then we do the traditional filtering approach. You mean instanceof check instead of getRandomAccessBits? I think that's good? When would a Bits implementation not want to support random-access? It seems its interface is implicitly random-access. If so, could we cut the setter from FixedBitSet (I'm moved it from OpenBitSet). Random access is actually slower when the number of docs that match the filter is tiny (like less than 1% of the index), so I think if it's a Bits instance we still need a way to force it to use the old way?
          Hide
          Chris Male added a comment -

          You mean instanceof check instead of getRandomAccessBits? I think that's good?

          Thats exactly what I mean.

          Random access is actually slower when the number of docs that match
          the filter is tiny (like less than 1% of the index), so I think if
          it's a Bits instance we still need a way to force it to use the
          old way?

          What if we were to expose cardinality through the Bits interface? We could then define some point in IndexSearcher where it decides which Query + Filter execution Strategy to use.

          Show
          Chris Male added a comment - You mean instanceof check instead of getRandomAccessBits? I think that's good? Thats exactly what I mean. Random access is actually slower when the number of docs that match the filter is tiny (like less than 1% of the index), so I think if it's a Bits instance we still need a way to force it to use the old way? What if we were to expose cardinality through the Bits interface? We could then define some point in IndexSearcher where it decides which Query + Filter execution Strategy to use.
          Hide
          Michael McCandless added a comment -

          What if we were to expose cardinality through the Bits interface? We could then define some point in IndexSearcher where it decides which Query + Filter execution Strategy to use.

          Hmm that makes me nervous, since computing cardinality isn't that cheap.

          Ie, it's better if this is done once up front and recorded on the DocIdSet, like CachingWrapperFilter does when the filter is cached.

          Show
          Michael McCandless added a comment - What if we were to expose cardinality through the Bits interface? We could then define some point in IndexSearcher where it decides which Query + Filter execution Strategy to use. Hmm that makes me nervous, since computing cardinality isn't that cheap. Ie, it's better if this is done once up front and recorded on the DocIdSet, like CachingWrapperFilter does when the filter is cached.
          Hide
          Chris Male added a comment - - edited

          Hmm that makes me nervous, since computing cardinality isn't that cheap.

          Okay, good point.

          Another alternative (just throwing out ideas) is to keep the getRandomAccessBits on DocIdSet, but pass into it the Bits liveDocs. Then it'd be up to the implementation to incorporate the liveDocs Bits (if it hasn't already). If it hadn't, it could use the AndBits like the patch currently does in IndexSearcher. IS would then become a little cleaner.

          If the DocIdSet didn't want random-access, then it would return null.

          Show
          Chris Male added a comment - - edited Hmm that makes me nervous, since computing cardinality isn't that cheap. Okay, good point. Another alternative (just throwing out ideas) is to keep the getRandomAccessBits on DocIdSet, but pass into it the Bits liveDocs. Then it'd be up to the implementation to incorporate the liveDocs Bits (if it hasn't already). If it hadn't, it could use the AndBits like the patch currently does in IndexSearcher. IS would then become a little cleaner. If the DocIdSet didn't want random-access, then it would return null.
          Hide
          Chris Male added a comment -

          Actually, the more I look at the nocommits, the less I like what I've suggested. I think having getRandomAccessBits as it is in the patch is fine. But I like we should maybe make setLiveDocsOnly and setAllowRandomAccessFiltering 1st class features of the Bits interface.

          Show
          Chris Male added a comment - Actually, the more I look at the nocommits, the less I like what I've suggested. I think having getRandomAccessBits as it is in the patch is fine. But I like we should maybe make setLiveDocsOnly and setAllowRandomAccessFiltering 1st class features of the Bits interface.
          Hide
          Michael McCandless added a comment -

          But I like we should maybe make setLiveDocsOnly and setAllowRandomAccessFiltering 1st class features of the Bits interface.

          Hmm... that also makes me a bit nervous Bits is too low-level for
          these concepts? Ie whether a filter/DIS "folded in" live docs
          already, and whether the filter/DIS is best applied by iteration vs by
          random access, are higher level filter concepts, not low level Bits
          concepts, I think?

          Also, Bits by definition is random-access so I don't think it should
          have set/getAllowRandomAccessFiltering.

          Show
          Michael McCandless added a comment - But I like we should maybe make setLiveDocsOnly and setAllowRandomAccessFiltering 1st class features of the Bits interface. Hmm... that also makes me a bit nervous Bits is too low-level for these concepts? Ie whether a filter/DIS "folded in" live docs already, and whether the filter/DIS is best applied by iteration vs by random access, are higher level filter concepts, not low level Bits concepts, I think? Also, Bits by definition is random-access so I don't think it should have set/getAllowRandomAccessFiltering.
          Hide
          Robert Muir added a comment -

          I didnt look too hard here at whats going on, but maybe we could use the RandomAccess marker interface from the jdk?

          Show
          Robert Muir added a comment - I didnt look too hard here at whats going on, but maybe we could use the RandomAccess marker interface from the jdk?
          Hide
          Chris Male added a comment -

          New patch which adds greater control over the random-access to DocIdSet.

          Also fixes most of the nocommits.

          Show
          Chris Male added a comment - New patch which adds greater control over the random-access to DocIdSet. Also fixes most of the nocommits.
          Hide
          Michael McCandless added a comment -

          New patch, fixing a few failing tests, adding a couple comments. I
          downgraded the 2 nocommits in LTC to TODOs, and removed the other one
          (false is correct default for those two?).

          For DocIdSet, can we nuke getRandomAccessBits? Ie, if
          supportRandomAccess() returns true, then we can cast the instance to
          Bits? Maybe we should rename supportRandomAccess to useRandomAccess?
          (Ie, it may support it, but we only want to use random access when the
          filter is dense enough).

          I changed MTQWF's and CachingWrapperFilter's threshold to a double
          percent (of the reader's maxDoc) instead, default 1.0%.

          Hmm FieldCacheRangeFilter never enables random access... not sure
          how/when we should compute that.

          I think we are getting close!

          Show
          Michael McCandless added a comment - New patch, fixing a few failing tests, adding a couple comments. I downgraded the 2 nocommits in LTC to TODOs, and removed the other one (false is correct default for those two?). For DocIdSet, can we nuke getRandomAccessBits? Ie, if supportRandomAccess() returns true, then we can cast the instance to Bits? Maybe we should rename supportRandomAccess to useRandomAccess? (Ie, it may support it, but we only want to use random access when the filter is dense enough). I changed MTQWF's and CachingWrapperFilter's threshold to a double percent (of the reader's maxDoc) instead, default 1.0%. Hmm FieldCacheRangeFilter never enables random access... not sure how/when we should compute that. I think we are getting close!
          Hide
          Uwe Schindler added a comment -

          Wouldn't it make sense for FCRF to always return random access=true? This filter has a very ineffective DISI, as it has to check each doc using the match method, so random access is much better for this one (maps to current matchDoc-method, which should be renamed to a simple Bits.get(int docId)). What's the sense to use a DISI for this one?

          Show
          Uwe Schindler added a comment - Wouldn't it make sense for FCRF to always return random access=true? This filter has a very ineffective DISI, as it has to check each doc using the match method, so random access is much better for this one (maps to current matchDoc-method, which should be renamed to a simple Bits.get(int docId)). What's the sense to use a DISI for this one?
          Hide
          Uwe Schindler added a comment -

          Why does CachingWrapperFilter refers to OpenBitSetDISI again? It should only use FixedBitSet as it already has al DISI methods?

          Show
          Uwe Schindler added a comment - Why does CachingWrapperFilter refers to OpenBitSetDISI again? It should only use FixedBitSet as it already has al DISI methods?
          Hide
          Chris Male added a comment -

          I haven't had a chance to look at the latest patch, but:

          For DocIdSet, can we nuke getRandomAccessBits? Ie, if
          supportRandomAccess() returns true, then we can cast the instance to
          Bits? Maybe we should rename supportRandomAccess to useRandomAccess?
          (Ie, it may support it, but we only want to use random access when the
          filter is dense enough).

          I'm definitely +1 to useRandomAccess() but I think there is a usability question mark around removing getRandomAccessBits(). If we assume that if DocIdSet.useRandomAccess() returns true then the DocIdSet must be also be a Bits implementation, then we need to prevent non-Bits implementations from returning true, or setting true in setUseRandomAccess. If we don't, we're likely to confuse even expert users because this all comes together in a method deep inside IndexSearcher.

          But if we're going to constrain useRandomAccess to only Bits implementations, then I once again feel these should be on Bits. What if we added to Bits allowRandomAccessFiltering() or something like that? So even though Bits is inherently random-access, we control whether the Bits should be used to do filtering.

          Alternatively we keep getRandomAccessBits() and see DocIdSet as a random-access Bits factory which currently just returns itself in most cases, but potentially might not in the future?

          Show
          Chris Male added a comment - I haven't had a chance to look at the latest patch, but: For DocIdSet, can we nuke getRandomAccessBits? Ie, if supportRandomAccess() returns true, then we can cast the instance to Bits? Maybe we should rename supportRandomAccess to useRandomAccess? (Ie, it may support it, but we only want to use random access when the filter is dense enough). I'm definitely +1 to useRandomAccess() but I think there is a usability question mark around removing getRandomAccessBits(). If we assume that if DocIdSet.useRandomAccess() returns true then the DocIdSet must be also be a Bits implementation, then we need to prevent non-Bits implementations from returning true, or setting true in setUseRandomAccess. If we don't, we're likely to confuse even expert users because this all comes together in a method deep inside IndexSearcher. But if we're going to constrain useRandomAccess to only Bits implementations, then I once again feel these should be on Bits. What if we added to Bits allowRandomAccessFiltering() or something like that? So even though Bits is inherently random-access, we control whether the Bits should be used to do filtering. Alternatively we keep getRandomAccessBits() and see DocIdSet as a random-access Bits factory which currently just returns itself in most cases, but potentially might not in the future?
          Hide
          Michael McCandless added a comment -

          Wouldn't it make sense for FCRF to always return random access=true? This filter has a very ineffective DISI, as it has to check each doc using the match method, so random access is much better for this one (maps to current matchDoc-method, which should be renamed to a simple Bits.get(int docId)). What's the sense to use a DISI for this one?

          +1 let's do that!

          Why does CachingWrapperFilter refers to OpenBitSetDISI again? It should only use FixedBitSet as it already has al DISI methods?

          Hmm I thought I fixed this in my last patch? Oh I see, I fixed the code but not the jdocs... I'll fix the jdocs.

          Show
          Michael McCandless added a comment - Wouldn't it make sense for FCRF to always return random access=true? This filter has a very ineffective DISI, as it has to check each doc using the match method, so random access is much better for this one (maps to current matchDoc-method, which should be renamed to a simple Bits.get(int docId)). What's the sense to use a DISI for this one? +1 let's do that! Why does CachingWrapperFilter refers to OpenBitSetDISI again? It should only use FixedBitSet as it already has al DISI methods? Hmm I thought I fixed this in my last patch? Oh I see, I fixed the code but not the jdocs... I'll fix the jdocs.
          Hide
          Michael McCandless added a comment -

          I'm definitely +1 to useRandomAccess() but I think there is a usability question mark around removing getRandomAccessBits(). If we assume that if DocIdSet.useRandomAccess() returns true then the DocIdSet must be also be a Bits implementation, then we need to prevent non-Bits implementations from returning true, or setting true in setUseRandomAccess. If we don't, we're likely to confuse even expert users because this all comes together in a method deep inside IndexSearcher.

          Well it's quite expert to implement your own random-access filter?

          We can fix IS so that if the DIS fails to cast you get a clear exception stating that useRandomAccess returned true yet the DIS isn't a Bits? Or it could be silent and fall back to iteration, if the cast fails... but I prefer the former (so you know your "true" isn't working).

          I really don't think we should pollute Bits w/ useRandomAccess method; random-access is the entire point of the Bits interface. It's too low-level to push it down there.

          And actually having a getter (getRandomAccessBits) is trappy I think, because the user may not realize it's called for every search and may do something crazy like allocate a new FixedBitSet every time.

          Show
          Michael McCandless added a comment - I'm definitely +1 to useRandomAccess() but I think there is a usability question mark around removing getRandomAccessBits(). If we assume that if DocIdSet.useRandomAccess() returns true then the DocIdSet must be also be a Bits implementation, then we need to prevent non-Bits implementations from returning true, or setting true in setUseRandomAccess. If we don't, we're likely to confuse even expert users because this all comes together in a method deep inside IndexSearcher. Well it's quite expert to implement your own random-access filter? We can fix IS so that if the DIS fails to cast you get a clear exception stating that useRandomAccess returned true yet the DIS isn't a Bits? Or it could be silent and fall back to iteration, if the cast fails... but I prefer the former (so you know your "true" isn't working). I really don't think we should pollute Bits w/ useRandomAccess method; random-access is the entire point of the Bits interface. It's too low-level to push it down there. And actually having a getter (getRandomAccessBits) is trappy I think, because the user may not realize it's called for every search and may do something crazy like allocate a new FixedBitSet every time.
          Hide
          Michael McCandless added a comment -

          New patch folding in Uwe's ideas – FCRF always uses random access; remove matchDoc and just override Bits.get; fixed CWF jdocs to not reference OpenBitSetDISI.

          Show
          Michael McCandless added a comment - New patch folding in Uwe's ideas – FCRF always uses random access; remove matchDoc and just override Bits.get; fixed CWF jdocs to not reference OpenBitSetDISI.
          Hide
          Chris Male added a comment -

          I really don't think we should pollute Bits w/ useRandomAccess method; random-access is the entire point of the Bits interface. It's too low-level to push it down there.

          Its not about marking whether random-access should be used or not, my idea was to mark whether it should be used to filter or not.

          Show
          Chris Male added a comment - I really don't think we should pollute Bits w/ useRandomAccess method; random-access is the entire point of the Bits interface. It's too low-level to push it down there. Its not about marking whether random-access should be used or not, my idea was to mark whether it should be used to filter or not.
          Hide
          Robert Muir added a comment -

          As a first (committable) step towards this, I think we should break out a new issue
          where we remove the hardcoded calls to IR.getLiveDocs() in all the Scorers?

          Then deleted docs could be supplied via the ScorerContext with something like acceptDocs.

          This seems cleaner and more flexible to me regardless of what we do.

          Show
          Robert Muir added a comment - As a first (committable) step towards this, I think we should break out a new issue where we remove the hardcoded calls to IR.getLiveDocs() in all the Scorers? Then deleted docs could be supplied via the ScorerContext with something like acceptDocs. This seems cleaner and more flexible to me regardless of what we do.
          Hide
          Michael McCandless added a comment -

          +1, that change is a rote cutover.

          Then separately we can figure out how a Filter/DIS/Bits conveys the "strategy" to IS. Really this is a part of the wider question of how Lucene should do query optimization in general...

          Show
          Michael McCandless added a comment - +1, that change is a rote cutover. Then separately we can figure out how a Filter/DIS/Bits conveys the "strategy" to IS. Really this is a part of the wider question of how Lucene should do query optimization in general...
          Hide
          Robert Muir added a comment -

          Then separately we can figure out how a Filter/DIS/Bits conveys the "strategy" to IS. Really this is a part of the wider question of how Lucene should do query optimization in general...

          I think once we do this, we should just add "Bits acceptDocs" parameter to IndexSearcher's searchWithFilter method?
          And I think a simple (protected) heuristic to IndexSearcher whether it should execute the filter 'via acceptDocs': the default just be instanceof Bits && firstSetBit < X, like what BQ does?
          Then we might add optional boolean method to Filter for whether it already 'incorporates deletes'.

          Then we can optimize for the 4 cases:

          1. sparse/non-random-access filter, doesn't incorporate deletes: same logic as today
          2. sparse/non-random-access filter, incorporates deletes: Here we might pass null as 'acceptDocs' to the scorercontext, so we no longer redundantly check deleted docs.
          3. heavy random-access filter, doesn't incorporate deletes: no filter instead with acceptDocs=AndBits(filter, liveDocs)
          4. heavy random-access filter, incorporates deletes: no filter instead with acceptDocs=filter (don't need to use AndBits since we know it incorporates liveDocs).
          Show
          Robert Muir added a comment - Then separately we can figure out how a Filter/DIS/Bits conveys the "strategy" to IS. Really this is a part of the wider question of how Lucene should do query optimization in general... I think once we do this, we should just add "Bits acceptDocs" parameter to IndexSearcher's searchWithFilter method? And I think a simple (protected) heuristic to IndexSearcher whether it should execute the filter 'via acceptDocs': the default just be instanceof Bits && firstSetBit < X, like what BQ does? Then we might add optional boolean method to Filter for whether it already 'incorporates deletes'. Then we can optimize for the 4 cases: sparse/non-random-access filter, doesn't incorporate deletes: same logic as today sparse/non-random-access filter, incorporates deletes: Here we might pass null as 'acceptDocs' to the scorercontext, so we no longer redundantly check deleted docs. heavy random-access filter, doesn't incorporate deletes: no filter instead with acceptDocs=AndBits(filter, liveDocs) heavy random-access filter, incorporates deletes: no filter instead with acceptDocs=filter (don't need to use AndBits since we know it incorporates liveDocs).
          Hide
          Michael McCandless added a comment -

          And I think a simple (protected) heuristic to IndexSearcher whether it should execute the filter 'via acceptDocs': the default just be instanceof Bits && firstSetBit < X, like what BQ does?

          That's a neat idea! Then we can sidestep this whole question about who/where/what "computes" whether the Bits is very sparse (< 1%) and so we should apply "up high", or not and so we should apply "down low".

          So this logic would run after we have a Bits... we could actually go and test every Mth bit (not just first N), to hopefully reduce false negatives (ie, a dense filter that appeared sparse just because it's first N docs were not set).

          It'd still be a heuristic and thus make mistakes sometimes... (vs actually calling .cardinality and making the "right" decision), but most of the time it should work.

          Separately we still need someone to declare whether the filter AND'd live docs already; maybe we put this on the Filter class, since it would not normally vary per-segment?

          Show
          Michael McCandless added a comment - And I think a simple (protected) heuristic to IndexSearcher whether it should execute the filter 'via acceptDocs': the default just be instanceof Bits && firstSetBit < X, like what BQ does? That's a neat idea! Then we can sidestep this whole question about who/where/what "computes" whether the Bits is very sparse (< 1%) and so we should apply "up high", or not and so we should apply "down low". So this logic would run after we have a Bits... we could actually go and test every Mth bit (not just first N), to hopefully reduce false negatives (ie, a dense filter that appeared sparse just because it's first N docs were not set). It'd still be a heuristic and thus make mistakes sometimes... (vs actually calling .cardinality and making the "right" decision), but most of the time it should work. Separately we still need someone to declare whether the filter AND'd live docs already; maybe we put this on the Filter class, since it would not normally vary per-segment?
          Hide
          Chris Male added a comment -

          +1 to this approach.

          Separately we still need someone to declare whether the filter AND'd live docs already; maybe we put this on the Filter class, since it would not normally vary per-segment?

          I kind of like how Filter doesn't have any properties associated with it. So perhaps we can keep the liveDocsOnly on DocIdSet?

          Show
          Chris Male added a comment - +1 to this approach. Separately we still need someone to declare whether the filter AND'd live docs already; maybe we put this on the Filter class, since it would not normally vary per-segment? I kind of like how Filter doesn't have any properties associated with it. So perhaps we can keep the liveDocsOnly on DocIdSet?
          Hide
          Michael McCandless added a comment -

          OK, DocIdSet seems good too? It already has isCacheable, so there's a precent of putting such "hints" on it.

          Maybe .containsOnlyLiveDocs()?

          Show
          Michael McCandless added a comment - OK, DocIdSet seems good too? It already has isCacheable, so there's a precent of putting such "hints" on it. Maybe .containsOnlyLiveDocs()?
          Hide
          Chris Male added a comment -

          Maybe .containsOnlyLiveDocs()?

          +1

          Show
          Chris Male added a comment - Maybe .containsOnlyLiveDocs()? +1
          Hide
          Chris Male added a comment -

          Patch updated following the changes in LUCENE-3474. (Note, this patch also fixes a missed change in LUCENE-3474, in MultiPhraseQuery.UnionDocsAndPositionsEnum).

          New patch features:

          • liveDocsOnly() -> containsOnlyLiveDocs()
          • getRandomAccessBits() and useRandomAccess() are gone, replaced by some additional logic in IS. Now we check if the DocIdSet is a Bits impl, and then we consult a protected method in IS called useRandomAccess().
          • At the moment useRandomAccess() just returns true, while we discuss what heuristics are best to apply here.
          Show
          Chris Male added a comment - Patch updated following the changes in LUCENE-3474 . (Note, this patch also fixes a missed change in LUCENE-3474 , in MultiPhraseQuery.UnionDocsAndPositionsEnum). New patch features: liveDocsOnly() -> containsOnlyLiveDocs() getRandomAccessBits() and useRandomAccess() are gone, replaced by some additional logic in IS. Now we check if the DocIdSet is a Bits impl, and then we consult a protected method in IS called useRandomAccess(). At the moment useRandomAccess() just returns true, while we discuss what heuristics are best to apply here.
          Hide
          Robert Muir added a comment -

          I don't understand the de-optimizations to termquery in the patch.

          Show
          Robert Muir added a comment - I don't understand the de-optimizations to termquery in the patch.
          Hide
          Chris Male added a comment -

          I used what has been in the patches so far. If its out of date, just tell me.

          Show
          Chris Male added a comment - I used what has been in the patches so far. If its out of date, just tell me.
          Hide
          Robert Muir added a comment -

          it doesnt have anything to do with this issue.

          Show
          Robert Muir added a comment - it doesnt have anything to do with this issue.
          Hide
          Michael McCandless added a comment -

          What a tiny patch this has turned into!

          It looks great.

          I agree the TQ changes look like a merge mistake? We should just keep
          trunk here?

          Can we name it DocIdSet.containsOnlyLiveDocs? I don't think we need
          the "is" prefix? Can you add @lucene.experimental to it?

          For the default IS heuristic, how about testing 100 evenly spaced
          bits? If more than 1 is set, and we can early-exit during the
          testing, we use random access? We can make the two values (100 and 1)
          settable in expert IS ctors?

          Show
          Michael McCandless added a comment - What a tiny patch this has turned into! It looks great. I agree the TQ changes look like a merge mistake? We should just keep trunk here? Can we name it DocIdSet.containsOnlyLiveDocs? I don't think we need the "is" prefix? Can you add @lucene.experimental to it? For the default IS heuristic, how about testing 100 evenly spaced bits? If more than 1 is set, and we can early-exit during the testing, we use random access? We can make the two values (100 and 1) settable in expert IS ctors?
          Hide
          Robert Muir added a comment -

          For the default IS heuristic, how about testing 100 evenly spaced
          bits?

          How about grab an iterator like today, pull the first bit, if this takes a long time because the filter is sparse,
          we dive into the conjunction alg we do today anyway, no wasted effort.

          Otherwise if its < 100, use it as liveDocs.

          we can nuke the 'boolean' protected method and figure out if/how to add abstractions later.

          Show
          Robert Muir added a comment - For the default IS heuristic, how about testing 100 evenly spaced bits? How about grab an iterator like today, pull the first bit, if this takes a long time because the filter is sparse, we dive into the conjunction alg we do today anyway, no wasted effort. Otherwise if its < 100, use it as liveDocs. we can nuke the 'boolean' protected method and figure out if/how to add abstractions later.
          Hide
          Robert Muir added a comment -

          Also, when we pass filter "down low" as liveDocs, we should ensure we set the appropriate inOrder/topLevel params so we can get BooleanScorer,
          since we won't need to advance() it.

          Show
          Robert Muir added a comment - Also, when we pass filter "down low" as liveDocs, we should ensure we set the appropriate inOrder/topLevel params so we can get BooleanScorer, since we won't need to advance() it.
          Hide
          Robert Muir added a comment -

          and current patch is missing the optimization for case 2 described above:

          2. sparse/non-random-access filter, incorporates deletes: Here we might pass null as 'acceptDocs' to the scorercontext, so we no longer redundantly check deleted docs.

          I'll update the patch to support BooleanScorer and optimize this case.

          Show
          Robert Muir added a comment - and current patch is missing the optimization for case 2 described above: 2. sparse/non-random-access filter, incorporates deletes: Here we might pass null as 'acceptDocs' to the scorercontext, so we no longer redundantly check deleted docs. I'll update the patch to support BooleanScorer and optimize this case.
          Hide
          Michael McCandless added a comment -

          How about grab an iterator like today, pull the first bit, if this takes a long time because the filter is sparse,
          we dive into the conjunction alg we do today anyway, no wasted effort.

          Otherwise if its < 100, use it as liveDocs.

          +1, I like that.

          But, < maxDoc()/100; and maybe we make expert setter for that 100.

          Also, when we pass filter "down low" as liveDocs, we should ensure we set the appropriate inOrder/topLevel params so we can get BooleanScorer,
          since we won't need to advance() it.

          Good catch, yes!

          Show
          Michael McCandless added a comment - How about grab an iterator like today, pull the first bit, if this takes a long time because the filter is sparse, we dive into the conjunction alg we do today anyway, no wasted effort. Otherwise if its < 100, use it as liveDocs. +1, I like that. But, < maxDoc()/100; and maybe we make expert setter for that 100. Also, when we pass filter "down low" as liveDocs, we should ensure we set the appropriate inOrder/topLevel params so we can get BooleanScorer, since we won't need to advance() it. Good catch, yes!
          Hide
          Michael McCandless added a comment -

          Duh, nevermind: I think it is just a < 100 check (no maxDoc involved). It's as if we are [approximately] checking how many set bits in first 100 docs, and if we hit at least 1 such but we assume that means filter is > 1% dense.

          Show
          Michael McCandless added a comment - Duh, nevermind: I think it is just a < 100 check (no maxDoc involved). It's as if we are [approximately] checking how many set bits in first 100 docs, and if we hit at least 1 such but we assume that means filter is > 1% dense.
          Hide
          Robert Muir added a comment -

          updated patch:

          • implements heuristic with getter/setter (defaults to random access if we estimate filter accepts > 1% of documents)
          • sets the inOrder/topLevel stuff correct when we use random access <-- but we should add a test that we get BS1 here!
          • when the filter is sparse, but contains only live docs, we pass null as liveDocs.
          Show
          Robert Muir added a comment - updated patch: implements heuristic with getter/setter (defaults to random access if we estimate filter accepts > 1% of documents) sets the inOrder/topLevel stuff correct when we use random access <-- but we should add a test that we get BS1 here! when the filter is sparse, but contains only live docs, we pass null as liveDocs.
          Hide
          Michael McCandless added a comment -

          Patch looks great!

          Show
          Michael McCandless added a comment - Patch looks great!
          Hide
          Robert Muir added a comment -

          just a code style tweak, a test to make sure we get BS1 with random access filters, and randomization of the parameter value in LuceneTestCase.

          Show
          Robert Muir added a comment - just a code style tweak, a test to make sure we get BS1 with random access filters, and randomization of the parameter value in LuceneTestCase.
          Hide
          Chris Male added a comment -

          Small improvements:

          • Changed isContainsOnlyLiveDocs to containsOnlyLiveDocs
          • Changed visibility of containsOnlyLiveDocs field since with the getter/setters it doesn't need to be protected.
          • Tidied the documentation on the getter/setters of the threshold in IS

          I think we're good to go?

          Show
          Chris Male added a comment - Small improvements: Changed isContainsOnlyLiveDocs to containsOnlyLiveDocs Changed visibility of containsOnlyLiveDocs field since with the getter/setters it doesn't need to be protected. Tidied the documentation on the getter/setters of the threshold in IS I think we're good to go?
          Hide
          Shay Banon added a comment -

          Hey, briefly checked the patch, and wondering out loud if it make sense to have similar random access logic in FilteredQuery?

          Show
          Shay Banon added a comment - Hey, briefly checked the patch, and wondering out loud if it make sense to have similar random access logic in FilteredQuery?
          Hide
          Chris Male added a comment -

          It certainly shares alot of similarities with the logic in IS so I think there would be benefit yeah.

          Show
          Chris Male added a comment - It certainly shares alot of similarities with the logic in IS so I think there would be benefit yeah.
          Hide
          Robert Muir added a comment -

          maybe for FilteredQuery just open a separate issue? In this case I think
          we should just give it another Scorer impl.

          Show
          Robert Muir added a comment - maybe for FilteredQuery just open a separate issue? In this case I think we should just give it another Scorer impl.
          Hide
          Michael McCandless added a comment -

          +1 for new issue for FilteredQuery

          Patch looks great Chris – I think it's ready! Finally

          Tiny fix to the jdocs for IS.setFilterRandomAccessThreshold – "Threshold use to heuristics" -> "Threshold used in heuristic".

          I think we should run before/after benchmarks before committing... I'll try to do this soon and post back.

          Show
          Michael McCandless added a comment - +1 for new issue for FilteredQuery Patch looks great Chris – I think it's ready! Finally Tiny fix to the jdocs for IS.setFilterRandomAccessThreshold – "Threshold use to heuristics" -> "Threshold used in heuristic". I think we should run before/after benchmarks before committing... I'll try to do this soon and post back.
          Hide
          Uwe Schindler added a comment - - edited

          Can you only remove the unchecked exception here (FieldCacheRangeFilter):

          public abstract boolean get(int doc) throws ArrayIndexOutOfBoundsException;

          This exception is only used internally to ignore range checks, but we dont need to declare (and impl classes no longer declare it at all).

          Also FieldCacheDocIdSet does not implement Bits completely?

          Show
          Uwe Schindler added a comment - - edited Can you only remove the unchecked exception here (FieldCacheRangeFilter): public abstract boolean get( int doc) throws ArrayIndexOutOfBoundsException; This exception is only used internally to ignore range checks, but we dont need to declare (and impl classes no longer declare it at all). Also FieldCacheDocIdSet does not implement Bits completely?
          Hide
          Uwe Schindler added a comment -

          New patch:

          • Fixed FieldCacheRangeFilterDocIdSet to implement bits
          • Removed deleted docs handling in this filter. We explicitely pass false. No the iterator and get(bits) may return deleted docs.
          • Made AndBits final (speed and no need to subclass)

          The question here: I see no filter that explicitely returns true for the deletedDocs handling. But e.g. most filters actually dont have deleted docs, like MultiTermQueryWF,...

          Show
          Uwe Schindler added a comment - New patch: Fixed FieldCacheRangeFilterDocIdSet to implement bits Removed deleted docs handling in this filter. We explicitely pass false. No the iterator and get(bits) may return deleted docs. Made AndBits final (speed and no need to subclass) The question here: I see no filter that explicitely returns true for the deletedDocs handling. But e.g. most filters actually dont have deleted docs, like MultiTermQueryWF,...
          Hide
          Uwe Schindler added a comment -

          I still disagree with the setter in DocIdSet to make it respect setContainsOnlyLiveDocs(). This is borken and only specific to FCRangeFilter. This method should not be there, instead the DocIdSet should return this info, e.g. FieldCacheRangeFilterDocIdSet should implement this method and return false.

          Show
          Uwe Schindler added a comment - I still disagree with the setter in DocIdSet to make it respect setContainsOnlyLiveDocs(). This is borken and only specific to FCRangeFilter. This method should not be there, instead the DocIdSet should return this info, e.g. FieldCacheRangeFilterDocIdSet should implement this method and return false.
          Hide
          Uwe Schindler added a comment -

          I modified my patch for FieldCacheRangeFilter to enforce that it does not respect deletes (it throws UOE on the setter).

          I checked some combinations, setContainsOnlyLiveDocs() cannot go in like that!!!

          CachingWrapperFilter should have its own handling of this and return the correct setting, but not modify an already created bitset/whatever DocIdSet. My above patch does no longer respect deleted docs in FieldCacheRangeFilter and returns false from the beginning. But if you cache this filter, it suddenly tries to set the boolean to true -> test fail

          Also all Filters that internally use deleted docs, should return true from the beginning. We can maybe add the setter to FixedBitSet, so filter impls can easily set this bit, but it should not be a setter in DocIdSet!

          Show
          Uwe Schindler added a comment - I modified my patch for FieldCacheRangeFilter to enforce that it does not respect deletes (it throws UOE on the setter). I checked some combinations, setContainsOnlyLiveDocs() cannot go in like that!!! CachingWrapperFilter should have its own handling of this and return the correct setting, but not modify an already created bitset/whatever DocIdSet. My above patch does no longer respect deleted docs in FieldCacheRangeFilter and returns false from the beginning. But if you cache this filter, it suddenly tries to set the boolean to true -> test fail Also all Filters that internally use deleted docs, should return true from the beginning. We can maybe add the setter to FixedBitSet, so filter impls can easily set this bit, but it should not be a setter in DocIdSet!
          Hide
          Uwe Schindler added a comment -

          Further investigations showed more problems:

          • FilteredDocIdSet does never implement Bits, but it should if the wrapped filter implements Bits. This cannot be done as two different implementation would be needed. I have no idea how to solve this.

          I uploaded a new patch that fixes the problems from before:

          • CachingWrapperFilter now only set the flag for containsOnlyLiveDocs to true, if it was true before, too. If the orginal filter returned a DocIdSet without that flag, the cached filter cannot suddenly set it to true
          • CachingWrapperFilter also copies the liveDocs when it copies to FixedBitSet (e.g. QueryWrapperFilter).
          • The default for containsOnlyLiveDocs is true, as all current filters were always resepcting this (exept FieldCacheRangeFilter since the rewrite). All filters in Lucene use liveDocs, because this was a requirement in older Lucene versions!
          • QueryWrapperFilter may ignore liveDocs and simply return false for the flag.

          In general I would like it more to rip the deleted docs handling in CachingWrapperFilter, as it no longer needs to take care. CWF should simply return containsOnlyLiveDocs=false if the deleted docs need to be merged in. There is no need to and them in using FilteredDocIdSet (which slows down for the random access case, see above)

          Show
          Uwe Schindler added a comment - Further investigations showed more problems: FilteredDocIdSet does never implement Bits, but it should if the wrapped filter implements Bits. This cannot be done as two different implementation would be needed. I have no idea how to solve this. I uploaded a new patch that fixes the problems from before: CachingWrapperFilter now only set the flag for containsOnlyLiveDocs to true, if it was true before, too. If the orginal filter returned a DocIdSet without that flag, the cached filter cannot suddenly set it to true CachingWrapperFilter also copies the liveDocs when it copies to FixedBitSet (e.g. QueryWrapperFilter). The default for containsOnlyLiveDocs is true, as all current filters were always resepcting this (exept FieldCacheRangeFilter since the rewrite). All filters in Lucene use liveDocs, because this was a requirement in older Lucene versions! QueryWrapperFilter may ignore liveDocs and simply return false for the flag. In general I would like it more to rip the deleted docs handling in CachingWrapperFilter, as it no longer needs to take care. CWF should simply return containsOnlyLiveDocs=false if the deleted docs need to be merged in. There is no need to and them in using FilteredDocIdSet (which slows down for the random access case, see above)
          Hide
          Uwe Schindler added a comment -

          More problems (I am currently rewriting the whole liveDocs stuff):

          • DocIdBitSet does not implement Bits, but its random access.

          Another idea I had:
          We could do Filter.getDocIdSet like Weight.scorer: Pass the live docs down. This could improve e.g. ChainedFilter as it can simply pass the results of filter down the chain, if its random access. Also QueryWrapperFilter could directly pass the incoming bits downto the wrapped Query (see comment in current code).

          And finally we would not need the containsOnlyLiveDocs flag at all. If IndexSearcher does not need deleted docs to be handled in the filter, it could pass down null, if it passes liveDocs down to the filter, the filter is required to respect them (like scorers).

          CachingWrapperFilter would use AndBits/FilteredDocIdSet in all cases to combine the cached result (which it caches always without liveDocs). This is still faster than the current approcah where deldocs are handled quite oftenh multiple times in the query execution.

          Show
          Uwe Schindler added a comment - More problems (I am currently rewriting the whole liveDocs stuff): DocIdBitSet does not implement Bits, but its random access. Another idea I had: We could do Filter.getDocIdSet like Weight.scorer: Pass the live docs down. This could improve e.g. ChainedFilter as it can simply pass the results of filter down the chain, if its random access. Also QueryWrapperFilter could directly pass the incoming bits downto the wrapped Query (see comment in current code). And finally we would not need the containsOnlyLiveDocs flag at all. If IndexSearcher does not need deleted docs to be handled in the filter, it could pass down null, if it passes liveDocs down to the filter, the filter is required to respect them (like scorers). CachingWrapperFilter would use AndBits/FilteredDocIdSet in all cases to combine the cached result (which it caches always without liveDocs). This is still faster than the current approcah where deldocs are handled quite oftenh multiple times in the query execution.
          Hide
          Chris Male added a comment -

          That is a lot to take in. Let me see if I understand:

          • Basically you're saying that allowing external code to setContainsOnlyLiveDocs is broken in CachingWrapperFilter since it makes a decision that a Filter contains only live docs, when in fact the Filter might not.
          • You're also arguing that most Filters take liveDocs into consideration anyway.
          • You suggest we pass liveDocs into Filter.getDocIdSet requiring the Filter to uses them (mirroring the Weight/Scorer API). I kind of like this idea.

          I don't think its a crisis if some DocIdSet impls don't implement Bits. I personally see this issue as providing the framework for doing random-access filtering, not necessarily making it happen with every DocIdSet.

          Show
          Chris Male added a comment - That is a lot to take in. Let me see if I understand: Basically you're saying that allowing external code to setContainsOnlyLiveDocs is broken in CachingWrapperFilter since it makes a decision that a Filter contains only live docs, when in fact the Filter might not. You're also arguing that most Filters take liveDocs into consideration anyway. You suggest we pass liveDocs into Filter.getDocIdSet requiring the Filter to uses them (mirroring the Weight/Scorer API). I kind of like this idea. I don't think its a crisis if some DocIdSet impls don't implement Bits. I personally see this issue as providing the framework for doing random-access filtering, not necessarily making it happen with every DocIdSet.
          Hide
          Robert Muir added a comment -

          Thanks Uwe for the reviewing/policing!

          Chris, I do agree its not a crisis, but i think its best if we set everything up to work nicely.

          I do think its really bad news if a filter claims it containsOnlyLiveDocs but in fact it does not... this is a big blocker for committing anything, because it will only create bugs.

          Show
          Robert Muir added a comment - Thanks Uwe for the reviewing/policing! Chris, I do agree its not a crisis, but i think its best if we set everything up to work nicely. I do think its really bad news if a filter claims it containsOnlyLiveDocs but in fact it does not... this is a big blocker for committing anything, because it will only create bugs.
          Hide
          Chris Male added a comment -

          What do you think of the idea of passing liveDocs into Filter.getDocIdSet?

          Show
          Chris Male added a comment - What do you think of the idea of passing liveDocs into Filter.getDocIdSet?
          Hide
          Uwe Schindler added a comment -

          Hi Chris, hi Male,

          I was going to bed after my last post. I had a crisis with two facts in the new API, that do no play nicely together. I thought the whole night about it again and I also started to recode some details last evening, but all was not so fine (but I found lots of problems, so it's a good thing that I started to code - especially on several filters that are not so basic like those which only use FixedFitSet/OpenBitSet):

          1. the hidden implementation of Bits is a nice idea, but has one big problem: Java is a strongly-typed language. If a DocIdSet implements Bits, but you want to wrap it using FilteredDocIdSet, this interface implementation might suddenly go away, because the wrapper class does not implement Bits. If we make FilteredDocIdSet implement Bits, its also wrong, as it might wrap another DocIdSet that is not random access. So I tend to keep DocIdSet abstrcat and let it only expose functions that return a Bits interface. The same is that DocIdSet does not directly implement DocIdSetIterator, it can just return one. So I would strongly recommend to add a method like iterator() that returns a impl and not rely on "marker interfaces". I would favor "Bits DocIdSet.bits()" - would be in line with the iterator method. If the implementing class like FixedBitSet implements it itsself and returns "this" is an implementation detail. If DocIdSet does not allow random access it should expose with an exception thrown by bits or if it returns null. Does not really matter to me. - In general a wrapper like FilteredDocIdSet can do this in one class, wrapping bits() would check if bits() returns non-null, and then wrap another wrapper around bits() that uses match() to filter. The impl of this class is fast and supports both (iterator and bits, if available).
          2. the other thing, I dont like, is the setContainsOnlyLiveDocs setter on DocIdSet. It allows anybody to change the DocIdSet (which should have an API that exposes only read-access). Only classes like FixedBitSet that implement this read-only interface might be able to change it from their own API (means the setter might be in the various DocIdSet implementations in oal.util). A consumer of the filter should not be able to change the DocIdSet behaviour from outside using a public API. I started to rewrite this yesterday and only left the getter in DocIdSet, but added the setter to FixedBitSet, OpenBitSet, DocIdBitSet,... The setter in the abstract base class also violates unmodifiable of EMPTY_DOCIDSET. This impl should be "containsOnlyLiveDocs=true") and this must be unchangeable fixed.
          3. Also DocIdSet is a class not really related solely to Filters, e.g. Scorer extends DocIdSetIterator or DocsEnum extends DocIdSetIterator, Solr Facetting uses DocIdSet. DocIdSet is just a holder class for a bunch of documents exposing a iterator (and a Bits API - this is why I want two getter methods and no interface magic)). The existence of live docs is outside it's scope. I therefore would like a similar API like for scorers, so IndexSearcher can ask the Filter for a DocIdSet based on the given liveDocs (like the scorer method in Weights). The returned DocIdSet would not know if it only has live Docs or not (as the Scorer itsself also does not expose this information). CachingWrapperFilter is little bit special, but this one would always ask the wrapped Filter for a DocidSet without deletions and cache that one, but always return a FilteredDocIdSet bringing the liveDocs passed from IndexSearcher in. The cache would then always be without LiveDocs and easier to maintain. Reopening segments would never need to reload cache. CachingWrapperFilter would just decide on the fact if IndexSearcher passes a liveDocs BitSet or not, if it needs to use it or not (in its own getDocIdSet method). If we have a query and only filter some documents, IndexSearcher already knows about liveDocs from the main scorer and would pass null to the filter. This would remove lots of additional checks to liveDocs. Only the main scorer would know about them, the filter will ignore them (so there is no overhead in CachingWrapperFilter, as it can return the cached filter directly to IndexSearcher, without wrapping). QueryWrapperFilter could pass the liveDocs through the wrapped filter, too.

          I may have time today to implement some parts of this, should not be to difficult.

          Show
          Uwe Schindler added a comment - Hi Chris, hi Male, I was going to bed after my last post. I had a crisis with two facts in the new API, that do no play nicely together. I thought the whole night about it again and I also started to recode some details last evening, but all was not so fine (but I found lots of problems, so it's a good thing that I started to code - especially on several filters that are not so basic like those which only use FixedFitSet/OpenBitSet): the hidden implementation of Bits is a nice idea, but has one big problem: Java is a strongly-typed language. If a DocIdSet implements Bits, but you want to wrap it using FilteredDocIdSet, this interface implementation might suddenly go away, because the wrapper class does not implement Bits. If we make FilteredDocIdSet implement Bits, its also wrong, as it might wrap another DocIdSet that is not random access. So I tend to keep DocIdSet abstrcat and let it only expose functions that return a Bits interface. The same is that DocIdSet does not directly implement DocIdSetIterator, it can just return one. So I would strongly recommend to add a method like iterator() that returns a impl and not rely on "marker interfaces". I would favor "Bits DocIdSet.bits()" - would be in line with the iterator method. If the implementing class like FixedBitSet implements it itsself and returns "this" is an implementation detail. If DocIdSet does not allow random access it should expose with an exception thrown by bits or if it returns null. Does not really matter to me. - In general a wrapper like FilteredDocIdSet can do this in one class, wrapping bits() would check if bits() returns non-null, and then wrap another wrapper around bits() that uses match() to filter. The impl of this class is fast and supports both (iterator and bits, if available). the other thing, I dont like, is the setContainsOnlyLiveDocs setter on DocIdSet. It allows anybody to change the DocIdSet (which should have an API that exposes only read-access). Only classes like FixedBitSet that implement this read-only interface might be able to change it from their own API (means the setter might be in the various DocIdSet implementations in oal.util). A consumer of the filter should not be able to change the DocIdSet behaviour from outside using a public API. I started to rewrite this yesterday and only left the getter in DocIdSet, but added the setter to FixedBitSet, OpenBitSet, DocIdBitSet,... The setter in the abstract base class also violates unmodifiable of EMPTY_DOCIDSET. This impl should be "containsOnlyLiveDocs=true") and this must be unchangeable fixed. Also DocIdSet is a class not really related solely to Filters, e.g. Scorer extends DocIdSetIterator or DocsEnum extends DocIdSetIterator, Solr Facetting uses DocIdSet. DocIdSet is just a holder class for a bunch of documents exposing a iterator (and a Bits API - this is why I want two getter methods and no interface magic)). The existence of live docs is outside it's scope. I therefore would like a similar API like for scorers, so IndexSearcher can ask the Filter for a DocIdSet based on the given liveDocs (like the scorer method in Weights). The returned DocIdSet would not know if it only has live Docs or not (as the Scorer itsself also does not expose this information). CachingWrapperFilter is little bit special, but this one would always ask the wrapped Filter for a DocidSet without deletions and cache that one, but always return a FilteredDocIdSet bringing the liveDocs passed from IndexSearcher in. The cache would then always be without LiveDocs and easier to maintain. Reopening segments would never need to reload cache. CachingWrapperFilter would just decide on the fact if IndexSearcher passes a liveDocs BitSet or not, if it needs to use it or not (in its own getDocIdSet method). If we have a query and only filter some documents, IndexSearcher already knows about liveDocs from the main scorer and would pass null to the filter. This would remove lots of additional checks to liveDocs. Only the main scorer would know about them, the filter will ignore them (so there is no overhead in CachingWrapperFilter, as it can return the cached filter directly to IndexSearcher, without wrapping). QueryWrapperFilter could pass the liveDocs through the wrapped filter, too. I may have time today to implement some parts of this, should not be to difficult.
          Hide
          Chris Male added a comment -

          Okay thats alot to take in again.

          You've made a good case for dropping setContainsOnlyLiveDocs, I totally agree. I really do like the idea of adding the acceptDocs to Filter.getDocIdSet.

          I'm also comfortable with adding .bits() to DocIdSet to address the typing problem.

          Should we bash out a quick patch making these changes and see how it looks?

          Show
          Chris Male added a comment - Okay thats alot to take in again. You've made a good case for dropping setContainsOnlyLiveDocs, I totally agree. I really do like the idea of adding the acceptDocs to Filter.getDocIdSet. I'm also comfortable with adding .bits() to DocIdSet to address the typing problem. Should we bash out a quick patch making these changes and see how it looks?
          Hide
          Uwe Schindler added a comment -

          +1, I have to revert here a lot again because I was trying to move the setLiveDocsOnly/liveDocsOnly down to FixedBitSet & Co, but this is too complicated.

          Should I start to hack something together? The biuggest change will be in all filter impls to add the parameter to getDocIdSet().

          Show
          Uwe Schindler added a comment - +1, I have to revert here a lot again because I was trying to move the setLiveDocsOnly/liveDocsOnly down to FixedBitSet & Co, but this is too complicated. Should I start to hack something together? The biuggest change will be in all filter impls to add the parameter to getDocIdSet().
          Hide
          Chris Male added a comment -

          Yes please put something together and then we'll review / iterate.

          Show
          Chris Male added a comment - Yes please put something together and then we'll review / iterate.
          Hide
          Uwe Schindler added a comment -

          A first rewrite of Lucene core to pass acceptDocs down to Filter.getDocIdSet:

          • optimized and simpliefied CachingWrapper* - no deletesmode anymore
          • FieldCacheTermsFilter has optimized DocIdSet
          • Added bits() to all DocIdSet
          • IndexSearcher.searchWithFilter was rewritten to pass liveDocs down.
          • AndBits is no longer needed

          The tests are not yet rewritten, still 55 compile errors.... This patch is just for review

          Show
          Uwe Schindler added a comment - A first rewrite of Lucene core to pass acceptDocs down to Filter.getDocIdSet: optimized and simpliefied CachingWrapper* - no deletesmode anymore FieldCacheTermsFilter has optimized DocIdSet Added bits() to all DocIdSet IndexSearcher.searchWithFilter was rewritten to pass liveDocs down. AndBits is no longer needed The tests are not yet rewritten, still 55 compile errors.... This patch is just for review
          Hide
          Robert Muir added a comment -
          I therefore would like a similar API like for scorers, so IndexSearcher can ask the Filter for a DocIdSet based on the given liveDocs (like the scorer method in Weights).
          

          If this is the case, then in the !randomAccess path of indexsearcher.java please pass null as liveDocs.

          Show
          Robert Muir added a comment - I therefore would like a similar API like for scorers, so IndexSearcher can ask the Filter for a DocIdSet based on the given liveDocs (like the scorer method in Weights). If this is the case, then in the !randomAccess path of indexsearcher.java please pass null as liveDocs.
          Hide
          Robert Muir added a comment -

          adding back this optimization, again.

          before committing please give me time to write tests to ensure we aren't losing these optimizations.

          Show
          Robert Muir added a comment - adding back this optimization, again. before committing please give me time to write tests to ensure we aren't losing these optimizations.
          Hide
          Uwe Schindler added a comment -

          Robert, thanks!

          I missed this line:

          Bits acceptDocs = filterContainsLiveDocs ? null : context.reader.getLiveDocs();
          

          As we now always use live docs in filter this would always be null!

          Show
          Uwe Schindler added a comment - Robert, thanks! I missed this line: Bits acceptDocs = filterContainsLiveDocs ? null : context.reader.getLiveDocs(); As we now always use live docs in filter this would always be null!
          Hide
          Uwe Schindler added a comment -

          Here a patch with almost all core tests rewritten (I left out the CachingWrapper tests, as I nuked DeletesMode). Its just for demonstartion.

          Some tests have really stupid filters and work only with optimized indexes. I added asserts in those filters (except one), that acceptDocs==null. The remaining one uses QueryUtils and I have no idea whats going on there, that the acceptDocs!=null.

          When looking at the code in IndexSearcher, I would propose to remove all Filter special handling in IndexSaercher and move all code over to FilteredQuery (with all our optimizations). If you call IS.search(query, filter,...), IndexSearcher would simply wrap with FilteredQuery and we would have no code duplication and much easier maintainability in IS.

          Show
          Uwe Schindler added a comment - Here a patch with almost all core tests rewritten (I left out the CachingWrapper tests, as I nuked DeletesMode). Its just for demonstartion. Some tests have really stupid filters and work only with optimized indexes. I added asserts in those filters (except one), that acceptDocs==null. The remaining one uses QueryUtils and I have no idea whats going on there, that the acceptDocs!=null. When looking at the code in IndexSearcher, I would propose to remove all Filter special handling in IndexSaercher and move all code over to FilteredQuery (with all our optimizations). If you call IS.search(query, filter,...), IndexSearcher would simply wrap with FilteredQuery and we would have no code duplication and much easier maintainability in IS.
          Hide
          Robert Muir added a comment -

          When looking at the code in IndexSearcher, I would propose to remove all Filter special handling in IndexSaercher and move all code over to FilteredQuery (with all our optimizations). If you call IS.search(query, filter,...), IndexSearcher would simply wrap with FilteredQuery and we would have no code duplication and much easier maintainability in IS.

          +1

          Also, we can nuke AndBits.java now?

          Show
          Robert Muir added a comment - When looking at the code in IndexSearcher, I would propose to remove all Filter special handling in IndexSaercher and move all code over to FilteredQuery (with all our optimizations). If you call IS.search(query, filter,...), IndexSearcher would simply wrap with FilteredQuery and we would have no code duplication and much easier maintainability in IS. +1 Also, we can nuke AndBits.java now?
          Hide
          Uwe Schindler added a comment -

          Also, we can nuke AndBits.java now?

          It was nuked here, but still made it into the patch

          Show
          Uwe Schindler added a comment - Also, we can nuke AndBits.java now? It was nuked here, but still made it into the patch
          Hide
          Uwe Schindler added a comment -

          New patch (still only Lucene Core, no contrib/modules/solr modified):

          • Nuked Filter handling completely from IndexSearcher. Algorithms and Random access optimizations were added to FilteredQuery. IS.search(Query, Filter,...) now only wraps the query with the Filter, if filter!=null (small helper method).
          • The random access threshhold is still in IndexSearcher.setFilterRandomAccessThreshold(), FilteredQuery gets it in it's weight from IndexSearcher. This is maybe not the best solutions, we can also add a setter to FilteredQuery and IS passes it to FilteredQuery.

          What do you think? Mike: Can you do perf tests?

          Show
          Uwe Schindler added a comment - New patch (still only Lucene Core, no contrib/modules/solr modified): Nuked Filter handling completely from IndexSearcher. Algorithms and Random access optimizations were added to FilteredQuery. IS.search(Query, Filter,...) now only wraps the query with the Filter, if filter!=null (small helper method). The random access threshhold is still in IndexSearcher.setFilterRandomAccessThreshold(), FilteredQuery gets it in it's weight from IndexSearcher. This is maybe not the best solutions, we can also add a setter to FilteredQuery and IS passes it to FilteredQuery. What do you think? Mike: Can you do perf tests?
          Hide
          Michael McCandless added a comment -

          I will do perf tests! Working on getting luceneutil to do random filters... but could be a few days (I'm offline for the next 3 days) unless I can commit to luceneutil and someone else can run the tests...

          Show
          Michael McCandless added a comment - I will do perf tests! Working on getting luceneutil to do random filters... but could be a few days (I'm offline for the next 3 days) unless I can commit to luceneutil and someone else can run the tests...
          Hide
          Uwe Schindler added a comment -

          I will add further tests tomorrow, to test all code paths in FilteredQuery. There is a short-circuit (it implements Scorer.score(Collector) for fast top-scorer as it existed in IndexSearcher.searchWithFilter before. To test the standard scorer behavior (nextDoc/advance), a test should be added that adds FilteredQuery as clause with others to a BQ, so ConjunctionScorer tries nextDoc/advance.

          Somebody else might look at the scorer and double check. I had to rewrite FilteredQuery#Weight#Scorer, as the filterIter is already advanced to first doc (to check the random access threshold).

          Show
          Uwe Schindler added a comment - I will add further tests tomorrow, to test all code paths in FilteredQuery. There is a short-circuit (it implements Scorer.score(Collector) for fast top-scorer as it existed in IndexSearcher.searchWithFilter before. To test the standard scorer behavior (nextDoc/advance), a test should be added that adds FilteredQuery as clause with others to a BQ, so ConjunctionScorer tries nextDoc/advance. Somebody else might look at the scorer and double check. I had to rewrite FilteredQuery#Weight#Scorer, as the filterIter is already advanced to first doc (to check the random access threshold).
          Hide
          Uwe Schindler added a comment -

          New patch:

          • Fixed the FilteredQuery-Scorer's advance by logic change. Its now much easier to understand. The corresponding tests are in TestFilteredQuery: All tests are executed 2 times: as random access filter and as iterator filter. Also FilteredQuery is added to BQ, so the conventional scorer (nextDoc/advance) is tested.

          The tests for CachingWrapper* are still disabled, have to rewrite them tomorrow. Then we can change contrib and Solr.

          Show
          Uwe Schindler added a comment - New patch: Fixed the FilteredQuery-Scorer's advance by logic change. Its now much easier to understand. The corresponding tests are in TestFilteredQuery: All tests are executed 2 times: as random access filter and as iterator filter. Also FilteredQuery is added to BQ, so the conventional scorer (nextDoc/advance) is tested. The tests for CachingWrapper* are still disabled, have to rewrite them tomorrow. Then we can change contrib and Solr.
          Hide
          Uwe Schindler added a comment -

          Improved patch with modules and contrib fixed, Solr still on TODO:

          • BooleanFilter and ChainedFilter do not apply acceptDocs to their Filter clauses. Instead they apply the acceptDocs on the final DocIdSet using BitsFilteredDocIdSet (see below). This improves filter performance, as the deleted documents are not applied on each clause.
          • New helper class BitsFilteredDocIdSet, which supplies a wrap method, that can apply a Bits (e.g. acceptDocs) to a DocIdSet. This is useful for Filters, that build DocIdSets without respecting the acceptDocs parameter and only want to apply the deletions live.
          • CachingWrapperFilter and CachingSpanFilter now also use the acceptDocs wrapper, as the filters are cached without acceptDocs.
          • Fixed Javadocs, small changes to IndexSearcher
          Show
          Uwe Schindler added a comment - Improved patch with modules and contrib fixed, Solr still on TODO: BooleanFilter and ChainedFilter do not apply acceptDocs to their Filter clauses. Instead they apply the acceptDocs on the final DocIdSet using BitsFilteredDocIdSet (see below). This improves filter performance, as the deleted documents are not applied on each clause. New helper class BitsFilteredDocIdSet, which supplies a wrap method, that can apply a Bits (e.g. acceptDocs) to a DocIdSet. This is useful for Filters, that build DocIdSets without respecting the acceptDocs parameter and only want to apply the deletions live. CachingWrapperFilter and CachingSpanFilter now also use the acceptDocs wrapper, as the filters are cached without acceptDocs. Fixed Javadocs, small changes to IndexSearcher
          Hide
          Robert Muir added a comment -

          This looks awesome!

          Show
          Robert Muir added a comment - This looks awesome!
          Hide
          Robert Muir added a comment -

          updated patch: i fixed solr to the api changes and simply disabled the optimization in SolrIndexSearcher.

          I think this is the most conservative way to proceed, we can then open a followup issue to make whatever changes are necessary to Solr APIs so it can use the optimization (looks complex)

          Show
          Robert Muir added a comment - updated patch: i fixed solr to the api changes and simply disabled the optimization in SolrIndexSearcher. I think this is the most conservative way to proceed, we can then open a followup issue to make whatever changes are necessary to Solr APIs so it can use the optimization (looks complex)
          Hide
          Chris Male added a comment -

          Awesome idea Robert, I was staring at the Solr code a little bewildered about how to integrate the optimization.

          Show
          Chris Male added a comment - Awesome idea Robert, I was staring at the Solr code a little bewildered about how to integrate the optimization.
          Hide
          Chris Male added a comment -

          Apart from Mike's benchmarks, are we waiting on any further changes to the patch?

          Show
          Chris Male added a comment - Apart from Mike's benchmarks, are we waiting on any further changes to the patch?
          Hide
          Robert Muir added a comment -

          Just until the policeman says 'final patch'

          Show
          Robert Muir added a comment - Just until the policeman says 'final patch'
          Hide
          Uwe Schindler added a comment -

          Chris: I have to fix the CachingWrapper tests (soon). And add some acceptDocs to solr, which Robert simply ignored.

          Show
          Uwe Schindler added a comment - Chris: I have to fix the CachingWrapper tests (soon). And add some acceptDocs to solr, which Robert simply ignored.
          Hide
          Uwe Schindler added a comment -

          Patch with Solr fixes by Robert improved. Now usage of acceptDocs is correct. There is room to optimize, but this is the correct way to solve (as Filters are required to respect deleted docs in trunk).

          Now the remaining caching wrapper changes, then we are ready.

          Show
          Uwe Schindler added a comment - Patch with Solr fixes by Robert improved. Now usage of acceptDocs is correct. There is room to optimize, but this is the correct way to solve (as Filters are required to respect deleted docs in trunk). Now the remaining caching wrapper changes, then we are ready.
          Hide
          Yonik Seeley added a comment - - edited

          At what levels of bitset sparseness does it make sense to use random access? I ask because sometimes Solr actually knows the sparseness of it's sets.

          Show
          Yonik Seeley added a comment - - edited At what levels of bitset sparseness does it make sense to use random access? I ask because sometimes Solr actually knows the sparseness of it's sets.
          Hide
          Chris Male added a comment -

          Mike suggested earlier in the issue anything denser than 1% sees benefits from random-access.

          Show
          Chris Male added a comment - Mike suggested earlier in the issue anything denser than 1% sees benefits from random-access.
          Hide
          Yonik Seeley added a comment -

          OK, thanks Chris. I'll take a shot at optimizing this patch for Solr a bit more.

          Show
          Yonik Seeley added a comment - OK, thanks Chris. I'll take a shot at optimizing this patch for Solr a bit more.
          Hide
          Jason Rutherglen added a comment -

          +1

          Show
          Jason Rutherglen added a comment - +1
          Hide
          Uwe Schindler added a comment -

          Final patch.

          The test for CachingWrapperFilter/CachingSpanFilter were simplified to only check that they actually respect deletions.

          I think thats ready to commit after some perf testing.

          Robert: If you have time?

          Show
          Uwe Schindler added a comment - Final patch. The test for CachingWrapperFilter/CachingSpanFilter were simplified to only check that they actually respect deletions. I think thats ready to commit after some perf testing. Robert: If you have time?
          Hide
          Yonik Seeley added a comment -

          OK, I'll start again from the final patch.

          Show
          Yonik Seeley added a comment - OK, I'll start again from the final patch.
          Hide
          Uwe Schindler added a comment -

          Sorry for interrupting you. I only changed Lucene classes, if that helps. With TortoiseSVN you can partly apply patches.

          Show
          Uwe Schindler added a comment - Sorry for interrupting you. I only changed Lucene classes, if that helps. With TortoiseSVN you can partly apply patches.
          Hide
          Robert Muir added a comment -

          Robert: If you have time?

          I'm attempting to benchmark the patch now with the example tasks mike added to luceneutil early this morning:
          http://code.google.com/a/apache-extras.org/p/luceneutil/source/browse/eg.filter.tasks?spec=svn3ea6dafca66a00e1dbf4563d1098b7418e386cbf&r=3ea6dafca66a00e1dbf4563d1098b7418e386cbf

          I'll report back if i'm able to get results... takes a few hours here.

          Show
          Robert Muir added a comment - Robert: If you have time? I'm attempting to benchmark the patch now with the example tasks mike added to luceneutil early this morning: http://code.google.com/a/apache-extras.org/p/luceneutil/source/browse/eg.filter.tasks?spec=svn3ea6dafca66a00e1dbf4563d1098b7418e386cbf&r=3ea6dafca66a00e1dbf4563d1098b7418e386cbf I'll report back if i'm able to get results... takes a few hours here.
          Hide
          Robert Muir added a comment -

          Here's the results... F0.1 for example means filter accepting a random 0.1% of documents.

                          Task   QPS trunkStdDev trunk   QPS patchStdDev patch      Pct diff
                    PhraseF0.1       67.61        1.89       29.85        2.52  -60% -  -50%
                    PhraseF0.5       20.08        0.72       13.09        1.11  -42% -  -26%
                    PhraseF1.0       12.37        0.46        8.84        0.88  -37% -  -18%
                OrHighHighF0.1       78.84        1.19       59.96        2.87  -28% -  -19%
                      TermF0.5      133.27        4.80      125.91        7.29  -14% -    3%
                    OrHighHigh       12.73        0.45       12.13        0.92  -14% -    6%
                        Fuzzy1       57.63        1.70       56.62        2.33   -8% -    5%
                        Fuzzy2       96.92        2.25       96.19        2.63   -5% -    4%
             AndHighHighF100.0       16.99        0.50       16.92        1.38  -11% -   10%
              AndHighHighF99.0       17.00        0.48       16.94        1.37  -10% -   10%
              AndHighHighF95.0       17.00        0.48       16.98        1.35  -10% -   10%
                    Fuzzy2F0.1      107.24        2.74      107.29        2.68   -4% -    5%
              AndHighHighF90.0       17.04        0.47       17.13        1.36   -9% -   11%
                    Fuzzy1F0.1       74.60        1.58       75.03        1.55   -3% -    4%
            SloppyPhraseF100.0        7.82        0.16        7.89        0.24   -4% -    6%
             SloppyPhraseF99.0        7.82        0.16        7.92        0.23   -3% -    6%
                  Fuzzy2F100.0       97.16        2.31       98.43        2.19   -3% -    6%
                      PKLookup      171.71        6.83      174.15        7.28   -6% -   10%
                  WildcardF0.1       67.96        1.06       69.08        1.95   -2% -    6%
                      Wildcard       43.40        0.89       44.13        0.92   -2% -    5%
                   Fuzzy2F99.0       96.83        2.46       98.49        2.21   -3% -    6%
                   Fuzzy2F95.0       97.01        2.47       98.79        2.18   -2% -    6%
                SpanNearF100.0        3.11        0.04        3.18        0.09   -1% -    6%
              AndHighHighF75.0       17.13        0.48       17.57        1.36   -7% -   13%
                   Fuzzy2F90.0       97.01        2.53       99.49        2.10   -2% -    7%
                OrHighHighF0.5       31.57        0.45       32.41        1.07   -2% -    7%
             SloppyPhraseF95.0        7.82        0.18        8.03        0.25   -2% -    8%
                 SpanNearF99.0        3.11        0.04        3.20        0.09   -1% -    7%
               AndHighHighF0.1      136.96        3.21      140.94        5.15   -3% -    9%
              SloppyPhraseF0.1       56.27        0.88       57.97        1.47   -1% -    7%
                    Fuzzy2F0.5      100.39        2.48      103.57        2.47   -1% -    8%
                    PhraseF2.0        7.95        0.31        8.20        0.65   -8% -   15%
                   AndHighHigh       17.97        0.46       18.55        0.84   -3% -   10%
                      TermF0.1      351.76        9.38      363.42       16.25   -3% -   10%
                  SloppyPhrase        7.90        0.16        8.19        0.19    0% -    8%
                        Phrase        3.69        0.12        3.83        0.13   -3% -   10%
                  WildcardF0.5       62.57        0.88       65.31        2.07    0% -    9%
             SloppyPhraseF90.0        7.83        0.16        8.18        0.24    0% -    9%
                   Fuzzy2F75.0       96.77        2.46      101.14        2.41    0% -    9%
                      SpanNear        3.15        0.04        3.30        0.07    1% -    8%
                          Term       71.54        4.98       74.98        5.61   -9% -   21%
                 SpanNearF95.0        3.11        0.05        3.26        0.09    0% -    9%
                  PhraseF100.0        3.49        0.13        3.68        0.15   -2% -   14%
                   PhraseF99.0        3.49        0.12        3.69        0.15   -2% -   14%
                  SpanNearF0.1       31.54        0.48       33.49        0.73    2% -   10%
                   PhraseF95.0        3.49        0.12        3.72        0.16   -1% -   15%
                 SpanNearF90.0        3.12        0.04        3.35        0.09    3% -   11%
                   Fuzzy2F50.0       97.08        2.32      104.79        2.66    2% -   13%
                   PhraseF90.0        3.49        0.13        3.78        0.16    0% -   17%
                  Fuzzy1F100.0       47.68        1.41       52.27        1.08    4% -   15%
                   Fuzzy1F99.0       47.57        1.49       52.28        1.19    4% -   16%
              AndHighHighF50.0       17.30        0.48       19.12        1.47    0% -   22%
                  WildcardF1.0       58.03        0.81       64.32        2.40    5% -   16%
                   Fuzzy1F95.0       47.59        1.50       52.84        1.17    5% -   17%
             SloppyPhraseF75.0        7.85        0.15        8.73        0.24    6% -   16%
                    Fuzzy2F1.0       98.59        2.36      110.12        2.89    6% -   17%
                   Fuzzy1F90.0       47.51        1.40       53.54        1.09    7% -   18%
                   PhraseF75.0        3.51        0.13        3.98        0.18    4% -   22%
                      TermF1.0       92.28        3.05      104.56        7.44    1% -   25%
                 WildcardF99.0       36.01        0.76       40.88        1.16    8% -   19%
                    Fuzzy1F0.5       59.00        1.10       67.10        1.36    9% -   18%
                WildcardF100.0       35.92        0.79       40.86        1.19    8% -   19%
                 WildcardF95.0       36.01        0.75       41.02        1.19    8% -   19%
                 WildcardF90.0       36.06        0.70       41.14        1.20    8% -   19%
                   Fuzzy2F20.0       98.32        2.34      112.69        2.91    9% -   20%
                 WildcardF75.0       36.19        0.62       41.69        1.15   10% -   20%
               AndHighHighF0.5       49.93        1.37       57.85        4.13    4% -   27%
                   Fuzzy1F75.0       47.25        1.50       55.55        1.11   11% -   23%
                   Fuzzy2F10.0       98.47        2.46      116.18        3.00   12% -   24%
                 WildcardF50.0       36.77        0.55       43.44        1.29   12% -   23%
                OrHighHighF1.0       24.37        0.38       28.99        1.90    9% -   28%
                    Fuzzy1F2.0       52.64        1.05       63.12        1.32   15% -   24%
                 SpanNearF75.0        3.11        0.04        3.74        0.10   15% -   24%
                    Fuzzy2F5.0       97.96        2.31      118.02        3.48   14% -   27%
                    Fuzzy2F2.0       98.02        2.22      119.13        3.42   15% -   27%
              OrHighHighF100.0        7.70        0.34        9.51        0.34   13% -   33%
               OrHighHighF99.0        7.70        0.36        9.56        0.34   14% -   34%
                   Fuzzy1F50.0       47.46        1.24       59.15        1.18   19% -   30%
                   PhraseF50.0        3.57        0.12        4.45        0.23   14% -   35%
               OrHighHighF95.0        7.73        0.35        9.73        0.35   16% -   36%
             SloppyPhraseF50.0        7.92        0.16       10.09        0.28   21% -   33%
                  WildcardF2.0       53.32        0.69       68.29        3.44   20% -   36%
               OrHighHighF90.0        7.77        0.35        9.97        0.35   18% -   39%
                 WildcardF20.0       41.13        0.60       54.63        2.12   25% -   39%
               OrHighHighF75.0        7.91        0.32       10.73        0.36   26% -   45%
                  WildcardF5.0       47.44        0.57       65.42        3.11   29% -   46%
                 WildcardF10.0       44.01        0.53       61.16        2.61   31% -   46%
                   Fuzzy1F20.0       49.57        1.20       69.49        1.70   33% -   47%
                    Fuzzy1F1.0       54.39        1.07       76.95        2.03   35% -   48%
               AndHighHighF1.0       34.63        1.07       50.01        4.02   28% -   60%
                    PhraseF5.0        5.16        0.20        7.61        0.75   27% -   68%
                   Fuzzy1F10.0       50.23        1.07       75.36        2.11   42% -   57%
               OrHighHighF50.0        8.36        0.29       12.58        0.48   39% -   61%
                OrHighHighF2.0       19.65        0.34       29.58        2.27   36% -   65%
                 SpanNearF50.0        3.11        0.04        4.76        0.12   47% -   58%
                      TermF2.0       68.99        2.38      106.22        8.65   36% -   72%
                    Fuzzy1F5.0       50.74        1.06       79.90        2.38   49% -   65%
                   PhraseF20.0        3.81        0.13        6.10        0.45   43% -   78%
                     TermF50.0       42.19        1.41       67.96        4.63   45% -   77%
                     TermF75.0       41.36        1.46       67.47        5.30   45% -   82%
                     TermF90.0       41.05        1.47       68.08        5.85   46% -   86%
                     TermF95.0       41.03        1.49       68.08        6.14   45% -   87%
                   PhraseF10.0        4.22        0.16        7.02        0.62   46% -   87%
                     TermF99.0       40.99        1.56       68.31        6.21   45% -   89%
                    TermF100.0       40.88        1.61       68.28        6.32   45% -   89%
              SloppyPhraseF0.5       18.81        0.30       31.53        0.96   59% -   75%
              AndHighHighF20.0       17.62        0.52       30.63        2.79   53% -   95%
                OrHighHighF5.0       14.99        0.29       27.44        1.98   66% -  100%
                  SpanNearF0.5        9.17        0.12       17.12        0.42   79% -   93%
                     TermF20.0       45.25        1.50       84.63        6.04   68% -  107%
               OrHighHighF20.0       10.35        0.25       19.60        1.08   74% -  104%
                      TermF5.0       52.49        1.71       99.90        8.02   69% -  112%
               AndHighHighF2.0       25.97        0.81       50.45        4.72   70% -  119%
               OrHighHighF10.0       12.36        0.22       24.25        1.56   80% -  112%
                     TermF10.0       46.97        1.47       92.60        7.08   76% -  119%
             SloppyPhraseF20.0        8.18        0.16       16.35        0.58   89% -  111%
                  SpanNearF1.0        6.05        0.09       12.21        0.28   94% -  109%
              AndHighHighF10.0       18.44        0.55       40.77        4.15   92% -  151%
               AndHighHighF5.0       20.34        0.63       50.83        5.67  115% -  186%
             SloppyPhraseF10.0        8.52        0.17       22.79        0.96  151% -  184%
                 SpanNearF20.0        3.15        0.05        9.03        0.24  174% -  198%
              SloppyPhraseF1.0       13.62        0.23       42.77        2.29  192% -  236%
                  SpanNearF2.0        4.45        0.06       14.31        0.37  209% -  234%
              SloppyPhraseF5.0        9.12        0.17       29.98        1.41  207% -  250%
              SloppyPhraseF2.0       10.85        0.19       38.31        2.00  229% -  278%
                 SpanNearF10.0        3.25        0.05       13.71        0.39  303% -  339%
                  SpanNearF5.0        3.52        0.05       19.51        0.67  428% -  481%
          
          Show
          Robert Muir added a comment - Here's the results... F0.1 for example means filter accepting a random 0.1% of documents. Task QPS trunkStdDev trunk QPS patchStdDev patch Pct diff PhraseF0.1 67.61 1.89 29.85 2.52 -60% - -50% PhraseF0.5 20.08 0.72 13.09 1.11 -42% - -26% PhraseF1.0 12.37 0.46 8.84 0.88 -37% - -18% OrHighHighF0.1 78.84 1.19 59.96 2.87 -28% - -19% TermF0.5 133.27 4.80 125.91 7.29 -14% - 3% OrHighHigh 12.73 0.45 12.13 0.92 -14% - 6% Fuzzy1 57.63 1.70 56.62 2.33 -8% - 5% Fuzzy2 96.92 2.25 96.19 2.63 -5% - 4% AndHighHighF100.0 16.99 0.50 16.92 1.38 -11% - 10% AndHighHighF99.0 17.00 0.48 16.94 1.37 -10% - 10% AndHighHighF95.0 17.00 0.48 16.98 1.35 -10% - 10% Fuzzy2F0.1 107.24 2.74 107.29 2.68 -4% - 5% AndHighHighF90.0 17.04 0.47 17.13 1.36 -9% - 11% Fuzzy1F0.1 74.60 1.58 75.03 1.55 -3% - 4% SloppyPhraseF100.0 7.82 0.16 7.89 0.24 -4% - 6% SloppyPhraseF99.0 7.82 0.16 7.92 0.23 -3% - 6% Fuzzy2F100.0 97.16 2.31 98.43 2.19 -3% - 6% PKLookup 171.71 6.83 174.15 7.28 -6% - 10% WildcardF0.1 67.96 1.06 69.08 1.95 -2% - 6% Wildcard 43.40 0.89 44.13 0.92 -2% - 5% Fuzzy2F99.0 96.83 2.46 98.49 2.21 -3% - 6% Fuzzy2F95.0 97.01 2.47 98.79 2.18 -2% - 6% SpanNearF100.0 3.11 0.04 3.18 0.09 -1% - 6% AndHighHighF75.0 17.13 0.48 17.57 1.36 -7% - 13% Fuzzy2F90.0 97.01 2.53 99.49 2.10 -2% - 7% OrHighHighF0.5 31.57 0.45 32.41 1.07 -2% - 7% SloppyPhraseF95.0 7.82 0.18 8.03 0.25 -2% - 8% SpanNearF99.0 3.11 0.04 3.20 0.09 -1% - 7% AndHighHighF0.1 136.96 3.21 140.94 5.15 -3% - 9% SloppyPhraseF0.1 56.27 0.88 57.97 1.47 -1% - 7% Fuzzy2F0.5 100.39 2.48 103.57 2.47 -1% - 8% PhraseF2.0 7.95 0.31 8.20 0.65 -8% - 15% AndHighHigh 17.97 0.46 18.55 0.84 -3% - 10% TermF0.1 351.76 9.38 363.42 16.25 -3% - 10% SloppyPhrase 7.90 0.16 8.19 0.19 0% - 8% Phrase 3.69 0.12 3.83 0.13 -3% - 10% WildcardF0.5 62.57 0.88 65.31 2.07 0% - 9% SloppyPhraseF90.0 7.83 0.16 8.18 0.24 0% - 9% Fuzzy2F75.0 96.77 2.46 101.14 2.41 0% - 9% SpanNear 3.15 0.04 3.30 0.07 1% - 8% Term 71.54 4.98 74.98 5.61 -9% - 21% SpanNearF95.0 3.11 0.05 3.26 0.09 0% - 9% PhraseF100.0 3.49 0.13 3.68 0.15 -2% - 14% PhraseF99.0 3.49 0.12 3.69 0.15 -2% - 14% SpanNearF0.1 31.54 0.48 33.49 0.73 2% - 10% PhraseF95.0 3.49 0.12 3.72 0.16 -1% - 15% SpanNearF90.0 3.12 0.04 3.35 0.09 3% - 11% Fuzzy2F50.0 97.08 2.32 104.79 2.66 2% - 13% PhraseF90.0 3.49 0.13 3.78 0.16 0% - 17% Fuzzy1F100.0 47.68 1.41 52.27 1.08 4% - 15% Fuzzy1F99.0 47.57 1.49 52.28 1.19 4% - 16% AndHighHighF50.0 17.30 0.48 19.12 1.47 0% - 22% WildcardF1.0 58.03 0.81 64.32 2.40 5% - 16% Fuzzy1F95.0 47.59 1.50 52.84 1.17 5% - 17% SloppyPhraseF75.0 7.85 0.15 8.73 0.24 6% - 16% Fuzzy2F1.0 98.59 2.36 110.12 2.89 6% - 17% Fuzzy1F90.0 47.51 1.40 53.54 1.09 7% - 18% PhraseF75.0 3.51 0.13 3.98 0.18 4% - 22% TermF1.0 92.28 3.05 104.56 7.44 1% - 25% WildcardF99.0 36.01 0.76 40.88 1.16 8% - 19% Fuzzy1F0.5 59.00 1.10 67.10 1.36 9% - 18% WildcardF100.0 35.92 0.79 40.86 1.19 8% - 19% WildcardF95.0 36.01 0.75 41.02 1.19 8% - 19% WildcardF90.0 36.06 0.70 41.14 1.20 8% - 19% Fuzzy2F20.0 98.32 2.34 112.69 2.91 9% - 20% WildcardF75.0 36.19 0.62 41.69 1.15 10% - 20% AndHighHighF0.5 49.93 1.37 57.85 4.13 4% - 27% Fuzzy1F75.0 47.25 1.50 55.55 1.11 11% - 23% Fuzzy2F10.0 98.47 2.46 116.18 3.00 12% - 24% WildcardF50.0 36.77 0.55 43.44 1.29 12% - 23% OrHighHighF1.0 24.37 0.38 28.99 1.90 9% - 28% Fuzzy1F2.0 52.64 1.05 63.12 1.32 15% - 24% SpanNearF75.0 3.11 0.04 3.74 0.10 15% - 24% Fuzzy2F5.0 97.96 2.31 118.02 3.48 14% - 27% Fuzzy2F2.0 98.02 2.22 119.13 3.42 15% - 27% OrHighHighF100.0 7.70 0.34 9.51 0.34 13% - 33% OrHighHighF99.0 7.70 0.36 9.56 0.34 14% - 34% Fuzzy1F50.0 47.46 1.24 59.15 1.18 19% - 30% PhraseF50.0 3.57 0.12 4.45 0.23 14% - 35% OrHighHighF95.0 7.73 0.35 9.73 0.35 16% - 36% SloppyPhraseF50.0 7.92 0.16 10.09 0.28 21% - 33% WildcardF2.0 53.32 0.69 68.29 3.44 20% - 36% OrHighHighF90.0 7.77 0.35 9.97 0.35 18% - 39% WildcardF20.0 41.13 0.60 54.63 2.12 25% - 39% OrHighHighF75.0 7.91 0.32 10.73 0.36 26% - 45% WildcardF5.0 47.44 0.57 65.42 3.11 29% - 46% WildcardF10.0 44.01 0.53 61.16 2.61 31% - 46% Fuzzy1F20.0 49.57 1.20 69.49 1.70 33% - 47% Fuzzy1F1.0 54.39 1.07 76.95 2.03 35% - 48% AndHighHighF1.0 34.63 1.07 50.01 4.02 28% - 60% PhraseF5.0 5.16 0.20 7.61 0.75 27% - 68% Fuzzy1F10.0 50.23 1.07 75.36 2.11 42% - 57% OrHighHighF50.0 8.36 0.29 12.58 0.48 39% - 61% OrHighHighF2.0 19.65 0.34 29.58 2.27 36% - 65% SpanNearF50.0 3.11 0.04 4.76 0.12 47% - 58% TermF2.0 68.99 2.38 106.22 8.65 36% - 72% Fuzzy1F5.0 50.74 1.06 79.90 2.38 49% - 65% PhraseF20.0 3.81 0.13 6.10 0.45 43% - 78% TermF50.0 42.19 1.41 67.96 4.63 45% - 77% TermF75.0 41.36 1.46 67.47 5.30 45% - 82% TermF90.0 41.05 1.47 68.08 5.85 46% - 86% TermF95.0 41.03 1.49 68.08 6.14 45% - 87% PhraseF10.0 4.22 0.16 7.02 0.62 46% - 87% TermF99.0 40.99 1.56 68.31 6.21 45% - 89% TermF100.0 40.88 1.61 68.28 6.32 45% - 89% SloppyPhraseF0.5 18.81 0.30 31.53 0.96 59% - 75% AndHighHighF20.0 17.62 0.52 30.63 2.79 53% - 95% OrHighHighF5.0 14.99 0.29 27.44 1.98 66% - 100% SpanNearF0.5 9.17 0.12 17.12 0.42 79% - 93% TermF20.0 45.25 1.50 84.63 6.04 68% - 107% OrHighHighF20.0 10.35 0.25 19.60 1.08 74% - 104% TermF5.0 52.49 1.71 99.90 8.02 69% - 112% AndHighHighF2.0 25.97 0.81 50.45 4.72 70% - 119% OrHighHighF10.0 12.36 0.22 24.25 1.56 80% - 112% TermF10.0 46.97 1.47 92.60 7.08 76% - 119% SloppyPhraseF20.0 8.18 0.16 16.35 0.58 89% - 111% SpanNearF1.0 6.05 0.09 12.21 0.28 94% - 109% AndHighHighF10.0 18.44 0.55 40.77 4.15 92% - 151% AndHighHighF5.0 20.34 0.63 50.83 5.67 115% - 186% SloppyPhraseF10.0 8.52 0.17 22.79 0.96 151% - 184% SpanNearF20.0 3.15 0.05 9.03 0.24 174% - 198% SloppyPhraseF1.0 13.62 0.23 42.77 2.29 192% - 236% SpanNearF2.0 4.45 0.06 14.31 0.37 209% - 234% SloppyPhraseF5.0 9.12 0.17 29.98 1.41 207% - 250% SloppyPhraseF2.0 10.85 0.19 38.31 2.00 229% - 278% SpanNearF10.0 3.25 0.05 13.71 0.39 303% - 339% SpanNearF5.0 3.52 0.05 19.51 0.67 428% - 481%
          Hide
          Robert Muir added a comment -

          by the way, luceneutil noticed some problems:

          Traceback (most recent call last):
            File "localrun.py", line 46, in <module>
              comp.benchmark("trunk_vs_patch")
            File "/home/rmuir/workspace/util/competition.py", line 194, in benchmark
              search=self.benchSearch, index=self.benchIndex, debugs=self._debug, debug=self._debug, verifyScores=self._verifyScores)
            File "/home/rmuir/workspace/util/searchBench.py", line 130, in run
              raise RuntimeError('results differ: %s' % str(cmpDiffs))
          RuntimeError: results differ: ([], ['query=body:changer~1.0 filter=CachingWrapperFilter(PreComputedRandomFilter(pctAccept=95.0)): hit 2 has wrong id/s [8684145] vs [6260043, 8684145]', 'query=body:changer~1.0 filter=CachingWrapperFilter(PreComputedRandomFilter(pctAccept=75.0)): wrong collapsed hit count: 4 vs 5', 'query=body:changer~1.0 filter=CachingWrapperFilter(PreComputedRandomFilter(pctAccept=99.0)): hit 2 has wrong id/s [8684145] vs [8043795]'])
          

          I have no idea whats going on, but i'll upload my modifications to these filters to make them work with the patch (maybe i jacked it up).

          Show
          Robert Muir added a comment - by the way, luceneutil noticed some problems: Traceback (most recent call last): File "localrun.py", line 46, in <module> comp.benchmark("trunk_vs_patch") File "/home/rmuir/workspace/util/competition.py", line 194, in benchmark search=self.benchSearch, index=self.benchIndex, debugs=self._debug, debug=self._debug, verifyScores=self._verifyScores) File "/home/rmuir/workspace/util/searchBench.py", line 130, in run raise RuntimeError('results differ: %s' % str(cmpDiffs)) RuntimeError: results differ: ([], ['query=body:changer~1.0 filter=CachingWrapperFilter(PreComputedRandomFilter(pctAccept=95.0)): hit 2 has wrong id/s [8684145] vs [6260043, 8684145]', 'query=body:changer~1.0 filter=CachingWrapperFilter(PreComputedRandomFilter(pctAccept=75.0)): wrong collapsed hit count: 4 vs 5', 'query=body:changer~1.0 filter=CachingWrapperFilter(PreComputedRandomFilter(pctAccept=99.0)): hit 2 has wrong id/s [8684145] vs [8043795]']) I have no idea whats going on, but i'll upload my modifications to these filters to make them work with the patch (maybe i jacked it up).
          Hide
          Uwe Schindler added a comment -

          I nice improvements!

          I dont understand the errors luceneutil prints, sorry. The patch looks correct, I see no issues. acceptDocs are applied consistent and correct. Maybe Mike can help what the messages mean. The question is: How does Luceneutil verifies the hits?

          Show
          Uwe Schindler added a comment - I nice improvements! I dont understand the errors luceneutil prints, sorry. The patch looks correct, I see no issues. acceptDocs are applied consistent and correct. Maybe Mike can help what the messages mean. The question is: How does Luceneutil verifies the hits?
          Hide
          Simon Willnauer added a comment -

          robert, how do you create the index? do you have two different indices for benchmarking or one? if you have two indices it could happen that one contains doc X < doc Y while the other has doc Y < doc X (doc ID wise). if both have the same score you get different order and luceneutil might fail? Just an idea...

          Show
          Simon Willnauer added a comment - robert, how do you create the index? do you have two different indices for benchmarking or one? if you have two indices it could happen that one contains doc X < doc Y while the other has doc Y < doc X (doc ID wise). if both have the same score you get different order and luceneutil might fail? Just an idea...
          Hide
          Uwe Schindler added a comment -

          The strange thing that I see is - two docids for one hit instead of one: "hit 2 has wrong id/s [8684145] vs [6260043, 8684145]" and a little bit later: "wrong collapsed hit count: 4 vs 5" - maybe an unrelated issue with grouping module? Was grouping also enabled during benchmarking? Otherwise I cannot explain those results.

          Simon: Robert said, both tests used the same, ähm, identical index created before.

          Show
          Uwe Schindler added a comment - The strange thing that I see is - two docids for one hit instead of one: "hit 2 has wrong id/s [8684145] vs [6260043, 8684145] " and a little bit later: "wrong collapsed hit count: 4 vs 5" - maybe an unrelated issue with grouping module? Was grouping also enabled during benchmarking? Otherwise I cannot explain those results. Simon: Robert said, both tests used the same, ähm, identical index created before.
          Hide
          Yonik Seeley added a comment -

          Here's an update that passes null where appropriate to prevent extra checking and implements bits() as appropriate.

          Sort of an open question if some of the function query stuff should be changed (ValueSourceScorer, getRangeScorer, etc). That's a more extensive change and can be in a diff issue if necessary.

          Show
          Yonik Seeley added a comment - Here's an update that passes null where appropriate to prevent extra checking and implements bits() as appropriate. Sort of an open question if some of the function query stuff should be changed (ValueSourceScorer, getRangeScorer, etc). That's a more extensive change and can be in a diff issue if necessary.
          Hide
          Uwe Schindler added a comment -

          Hi Yonik, thanks!

          Just to note, if you implement something with "new DocIdSet()

          {....}

          ", there is no need to override bits() to return null, as the default always returns null. Only OpenBitSet/FixedBitSet and some FieldCache filters in Lucene core automatically implement bits() if directly used as DocIdSet.

          So the major changes in your patch are BitDocSet to implement random access and passing null as acceptDocs at some places (see comments)? I will merge all your changes into my checkout.

          Show
          Uwe Schindler added a comment - Hi Yonik, thanks! Just to note, if you implement something with "new DocIdSet() {....} ", there is no need to override bits() to return null, as the default always returns null. Only OpenBitSet/FixedBitSet and some FieldCache filters in Lucene core automatically implement bits() if directly used as DocIdSet. So the major changes in your patch are BitDocSet to implement random access and passing null as acceptDocs at some places (see comments)? I will merge all your changes into my checkout.
          Hide
          Uwe Schindler added a comment -

          Attached you will find a new patch LUCENE-1536.patch, incorporating Yonik's changes plus some minor improvements:

          • changed Javadocs of DIS.bits() to explain what you should do/not do.
          • Added another early exit condition in FilteredQuery#Weight.scorer(): As we already get the first matching doc of the filter iterator before looking at bits or creating the query scorer, we should erly exit, if the first matching doc is Disi.NO_MORE_DOCS. This saves us from creating the Query Scorer.
          • I removed Robert's safety TODO in SolrIndexSearcher. It no longer disabled random access completely. After Yoniks changes, all places in Solr that are not random access secure are disabled - e.g. SolrIndexSearcher.FilterImpl (not sure what this class does, maybe it should also implement bits()?) - we should do that in a Solr specific optimization issue.

          Some other cool thing with filters is ANDing filters without ChainedFilter (this approach is is very effective with random access as it does not allocate additional BitsSet). If you want to AND together several filters and apply them to a Query, do the following:

          IS.search(new FilteredQuery(query,filter2), filter1,...);
          

          You can chain even more filters in by adding more FilteredQueries. What this does:
          IS will automatically create another FilteredQuery to apply the filter and get the Weight of the top-level FilteredQuery. The scorer of this one will be top-level, get the filter and if it is random access, it will execute the filter with acceptDocs==liveDocs. The result bits of this filter will be passed to Weight.scorer of the second FilteredQuery as acceptDocs. This one will pass the acceptDocs (which are already filtered) to its Filter and if again random access pass those as acceptDocs to the inner Query's scorer. Finally the top-level IS will execute scorer.score(Collector), which in fact is the inner Query's scorer (no wrappers!) with all filtering applied in acceptDocs. This is incredible cool

          One thing about large patches in an issue:
          If you are working on an issue and have you local changes in your checkout and posted a patch to an issue and somebody else, posted an updated patch to an issue, it is often nice to see the diff between those patches. I wanted to see what Yonik changed, but a 140 K patch is not easy to handle. The trick is "interdiff" from patchutils package: You can call "interdiff LUCENE-1536-original.patch LUCENE-1536-yonik.patch" and you get a patch of only changes applied by Yonik. This patch can even be applied to your local already patched checkout.

          The changes-yonik-uwe.patch was generated that way and shows, what changes I did in my last patch in contrast to Yoniks original.

          Show
          Uwe Schindler added a comment - Attached you will find a new patch LUCENE-1536 .patch, incorporating Yonik's changes plus some minor improvements: changed Javadocs of DIS.bits() to explain what you should do/not do. Added another early exit condition in FilteredQuery#Weight.scorer(): As we already get the first matching doc of the filter iterator before looking at bits or creating the query scorer, we should erly exit, if the first matching doc is Disi.NO_MORE_DOCS. This saves us from creating the Query Scorer. I removed Robert's safety TODO in SolrIndexSearcher. It no longer disabled random access completely. After Yoniks changes, all places in Solr that are not random access secure are disabled - e.g. SolrIndexSearcher.FilterImpl (not sure what this class does, maybe it should also implement bits()?) - we should do that in a Solr specific optimization issue. Some other cool thing with filters is ANDing filters without ChainedFilter (this approach is is very effective with random access as it does not allocate additional BitsSet). If you want to AND together several filters and apply them to a Query, do the following: IS.search( new FilteredQuery(query,filter2), filter1,...); You can chain even more filters in by adding more FilteredQueries. What this does: IS will automatically create another FilteredQuery to apply the filter and get the Weight of the top-level FilteredQuery. The scorer of this one will be top-level, get the filter and if it is random access, it will execute the filter with acceptDocs==liveDocs. The result bits of this filter will be passed to Weight.scorer of the second FilteredQuery as acceptDocs. This one will pass the acceptDocs (which are already filtered) to its Filter and if again random access pass those as acceptDocs to the inner Query's scorer. Finally the top-level IS will execute scorer.score(Collector), which in fact is the inner Query's scorer (no wrappers!) with all filtering applied in acceptDocs. This is incredible cool One thing about large patches in an issue: If you are working on an issue and have you local changes in your checkout and posted a patch to an issue and somebody else, posted an updated patch to an issue, it is often nice to see the diff between those patches. I wanted to see what Yonik changed, but a 140 K patch is not easy to handle. The trick is "interdiff" from patchutils package: You can call "interdiff LUCENE-1536 -original.patch LUCENE-1536 -yonik.patch" and you get a patch of only changes applied by Yonik. This patch can even be applied to your local already patched checkout. The changes-yonik-uwe.patch was generated that way and shows, what changes I did in my last patch in contrast to Yoniks original.
          Hide
          Chris Male added a comment -

          I really think we should commit this to trunk (assuming all tests are passing) as soon as possible. The patch is massive and contains a lot of changes. Any further optimizations can then be done in small chunks.

          Show
          Chris Male added a comment - I really think we should commit this to trunk (assuming all tests are passing) as soon as possible. The patch is massive and contains a lot of changes. Any further optimizations can then be done in small chunks.
          Hide
          Robert Muir added a comment -

          i dont think we should do that chris.

          Luceneutil hints that something is possibly wrong, I want to know what is going on there before any committing

          Show
          Robert Muir added a comment - i dont think we should do that chris. Luceneutil hints that something is possibly wrong, I want to know what is going on there before any committing
          Hide
          Chris Male added a comment -

          I agree, I count that as a test failing

          Show
          Chris Male added a comment - I agree, I count that as a test failing
          Hide
          Yonik Seeley added a comment -

          Just to note, if you implement something with "new DocIdSet() {....}", there is no need to override bits() to return null, as the default always returns null.

          Right - but I felt more comfortable being explicit about what sets would definitely not be using random access. A comment would have served in those cases too.

          Show
          Yonik Seeley added a comment - Just to note, if you implement something with "new DocIdSet() {....}", there is no need to override bits() to return null, as the default always returns null. Right - but I felt more comfortable being explicit about what sets would definitely not be using random access. A comment would have served in those cases too.
          Hide
          Yonik Seeley added a comment -

          The changes-yonik-uwe.patch was generated that way and shows, what changes I did in my last patch in contrast to Yoniks original.

          Thanks for the diff-diff. It is a pain trying to review differences between patches.

          Show
          Yonik Seeley added a comment - The changes-yonik-uwe.patch was generated that way and shows, what changes I did in my last patch in contrast to Yoniks original. Thanks for the diff-diff. It is a pain trying to review differences between patches.
          Hide
          Robert Muir added a comment -

          I agree, I count that as a test failing

          Yeah a 2 hour long test!

          Here's what I'll do: I'll run the benchmark again, against two clean checkouts of trunk.

          If it gives the same error message then I think we should chalk it up as some luceneutil problem... otherwise we should dig into it.

          Show
          Robert Muir added a comment - I agree, I count that as a test failing Yeah a 2 hour long test! Here's what I'll do: I'll run the benchmark again, against two clean checkouts of trunk. If it gives the same error message then I think we should chalk it up as some luceneutil problem... otherwise we should dig into it.
          Hide
          Robert Muir added a comment -

          I ask because sometimes Solr actually knows the sparseness of it's sets.

          Yonik, it knows this on a per-filter basis?

          One idea now that the heuristic is actually in FilteredQuery would be to add a (clearly marked expert/internal!) hook
          to filteredquery to override the default heuristic for cases where someone "knows" for sure the density of the filter.

          So instead of IS.search(query, filter, ...) which will just wrap with FilteredQuery,
          an expert could just do IS.search(new FilteredQuery(query, filter) {
          @Override
          boolean whatever()

          { return true; }

          });

          Show
          Robert Muir added a comment - I ask because sometimes Solr actually knows the sparseness of it's sets. Yonik, it knows this on a per-filter basis? One idea now that the heuristic is actually in FilteredQuery would be to add a (clearly marked expert/internal!) hook to filteredquery to override the default heuristic for cases where someone "knows" for sure the density of the filter. So instead of IS.search(query, filter, ...) which will just wrap with FilteredQuery, an expert could just do IS.search(new FilteredQuery(query, filter) { @Override boolean whatever() { return true; } });
          Hide
          Yonik Seeley added a comment -

          Hmmm, so solr passes null for acceptDocs where it can... but other methods like IndexSearcher.search still pass liveDocs (and filters derived from Solr's DocSets always respect liveDocs). Perhaps we should check if (acceptDocs==null || acceptDocs==liveDocs) and not wrap in that case.

          Show
          Yonik Seeley added a comment - Hmmm, so solr passes null for acceptDocs where it can... but other methods like IndexSearcher.search still pass liveDocs (and filters derived from Solr's DocSets always respect liveDocs). Perhaps we should check if (acceptDocs==null || acceptDocs==liveDocs) and not wrap in that case.
          Hide
          Chris Male added a comment - - edited

          Sounds like a good idea to me Robert.

          Could we also possibly add another template method to return the threshold value used in the current heuristic (so it could be removed from IndexSearcher). That way if anybody wanted to toy with either just the threshold or the full heuristic, they could just override the appropriate method. Doing either are very expert.

          Show
          Chris Male added a comment - - edited Sounds like a good idea to me Robert. Could we also possibly add another template method to return the threshold value used in the current heuristic (so it could be removed from IndexSearcher). That way if anybody wanted to toy with either just the threshold or the full heuristic, they could just override the appropriate method. Doing either are very expert.
          Hide
          Yonik Seeley added a comment -

          > I ask because sometimes Solr actually knows the sparseness of it's sets.

          Yonik, it knows this on a per-filter basis?

          A per-filter-implementation basis.
          It's the filters that are derived from DocSets (DocSets always have liveDocs baked in).

          Right now, at the point of calling IS.search(), we no longer know if the filter is of that type (since we also support filters that are not derived from DocSets), so it would seem easiest to just solve in the specific getDocIdSet() implementations to avoid wrapping if passed liveDocs.

          Show
          Yonik Seeley added a comment - > I ask because sometimes Solr actually knows the sparseness of it's sets. Yonik, it knows this on a per-filter basis? A per-filter-implementation basis. It's the filters that are derived from DocSets (DocSets always have liveDocs baked in). Right now, at the point of calling IS.search(), we no longer know if the filter is of that type (since we also support filters that are not derived from DocSets), so it would seem easiest to just solve in the specific getDocIdSet() implementations to avoid wrapping if passed liveDocs.
          Hide
          Robert Muir added a comment -

          Yonik, ok thanks.

          I still want to rework it this way because I don't like adding the strange parameters to IndexSearcher, it clutters it up with internal details.

          in my local, i removed this stuff entirely and just did this in FilteredQuery.

            /**
             * Expert: decides if a filter should be executed as "random-access" or not.
             * random-access means the filter "filters" in a similar way as deleted docs are filtered
             * in lucene. This is faster when the filter accepts many documents.
             * However, when the filter is very sparse, it can be faster to execute the query+filter
             * as a conjunction in some cases.
             * 
             * The default implementation returns true if the first document accepted by the
             * filter is < 100.
             * 
             * @lucene.internal
             */
            protected boolean useRandomAccess(Bits bits, int firstFilterDoc) {
              return firstFilterDoc < 100;
            }
          

          patch coming after tests finish.

          Show
          Robert Muir added a comment - Yonik, ok thanks. I still want to rework it this way because I don't like adding the strange parameters to IndexSearcher, it clutters it up with internal details. in my local, i removed this stuff entirely and just did this in FilteredQuery. /** * Expert: decides if a filter should be executed as "random-access" or not. * random-access means the filter "filters" in a similar way as deleted docs are filtered * in lucene. This is faster when the filter accepts many documents. * However, when the filter is very sparse, it can be faster to execute the query+filter * as a conjunction in some cases. * * The default implementation returns true if the first document accepted by the * filter is < 100. * * @lucene.internal */ protected boolean useRandomAccess(Bits bits, int firstFilterDoc) { return firstFilterDoc < 100; } patch coming after tests finish.
          Hide
          Yonik Seeley added a comment -

          Yeah, I agree there should be a way to control on a per-search/filter basis, regardless of how we end up solving solr's issues.

          Show
          Yonik Seeley added a comment - Yeah, I agree there should be a way to control on a per-search/filter basis, regardless of how we end up solving solr's issues.
          Hide
          Robert Muir added a comment -

          patch moving the heuristic to FilteredQuery, AssertingIndexSearcher overrides wrapFilter() and returns random.nextBoolean().

          In some tests we explicitly tested on/off, i fixed these to still do this, however i think its a little overkill (since newSearcher randomizes this anyway), but i left them working the same way.

          Show
          Robert Muir added a comment - patch moving the heuristic to FilteredQuery, AssertingIndexSearcher overrides wrapFilter() and returns random.nextBoolean(). In some tests we explicitly tested on/off, i fixed these to still do this, however i think its a little overkill (since newSearcher randomizes this anyway), but i left them working the same way.
          Hide
          Uwe Schindler added a comment -

          Hmmm, so solr passes null for acceptDocs where it can... but other methods like IndexSearcher.search still pass liveDocs (and filters derived from Solr's DocSets always respect liveDocs). Perhaps we should check if (acceptDocs==null || acceptDocs==liveDocs) and not wrap in that case.

          Yonik: You can do this, but thats out of the scope of this issue. In my original Solr patches I added the BitsFilteredDocIdSet everywhere in Solr, as the Lucene trunk requirements are to 100% respect deleted docs / accept docs in Filter.getDocIdSet(). This was not needed before, as deleted docs were applied after the filters, so a Filter that contained deleted docs, was not a problem at all.

          Now: If you have a Filter that magically gets the deleted docs from outside like Solr's DocSet filters, then you can safely ignore the acceptDocs given to getDocIdSet(), but only if they are == getLiveDocs(). So simply add a check for that.

          If the acceptDocs Unable to render embedded object: File (= liveDocs, you have to respect them, otherwise your Filter may return wrong docs. An exaple is the chained filter case I explained above [new FilterQuery(new FilterQuery(query, filter2), filter1)]. In that case, the inner filter2 would get different acceptDocs) not found.

          Show
          Uwe Schindler added a comment - Hmmm, so solr passes null for acceptDocs where it can... but other methods like IndexSearcher.search still pass liveDocs (and filters derived from Solr's DocSets always respect liveDocs). Perhaps we should check if (acceptDocs==null || acceptDocs==liveDocs) and not wrap in that case. Yonik: You can do this, but thats out of the scope of this issue. In my original Solr patches I added the BitsFilteredDocIdSet everywhere in Solr, as the Lucene trunk requirements are to 100% respect deleted docs / accept docs in Filter.getDocIdSet(). This was not needed before, as deleted docs were applied after the filters, so a Filter that contained deleted docs, was not a problem at all. Now: If you have a Filter that magically gets the deleted docs from outside like Solr's DocSet filters, then you can safely ignore the acceptDocs given to getDocIdSet(), but only if they are == getLiveDocs(). So simply add a check for that. If the acceptDocs Unable to render embedded object: File (= liveDocs, you have to respect them, otherwise your Filter may return wrong docs. An exaple is the chained filter case I explained above [new FilterQuery(new FilterQuery(query, filter2), filter1)]. In that case, the inner filter2 would get different acceptDocs) not found.
          Hide
          Robert Muir added a comment -

          Here's what I'll do: I'll run the benchmark again, against two clean checkouts of trunk.

          If it gives the same error message then I think we should chalk it up as some luceneutil problem... otherwise we should dig into it.

          I ran the luceneutil benchmark against two clean checkouts, no errors from grouping.

          This doesn't mean there is a bug in the patch, it could be a bug in grouping or a false warning or bug in the benchmark itself,
          but still i think we need to get to the bottom of this.

          Show
          Robert Muir added a comment - Here's what I'll do: I'll run the benchmark again, against two clean checkouts of trunk. If it gives the same error message then I think we should chalk it up as some luceneutil problem... otherwise we should dig into it. I ran the luceneutil benchmark against two clean checkouts, no errors from grouping. This doesn't mean there is a bug in the patch, it could be a bug in grouping or a false warning or bug in the benchmark itself, but still i think we need to get to the bottom of this.
          Hide
          Yonik Seeley added a comment -

          Now: If you have a Filter that magically gets the deleted docs from outside like Solr's DocSet filters, then you can safely ignore the acceptDocs given to getDocIdSet(), but only if they are == getLiveDocs(). So simply add a check for that.

          Yes, that's exactly what I was saying.

          Show
          Yonik Seeley added a comment - Now: If you have a Filter that magically gets the deleted docs from outside like Solr's DocSet filters, then you can safely ignore the acceptDocs given to getDocIdSet(), but only if they are == getLiveDocs(). So simply add a check for that. Yes, that's exactly what I was saying.
          Hide
          Uwe Schindler added a comment -

          Roberts patch with TestFilteredQuery cleaned up a little bit (not thousands of anonymous subclasses)

          Show
          Uwe Schindler added a comment - Roberts patch with TestFilteredQuery cleaned up a little bit (not thousands of anonymous subclasses)
          Hide
          Uwe Schindler added a comment -

          Improved the test for random access with BucktScorer to also chain two filters. This should pass the acceptDocs down so both filters are ANDed together.

          Also made AssertingIndexSearcher also use the autodetection. In half of all cases it uses autodetection from parent class, in the other half it decides between random access and iterator access.

          Show
          Uwe Schindler added a comment - Improved the test for random access with BucktScorer to also chain two filters. This should pass the acceptDocs down so both filters are ANDed together. Also made AssertingIndexSearcher also use the autodetection. In half of all cases it uses autodetection from parent class, in the other half it decides between random access and iterator access.
          Hide
          Uwe Schindler added a comment -

          Robert and me were talking about causes of the problems in the luceneutil runs. One idea would be an until-now hidden failure in grouping: FilteredQuery does not pass liveDocs down the scorer-chain (if iterative filtering is used), in the case it knows, that the Filter already uses it. This may confuse grouping!

          We should also check grouping with filters and old-style-iterator collecting.

          Show
          Uwe Schindler added a comment - Robert and me were talking about causes of the problems in the luceneutil runs. One idea would be an until-now hidden failure in grouping: FilteredQuery does not pass liveDocs down the scorer-chain (if iterative filtering is used), in the case it knows, that the Filter already uses it. This may confuse grouping! We should also check grouping with filters and old-style-iterator collecting.
          Hide
          Chris Male added a comment -

          Are these errors in the luceneutil runs repeatable? Are we to get more information about them to possibly replicate in a smaller test?

          Show
          Chris Male added a comment - Are these errors in the luceneutil runs repeatable? Are we to get more information about them to possibly replicate in a smaller test?
          Hide
          Robert Muir added a comment -

          I don't have the hardware or time to run this intensive thing over and over (takes hours here).

          Show
          Robert Muir added a comment - I don't have the hardware or time to run this intensive thing over and over (takes hours here).
          Hide
          Simon Willnauer added a comment -

          I don't have the hardware or time to run this intensive thing over and over (takes hours here).

          I will see if I can help here on monday! I will report back once I find something.

          Show
          Simon Willnauer added a comment - I don't have the hardware or time to run this intensive thing over and over (takes hours here). I will see if I can help here on monday! I will report back once I find something.
          Hide
          Michael McCandless added a comment -

          Hmm, with the latest patch have we lost the RECACHE option for CachingWrapperFilter? Ie, to re-AND the filter bits with the latest live docs and cache that. This is useful if you only periodically reopen the reader (and it has new deletes), so you don't have to AND the deletes in for every query.

          Show
          Michael McCandless added a comment - Hmm, with the latest patch have we lost the RECACHE option for CachingWrapperFilter? Ie, to re-AND the filter bits with the latest live docs and cache that. This is useful if you only periodically reopen the reader (and it has new deletes), so you don't have to AND the deletes in for every query.
          Hide
          Robert Muir added a comment -

          +1 to keep this option nuked, otherwise it limits acceptDocs to liveDocs.

          Show
          Robert Muir added a comment - +1 to keep this option nuked, otherwise it limits acceptDocs to liveDocs.
          Hide
          Robert Muir added a comment -

          sounds like we should split out the 'add acceptDocs to getDocIdSet' as a separate issue, just like we did for weight.scorer.

          Show
          Robert Muir added a comment - sounds like we should split out the 'add acceptDocs to getDocIdSet' as a separate issue, just like we did for weight.scorer.
          Hide
          Chris Male added a comment -

          Isn't that what this issue has basically become? plus some magic in FilteredQuery.

          Show
          Chris Male added a comment - Isn't that what this issue has basically become? plus some magic in FilteredQuery.
          Hide
          Robert Muir added a comment -

          except that api change could actually be committed... this issue can't because it grows too complex and has a bug.

          Show
          Robert Muir added a comment - except that api change could actually be committed... this issue can't because it grows too complex and has a bug.
          Hide
          Chris Male added a comment -

          I agree this has grown too big and complex. Go for it.

          Show
          Chris Male added a comment - I agree this has grown too big and complex. Go for it.
          Hide
          Uwe Schindler added a comment -

          Hmm, with the latest patch have we lost the RECACHE option for CachingWrapperFilter? Ie, to re-AND the filter bits with the latest live docs and cache that. This is useful if you only periodically reopen the reader (and it has new deletes), so you don't have to AND the deletes in for every query.

          Mike: We can no longer do this, as the acceptDocs passed to the getDocIdSet() are no longer always liveDocs, they can be everything. Simple example is two chained FilteredQuery. At least you would need to add the liveDocs instance as key to the cache.

          Even without recache we are still faster than before for a query, as the liveDocs are now only applied once! Before they were not applied in the filter, but everywhere else in the query. Now they are applied once per query. Putting them into the cache is stupid and wrong and would only save us one AND. The complexity in CachingWrapperFilter does not rectify this.

          About splitting that in two patches: I have no time to do it, I am on a business trip this week.

          Show
          Uwe Schindler added a comment - Hmm, with the latest patch have we lost the RECACHE option for CachingWrapperFilter? Ie, to re-AND the filter bits with the latest live docs and cache that. This is useful if you only periodically reopen the reader (and it has new deletes), so you don't have to AND the deletes in for every query. Mike: We can no longer do this, as the acceptDocs passed to the getDocIdSet() are no longer always liveDocs, they can be everything. Simple example is two chained FilteredQuery. At least you would need to add the liveDocs instance as key to the cache. Even without recache we are still faster than before for a query, as the liveDocs are now only applied once! Before they were not applied in the filter, but everywhere else in the query. Now they are applied once per query. Putting them into the cache is stupid and wrong and would only save us one AND. The complexity in CachingWrapperFilter does not rectify this. About splitting that in two patches: I have no time to do it, I am on a business trip this week.
          Hide
          Uwe Schindler added a comment - - edited

          Before they were not applied in the filter, but everywhere else in the query. Now they are applied once per query

          Sorry this is only correct for the iterator based advancing. For the filter-down-low approach they are of-course still applied. But still we should show benchmarks, that this really hurts. Because caching acceptDocs (not liveDocs!!!!) is very hard to do. Of course a chained FilteredQuery with lots of chanined filters could be simplier (only one static BitSet cached for the whole filter chain).

          Robert and me had more ideas how to optimize the always appliying acceptDocs case in every scorer: E.g. ConjunctionTermScorer could pass null down for all but one sub-scorer. Ideally the one that gets the liveDocs should be the one with lowest docFreq. The others don't need liveDocs, as the lowDocFreq scorer already applied them and they can never be appear in hits, because the other scorers would then advance over it. We should open new issues for those optimizations.

          Show
          Uwe Schindler added a comment - - edited Before they were not applied in the filter, but everywhere else in the query. Now they are applied once per query Sorry this is only correct for the iterator based advancing. For the filter-down-low approach they are of-course still applied. But still we should show benchmarks, that this really hurts. Because caching acceptDocs (not liveDocs!!!!) is very hard to do. Of course a chained FilteredQuery with lots of chanined filters could be simplier (only one static BitSet cached for the whole filter chain). Robert and me had more ideas how to optimize the always appliying acceptDocs case in every scorer: E.g. ConjunctionTermScorer could pass null down for all but one sub-scorer. Ideally the one that gets the liveDocs should be the one with lowest docFreq. The others don't need liveDocs, as the lowDocFreq scorer already applied them and they can never be appear in hits, because the other scorers would then advance over it. We should open new issues for those optimizations.
          Hide
          Michael McCandless added a comment -

          On the diff that luceneutil hits, it looks like there's an float iota
          difference:

          On trunk we get these results:

          TASK: cat=Fuzzy1F90.0 q=body:changer~1.0 s=null f=CachingWrapperFilter(PreComputedRandomFilter(pctAccept=90.0)) group=null hits=198715
            32.160243 msec
            thread 5
            doc=6199951 score=40.27584
            doc=6199960 score=40.27584
            doc=6200023 score=40.27584
            doc=7580697 score=40.27584
            doc=7995191 score=33.34529
            doc=8684145 score=31.100195
            doc=6260043 score=31.100193
            doc=7320778 score=31.100193
            doc=7454704 score=31.100193
            doc=7979518 score=26.333052
            50 expanded terms
          

          With the patch we get this:

          TASK: cat=Fuzzy1F90.0 q=body:changer~1.0 s=null f=CachingWrapperFilter(PreComputedRandomFilter(pctAccept=90.0)) group=null hits=198715
            19.300811 msec
            thread 4
            doc=6199951 score=40.27584
            doc=6199960 score=40.27584
            doc=6200023 score=40.27584
            doc=7580697 score=40.27584
            doc=7995191 score=33.34529
            doc=6260043 score=31.100195
            doc=7454704 score=31.100195
            doc=7320778 score=31.100193
            doc=8684145 score=31.100193
            doc=7979518 score=26.333052
            50 expanded terms
          

          several of the docs with score 31.100193 or 31.100195 flipped around.

          Show
          Michael McCandless added a comment - On the diff that luceneutil hits, it looks like there's an float iota difference: On trunk we get these results: TASK: cat=Fuzzy1F90.0 q=body:changer~1.0 s=null f=CachingWrapperFilter(PreComputedRandomFilter(pctAccept=90.0)) group=null hits=198715 32.160243 msec thread 5 doc=6199951 score=40.27584 doc=6199960 score=40.27584 doc=6200023 score=40.27584 doc=7580697 score=40.27584 doc=7995191 score=33.34529 doc=8684145 score=31.100195 doc=6260043 score=31.100193 doc=7320778 score=31.100193 doc=7454704 score=31.100193 doc=7979518 score=26.333052 50 expanded terms With the patch we get this: TASK: cat=Fuzzy1F90.0 q=body:changer~1.0 s=null f=CachingWrapperFilter(PreComputedRandomFilter(pctAccept=90.0)) group=null hits=198715 19.300811 msec thread 4 doc=6199951 score=40.27584 doc=6199960 score=40.27584 doc=6200023 score=40.27584 doc=7580697 score=40.27584 doc=7995191 score=33.34529 doc=6260043 score=31.100195 doc=7454704 score=31.100195 doc=7320778 score=31.100193 doc=8684145 score=31.100193 doc=7979518 score=26.333052 50 expanded terms several of the docs with score 31.100193 or 31.100195 flipped around.
          Hide
          Chris Male added a comment -

          So where does this leave us?

          Show
          Chris Male added a comment - So where does this leave us?
          Hide
          Robert Muir added a comment -

          this patch shouldn't be changing scores, I think even a small difference could be indicative of a larger problem: we need to understand what is causing this.

          Show
          Robert Muir added a comment - this patch shouldn't be changing scores, I think even a small difference could be indicative of a larger problem: we need to understand what is causing this.
          Hide
          Chris Male added a comment -

          Are any deletes made in the above benchmarking? Might try to simulate the same change in a small unit test.

          Show
          Chris Male added a comment - Are any deletes made in the above benchmarking? Might try to simulate the same change in a small unit test.
          Hide
          Robert Muir added a comment -

          deletes dont affect scoring.

          Show
          Robert Muir added a comment - deletes dont affect scoring.
          Hide
          Chris Male added a comment -

          I realise that, I was just wanting to replicate the same conditions.

          Show
          Chris Male added a comment - I realise that, I was just wanting to replicate the same conditions.
          Hide
          Robert Muir added a comment -

          well, i think you are on the right path.

          if our unit tests pass but luceneutil 'fails' i think thats a bad sign of the quality of our tests... it sounds like
          we need to improve the tests to have more coverage for filters & deletions?

          Show
          Robert Muir added a comment - well, i think you are on the right path. if our unit tests pass but luceneutil 'fails' i think thats a bad sign of the quality of our tests... it sounds like we need to improve the tests to have more coverage for filters & deletions?
          Hide
          Robert Muir added a comment -

          one bug is that FilteredQuery in the patch runs some heuristics per segment which determine how the booleans get set that drive the BS1 versus B2 decision.

          This means that some segments could get BS1, and others get BS2, meaning we will rank some documents arbitrarily higher than others when they actually have the same underlying index statistics... this is bad!

          So I think at least the parameters to subscorer (topLevel/inOrder) must be consistently applied to all segments from that Weight.

          Show
          Robert Muir added a comment - one bug is that FilteredQuery in the patch runs some heuristics per segment which determine how the booleans get set that drive the BS1 versus B2 decision. This means that some segments could get BS1, and others get BS2, meaning we will rank some documents arbitrarily higher than others when they actually have the same underlying index statistics... this is bad! So I think at least the parameters to subscorer (topLevel/inOrder) must be consistently applied to all segments from that Weight.
          Hide
          Michael McCandless added a comment -

          Hmm, another bug is: we are never using BS1 when the filter is applied 'down low'; this is because FilteredQuery's Weight impl does not override scoresDocsOutOfOrder. I think it should do so? And if the filter will be applied 'down low', it should delegate to the wrapped Weight?

          Show
          Michael McCandless added a comment - Hmm, another bug is: we are never using BS1 when the filter is applied 'down low'; this is because FilteredQuery's Weight impl does not override scoresDocsOutOfOrder. I think it should do so? And if the filter will be applied 'down low', it should delegate to the wrapped Weight?
          Hide
          Uwe Schindler added a comment -

          Mike: That could be the reason for the problems: Currently it delegates to the wrapped Weight, but id does not wrap all methods.

          Show
          Uwe Schindler added a comment - Mike: That could be the reason for the problems: Currently it delegates to the wrapped Weight, but id does not wrap all methods.
          Hide
          Uwe Schindler added a comment -

          one bug is that FilteredQuery in the patch runs some heuristics per segment which determine how the booleans get set that drive the BS1 versus B2 decision.

          How can BS1 and BS2 return different scores, this would be a bug? Theoretically it should be possible to have one segment with BS1 the other one with BS2.

          By the way: That was not different without FilteredQuery in the older patches.

          Of course the selection of the right scorer based on out of order should be done based on scoresDocOutOfOrder returned by the weight. This is a bug in FilteredQuery#Weight. But easy to fix.

          By the way: This was also not different without FilteredQuery in the older patches.

          Show
          Uwe Schindler added a comment - one bug is that FilteredQuery in the patch runs some heuristics per segment which determine how the booleans get set that drive the BS1 versus B2 decision. How can BS1 and BS2 return different scores, this would be a bug? Theoretically it should be possible to have one segment with BS1 the other one with BS2. By the way: That was not different without FilteredQuery in the older patches. Of course the selection of the right scorer based on out of order should be done based on scoresDocOutOfOrder returned by the weight. This is a bug in FilteredQuery#Weight. But easy to fix. By the way: This was also not different without FilteredQuery in the older patches.
          Hide
          Robert Muir added a comment -

          How can BS1 and BS2 return different scores, this would be a bug? Theoretically it should be possible to have one segment with BS1 the other one with BS2.

          Well they are different Scorer.java's ? I think its bad to use different code to score different segments, in this case two different algorithms
          could cause floating point operations to be done in different order?

          Its also a bug that the scoresDocsOutOfOrder is wrong: and this is really the whole bug. Somehow FilteredQuery#Weight needs to determine what its gonna do
          up front so that collector specialization is working, so that we use BS1 or BS2 consistently across all segments, etc.

          Show
          Robert Muir added a comment - How can BS1 and BS2 return different scores, this would be a bug? Theoretically it should be possible to have one segment with BS1 the other one with BS2. Well they are different Scorer.java's ? I think its bad to use different code to score different segments, in this case two different algorithms could cause floating point operations to be done in different order? Its also a bug that the scoresDocsOutOfOrder is wrong: and this is really the whole bug. Somehow FilteredQuery#Weight needs to determine what its gonna do up front so that collector specialization is working, so that we use BS1 or BS2 consistently across all segments, etc.
          Hide
          Robert Muir added a comment -

          hack patch that computes the heuristic up front in weight init, so it scores all segments consistently and returns the proper scoresDocsOutOfOrder for BS1.

          Uwe's new test (the nestedFilterQuery) doesnt pass yet, don't know why.

          I recomputed the benchmarks:

                          Task   QPS trunkStdDev trunk   QPS patchStdDev patch      Pct diff
                    PhraseF1.0       11.99        0.20        7.79        0.23  -37% -  -31%
                      TermF0.5      135.14        7.62      116.57        0.36  -18% -   -8%
             AndHighHighF100.0       17.34        0.78       15.44        0.15  -15% -   -5%
              AndHighHighF95.0       17.28        0.66       15.48        0.17  -14% -   -5%
              AndHighHighF90.0       17.31        0.76       15.58        0.19  -14% -   -4%
              AndHighHighF99.0       17.05        1.02       15.45        0.17  -15% -   -2%
              AndHighHighF75.0       17.47        0.78       16.03        0.15  -12% -   -3%
               AndHighHighF5.0       20.69        0.95       19.78        0.23   -9% -    1%
               AndHighHighF1.0       35.11        1.46       33.64        0.36   -8% -    1%
               AndHighHighF0.1      136.04        3.70      132.00        1.41   -6% -    0%
                   AndHighHigh       18.25        0.70       17.74        0.20   -7% -    2%
               AndHighHighF0.5       49.84        1.72       48.58        0.49   -6% -    1%
                      TermF0.1      351.18       11.01      345.85        1.73   -4% -    2%
                  Fuzzy2F100.0       95.52        4.21       94.33        2.07   -7% -    5%
            SloppyPhraseF100.0        8.01        0.28        7.91        0.09   -5% -    3%
                   Fuzzy2F90.0       95.42        3.86       94.51        1.74   -6% -    5%
                   Fuzzy2F95.0       95.20        4.86       94.33        1.83   -7% -    6%
                    Fuzzy1F1.0       54.02        1.67       53.56        1.07   -5% -    4%
                    PhraseF2.0        7.73        0.07        7.68        0.18   -3% -    2%
             SloppyPhraseF99.0        7.99        0.23        7.95        0.10   -4% -    3%
              AndHighHighF50.0       17.54        0.79       17.46        0.12   -5% -    4%
                    Fuzzy2F0.1      105.39        3.93      105.34        3.74   -7% -    7%
                SpanNearF100.0        3.16        0.06        3.16        0.04   -2% -    2%
                   Fuzzy2F99.0       94.02        6.86       94.21        1.97   -8% -   10%
                   Fuzzy2F75.0       95.56        3.51       95.76        2.02   -5% -    6%
                  WildcardF2.0       52.79        0.27       53.05        0.57   -1% -    2%
                    Fuzzy1F0.5       58.12        1.83       58.43        1.22   -4% -    5%
                    PhraseF0.1       66.34        0.78       66.73        1.68   -3% -    4%
              SloppyPhraseF0.1       56.15        1.52       56.79        0.64   -2% -    5%
                  SloppyPhrase        8.08        0.26        8.18        0.08   -2% -    5%
                      PKLookup      176.59        5.07      178.96        5.71   -4% -    7%
                  SpanNearF0.1       32.36        0.56       32.83        0.54   -1% -    4%
                OrHighHighF0.1       78.20        0.52       79.44        0.74    0% -    3%
             SloppyPhraseF95.0        7.91        0.08        8.05        0.09    0% -    3%
                        Fuzzy2       94.87        3.72       96.49        1.62   -3% -    7%
                OrHighHighF0.5       31.41        0.47       31.96        0.33    0% -    4%
                 SpanNearF99.0        3.12        0.06        3.18        0.03    0% -    4%
                  WildcardF0.5       61.97        0.56       63.28        0.82    0% -    4%
                    PhraseF0.5       19.78        0.26       20.29        0.31    0% -    5%
                      SpanNear        3.19        0.08        3.27        0.05   -1% -    6%
                  WildcardF0.1       67.45        0.64       69.24        0.89    0% -    4%
             SloppyPhraseF90.0        8.00        0.29        8.21        0.12   -2% -    8%
                 SpanNearF95.0        3.13        0.04        3.23        0.03    1% -    5%
                      Wildcard       43.19        0.34       44.64        1.40    0% -    7%
                   Fuzzy2F50.0       95.12        4.22       98.69        2.28   -2% -   11%
                        Fuzzy1       55.28        4.53       57.68        0.76   -4% -   15%
                    OrHighHigh       12.13        0.99       12.71        0.43   -6% -   18%
                        Phrase        3.60        0.04        3.81        0.04    3% -    7%
                 SpanNearF90.0        3.15        0.05        3.35        0.04    3% -    9%
                          Term       71.69        0.40       76.53        4.13    0% -   13%
                   PhraseF99.0        3.43        0.03        3.68        0.04    5% -    9%
                  PhraseF100.0        3.39        0.05        3.67        0.04    5% -   10%
             SloppyPhraseF75.0        8.04        0.26        8.74        0.12    3% -   13%
                   Fuzzy2F20.0       97.38        4.03      106.17        2.88    1% -   16%
                   PhraseF95.0        3.38        0.03        3.70        0.05    6% -   11%
                   PhraseF90.0        3.42        0.02        3.76        0.03    8% -   11%
                   Fuzzy2F10.0       97.19        3.69      109.27        3.23    5% -   20%
                   PhraseF75.0        3.44        0.02        3.94        0.04   12% -   16%
                    Fuzzy2F5.0       96.77        4.17      112.60        3.30    8% -   25%
                    Fuzzy1F0.1       73.61        2.43       86.22        2.79    9% -   25%
                WildcardF100.0       35.49        0.33       41.92        1.05   14% -   22%
                 SpanNearF75.0        3.15        0.07        3.72        0.04   14% -   22%
                 WildcardF95.0       35.43        0.24       41.90        0.99   14% -   21%
                 WildcardF90.0       35.59        0.32       42.11        1.11   14% -   22%
                 WildcardF99.0       35.43        0.34       41.94        1.09   14% -   22%
                   Fuzzy1F99.0       47.41        1.79       56.45        0.78   13% -   25%
                 WildcardF75.0       35.64        0.29       42.51        0.87   15% -   22%
                  Fuzzy1F100.0       46.85        1.83       56.42        0.55   14% -   26%
                    Fuzzy2F1.0       96.75        3.75      116.85        4.32   11% -   30%
                   Fuzzy1F95.0       46.91        1.37       56.69        0.69   15% -   25%
                    Fuzzy2F0.5       97.33        4.15      117.64        4.17   11% -   30%
                    Fuzzy2F2.0       95.51        3.65      115.66        3.95   12% -   30%
                   Fuzzy1F90.0       46.84        1.95       56.83        0.78   14% -   28%
                 WildcardF50.0       36.28        0.23       44.23        0.58   19% -   24%
                      TermF1.0       93.99        4.90      114.60        0.49   15% -   29%
                   Fuzzy1F75.0       47.12        1.68       58.11        0.82   17% -   29%
                  WildcardF1.0       56.94        0.80       71.15        0.80   21% -   28%
                   PhraseF50.0        3.49        0.01        4.39        0.04   24% -   27%
             SloppyPhraseF50.0        8.03        0.28       10.12        0.13   20% -   32%
                   Fuzzy1F50.0       46.64        2.22       60.78        0.95   22% -   38%
                OrHighHighF1.0       24.15        0.35       32.46        0.25   31% -   37%
                 WildcardF20.0       40.55        0.30       55.72        0.73   34% -   40%
                   Fuzzy1F20.0       49.29        1.52       69.91        1.26   35% -   48%
                  WildcardF5.0       47.02        0.33       67.11        0.81   40% -   45%
                    PhraseF5.0        5.03        0.06        7.29        0.13   40% -   49%
                 WildcardF10.0       43.04        0.57       62.68        0.69   42% -   49%
                 SpanNearF50.0        3.16        0.07        4.77        0.06   45% -   56%
              AndHighHighF20.0       17.76        0.64       27.44        0.25   47% -   61%
                   Fuzzy1F10.0       48.53        2.47       75.31        1.60   44% -   66%
                    Fuzzy1F5.0       50.45        1.70       79.01        2.08   47% -   66%
                    Fuzzy1F2.0       52.03        1.54       82.41        2.46   49% -   68%
              OrHighHighF100.0        7.69        0.20       12.19        0.33   50% -   67%
               OrHighHighF99.0        7.72        0.35       12.25        0.34   47% -   70%
                   PhraseF20.0        3.74        0.03        5.95        0.05   56% -   61%
               OrHighHighF95.0        7.79        0.28       12.43        0.33   49% -   69%
                OrHighHighF2.0       19.60        0.24       31.28        0.14   56% -   62%
                      TermF2.0       70.16        3.78      112.18        0.53   51% -   69%
               OrHighHighF90.0        7.83        0.23       12.56        0.33   51% -   69%
                   PhraseF10.0        4.14        0.04        6.76        0.09   59% -   66%
                     TermF50.0       42.57        1.77       70.63        1.25   56% -   76%
                     TermF75.0       41.43        1.61       70.40        2.44   57% -   82%
               OrHighHighF75.0        7.81        0.22       13.33        0.36   61% -   80%
                     TermF95.0       41.07        1.65       70.96        3.30   58% -   88%
                     TermF99.0       41.12        1.57       71.14        3.41   58% -   88%
                     TermF90.0       40.92        1.58       70.81        3.00   59% -   87%
                    TermF100.0       40.01        0.73       71.10        3.36   66% -   89%
               OrHighHighF50.0        8.39        0.24       14.92        0.29   69% -   86%
              AndHighHighF10.0       18.68        0.61       36.17        0.23   86% -  101%
                     TermF20.0       45.66        1.98       88.55        0.52   84% -  103%
                OrHighHighF5.0       14.89        0.30       29.12        0.20   90% -  100%
             SloppyPhraseF20.0        8.34        0.29       16.36        0.26   86% -  106%
                      TermF5.0       52.71        1.88      105.42        0.46   92% -  108%
                     TermF10.0       47.53        1.99       97.62        0.51   96% -  115%
               AndHighHighF2.0       26.32        1.16       54.83        0.27   98% -  119%
               OrHighHighF20.0       10.36        0.19       22.12        0.22  107% -  119%
               OrHighHighF10.0       12.31        0.35       26.43        0.23  106% -  122%
             SloppyPhraseF10.0        8.73        0.28       22.70        0.38  147% -  172%
              SloppyPhraseF0.5       19.21        0.58       52.13        0.61  160% -  183%
                 SpanNearF20.0        3.20        0.05        8.98        0.16  171% -  189%
              SloppyPhraseF5.0        9.30        0.33       30.17        0.44  208% -  241%
              SloppyPhraseF1.0       13.84        0.44       46.77        0.64  223% -  253%
              SloppyPhraseF2.0       11.00        0.36       39.85        0.54  246% -  279%
                 SpanNearF10.0        3.31        0.07       13.57        0.23  294% -  325%
                  SpanNearF0.5        9.25        0.14       39.86        0.39  320% -  341%
                  SpanNearF5.0        3.54        0.07       19.35        0.38  425% -  468%
                  SpanNearF1.0        6.15        0.11       34.27        0.48  439% -  474%
                  SpanNearF2.0        4.52        0.09       28.11        0.43  500% -  543%
          
          Show
          Robert Muir added a comment - hack patch that computes the heuristic up front in weight init, so it scores all segments consistently and returns the proper scoresDocsOutOfOrder for BS1. Uwe's new test (the nestedFilterQuery) doesnt pass yet, don't know why. I recomputed the benchmarks: Task QPS trunkStdDev trunk QPS patchStdDev patch Pct diff PhraseF1.0 11.99 0.20 7.79 0.23 -37% - -31% TermF0.5 135.14 7.62 116.57 0.36 -18% - -8% AndHighHighF100.0 17.34 0.78 15.44 0.15 -15% - -5% AndHighHighF95.0 17.28 0.66 15.48 0.17 -14% - -5% AndHighHighF90.0 17.31 0.76 15.58 0.19 -14% - -4% AndHighHighF99.0 17.05 1.02 15.45 0.17 -15% - -2% AndHighHighF75.0 17.47 0.78 16.03 0.15 -12% - -3% AndHighHighF5.0 20.69 0.95 19.78 0.23 -9% - 1% AndHighHighF1.0 35.11 1.46 33.64 0.36 -8% - 1% AndHighHighF0.1 136.04 3.70 132.00 1.41 -6% - 0% AndHighHigh 18.25 0.70 17.74 0.20 -7% - 2% AndHighHighF0.5 49.84 1.72 48.58 0.49 -6% - 1% TermF0.1 351.18 11.01 345.85 1.73 -4% - 2% Fuzzy2F100.0 95.52 4.21 94.33 2.07 -7% - 5% SloppyPhraseF100.0 8.01 0.28 7.91 0.09 -5% - 3% Fuzzy2F90.0 95.42 3.86 94.51 1.74 -6% - 5% Fuzzy2F95.0 95.20 4.86 94.33 1.83 -7% - 6% Fuzzy1F1.0 54.02 1.67 53.56 1.07 -5% - 4% PhraseF2.0 7.73 0.07 7.68 0.18 -3% - 2% SloppyPhraseF99.0 7.99 0.23 7.95 0.10 -4% - 3% AndHighHighF50.0 17.54 0.79 17.46 0.12 -5% - 4% Fuzzy2F0.1 105.39 3.93 105.34 3.74 -7% - 7% SpanNearF100.0 3.16 0.06 3.16 0.04 -2% - 2% Fuzzy2F99.0 94.02 6.86 94.21 1.97 -8% - 10% Fuzzy2F75.0 95.56 3.51 95.76 2.02 -5% - 6% WildcardF2.0 52.79 0.27 53.05 0.57 -1% - 2% Fuzzy1F0.5 58.12 1.83 58.43 1.22 -4% - 5% PhraseF0.1 66.34 0.78 66.73 1.68 -3% - 4% SloppyPhraseF0.1 56.15 1.52 56.79 0.64 -2% - 5% SloppyPhrase 8.08 0.26 8.18 0.08 -2% - 5% PKLookup 176.59 5.07 178.96 5.71 -4% - 7% SpanNearF0.1 32.36 0.56 32.83 0.54 -1% - 4% OrHighHighF0.1 78.20 0.52 79.44 0.74 0% - 3% SloppyPhraseF95.0 7.91 0.08 8.05 0.09 0% - 3% Fuzzy2 94.87 3.72 96.49 1.62 -3% - 7% OrHighHighF0.5 31.41 0.47 31.96 0.33 0% - 4% SpanNearF99.0 3.12 0.06 3.18 0.03 0% - 4% WildcardF0.5 61.97 0.56 63.28 0.82 0% - 4% PhraseF0.5 19.78 0.26 20.29 0.31 0% - 5% SpanNear 3.19 0.08 3.27 0.05 -1% - 6% WildcardF0.1 67.45 0.64 69.24 0.89 0% - 4% SloppyPhraseF90.0 8.00 0.29 8.21 0.12 -2% - 8% SpanNearF95.0 3.13 0.04 3.23 0.03 1% - 5% Wildcard 43.19 0.34 44.64 1.40 0% - 7% Fuzzy2F50.0 95.12 4.22 98.69 2.28 -2% - 11% Fuzzy1 55.28 4.53 57.68 0.76 -4% - 15% OrHighHigh 12.13 0.99 12.71 0.43 -6% - 18% Phrase 3.60 0.04 3.81 0.04 3% - 7% SpanNearF90.0 3.15 0.05 3.35 0.04 3% - 9% Term 71.69 0.40 76.53 4.13 0% - 13% PhraseF99.0 3.43 0.03 3.68 0.04 5% - 9% PhraseF100.0 3.39 0.05 3.67 0.04 5% - 10% SloppyPhraseF75.0 8.04 0.26 8.74 0.12 3% - 13% Fuzzy2F20.0 97.38 4.03 106.17 2.88 1% - 16% PhraseF95.0 3.38 0.03 3.70 0.05 6% - 11% PhraseF90.0 3.42 0.02 3.76 0.03 8% - 11% Fuzzy2F10.0 97.19 3.69 109.27 3.23 5% - 20% PhraseF75.0 3.44 0.02 3.94 0.04 12% - 16% Fuzzy2F5.0 96.77 4.17 112.60 3.30 8% - 25% Fuzzy1F0.1 73.61 2.43 86.22 2.79 9% - 25% WildcardF100.0 35.49 0.33 41.92 1.05 14% - 22% SpanNearF75.0 3.15 0.07 3.72 0.04 14% - 22% WildcardF95.0 35.43 0.24 41.90 0.99 14% - 21% WildcardF90.0 35.59 0.32 42.11 1.11 14% - 22% WildcardF99.0 35.43 0.34 41.94 1.09 14% - 22% Fuzzy1F99.0 47.41 1.79 56.45 0.78 13% - 25% WildcardF75.0 35.64 0.29 42.51 0.87 15% - 22% Fuzzy1F100.0 46.85 1.83 56.42 0.55 14% - 26% Fuzzy2F1.0 96.75 3.75 116.85 4.32 11% - 30% Fuzzy1F95.0 46.91 1.37 56.69 0.69 15% - 25% Fuzzy2F0.5 97.33 4.15 117.64 4.17 11% - 30% Fuzzy2F2.0 95.51 3.65 115.66 3.95 12% - 30% Fuzzy1F90.0 46.84 1.95 56.83 0.78 14% - 28% WildcardF50.0 36.28 0.23 44.23 0.58 19% - 24% TermF1.0 93.99 4.90 114.60 0.49 15% - 29% Fuzzy1F75.0 47.12 1.68 58.11 0.82 17% - 29% WildcardF1.0 56.94 0.80 71.15 0.80 21% - 28% PhraseF50.0 3.49 0.01 4.39 0.04 24% - 27% SloppyPhraseF50.0 8.03 0.28 10.12 0.13 20% - 32% Fuzzy1F50.0 46.64 2.22 60.78 0.95 22% - 38% OrHighHighF1.0 24.15 0.35 32.46 0.25 31% - 37% WildcardF20.0 40.55 0.30 55.72 0.73 34% - 40% Fuzzy1F20.0 49.29 1.52 69.91 1.26 35% - 48% WildcardF5.0 47.02 0.33 67.11 0.81 40% - 45% PhraseF5.0 5.03 0.06 7.29 0.13 40% - 49% WildcardF10.0 43.04 0.57 62.68 0.69 42% - 49% SpanNearF50.0 3.16 0.07 4.77 0.06 45% - 56% AndHighHighF20.0 17.76 0.64 27.44 0.25 47% - 61% Fuzzy1F10.0 48.53 2.47 75.31 1.60 44% - 66% Fuzzy1F5.0 50.45 1.70 79.01 2.08 47% - 66% Fuzzy1F2.0 52.03 1.54 82.41 2.46 49% - 68% OrHighHighF100.0 7.69 0.20 12.19 0.33 50% - 67% OrHighHighF99.0 7.72 0.35 12.25 0.34 47% - 70% PhraseF20.0 3.74 0.03 5.95 0.05 56% - 61% OrHighHighF95.0 7.79 0.28 12.43 0.33 49% - 69% OrHighHighF2.0 19.60 0.24 31.28 0.14 56% - 62% TermF2.0 70.16 3.78 112.18 0.53 51% - 69% OrHighHighF90.0 7.83 0.23 12.56 0.33 51% - 69% PhraseF10.0 4.14 0.04 6.76 0.09 59% - 66% TermF50.0 42.57 1.77 70.63 1.25 56% - 76% TermF75.0 41.43 1.61 70.40 2.44 57% - 82% OrHighHighF75.0 7.81 0.22 13.33 0.36 61% - 80% TermF95.0 41.07 1.65 70.96 3.30 58% - 88% TermF99.0 41.12 1.57 71.14 3.41 58% - 88% TermF90.0 40.92 1.58 70.81 3.00 59% - 87% TermF100.0 40.01 0.73 71.10 3.36 66% - 89% OrHighHighF50.0 8.39 0.24 14.92 0.29 69% - 86% AndHighHighF10.0 18.68 0.61 36.17 0.23 86% - 101% TermF20.0 45.66 1.98 88.55 0.52 84% - 103% OrHighHighF5.0 14.89 0.30 29.12 0.20 90% - 100% SloppyPhraseF20.0 8.34 0.29 16.36 0.26 86% - 106% TermF5.0 52.71 1.88 105.42 0.46 92% - 108% TermF10.0 47.53 1.99 97.62 0.51 96% - 115% AndHighHighF2.0 26.32 1.16 54.83 0.27 98% - 119% OrHighHighF20.0 10.36 0.19 22.12 0.22 107% - 119% OrHighHighF10.0 12.31 0.35 26.43 0.23 106% - 122% SloppyPhraseF10.0 8.73 0.28 22.70 0.38 147% - 172% SloppyPhraseF0.5 19.21 0.58 52.13 0.61 160% - 183% SpanNearF20.0 3.20 0.05 8.98 0.16 171% - 189% SloppyPhraseF5.0 9.30 0.33 30.17 0.44 208% - 241% SloppyPhraseF1.0 13.84 0.44 46.77 0.64 223% - 253% SloppyPhraseF2.0 11.00 0.36 39.85 0.54 246% - 279% SpanNearF10.0 3.31 0.07 13.57 0.23 294% - 325% SpanNearF0.5 9.25 0.14 39.86 0.39 320% - 341% SpanNearF5.0 3.54 0.07 19.35 0.38 425% - 468% SpanNearF1.0 6.15 0.11 34.27 0.48 439% - 474% SpanNearF2.0 4.52 0.09 28.11 0.43 500% - 543%
          Hide
          Michael McCandless added a comment -

          I opened LUCENE-3503 for the score diff issue; it's a pre-existing bug.

          Show
          Michael McCandless added a comment - I opened LUCENE-3503 for the score diff issue; it's a pre-existing bug.
          Hide
          Michael McCandless added a comment -

          I also bench'd Robert's patch (turned off verifyScores in lucenebench because of LUCENE-3503); results look very similar:

                          Task    QPS base StdDev baseQPS filterlowStdDev filterlow      Pct diff
                    PhraseF0.5       20.18        0.65        8.05        0.56  -64% -  -55%
                    PhraseF1.0       12.26        0.33        7.96        0.54  -41% -  -28%
              AndHighHighF95.0       16.56        0.13       15.98        1.09  -10% -    3%
                   Fuzzy2F99.0       80.52        4.67       77.72        2.34  -11% -    5%
              AndHighHighF99.0       16.55        0.12       15.97        1.05  -10% -    3%
             AndHighHighF100.0       16.54        0.13       15.98        1.06  -10% -    3%
                  Fuzzy2F100.0       80.32        4.60       77.64        2.34  -11% -    5%
                   Fuzzy2F90.0       80.80        5.17       78.19        2.77  -12% -    7%
              AndHighHighF90.0       16.57        0.15       16.05        1.13  -10% -    4%
                OrHighHighF0.1       72.17        3.60       70.11        3.69  -12% -    7%
                OrHighHighF0.5       29.26        1.23       28.44        1.50  -11% -    6%
                   Fuzzy2F95.0       79.95        4.49       77.86        2.10  -10% -    5%
                  WildcardF0.1       59.21        4.21       58.01        3.42  -13% -   11%
                  WildcardF0.5       54.94        3.78       53.88        3.08  -13% -   11%
                  WildcardF1.0       51.31        3.31       50.35        2.44  -12% -    9%
                  WildcardF2.0       46.99        2.93       46.13        2.15  -11% -    9%
                      Wildcard       38.73        1.94       38.14        1.78  -10% -    8%
                   Fuzzy2F75.0       80.57        5.03       79.38        2.04   -9% -    7%
              AndHighHighF75.0       16.63        0.14       16.41        1.21   -9% -    6%
            SloppyPhraseF100.0        7.73        0.15        7.64        0.25   -6% -    4%
             SloppyPhraseF99.0        7.74        0.15        7.66        0.26   -6% -    4%
                      TermF0.1      328.10       15.20      325.54       16.82  -10% -    9%
                    OrHighHigh       10.68        1.11       10.61        0.75  -16% -   18%
                      TermF0.5      127.55        3.70      126.88        6.02   -7% -    7%
                    PhraseF0.1       63.93        2.25       63.62        2.87   -8% -    7%
                    PhraseF2.0        7.88        0.19        7.86        0.31   -6% -    6%
               AndHighHighF0.1      129.64        5.02      129.28        6.98   -9% -    9%
              SloppyPhraseF0.1       53.80        0.79       53.86        1.84   -4% -    5%
             SloppyPhraseF95.0        7.74        0.15        7.75        0.27   -5% -    5%
              SloppyPhraseF0.5       18.44        0.31       18.47        0.64   -4% -    5%
              SloppyPhraseF1.0       13.10        0.23       13.13        0.47   -5% -    5%
                  SloppyPhrase        7.81        0.10        7.83        0.30   -4% -    5%
               AndHighHighF0.5       47.61        1.00       47.76        2.33   -6% -    7%
                    Fuzzy2F1.0       81.49        4.85       81.96        0.96   -6% -    8%
                        Fuzzy1       47.97        3.71       48.35        1.94  -10% -   13%
                    Fuzzy1F0.1       64.31        3.56       64.82        0.83   -5% -    8%
                        Fuzzy2       80.93        6.15       81.61        1.74   -8% -   11%
                        Phrase        3.58        0.10        3.63        0.18   -6% -    9%
                SpanNearF100.0        2.98        0.10        3.03        0.12   -5% -    9%
             SloppyPhraseF90.0        7.74        0.15        7.87        0.28   -3% -    7%
                   AndHighHigh       17.31        0.24       17.62        0.64   -3% -    6%
                    Fuzzy2F0.1       89.54        5.78       91.38        1.44   -5% -   10%
                 SpanNearF99.0        2.98        0.09        3.04        0.13   -5% -    9%
                          Term       58.94        6.06       60.38        4.40  -13% -   22%
                  SpanNearF0.1       29.91        1.07       30.70        1.43   -5% -   11%
                  SpanNearF0.5        8.73        0.30        8.98        0.41   -5% -   11%
                  SpanNearF5.0        3.33        0.11        3.42        0.16   -5% -   11%
                   Fuzzy2F50.0       80.90        5.19       83.29        2.28   -5% -   13%
                      SpanNear        3.01        0.10        3.10        0.14   -4% -   11%
                      TermF1.0       87.07        2.01       89.92        6.38   -6% -   13%
                 SpanNearF95.0        2.98        0.10        3.10        0.13   -3% -   12%
                  PhraseF100.0        3.37        0.06        3.51        0.17   -2% -   11%
                   PhraseF99.0        3.37        0.05        3.52        0.17   -2% -   11%
                   PhraseF95.0        3.37        0.06        3.56        0.18   -1% -   12%
                      PKLookup      126.08        5.73      133.37        1.97    0% -   12%
                 SpanNearF90.0        2.98        0.10        3.18        0.14   -1% -   15%
                   PhraseF90.0        3.38        0.06        3.61        0.18    0% -   14%
                WildcardF100.0       32.22        1.76       34.59        1.43   -2% -   18%
                 WildcardF99.0       32.23        1.79       34.61        1.39   -2% -   18%
             SloppyPhraseF75.0        7.74        0.16        8.32        0.33    1% -   14%
                 WildcardF95.0       32.15        1.83       34.72        1.37   -1% -   19%
                 WildcardF90.0       32.10        1.82       34.90        1.29    0% -   19%
                  Fuzzy1F100.0       42.36        1.85       46.19        2.07    0% -   19%
              AndHighHighF50.0       16.76        0.10       18.30        1.44    0% -   18%
                   Fuzzy1F99.0       42.21        1.84       46.21        1.96    0% -   19%
                   Fuzzy2F20.0       81.24        5.06       88.97        2.11    0% -   19%
                   Fuzzy1F95.0       42.25        1.85       46.54        2.10    0% -   20%
                 WildcardF75.0       31.98        1.81       35.49        1.32    1% -   22%
                   Fuzzy1F90.0       42.15        1.77       46.84        1.91    2% -   20%
                   PhraseF75.0        3.39        0.06        3.82        0.20    4% -   20%
                   Fuzzy1F75.0       42.01        1.55       47.63        1.98    4% -   22%
                OrHighHighF1.0       22.54        0.94       25.87        1.81    2% -   28%
                   Fuzzy2F10.0       81.10        5.01       93.81        2.75    5% -   26%
                 WildcardF50.0       32.66        1.88       37.81        1.33    5% -   27%
                   Fuzzy1F50.0       42.25        1.68       49.68        1.91    8% -   27%
                 SpanNearF75.0        2.98        0.10        3.51        0.16    9% -   27%
                    Fuzzy2F5.0       80.39        4.38       96.21        2.19   10% -   29%
                    Fuzzy2F0.5       83.14        4.67       99.73        1.75   11% -   29%
                    Fuzzy2F2.0       80.95        4.92       98.00        1.76   12% -   31%
             SloppyPhraseF50.0        7.78        0.16        9.62        0.43   15% -   31%
                   PhraseF50.0        3.45        0.06        4.36        0.24   17% -   35%
                 WildcardF20.0       35.76        2.01       45.85        1.89   16% -   41%
                  WildcardF5.0       41.47        2.41       53.54        2.38   16% -   43%
                   Fuzzy1F20.0       43.60        1.76       57.50        2.00   22% -   42%
                 WildcardF10.0       38.26        2.17       50.63        2.11   20% -   46%
                     TermF99.0       40.49        1.22       54.84        4.93   19% -   52%
                    TermF100.0       40.51        1.29       54.99        4.92   19% -   52%
                     TermF95.0       40.44        1.19       54.95        4.82   20% -   52%
                     TermF90.0       40.34        1.08       55.00        4.58   21% -   51%
                OrHighHighF2.0       18.15        0.69       24.94        1.69   23% -   52%
                      TermF2.0       63.47        1.48       87.39        5.94   25% -   50%
                     TermF75.0       40.05        0.92       55.28        4.38   24% -   52%
                    Fuzzy1F0.5       51.14        2.45       71.30        1.82   29% -   50%
              OrHighHighF100.0        7.05        0.15        9.96        0.73   28% -   54%
               OrHighHighF99.0        7.04        0.15        9.97        0.72   28% -   55%
                     TermF50.0       40.94        0.70       58.33        4.05   30% -   55%
                   Fuzzy1F10.0       43.92        1.78       62.74        1.47   34% -   52%
               OrHighHighF95.0        7.08        0.14       10.12        0.70   30% -   56%
               OrHighHighF90.0        7.10        0.15       10.31        0.71   32% -   58%
                    PhraseF5.0        5.02        0.10        7.33        0.48   33% -   58%
                    Fuzzy1F1.0       47.45        2.15       70.55        1.95   38% -   60%
                    Fuzzy1F5.0       44.47        1.99       66.38        1.89   38% -   60%
                    Fuzzy1F2.0       46.09        1.98       69.35        1.65   40% -   60%
                 SpanNearF50.0        2.98        0.10        4.51        0.23   39% -   64%
               OrHighHighF75.0        7.20        0.15       10.97        0.73   39% -   65%
              AndHighHighF20.0       16.92        0.14       26.80        2.77   40% -   76%
                   PhraseF20.0        3.69        0.06        5.86        0.36   46% -   71%
                     TermF20.0       42.65        0.76       69.54        4.48   49% -   76%
                   PhraseF10.0        4.10        0.07        6.74        0.43   51% -   78%
               OrHighHighF50.0        7.61        0.17       12.77        0.76   54% -   81%
                OrHighHighF5.0       13.68        0.48       23.13        1.55   52% -   86%
                      TermF5.0       47.37        1.30       81.16        5.35   55% -   87%
                     TermF10.0       43.07        0.95       74.83        4.64   59% -   88%
               AndHighHighF1.0       32.98        0.48       59.25        8.46   51% -  108%
             SloppyPhraseF20.0        8.00        0.16       14.72        0.84   70% -   98%
               OrHighHighF10.0       11.20        0.34       21.25        1.38   72% -  108%
               OrHighHighF20.0        9.32        0.22       18.10        1.12   77% -  111%
              AndHighHighF10.0       17.54        0.16       35.08        4.05   75% -  125%
               AndHighHighF2.0       24.58        0.27       52.49        7.16   82% -  145%
               AndHighHighF5.0       19.26        0.17       43.11        5.37   94% -  154%
             SloppyPhraseF10.0        8.24        0.16       19.96        1.29  122% -  162%
                 SpanNearF20.0        3.01        0.10        8.24        0.48  149% -  199%
              SloppyPhraseF5.0        8.75        0.17       26.13        1.80  172% -  225%
              SloppyPhraseF2.0       10.35        0.20       33.95        2.51  198% -  259%
                 SpanNearF10.0        3.09        0.10       12.23        0.76  259% -  334%
                  SpanNearF1.0        5.75        0.19       30.48        2.42  372% -  492%
                  SpanNearF2.0        4.21        0.13       24.77        1.80  428% -  551%
          
          Show
          Michael McCandless added a comment - I also bench'd Robert's patch (turned off verifyScores in lucenebench because of LUCENE-3503 ); results look very similar: Task QPS base StdDev baseQPS filterlowStdDev filterlow Pct diff PhraseF0.5 20.18 0.65 8.05 0.56 -64% - -55% PhraseF1.0 12.26 0.33 7.96 0.54 -41% - -28% AndHighHighF95.0 16.56 0.13 15.98 1.09 -10% - 3% Fuzzy2F99.0 80.52 4.67 77.72 2.34 -11% - 5% AndHighHighF99.0 16.55 0.12 15.97 1.05 -10% - 3% AndHighHighF100.0 16.54 0.13 15.98 1.06 -10% - 3% Fuzzy2F100.0 80.32 4.60 77.64 2.34 -11% - 5% Fuzzy2F90.0 80.80 5.17 78.19 2.77 -12% - 7% AndHighHighF90.0 16.57 0.15 16.05 1.13 -10% - 4% OrHighHighF0.1 72.17 3.60 70.11 3.69 -12% - 7% OrHighHighF0.5 29.26 1.23 28.44 1.50 -11% - 6% Fuzzy2F95.0 79.95 4.49 77.86 2.10 -10% - 5% WildcardF0.1 59.21 4.21 58.01 3.42 -13% - 11% WildcardF0.5 54.94 3.78 53.88 3.08 -13% - 11% WildcardF1.0 51.31 3.31 50.35 2.44 -12% - 9% WildcardF2.0 46.99 2.93 46.13 2.15 -11% - 9% Wildcard 38.73 1.94 38.14 1.78 -10% - 8% Fuzzy2F75.0 80.57 5.03 79.38 2.04 -9% - 7% AndHighHighF75.0 16.63 0.14 16.41 1.21 -9% - 6% SloppyPhraseF100.0 7.73 0.15 7.64 0.25 -6% - 4% SloppyPhraseF99.0 7.74 0.15 7.66 0.26 -6% - 4% TermF0.1 328.10 15.20 325.54 16.82 -10% - 9% OrHighHigh 10.68 1.11 10.61 0.75 -16% - 18% TermF0.5 127.55 3.70 126.88 6.02 -7% - 7% PhraseF0.1 63.93 2.25 63.62 2.87 -8% - 7% PhraseF2.0 7.88 0.19 7.86 0.31 -6% - 6% AndHighHighF0.1 129.64 5.02 129.28 6.98 -9% - 9% SloppyPhraseF0.1 53.80 0.79 53.86 1.84 -4% - 5% SloppyPhraseF95.0 7.74 0.15 7.75 0.27 -5% - 5% SloppyPhraseF0.5 18.44 0.31 18.47 0.64 -4% - 5% SloppyPhraseF1.0 13.10 0.23 13.13 0.47 -5% - 5% SloppyPhrase 7.81 0.10 7.83 0.30 -4% - 5% AndHighHighF0.5 47.61 1.00 47.76 2.33 -6% - 7% Fuzzy2F1.0 81.49 4.85 81.96 0.96 -6% - 8% Fuzzy1 47.97 3.71 48.35 1.94 -10% - 13% Fuzzy1F0.1 64.31 3.56 64.82 0.83 -5% - 8% Fuzzy2 80.93 6.15 81.61 1.74 -8% - 11% Phrase 3.58 0.10 3.63 0.18 -6% - 9% SpanNearF100.0 2.98 0.10 3.03 0.12 -5% - 9% SloppyPhraseF90.0 7.74 0.15 7.87 0.28 -3% - 7% AndHighHigh 17.31 0.24 17.62 0.64 -3% - 6% Fuzzy2F0.1 89.54 5.78 91.38 1.44 -5% - 10% SpanNearF99.0 2.98 0.09 3.04 0.13 -5% - 9% Term 58.94 6.06 60.38 4.40 -13% - 22% SpanNearF0.1 29.91 1.07 30.70 1.43 -5% - 11% SpanNearF0.5 8.73 0.30 8.98 0.41 -5% - 11% SpanNearF5.0 3.33 0.11 3.42 0.16 -5% - 11% Fuzzy2F50.0 80.90 5.19 83.29 2.28 -5% - 13% SpanNear 3.01 0.10 3.10 0.14 -4% - 11% TermF1.0 87.07 2.01 89.92 6.38 -6% - 13% SpanNearF95.0 2.98 0.10 3.10 0.13 -3% - 12% PhraseF100.0 3.37 0.06 3.51 0.17 -2% - 11% PhraseF99.0 3.37 0.05 3.52 0.17 -2% - 11% PhraseF95.0 3.37 0.06 3.56 0.18 -1% - 12% PKLookup 126.08 5.73 133.37 1.97 0% - 12% SpanNearF90.0 2.98 0.10 3.18 0.14 -1% - 15% PhraseF90.0 3.38 0.06 3.61 0.18 0% - 14% WildcardF100.0 32.22 1.76 34.59 1.43 -2% - 18% WildcardF99.0 32.23 1.79 34.61 1.39 -2% - 18% SloppyPhraseF75.0 7.74 0.16 8.32 0.33 1% - 14% WildcardF95.0 32.15 1.83 34.72 1.37 -1% - 19% WildcardF90.0 32.10 1.82 34.90 1.29 0% - 19% Fuzzy1F100.0 42.36 1.85 46.19 2.07 0% - 19% AndHighHighF50.0 16.76 0.10 18.30 1.44 0% - 18% Fuzzy1F99.0 42.21 1.84 46.21 1.96 0% - 19% Fuzzy2F20.0 81.24 5.06 88.97 2.11 0% - 19% Fuzzy1F95.0 42.25 1.85 46.54 2.10 0% - 20% WildcardF75.0 31.98 1.81 35.49 1.32 1% - 22% Fuzzy1F90.0 42.15 1.77 46.84 1.91 2% - 20% PhraseF75.0 3.39 0.06 3.82 0.20 4% - 20% Fuzzy1F75.0 42.01 1.55 47.63 1.98 4% - 22% OrHighHighF1.0 22.54 0.94 25.87 1.81 2% - 28% Fuzzy2F10.0 81.10 5.01 93.81 2.75 5% - 26% WildcardF50.0 32.66 1.88 37.81 1.33 5% - 27% Fuzzy1F50.0 42.25 1.68 49.68 1.91 8% - 27% SpanNearF75.0 2.98 0.10 3.51 0.16 9% - 27% Fuzzy2F5.0 80.39 4.38 96.21 2.19 10% - 29% Fuzzy2F0.5 83.14 4.67 99.73 1.75 11% - 29% Fuzzy2F2.0 80.95 4.92 98.00 1.76 12% - 31% SloppyPhraseF50.0 7.78 0.16 9.62 0.43 15% - 31% PhraseF50.0 3.45 0.06 4.36 0.24 17% - 35% WildcardF20.0 35.76 2.01 45.85 1.89 16% - 41% WildcardF5.0 41.47 2.41 53.54 2.38 16% - 43% Fuzzy1F20.0 43.60 1.76 57.50 2.00 22% - 42% WildcardF10.0 38.26 2.17 50.63 2.11 20% - 46% TermF99.0 40.49 1.22 54.84 4.93 19% - 52% TermF100.0 40.51 1.29 54.99 4.92 19% - 52% TermF95.0 40.44 1.19 54.95 4.82 20% - 52% TermF90.0 40.34 1.08 55.00 4.58 21% - 51% OrHighHighF2.0 18.15 0.69 24.94 1.69 23% - 52% TermF2.0 63.47 1.48 87.39 5.94 25% - 50% TermF75.0 40.05 0.92 55.28 4.38 24% - 52% Fuzzy1F0.5 51.14 2.45 71.30 1.82 29% - 50% OrHighHighF100.0 7.05 0.15 9.96 0.73 28% - 54% OrHighHighF99.0 7.04 0.15 9.97 0.72 28% - 55% TermF50.0 40.94 0.70 58.33 4.05 30% - 55% Fuzzy1F10.0 43.92 1.78 62.74 1.47 34% - 52% OrHighHighF95.0 7.08 0.14 10.12 0.70 30% - 56% OrHighHighF90.0 7.10 0.15 10.31 0.71 32% - 58% PhraseF5.0 5.02 0.10 7.33 0.48 33% - 58% Fuzzy1F1.0 47.45 2.15 70.55 1.95 38% - 60% Fuzzy1F5.0 44.47 1.99 66.38 1.89 38% - 60% Fuzzy1F2.0 46.09 1.98 69.35 1.65 40% - 60% SpanNearF50.0 2.98 0.10 4.51 0.23 39% - 64% OrHighHighF75.0 7.20 0.15 10.97 0.73 39% - 65% AndHighHighF20.0 16.92 0.14 26.80 2.77 40% - 76% PhraseF20.0 3.69 0.06 5.86 0.36 46% - 71% TermF20.0 42.65 0.76 69.54 4.48 49% - 76% PhraseF10.0 4.10 0.07 6.74 0.43 51% - 78% OrHighHighF50.0 7.61 0.17 12.77 0.76 54% - 81% OrHighHighF5.0 13.68 0.48 23.13 1.55 52% - 86% TermF5.0 47.37 1.30 81.16 5.35 55% - 87% TermF10.0 43.07 0.95 74.83 4.64 59% - 88% AndHighHighF1.0 32.98 0.48 59.25 8.46 51% - 108% SloppyPhraseF20.0 8.00 0.16 14.72 0.84 70% - 98% OrHighHighF10.0 11.20 0.34 21.25 1.38 72% - 108% OrHighHighF20.0 9.32 0.22 18.10 1.12 77% - 111% AndHighHighF10.0 17.54 0.16 35.08 4.05 75% - 125% AndHighHighF2.0 24.58 0.27 52.49 7.16 82% - 145% AndHighHighF5.0 19.26 0.17 43.11 5.37 94% - 154% SloppyPhraseF10.0 8.24 0.16 19.96 1.29 122% - 162% SpanNearF20.0 3.01 0.10 8.24 0.48 149% - 199% SloppyPhraseF5.0 8.75 0.17 26.13 1.80 172% - 225% SloppyPhraseF2.0 10.35 0.20 33.95 2.51 198% - 259% SpanNearF10.0 3.09 0.10 12.23 0.76 259% - 334% SpanNearF1.0 5.75 0.19 30.48 2.42 372% - 492% SpanNearF2.0 4.21 0.13 24.77 1.80 428% - 551%
          Hide
          Michael McCandless added a comment -

          Mike: We can no longer do this, as the acceptDocs passed to the getDocIdSet() are no longer always liveDocs, they can be everything.

          But CWF's job is still the same with this patch?

          It's just that the cache key is now a reader + acceptDocs (instead of
          just reader), and the "policy" must be more careful not to cache just
          any acceptDocs.

          Ie, we could easily add back the RECACHE option (maybe just a boolean
          "cacheLiveDocs" or something)? Or am I missing something?

          The IGNORE option must go away, since no filter impl is allowed to ignore
          the incoming acceptDocs. The DYNAMIC option is what the patch now
          hardwires.

          I think this use case (app using CWF, doing deletes and reopening
          periodically) is important. For this use case we should do the AND w/
          liveDocs only once on each reopen, and cache & reuse that, instead of
          re-ANDing over and over for every query.

          Show
          Michael McCandless added a comment - Mike: We can no longer do this, as the acceptDocs passed to the getDocIdSet() are no longer always liveDocs, they can be everything. But CWF's job is still the same with this patch? It's just that the cache key is now a reader + acceptDocs (instead of just reader), and the "policy" must be more careful not to cache just any acceptDocs. Ie, we could easily add back the RECACHE option (maybe just a boolean "cacheLiveDocs" or something)? Or am I missing something? The IGNORE option must go away, since no filter impl is allowed to ignore the incoming acceptDocs. The DYNAMIC option is what the patch now hardwires. I think this use case (app using CWF, doing deletes and reopening periodically) is important. For this use case we should do the AND w/ liveDocs only once on each reopen, and cache & reuse that, instead of re-ANDing over and over for every query.
          Hide
          Uwe Schindler added a comment -

          hack patch that computes the heuristic up front in weight init, so it scores all segments consistently and returns the proper scoresDocsOutOfOrder for BS1.

          Uwe's new test (the nestedFilterQuery) doesnt pass yet, don't know why.

          Very easy to explain: Because it's a hack! The problem is simple: The new test explicitely checks that acceptDocs are correctly handled by the query, which is not the case for your modifications. In createWeight you get the first segemnt and create the filter's docidset on it, passing liveDocs (because you have nothing else). You cache this first DocIdSet (to not need to execute getDocIdSet for the first filter 2 times) and by that miss the real acceptDocs (which are != liveDocs in this test). The firts segment therefore returns more documents that it should.

          Alltogether, the hack is of course uncommitable and the source of outr problem only lies in the out of order setting. The fix in your patch is fine, but too much. The scoresDocsOutOfOrder method should simply return, what the inner weight returns, because it may return docs out of order. It can still retun them in order (if a filter needs to be applied using iterator). This is not different to behaviour before. So the fix is easy: Do the same like in ConstantScoreQuery, where we return the setting from the inner weight.

          Being consistent in selecting scorer implementations between segments is not an issue of this special case, it's a general problem and cannot be solved by a hack. The selection of Scorer for BooleanQuery can be different even without FilteredQuery, as BooleanWeight might return different different scorer, too (so the problem is BooleanScorer that does selection of its Scorer per-segment). To fix this, BooleanWeight must do all the scorer descisions in it's ctor, so we would need to pass also scoreInOrder and other parameters to the Weight's ctor.

          Please remove the hack, and only correctly implement scoresDocsOutOfOrder (which is the reason for the problem, as it suddenly returns documents in a different order). We can still get the documents with that patch in different order if we have random access enabled together with the filter but the old IndexSearcher used DocIdSetIterator (in-order). We should ignore those differences in document order, if score is identical (and Mike's output shows scores are equal). If we want to check that the results are identical, the benchmark test must explicitely request docs-in-order on trunk vs. patch to be consistent. But then it's no longer a benchmark.

          Conclusion: In general we explained the differences between the patches and I think, my original patch is fine except the Weight.scoresDocsOutOfOrder, which should return the inner Weight's setting (like CSQ does) - no magic needed. Our patch does not return wrong documents, just the order of equal-scoring documents is different, which is perfectly fine.

          Show
          Uwe Schindler added a comment - hack patch that computes the heuristic up front in weight init, so it scores all segments consistently and returns the proper scoresDocsOutOfOrder for BS1. Uwe's new test (the nestedFilterQuery) doesnt pass yet, don't know why. Very easy to explain: Because it's a hack! The problem is simple: The new test explicitely checks that acceptDocs are correctly handled by the query, which is not the case for your modifications. In createWeight you get the first segemnt and create the filter's docidset on it, passing liveDocs (because you have nothing else). You cache this first DocIdSet (to not need to execute getDocIdSet for the first filter 2 times) and by that miss the real acceptDocs (which are != liveDocs in this test). The firts segment therefore returns more documents that it should. Alltogether, the hack is of course uncommitable and the source of outr problem only lies in the out of order setting. The fix in your patch is fine, but too much. The scoresDocsOutOfOrder method should simply return, what the inner weight returns, because it may return docs out of order. It can still retun them in order (if a filter needs to be applied using iterator). This is not different to behaviour before. So the fix is easy: Do the same like in ConstantScoreQuery, where we return the setting from the inner weight. Being consistent in selecting scorer implementations between segments is not an issue of this special case, it's a general problem and cannot be solved by a hack. The selection of Scorer for BooleanQuery can be different even without FilteredQuery, as BooleanWeight might return different different scorer, too (so the problem is BooleanScorer that does selection of its Scorer per-segment). To fix this, BooleanWeight must do all the scorer descisions in it's ctor, so we would need to pass also scoreInOrder and other parameters to the Weight's ctor. Please remove the hack, and only correctly implement scoresDocsOutOfOrder (which is the reason for the problem, as it suddenly returns documents in a different order). We can still get the documents with that patch in different order if we have random access enabled together with the filter but the old IndexSearcher used DocIdSetIterator (in-order). We should ignore those differences in document order, if score is identical (and Mike's output shows scores are equal). If we want to check that the results are identical, the benchmark test must explicitely request docs-in-order on trunk vs. patch to be consistent. But then it's no longer a benchmark. Conclusion: In general we explained the differences between the patches and I think, my original patch is fine except the Weight.scoresDocsOutOfOrder, which should return the inner Weight's setting (like CSQ does) - no magic needed. Our patch does not return wrong documents, just the order of equal-scoring documents is different, which is perfectly fine.
          Hide
          Uwe Schindler added a comment -

          Patch that fixes the Weight.scoreDocsOutOfOrder method to return the inner weight's setting. The scorer can still return docs in order, but that was identical behaviour in previous unpatched trunk (IS looked at the out-of- order setting of the weight and uses correct collector, but once a filter was applied, the documents came in order). My patch only missed to pass this setting to our wrapper query.

          Mike: If you have time, can you check this? We may need a test, that uses a larger index and tests FilteredQuery on top of it, the current indexes used for filtering are simply too small and in most cases have only one segment

          There is no need for Robert's hack (that does not work correctly with aceptDocs != liveDocs), if different BooleanScorers return significant different scores, it as a bug, not a problem in FilteredQuery. Slight score changes and therefor different order in results is not a problem at all - this is just my opinion.

          If we want to check that the results are identical, the benchmark test must explicitely request docs-in-order on trunk vs. patch to be consistent. But then it's no longer a benchmark.

          This is of course untrue, sorry. If the weight returns that docs may come out of order, the collector should handle this.

          Show
          Uwe Schindler added a comment - Patch that fixes the Weight.scoreDocsOutOfOrder method to return the inner weight's setting. The scorer can still return docs in order, but that was identical behaviour in previous unpatched trunk (IS looked at the out-of- order setting of the weight and uses correct collector, but once a filter was applied, the documents came in order). My patch only missed to pass this setting to our wrapper query. Mike: If you have time, can you check this? We may need a test, that uses a larger index and tests FilteredQuery on top of it, the current indexes used for filtering are simply too small and in most cases have only one segment There is no need for Robert's hack (that does not work correctly with aceptDocs != liveDocs), if different BooleanScorers return significant different scores, it as a bug, not a problem in FilteredQuery. Slight score changes and therefor different order in results is not a problem at all - this is just my opinion. If we want to check that the results are identical, the benchmark test must explicitely request docs-in-order on trunk vs. patch to be consistent. But then it's no longer a benchmark. This is of course untrue, sorry. If the weight returns that docs may come out of order, the collector should handle this.
          Hide
          Robert Muir added a comment -
          There is no need for Robert's hack (that does not work correctly with aceptDocs != liveDocs), if different BooleanScorers return significant different scores, it as a bug, not a problem in FilteredQuery. Slight score changes and therefor different order in results is not a problem at all - this is just my opinion.
          

          Uwe, you are very confused.

          BooleanWeight always returns BS1 or BS2, and BS2 always returns the same subscorer hierarchy, the decisions are based all on nothing IR-dependent.
          We cannot do as your patch does, it is incorrect.

          Here is my standing -1 against this patch that returns different scorer implementations for different segments, it totally breaks scoring for
          filtered queries in lucene. This is unacceptable, as filters should not affect the score.

          Show
          Robert Muir added a comment - There is no need for Robert's hack (that does not work correctly with aceptDocs != liveDocs), if different BooleanScorers return significant different scores, it as a bug, not a problem in FilteredQuery. Slight score changes and therefor different order in results is not a problem at all - this is just my opinion. Uwe, you are very confused. BooleanWeight always returns BS1 or BS2, and BS2 always returns the same subscorer hierarchy, the decisions are based all on nothing IR-dependent. We cannot do as your patch does, it is incorrect. Here is my standing -1 against this patch that returns different scorer implementations for different segments, it totally breaks scoring for filtered queries in lucene. This is unacceptable, as filters should not affect the score.
          Hide
          Robert Muir added a comment -

          Its not even just the specific patch here thats broken, the design is broken too.

          Because things like whether a filter can be accessible random access or not are not per-segment things (since it must be scored in a consistent way: same scorer, across all segments).

          currently a filter could return non-null Bits for one segment and null for another.

          Show
          Robert Muir added a comment - Its not even just the specific patch here thats broken, the design is broken too. Because things like whether a filter can be accessible random access or not are not per-segment things (since it must be scored in a consistent way: same scorer, across all segments). currently a filter could return non-null Bits for one segment and null for another.
          Hide
          Uwe Schindler added a comment -

          currently a filter could return non-null Bits for one segment and null for another.

          And this also affects choosing scorers (and does also in current Lucene 3.x: because once a filter returns non-null or non-EMPTY_DOC_ID_SET for one segment, it will score in order). So you bring up another issue that has nothing to do with this patch. The current scorer design is that the Weight decides per segment which scorer to use. To make that consisten, we have to change the way how scorers are created. This has nothing to do with this patch.

          The "bug" (which is none in my opinion) is definitely not in this filter, it was there since the beginning.

          I still disagree with you: BooleanScorer1 and 2 should return same scores. PERIOD. If they don't they are buggy.

          Show
          Uwe Schindler added a comment - currently a filter could return non-null Bits for one segment and null for another. And this also affects choosing scorers (and does also in current Lucene 3.x: because once a filter returns non-null or non-EMPTY_DOC_ID_SET for one segment, it will score in order). So you bring up another issue that has nothing to do with this patch. The current scorer design is that the Weight decides per segment which scorer to use. To make that consisten, we have to change the way how scorers are created. This has nothing to do with this patch. The "bug" (which is none in my opinion) is definitely not in this filter, it was there since the beginning. I still disagree with you: BooleanScorer1 and 2 should return same scores. PERIOD. If they don't they are buggy.
          Hide
          Robert Muir added a comment -

          And this also affects choosing scorers (and does also in current Lucene 3.x: because once a filter returns non-null or non-EMPTY_DOC_ID_SET for one segment, it will score in order).

          Thats a good point, ill open a bug for this.

          We need to wrangle all these scoring bugs, get them under control, and add tests for this stuff.

          Show
          Robert Muir added a comment - And this also affects choosing scorers (and does also in current Lucene 3.x: because once a filter returns non-null or non-EMPTY_DOC_ID_SET for one segment, it will score in order). Thats a good point, ill open a bug for this. We need to wrangle all these scoring bugs, get them under control, and add tests for this stuff.
          Hide
          Uwe Schindler added a comment -

          Thats a good point, ill open a bug for this.

          We don't need for that case: if filter returns null, no docs are scored So it doe snot matter which scorer is used.

          But still the Weight API is confusing an should be improved, I agree with you. BooleanWeight should ensure that it always returns the same score impl, not our filter handling is responsible. The problem with in-order/out-of order + autodetection of sparseness exists in all previous patches on this issue (since the addition of the autodetection); it's not my fault!

          Show
          Uwe Schindler added a comment - Thats a good point, ill open a bug for this. We don't need for that case: if filter returns null, no docs are scored So it doe snot matter which scorer is used. But still the Weight API is confusing an should be improved, I agree with you. BooleanWeight should ensure that it always returns the same score impl, not our filter handling is responsible. The problem with in-order/out-of order + autodetection of sparseness exists in all previous patches on this issue (since the addition of the autodetection); it's not my fault!
          Hide
          Uwe Schindler added a comment -

          Sorry last patch upload was somehow corrupted, maybe because of JIRA issues. This is the one only implementing Weight.scoreDocsOutOfOrder(), no hacks - to have a clean start.

          Show
          Uwe Schindler added a comment - Sorry last patch upload was somehow corrupted, maybe because of JIRA issues. This is the one only implementing Weight.scoreDocsOutOfOrder(), no hacks - to have a clean start.
          Hide
          Uwe Schindler added a comment -

          Strange things going on. With Google Chrome, uploading patch files corrupts the file. With MSIE it worked. Sorry for the noise. Yesterday it worked normally...

          Show
          Uwe Schindler added a comment - Strange things going on. With Google Chrome, uploading patch files corrupts the file. With MSIE it worked. Sorry for the noise. Yesterday it worked normally...
          Hide
          Robert Muir added a comment -

          you still misunderstand my patch, btw:

          . You cache this first DocIdSet (to not need to execute getDocIdSet for the first filter 2 times) and by that miss the real acceptDocs (which are != liveDocs in this test).

          +        // try to reuse from our previous heuristic sampling
          +        if (context == plan.firstLeaf && acceptDocs == plan.liveDocs) {
          

          So how is it != liveDocs? We only re-use the cache if this 'if' is true...

          Show
          Robert Muir added a comment - you still misunderstand my patch, btw: . You cache this first DocIdSet (to not need to execute getDocIdSet for the first filter 2 times) and by that miss the real acceptDocs (which are != liveDocs in this test). + // try to reuse from our previous heuristic sampling + if (context == plan.firstLeaf && acceptDocs == plan.liveDocs) { So how is it != liveDocs? We only re-use the cache if this 'if' is true...
          Hide
          Uwe Schindler added a comment -

          Sorry Robert, if that works its fine, but test is still failing, so something is wrong.

          My problem with the patch here is more, that for most filters, the call to getDocIdSet() is the most expensive one. So you are right with caching the result. But we actually calculate the DocIdSet twice (unless we use CachingWrapperFilter), if the acceptDocs != liveDocs. And as the first segment is generally the largest one, this is even worse.

          In my opinion, the whole approach of looking into the sparseness of the DocIdSet is broken for this case, as we can correctly do this only per segment, but we later require all segments to use the same scorer implementation. I have no idea, how to solve this. It would not even be enough like Chris/Mikes orginal approaches to support something like DocIdSet.useBits()/isSparse() whatever, as this is also by segment.

          There is also a second problem: It might happen that one filter returns a DocIdSet that does not support bits() for one segment, but another one for other segments? How to handle that? There is one case where this happens (DocIdSet.EMPTY_DOCIDSET always returns null for bits) - but this one is grafefully handled by an early exit condition, so we won't get NPE.

          The only possible solution is to make Filters always request in-order scoring, but this would limit our optimization possibilities.

          Finally I still think we should fix BS1 and BS2 to return identical scores (and write a test for that which compares scores). Second, in Mike's document/score listing above with/wo patch, I see no score differences, only order of docs is different (which is caused by out-of-order missing), so where is the problem?

          Show
          Uwe Schindler added a comment - Sorry Robert, if that works its fine, but test is still failing, so something is wrong. My problem with the patch here is more, that for most filters, the call to getDocIdSet() is the most expensive one. So you are right with caching the result. But we actually calculate the DocIdSet twice (unless we use CachingWrapperFilter), if the acceptDocs != liveDocs. And as the first segment is generally the largest one, this is even worse. In my opinion, the whole approach of looking into the sparseness of the DocIdSet is broken for this case, as we can correctly do this only per segment, but we later require all segments to use the same scorer implementation. I have no idea, how to solve this. It would not even be enough like Chris/Mikes orginal approaches to support something like DocIdSet.useBits()/isSparse() whatever, as this is also by segment. There is also a second problem: It might happen that one filter returns a DocIdSet that does not support bits() for one segment, but another one for other segments? How to handle that? There is one case where this happens (DocIdSet.EMPTY_DOCIDSET always returns null for bits) - but this one is grafefully handled by an early exit condition, so we won't get NPE. The only possible solution is to make Filters always request in-order scoring, but this would limit our optimization possibilities. Finally I still think we should fix BS1 and BS2 to return identical scores (and write a test for that which compares scores). Second, in Mike's document/score listing above with/wo patch, I see no score differences, only order of docs is different (which is caused by out-of-order missing), so where is the problem?
          Hide
          Robert Muir added a comment -

          There is also a second problem: It might happen that one filter returns a DocIdSet that does not support bits() for one segment, but another one for other segments? How to handle that?

          This is the most serious problem I think.

          One solution: we can always JUST only ever use BS2. This is all
          filters/filteredquery does today so its no regression, just an optimization we leave on
          the table until it can be implemented cleanly.

          If we decide to do that, we are still left with the random-access-or-not-heuristic. But we could safely do this per-segment because BS2 is going to return the same set of scorers with or without bits (we know this ourselves, so its safe to do).

          And I already committed LUCENE-3503 so it should be consistent with itself

          Show
          Robert Muir added a comment - There is also a second problem: It might happen that one filter returns a DocIdSet that does not support bits() for one segment, but another one for other segments? How to handle that? This is the most serious problem I think. One solution: we can always JUST only ever use BS2. This is all filters/filteredquery does today so its no regression, just an optimization we leave on the table until it can be implemented cleanly. If we decide to do that, we are still left with the random-access-or-not-heuristic. But we could safely do this per-segment because BS2 is going to return the same set of scorers with or without bits (we know this ourselves, so its safe to do). And I already committed LUCENE-3503 so it should be consistent with itself
          Hide
          Robert Muir added a comment -

          Also is there a reason why indexsearcher/filteredquery wouldnt just use ReqExclScorer when its not random-access?

          Show
          Robert Muir added a comment - Also is there a reason why indexsearcher/filteredquery wouldnt just use ReqExclScorer when its not random-access?
          Hide
          Robert Muir added a comment -

          i see, we would have to negate the bits, maybe we should pull out FilteredQ's anon scorer into a separate .java file for consistency.

          Show
          Robert Muir added a comment - i see, we would have to negate the bits, maybe we should pull out FilteredQ's anon scorer into a separate .java file for consistency.
          Hide
          Chris Male added a comment -

          Doing that (pulling the anon scorer out) would make the classes more readable IMO too.

          Show
          Chris Male added a comment - Doing that (pulling the anon scorer out) would make the classes more readable IMO too.
          Hide
          Michael McCandless added a comment -

          Uwe, there are tiny (iota) score diffs for a few docs... eg 8684145 has score 31.100195 in one case but 31.100193 in the other (last digit differs).

          Show
          Michael McCandless added a comment - Uwe, there are tiny (iota) score diffs for a few docs... eg 8684145 has score 31.100195 in one case but 31.100193 in the other (last digit differs).
          Hide
          Michael McCandless added a comment -

          Isn't it strange to ask each filter impl to do ANDing for us?

          Like shouldn't we AND "on top" ourselves? Are there compelling
          performance gains by requiring every filter impl to do this for us?
          If the filter impl just delegates to BitsFilteredDocIdSet.wrap then we
          may as well just do that on top instead?

          With the scorer API, it is compelling to do this since our enum impls
          already take a liveDocs, so it's easy for scorers to respect this and
          we get enormous perf gains by pushing the AND inside the scorers.

          So I'm not sure this API change makes sense... unless there really is
          no other (cleaner) way for us to note that a filter already contains
          only live docs.

          But then implementing your own filter is rather expert so maybe it's
          not such a big deal to ask you to AND this other bitset we hand you.
          And this solution (I think! see comment above) will still allow for
          CachingWrapperFilter to re-AND liveDocs only once on reopen and then
          cache and reuse that.

          Show
          Michael McCandless added a comment - Isn't it strange to ask each filter impl to do ANDing for us? Like shouldn't we AND "on top" ourselves? Are there compelling performance gains by requiring every filter impl to do this for us? If the filter impl just delegates to BitsFilteredDocIdSet.wrap then we may as well just do that on top instead? With the scorer API, it is compelling to do this since our enum impls already take a liveDocs, so it's easy for scorers to respect this and we get enormous perf gains by pushing the AND inside the scorers. So I'm not sure this API change makes sense... unless there really is no other (cleaner) way for us to note that a filter already contains only live docs. But then implementing your own filter is rather expert so maybe it's not such a big deal to ask you to AND this other bitset we hand you. And this solution (I think! see comment above) will still allow for CachingWrapperFilter to re-AND liveDocs only once on reopen and then cache and reuse that.
          Hide
          Uwe Schindler added a comment -

          Patch that for now (until BooleanWeight is fixed) rewuires in-order scoring when a filter is applied, as suggested by Robert.

          There was also a silly bug in CachingWrapperFilter that made it not to apply acceptDocs on cached results. This may also be a reason for result differences.

          Mike: "Normal Filters" always respect acceptDocs, as they generally use the acceptDocs to pass them to IndexReader. E.g. MTQWF or all other filters. Only some special filters e.g. working on FieldCache have to respect acceptDocs. So there is no slowdown at all, it all exactly as before (pre-acceptDocs Filters always used getLiveDocs() - although they were not required to do so). The silly thing was that we applied accept docs simply at too many places, so we decided to make the filters do it themselves (as they can, because in previous patch they always called IR.getLiveDocs(); except FiledCacheFilters).

          The optimizations for CachingWrapperFilter to also cache acceptDocs should maybe done in a separate issue. We have currently no slowdown, as its still faster than before. Improvements can come later.

          I agree, we can make the pair (IR, acceptDocs) a key into the cache map, the only problem is that accpetDocs are not required to implement equals/hashCode - and if they do, like in FixedBitSet, it's slow. So the cache should do this using systemHashCode/IdentityHashMap-like algorithm (so only compare the Bits pointer, not contents).

          Show
          Uwe Schindler added a comment - Patch that for now (until BooleanWeight is fixed) rewuires in-order scoring when a filter is applied, as suggested by Robert. There was also a silly bug in CachingWrapperFilter that made it not to apply acceptDocs on cached results. This may also be a reason for result differences. Mike: "Normal Filters" always respect acceptDocs, as they generally use the acceptDocs to pass them to IndexReader. E.g. MTQWF or all other filters. Only some special filters e.g. working on FieldCache have to respect acceptDocs. So there is no slowdown at all, it all exactly as before (pre-acceptDocs Filters always used getLiveDocs() - although they were not required to do so). The silly thing was that we applied accept docs simply at too many places, so we decided to make the filters do it themselves (as they can, because in previous patch they always called IR.getLiveDocs(); except FiledCacheFilters). The optimizations for CachingWrapperFilter to also cache acceptDocs should maybe done in a separate issue. We have currently no slowdown, as its still faster than before. Improvements can come later. I agree, we can make the pair (IR, acceptDocs) a key into the cache map, the only problem is that accpetDocs are not required to implement equals/hashCode - and if they do, like in FixedBitSet, it's slow. So the cache should do this using systemHashCode/IdentityHashMap-like algorithm (so only compare the Bits pointer, not contents).
          Hide
          Robert Muir added a comment -

          Patch that for now (until BooleanWeight is fixed) rewuires in-order scoring when a filter is applied, as suggested by Robert.

          I'm still not sure BooleanWeight needs to be fixed: given the same parameters it will always return the same scorers.

          It seems to me these parameters (topLevel/scoresInOrder) really shouldn't be parameters to weight.scorer()!

          Show
          Robert Muir added a comment - Patch that for now (until BooleanWeight is fixed) rewuires in-order scoring when a filter is applied, as suggested by Robert. I'm still not sure BooleanWeight needs to be fixed: given the same parameters it will always return the same scorers. It seems to me these parameters (topLevel/scoresInOrder) really shouldn't be parameters to weight.scorer()!
          Hide
          Michael McCandless added a comment -

          Mike: "Normal Filters" always respect acceptDocs, as they generally use the acceptDocs to pass them to IndexReader.

          OK this makes sense (that many filter impls will need to pass through the accept docs down to eventual enums).

          So I think the API change is good!

          The optimizations for CachingWrapperFilter to also cache acceptDocs should maybe done in a separate issue. We have currently no slowdown, as its still faster than before. Improvements can come later.

          OK we can add it back under a new issue after committing this; but I
          think it's important to not lose this (CachingWrapperFilter today is
          able to pre-AND the liveDocs and cache that).

          It sounds like the cache key just has to become a pair of reader and
          identity(acceptDocs), but we should only cache when acceptDocs ==
          reader.getLiveDocs, else we can easily over-cache if in the future we
          pass "more interesting" acceptDocs down.

          Great!

          Show
          Michael McCandless added a comment - Mike: "Normal Filters" always respect acceptDocs, as they generally use the acceptDocs to pass them to IndexReader. OK this makes sense (that many filter impls will need to pass through the accept docs down to eventual enums). So I think the API change is good! The optimizations for CachingWrapperFilter to also cache acceptDocs should maybe done in a separate issue. We have currently no slowdown, as its still faster than before. Improvements can come later. OK we can add it back under a new issue after committing this; but I think it's important to not lose this (CachingWrapperFilter today is able to pre-AND the liveDocs and cache that). It sounds like the cache key just has to become a pair of reader and identity(acceptDocs), but we should only cache when acceptDocs == reader.getLiveDocs, else we can easily over-cache if in the future we pass "more interesting" acceptDocs down. Great!
          Hide
          Uwe Schindler added a comment -

          Do we have a conclusion about the current patch, so we can commit and work in other issues to improve? I also want to open issues to remove broken SpanFilter from core and move to sandbox.

          Show
          Uwe Schindler added a comment - Do we have a conclusion about the current patch, so we can commit and work in other issues to improve? I also want to open issues to remove broken SpanFilter from core and move to sandbox.
          Hide
          Chris Male added a comment -

          Is the only question mark remaining around the BooleanWeight work? If so, I think its definitely worth examining that in a wider separate issue after this is committed.

          Show
          Chris Male added a comment - Is the only question mark remaining around the BooleanWeight work? If so, I think its definitely worth examining that in a wider separate issue after this is committed.
          Hide
          Uwe Schindler added a comment -

          Is the only question mark remaining around the BooleanWeight work? If so, I think its definitely worth examining that in a wider separate issue after this is committed.

          The patch requests scorer always in order for now, so BooleanWeight is not mixed up for different segments. This is not different as in current trunk, as Scorers are always requested in order if filters are used. The optimization in the future would be to use out-of-order scoring if random access bits are used.

          Show
          Uwe Schindler added a comment - Is the only question mark remaining around the BooleanWeight work? If so, I think its definitely worth examining that in a wider separate issue after this is committed. The patch requests scorer always in order for now, so BooleanWeight is not mixed up for different segments. This is not different as in current trunk, as Scorers are always requested in order if filters are used. The optimization in the future would be to use out-of-order scoring if random access bits are used.
          Hide
          Chris Male added a comment -

          I was more referring to Robert's comment:

          It seems to me these parameters (topLevel/scoresInOrder) really shouldn't be parameters to weight.scorer()!

          Show
          Chris Male added a comment - I was more referring to Robert's comment: It seems to me these parameters (topLevel/scoresInOrder) really shouldn't be parameters to weight.scorer()!
          Hide
          Uwe Schindler added a comment -

          Yes, that should be sorted out in another issue. We have a working fix, the rest is optimization and unrelated api changes.

          Show
          Uwe Schindler added a comment - Yes, that should be sorted out in another issue. We have a working fix, the rest is optimization and unrelated api changes.
          Hide
          Uwe Schindler added a comment -

          I will commit this tomorrow, if nobody objects and we will work on further issues to improve Weight.scorer() API, CachingWrapperFilter,... There is no slowdown, only speedups with room to improve.

          Show
          Uwe Schindler added a comment - I will commit this tomorrow, if nobody objects and we will work on further issues to improve Weight.scorer() API, CachingWrapperFilter,... There is no slowdown, only speedups with room to improve.
          Hide
          Robert Muir added a comment -

          +1, lets commit this one and make progress here.

          Show
          Robert Muir added a comment - +1, lets commit this one and make progress here.
          Hide
          Uwe Schindler added a comment -

          Here the updated patch after some changes in trunk. It also adds missCount checks back to Caching*Filters, I lost then during cleanup.

          Show
          Uwe Schindler added a comment - Here the updated patch after some changes in trunk. It also adds missCount checks back to Caching*Filters, I lost then during cleanup.
          Hide
          Uwe Schindler added a comment -

          Committed trunk revision: 1188624

          Thanks to all!

          Show
          Uwe Schindler added a comment - Committed trunk revision: 1188624 Thanks to all!
          Hide
          Chris Male added a comment -

          Yay!

          Show
          Chris Male added a comment - Yay!

            People

            • Assignee:
              Uwe Schindler
              Reporter:
              Michael McCandless
            • Votes:
              2 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development