Fix failing BaseKnnVectorQueryTestCase#testTimeout #13283
Conversation
|
Side note: the original command to reproduce from #13202 (comment): ..does not work until we remove the Moreover, it deterministically fails with a minimal command of (stripping extra params): ..which does not reproduce without I tried running the minimal command above using all 3 seed values from the linked comment, and the test fails deterministically before this PR and passes afterwards |
There was a problem hiding this comment.
Good find, I agree with the fix. The current test was fragile. CountingQueryTimeout can only count the no. of times queryTimeout.shouldExit() was invoked. The test fails if it gets called before a collect() call, which is happening here when we find the best entry point.
Since QueryTimeout doesn't have access to no. of results collected, I don't see of a reliable way of timing out only after x results are picked. We can only provide an upper bound on the assert, not a lower bound.
|
Let us also unmute the broken tests as part of this change? |
| assertEquals(1, searcher.count(query)); // Expect only 1 result | ||
| // Note: We get partial results when the HNSW graph has 1 layer, but no results for > 1 layer | ||
| // because the timeout is exhausted while finding the best entry node for the last level | ||
| assertTrue(searcher.count(query) <= 1); // Expect at most 1 result |
* Fix failing BaseKnnVectorQueryTestCase#testTimeout * Also fix ParentBlockJoinKnnVectorQueryTestCase#testTimeout --------- Co-authored-by: Kaival Parikh <[email protected]>
* Fix failing BaseKnnVectorQueryTestCase#testTimeout * Also fix ParentBlockJoinKnnVectorQueryTestCase#testTimeout --------- Co-authored-by: Kaival Parikh <[email protected]>
|
Backported to |
Description
Fixes #13272
The failure happens because of this assumption of the HNSW graph having just 1 level, but we get more than 1 for some seed values -- so the counting timeout of 1 node is exhausted while trying to find the best entry point for the last level, and we consequently get no results, failing this assertion
The assertion wants to check that when an approximate search is performed (some docs are fed to the collector), we return partial results even after a timeout -- and we can check this explicitly in a separate test for
TimeLimitingKnnCollectorManagerWith that test in place, and since we cannot easily control the number of levels in the underlying graph, can we relax the test to check for "at most 1 result" instead of "exactly 1 result"?