[GitHub] [lucene-solr] atris commented on issue #769: LUCENE-8905: Better Error Handling For Illegal Arguments
atris commented on issue #769: LUCENE-8905: Better Error Handling For Illegal Arguments URL: https://github.com/apache/lucene-solr/pull/769#issuecomment-528487667 > TestDiversifiedTopDocsCollector Thanks for taking a look. The test failure is not really a code issue but a test issue in `TestDiversifiedTopDocsCollector#testInvalidArguments` where it needed updating to expect exceptions and error messages. Fixed the same and raised #858 , please take a look This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] atris commented on issue #769: LUCENE-8905: Better Error Handling For Illegal Arguments
atris commented on issue #769: LUCENE-8905: Better Error Handling For Illegal Arguments URL: https://github.com/apache/lucene-solr/pull/769#issuecomment-528333650 @jpountz Updated the PR. Ran the test suite, came in clean. Also, beasted `TestGrouping` 20 times, ran clean. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] atris commented on issue #769: LUCENE-8905: Better Error Handling For Illegal Arguments
atris commented on issue #769: LUCENE-8905: Better Error Handling For Illegal Arguments URL: https://github.com/apache/lucene-solr/pull/769#issuecomment-528310009 @jpountz Thinking more about this, should we be asserting if howMany is greater than start? Would that still be true for pagination? This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] atris commented on issue #769: LUCENE-8905: Better Error Handling For Illegal Arguments
atris commented on issue #769: LUCENE-8905: Better Error Handling For Illegal Arguments URL: https://github.com/apache/lucene-solr/pull/769#issuecomment-528260449 @jpountz Updated, please see This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] atris commented on issue #769: LUCENE-8905: Better Error Handling For Illegal Arguments
atris commented on issue #769: LUCENE-8905: Better Error Handling For Illegal Arguments URL: https://github.com/apache/lucene-solr/pull/769#issuecomment-515451533 I looked at the testGrouping failure -- looks like the test assumes that the second pass collector will always collect atleast docsInGroup hits, and thus takes a random offset there to start with. However, that is not always true, and the number of hits collected can be lesser. I think the ideal way of usage of topDocs is to get the totalHits from the corresponding TopDocsCollector and use that as the fence to set the starting point. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] atris commented on issue #769: LUCENE-8905: Better Error Handling For Illegal Arguments
atris commented on issue #769: LUCENE-8905: Better Error Handling For Illegal Arguments URL: https://github.com/apache/lucene-solr/pull/769#issuecomment-515440912 > > I'm wondering whether we should consider removing this `topDocs` variant which takes a `howMany` parameter. I can't think of a good reason to pass something else than `numHits-start` there? Is there a use-case I'm missing? > > I think it was intended to allow people to "scroll" through hits and "pick" a small subset to get e.g. get the hits from 4th best to 8th best. I agree that there is no concrete use case that comes to mind though, so +1 to removing it Another question is -- what is the expected behaviour when start > size? Should we error out, or return a null topdocs (as we do today), or normalize the start value to be 0? This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] atris commented on issue #769: LUCENE-8905: Better Error Handling For Illegal Arguments
atris commented on issue #769: LUCENE-8905: Better Error Handling For Illegal Arguments URL: https://github.com/apache/lucene-solr/pull/769#issuecomment-515432956 > I'm wondering whether we should consider removing this `topDocs` variant which takes a `howMany` parameter. I can't think of a good reason to pass something else than `numHits-start` there? Is there a use-case I'm missing? I think it was intended to allow people to "scroll" through hits and "pick" a small subset to get e.g. get the hits from 4th best to 8th best. I agree that there is no concrete use case that comes to mind though, so +1 to removing it This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] atris commented on issue #769: LUCENE-8905: Better Error Handling For Illegal Arguments
atris commented on issue #769: LUCENE-8905: Better Error Handling For Illegal
Arguments
URL: https://github.com/apache/lucene-solr/pull/769#issuecomment-515429984
> I ran top-level `ant test` and hit this exception on the PR as of
yesterday:
>
> ```
>[junit4] Suite: org.apache.lucene.search.grouping.TestGrouping
>[junit4] 2> NOTE: reproduce with: ant test -Dtestcase=TestGrouping
-Dtests.method=testRandom -Dtests.seed=F08BE82DC9B3756C -Dtests.badapples=true
-Dtests.locale=es-BZ -Dtests.timezone\
> =Africa/Gaborone -Dtests.asserts=true -Dtests.file.encoding=ISO-8859-1
>[junit4] ERROR 1.65s J0 | TestGrouping.testRandom <<<
>[junit4]> Throwable #1: java.lang.IllegalArgumentException:
Expected value of starting position is between 0 and 3, got 7
>[junit4]>at
__randomizedtesting.SeedInfo.seed([F08BE82DC9B3756C:82C7CD2278D3C31F]:0)
>[junit4]>at
org.apache.lucene.search.TopDocsCollector.topDocs(TopDocsCollector.java:141)
>[junit4]>at
org.apache.lucene.search.grouping.TopGroupsCollector.getTopGroups(TopGroupsCollector.java:163)
>[junit4]>at
org.apache.lucene.search.grouping.TestGrouping.getTopGroups(TestGrouping.java:319)
>[junit4]>at
org.apache.lucene.search.grouping.TestGrouping.testRandom(TestGrouping.java:965)
>[junit4]>at
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
>[junit4]>at
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
>[junit4]>at
java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
>[junit4]>at
java.base/java.lang.reflect.Method.invoke(Method.java:566)
>[junit4]>at java.base/java.lang.Thread.run(Thread.java:834)
>[junit4] 2> NOTE: test params are: codec=Asserting(Lucene80):
{groupend=TestBloomFilteredLucenePostings(BloomFilteringPostingsFormat(Lucene50(blocksize=128))),
sort1=FST50, sort2=TestB\
>
loomFilteredLucenePostings(BloomFilteringPostingsFormat(Lucene50(blocksize=128))),
content=PostingsFormat(name=Direct), group=FST50},
docValues:{author=DocValuesFormat(name=Asserting), sort\
> 1=DocValuesFormat(name=Lucene80), id=DocValuesFormat(name=Lucene80),
sort2=DocValuesFormat(name=Direct),
___soft_deletes=DocValuesFormat(name=Lucene80),
group=DocValuesFormat(name=Lucene80)\
> }, maxPointsInLeafNode=2036, maxMBSortInHeap=7.045070063116054,
sim=Asserting(org.apache.lucene.search.similarities.AssertingSimilarity@19474185),
locale=es-BZ, timezone=Africa/Gaborone
>[junit4] 2> NOTE: Linux 4.4.0-96-generic amd64/Oracle Corporation
11.0.2 (64-bit)/cpus=8,threads=1,free=510432528,total=536870912
>[junit4] 2> NOTE: All tests run in this JVM: [TestGrouping]
> ```
Interesting, I cannot see this for previous versions of Lucene, but rebasing
got me this error. I added the check Adrien suggested and got the following
failure:
java.lang.IllegalArgumentException: Requested number of hits greater than
original hits limit for the collector. size 4 start 2 howMany 49
I think why that is happening is because today, we fix howMany to be within
valid limits in case it is larger than the available number of hits, rather
than erroring out.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] atris commented on issue #769: LUCENE-8905: Better Error Handling For Illegal Arguments
atris commented on issue #769: LUCENE-8905: Better Error Handling For Illegal Arguments URL: https://github.com/apache/lucene-solr/pull/769#issuecomment-514522181 Any further thoughts on this one? Happy to iterate This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] atris commented on issue #769: LUCENE-8905: Better Error Handling For Illegal Arguments
atris commented on issue #769: LUCENE-8905: Better Error Handling For Illegal Arguments URL: https://github.com/apache/lucene-solr/pull/769#issuecomment-512156642 @jpountz Updated the PR. With the current version, all tests for both Lucene and Solo pass (yay!). This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] atris commented on issue #769: LUCENE-8905: Better Error Handling For Illegal Arguments
atris commented on issue #769: LUCENE-8905: Better Error Handling For Illegal Arguments URL: https://github.com/apache/lucene-solr/pull/769#issuecomment-511831246 > @atris I think the check you introduced always fails a query when there are no hits? @jpountz Thanks for highlighting that. Interesting that TopDocsCollector handles no hits and illegal argument cases in the same check. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] atris commented on issue #769: LUCENE-8905: Better Error Handling For Illegal Arguments
atris commented on issue #769: LUCENE-8905: Better Error Handling For Illegal Arguments URL: https://github.com/apache/lucene-solr/pull/769#issuecomment-510879760 @mikemccand Thanks, updated the PR. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] atris commented on issue #769: LUCENE-8905: Better Error Handling For Illegal Arguments
atris commented on issue #769: LUCENE-8905: Better Error Handling For Illegal Arguments URL: https://github.com/apache/lucene-solr/pull/769#issuecomment-510879042 @jpountz I updated the PR per comments. Given some of the error stack traces, what is your advice on the next step forward? This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] atris commented on issue #769: LUCENE-8905: Better Error Handling For Illegal Arguments
atris commented on issue #769: LUCENE-8905: Better Error Handling For Illegal Arguments URL: https://github.com/apache/lucene-solr/pull/769#issuecomment-510486183 @jpountz Another one: TestBooleanMinShouldMatch.testMinHigherThenNumOptional <<< [junit4]> Throwable #1: java.lang.IllegalArgumentException: Illegal arguments when requesting top docs [junit4]> at __randomizedtesting.SeedInfo.seed([E2642EBDFCA3189E:98AE10AFC431B37C]:0) [junit4]> at org.apache.lucene.search.TopDocsCollector.topDocs(TopDocsCollector.java:141) [junit4]> at org.apache.lucene.search.TopDocsCollector.topDocs(TopDocsCollector.java:96) [junit4]> at org.apache.lucene.search.IndexSearcher$2.reduce(IndexSearcher.java:480) [junit4]> at org.apache.lucene.search.IndexSearcher$2.reduce(IndexSearcher.java:468) [junit4]> at org.apache.lucene.search.IndexSearcher.search(IndexSearcher.java:636) [junit4]> at org.apache.lucene.search.IndexSearcher.searchAfter(IndexSearcher.java:487) [junit4]> at org.apache.lucene.search.IndexSearcher.search(IndexSearcher.java:498) [junit4]> at org.apache.lucene.search.TestBooleanMinShouldMatch.verifyNrHits(TestBooleanMinShouldMatch.java:87) [junit4]> at org.apache.lucene.search.TestBooleanMinShouldMatch.testMinHigherThenNumOptional(TestBooleanMinShouldMatch.java:256) [junit4]> at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method) [junit4]> at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) [junit4]> at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) [junit4]> at java.base/java.lang.reflect.Method.invoke(Method.java:566) [junit4]> at java.base/java.lang.Thread.run(Thread.java:834) This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] atris commented on issue #769: LUCENE-8905: Better Error Handling For Illegal Arguments
atris commented on issue #769: LUCENE-8905: Better Error Handling For Illegal Arguments URL: https://github.com/apache/lucene-solr/pull/769#issuecomment-510398140 > For instance I suspect that most of those are due to the call to `return TopFieldCollector.create(rewrittenSort, cappedNumHits, after, TOTAL_HITS_THRESHOLD);` in IndexSearcher, which we'd just need to replace with `return TopFieldCollector.create(rewrittenSort, cappedNumHits, after, Math.max(cappedNumHits, TOTAL_HITS_THRESHOLD));`? -Dtests.timezone=Etc/GMT+5 -Dtests.asserts=true -Dtests.file.encoding=UTF-8 [junit4] ERROR 0.01s J0 | TestIndexSearcher.testHugeN <<< Throwable #1: java.lang.IllegalArgumentException: Illegal arguments when requesting top docs [junit4]> at __randomizedtesting.SeedInfo.seed([5E8112825AFE9F23:231370EC0B1BDF30]:0) [junit4]> at org.apache.lucene.search.TopDocsCollector.topDocs(TopDocsCollector.java:141) [junit4]> at org.apache.lucene.search.TopDocsCollector.topDocs(TopDocsCollector.java:96) [junit4]> at org.apache.lucene.search.IndexSearcher$2.reduce(IndexSearcher.java:480) [junit4]> at org.apache.lucene.search.IndexSearcher$2.reduce(IndexSearcher.java:468) [junit4]> at org.apache.lucene.search.IndexSearcher.search(IndexSearcher.java:636) [junit4]> at org.apache.lucene.search.IndexSearcher.searchAfter(IndexSearcher.java:487) [junit4]> at org.apache.lucene.search.TestIndexSearcher.testHugeN(TestIndexSearcher.java:104) [junit4]> at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method) [junit4]> at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) [junit4]> at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] atris commented on issue #769: LUCENE-8905: Better Error Handling For Illegal Arguments
atris commented on issue #769: LUCENE-8905: Better Error Handling For Illegal Arguments URL: https://github.com/apache/lucene-solr/pull/769#issuecomment-509649701 [junit4] Tests with failures [seed: 214CBCDED4C34EFB] (first 10 out of 156): [junit4] - org.apache.lucene.search.spans.TestNearSpansOrdered.testOverlappedOrderedSpan [junit4] - org.apache.lucene.search.TestTopFieldCollectorEarlyTermination.testEarlyTermination [junit4] - org.apache.lucene.search.TestTopFieldCollectorEarlyTermination.testEarlyTerminationWhenPaging [junit4] - org.apache.lucene.search.TestSloppyPhraseQuery2.testIncreasingSloppiness [junit4] - org.apache.lucene.search.TestSloppyPhraseQuery2.testRepetitiveIncreasingSloppinessWithHoles [junit4] - org.apache.lucene.search.TestSloppyPhraseQuery2.testIncreasingSloppiness3WithHoles [junit4] - org.apache.lucene.search.TestSloppyPhraseQuery2.testRepetitiveIncreasingSloppiness3 [junit4] - org.apache.lucene.search.TestSloppyPhraseQuery2.testIncreasingSloppiness3 [junit4] - org.apache.lucene.search.TestSloppyPhraseQuery2.testIncreasingSloppinessWithHoles [junit4] - org.apache.lucene.search.TestSloppyPhraseQuery2.testRepetitiveIncreasingSloppiness This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] atris commented on issue #769: LUCENE-8905: Better Error Handling For Illegal Arguments
atris commented on issue #769: LUCENE-8905: Better Error Handling For Illegal Arguments URL: https://github.com/apache/lucene-solr/pull/769#issuecomment-509592047 Thanks @mikemccand and @msokolov Unfortunately the number of tests failing is a bit too huge :( I was hoping we could have a gradual effort (maybe take this on as a technical debt and push it to a branch?) This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] atris commented on issue #769: LUCENE-8905: Better Error Handling For Illegal Arguments
atris commented on issue #769: LUCENE-8905: Better Error Handling For Illegal Arguments URL: https://github.com/apache/lucene-solr/pull/769#issuecomment-509478042 Ok, it breaks quite a lot of tests. I investigated around 30 odd failures, and looks like all of those tests are failing on the new exception. Does that mean that our tests are silently passing in malformed arguments, and relying on the inability of TopDocsCollector to complain? This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
