LUCENE-10641: IndexSearcher#setTimeout should also abort query rewrites, point ranges and vector searches #12345

Deepika0510 · 2023-06-01T19:14:38Z

Description

IndexSearcher only checks the query timeout in the collection phase for now. Need to add timeout support in case of other operations that may take time such as query rewrite, point ranges and vector searches. In this PR, we are covering the case of query re-write

Solution

Added timeout support in case of query rewrite operation in IndexSearcher.

Tests

Added UT for testing timeout during query re-write operation

Checklist

Please review the following and check all that apply:

I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
I have given Lucene maintainers access to contribute to my PR branch. (optional but recommended)
I have developed this patch against the main branch.
I have run ./gradlew check.
I have added tests for my changes.

mikemccand · 2023-06-09T16:09:24Z

lucene/core/src/java/org/apache/lucene/search/IndexSearcher.java

@@ -763,6 +763,11 @@ public Query rewrite(Query original) throws IOException {
    for (Query rewrittenQuery = query.rewrite(this);
        rewrittenQuery != query;
        rewrittenQuery = query.rewrite(this)) {
+      if (queryTimeout != null) {


This is an improvement, however, it's only checking at the very end of rewrite? Which is not so different from checking at the start of iterating through the hits? I.e. in practice it seems like it won't make much of an improvement?

I wonder if we can somehow check the timeout during rewrite? Seems tricky though ...

The timeout implementation that wraps Directory can achieve that by checking during I/O.

+1 to wrap the directory similarly to what ExitableDirectoryReader does

Thank you @mikemccand and @jpountz!

@jpountz I didn't get the idea behind wrapping Directory here. The rewrite method produce a more efficient query for search, I wonder how it is related to Directory class?

@Deepika0510 I think what @jpountz meant is we should somehow intercept the I/O Lucene is inevitably doing in a rewrite implementation to check for timeout. Similar to how ExitableDirectoryReader wraps the low level Lucene APIs and checks for timeout.

But I don't see how we could cleanly do this (wrap Directory) in the context of rewrite.

Maybe instead we just use ExitableDirectoryReader, but make a new generalized version ExitableIndexReader that takes any IndexReader (not just the DirectoryReader implementation)?

Then, if a timeout is set on IndexSearcher, we use this wrapped timeout IndexReader in rewrite somehow? We should maybe create that wrapped reader once on IndexSearcher.setTimeout calls and reuse it across all concurrent rewrites?

Woops, sorry for missing your question. @mikemccand 's understanding is correct, something like ExitableDirectoryReader. +1 to @mikemccand 's suggestion:

Implement a package-private ExitableIndexReader, which is really the same as ExitableDirectoryReader but for any IndexReader. It should wrap terms and points, but not postings and doc values (which are already covered via the bulk scorer).

Update IndexSearcher#getIndexReader to return the wrapped index reader. This will automatically make sure expensive rewrites are intercepted since IndexSearcher#getIndexReader is their only way to get access to the reader.

FYI, I just noticed this. ExitableDirectoryReader.ExitableFilterAtomicReader does indeed already have timeout checking for searching vectors. But it only has it for float[]. I am guessing something was missed during the byte[] rewrite and update. I am going to add support for timeout checking for byte[] in that class.

Just in case you get weird merge conflicts while working through this.

Thank you @mikemccand and @jpountz for suggestion!

I was going through the code and came across the below scenario. We want to wrap terms and points, but the related methods are in LeafReader class and we access these method generally using the getContext().leaves().get(0).reader().<method>.   

E.g. 

IndexReaderContext topReaderContext = reader.getContext(); for (LeafReaderContext context : topReaderContext.leaves()) { final Terms terms = context.reader().terms(query.field); …… …… …… }

So, even if we wrap LeafReader and update IndexSearcher#getIndexReader() to return the wrapped reader, we would not be able to return wrapped class at such above point where we get the reader using ReaderContext .

Maybe one way out would be to create a separate ReaderContext such that it returns our wrapped IndexReader?

This should work automatically if you create an IndexReader wrapper. See how CompositeReader#getContext is implemented.

Deepika0510

@jpountz, @mikemccand
I have created a wrapper class of IndexReader and tried to make minimal changes in other files. I added logic in CompositeReaderContext to handle ExitableIndexReader and needed to handle closing of inner wrapped IndexReader to avoid thread leaks.

jpountz · 2023-07-20T16:40:16Z

@Deepika0510 You don't only need to wrap the IndexReader, you also need to wrap all its leaves.

Deepika0510 · 2023-09-17T10:21:33Z

@jpountz To wrap all the leaves, we would need to wrap ReaderContext classes along with LeafReader classes as well right? Since, we generally access the leaves through the ReaderContext class. Something like this:

reader().getContext().leaves().reader()

mikemccand · 2023-10-19T14:19:21Z

I don't think you need to wrap ReaderContext classes -- you can create your new TimeoutLeafReader class, subclassing FilterLeafReader, and overriding the methods (likely with additional wrapping on their returned objects?) that are needing added timeouts.

Then a call lke reader().getContext().leaves().reader() will return your TimeoutLeafReader but it will implement all necessary functionality of a LeafReader.

Deepika0510 · 2023-10-29T11:56:39Z

However, to get that TimeoutLeafReader in use, we would need to go through the ReaderContext class route(?). In a way we would need some mechanism in ReaderClass to know if timeout is applied and then return TimeoutLeafReader object.

mikemccand · 2023-10-29T15:57:44Z

Hmm I'm confused: why would you need to get to the TimeoutLeafReader? Don't you create this timeout reader, passing the timeout to it (which will apply to all queries) and then you don't need to get to it anymore? Just catch the timeout exception when running searching?

Deepika0510 · 2023-11-02T15:24:40Z

What I meant to ask is that after creating the TimeoutLeafReader class, how would we make sure that this wrapped class's object is used instead of any normal LeafReader instance?

E.g. when we created wrapped IndexReader then we modified the getIndexReader() method such that if timeout is applied then we make sure that we return the wrapped IndexReader object.

Similarly, we have to ensure the same for the TimeoutLeafReader as well right? That is where my doubt lies that since LeafReader is accessed through indexReader.getContext().leaves().reader() (like here) then shouldn’t we need to intercept in between this to return the object of TimeoutLeafReader?

Deepika0510 · 2023-11-02T15:32:02Z

Came across SoftDeletesDirectoryReaderWrapper where we have wrap method for wrapping underlying LeafReader.
However, I believe we will still have the problem when the leaves are directly accessed through ctx object like here. Is there any other way around? @mikemccand @jpountz

mikemccand · 2023-11-02T17:00:32Z

Hi @Deepika0510 -- what is the problem when callers access the leaves? Since you would subclass FilterLeafReader (which subclasses LeafReader) it should be fine to existing code? Like that line you linked to above will still be able to call .maxDoc().

Deepika0510

Thank you @mikemccand @jpountz . I looked into SoftDeletesDirectoryReaderWrapper and tried wrapping leafReader the way we are wrapping SubReader in doWrapDirectoryReader. I was able to wrap leadReader and we are able to enforce time out.

Deepika0510

Fix failing gradle check.

mikemccand

This is looking closer! Thanks @Deepika0510.

mikemccand · 2023-12-05T11:38:04Z

lucene/core/src/java/org/apache/lucene/index/ExitableIndexReader.java

+      for (LeafReaderContext leafCtx : leaves) {
+        LeafReader reader = leafCtx.reader();
+        readers.add(reader);
+        // we try to reuse the life docs instances here if the reader cache key didn't change


life -> live

mikemccand · 2023-12-05T11:39:49Z

lucene/core/src/java/org/apache/lucene/index/ExitableIndexReader.java

+   * TimeoutLeafReader is wrapper class for FilterLeafReader which is imposing timeout on different
+   * operations of FilterLeafReader
+   */
+  public static class TimeoutLeafReader extends FilterLeafReader {


Does it need to be public?

mikemccand · 2023-12-05T11:40:25Z

lucene/core/src/java/org/apache/lucene/index/ExitableIndexReader.java

+     */
+    protected TimeoutLeafReader(LeafReader in, QueryTimeout queryTimeout) {
+      super(in);
+      if (in == null) {


You could instead / more compactly do:

super(Objects.requireNonNull(in));

I think?

mikemccand · 2023-12-05T11:41:53Z

lucene/core/src/test/org/apache/lucene/index/TestExitableIndexReader.java

+  private static QueryTimeout countingQueryTimeout(int timeallowed) {
+
+    return new QueryTimeout() {
+      static int counter = 0;


You don't need = 0 -- it's already Java's default.

mikemccand · 2023-12-05T11:42:09Z

lucene/core/src/test/org/apache/lucene/index/TestExitableIndexReader.java

+    directory.close();
+  }
+
+  private static QueryTimeout countingQueryTimeout(int timeallowed) {


Rename to timeAllowed?

mikemccand · 2023-12-05T11:43:00Z

lucene/facet/src/java/org/apache/lucene/facet/StringDocValuesReaderState.java

@@ -46,9 +47,13 @@ public class StringDocValuesReaderState {
   * (e.g., to pickup NRT updates) requires constructing a new state instance.
   */
  public StringDocValuesReaderState(IndexReader reader, String field) throws IOException {
-    this.reader = reader;
+    if (reader instanceof ExitableIndexReader) {


I don't think we should do this here? Caller should not be using a ExitableDirectoryReader when constructing this state?

mikemccand · 2023-12-05T11:43:48Z

lucene/core/src/java/org/apache/lucene/search/IndexSearcher.java

@@ -372,6 +373,9 @@ public static LeafSlice[] slices(

  /** Return the {@link IndexReader} this searches. */
  public IndexReader getIndexReader() {
+    if (queryTimeout != null) {


Hmm this seems dangerous -- getters shouldn't be doing magical / surprising things I think? Could we instead require/expect caller to creating the IndexSearcher with the ExitableDirectoryReader?

Edit: or perhaps when IndexSearcher calls rewrite, specifically, if there is a queryTimeout, it could wrap the reader at that point, only?

mikemccand · 2023-12-05T11:44:35Z

lucene/facet/src/test/org/apache/lucene/facet/TestRandomSamplingFacetsCollector.java

@@ -147,7 +149,10 @@ public void testRandomSampling() throws Exception {
          Math.min(5 * sampled.value.floatValue(), numDocs / 10.f),
          1.0);
    }
-
-    IOUtils.close(searcher.getIndexReader(), taxoReader, dir, taxoDir);
+    IndexReader r = searcher.getIndexReader();


We shouldn't do this either? In general all code using IndexReader should not need to specially check for ExitableDirectoryReader. Rather, callers should not pass ExitableDirectoryReader to these places.

mikemccand · 2023-12-05T11:48:47Z

lucene/core/src/java/org/apache/lucene/index/ExitableIndexReader.java

+      if (queryTimeout.shouldExit()) {
+        throw new ExitableIndexReader.TimeExceededException();
+      }
+      return in.terms(field);


Hmm I think you need to further wrap here? Likely the timeout will not have been hit when the rewrite first starts. That rewrite first calls .terms() for each segment, and then does the hard work of getting the .iterator() from it and stepping / seeking through the resulting TermsEnum.

So I think you would need to return a ExitableTerms, which in turn returns an ExitableTermsEnum, etc.?

And perhaps same thing for points, doc values, though we could defer those for a followon change?

Deepika0510

Thank you @mikemccand for reviewing the PR. I have addressed the comment. Please have a look.

mikemccand

Thanks @Deepika0510 -- this looks closer! I left a few comments.

mikemccand · 2024-01-02T19:18:51Z

lucene/core/src/java/org/apache/lucene/index/ExitableIndexReader.java

+      }
+      ExitableSubReaderWrapper exitableSubReaderWrapper =
+          new ExitableSubReaderWrapper(readerCache, queryTimeout);
+      exitableSubReaderWrapper.wrap(readers);


Hmm shouldn't we do something with the returned wrap'd readers?

mikemccand · 2024-01-02T19:19:55Z

lucene/core/src/java/org/apache/lucene/index/ExitableIndexReader.java

+        // creating a new one
+        return mapping.get(readerCacheHelper.getKey());
+      }
+      return new TimeoutLeafReader(reader, queryTimeout);


Shouldn't we put this new wrap'd reader into the mapping too?

mikemccand · 2024-01-02T19:23:15Z

lucene/core/src/java/org/apache/lucene/index/ExitableIndexReader.java

+  public ExitableIndexReader(IndexReader indexReader, QueryTimeout queryTimeout) {
+    this.indexReader = indexReader;
+    this.queryTimeout = queryTimeout;
+    doWrapIndexReader(indexReader, queryTimeout);


I think this should return something and maybe you do this.indexReader = doWrapIndexReader(...); or so?

mikemccand · 2024-01-02T19:24:27Z

lucene/core/src/java/org/apache/lucene/index/ExitableIndexReader.java

+     * Throws {@link ExitableDirectoryReader.ExitingReaderException} if {@link
+     * QueryTimeout#shouldExit()} returns true, or if {@link Thread#interrupted()} returns true.
+     */
+    private void checkTimeoutWithSampling() {


Hmm why is this named WithSampling? Is it supposed to try to check less often since this might be a hot spot? But it seems to just check every time it's called.

mikemccand · 2024-01-02T19:30:20Z

lucene/core/src/java/org/apache/lucene/index/ExitableIndexReader.java

+      if (queryTimeout.shouldExit()) {
+        throw new ExitableIndexReader.TimeExceededException();
+      }
+      return in.getPointValues(field);


Should we wrap points as well? Or we can open a follow-on issue. Timeout during points intersect may not be so important? Not sure ...

github-actions · 2024-01-17T00:13:59Z

This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the [email protected] list. Thank you for your contribution!

Deepika0510

Thank you @mikemccand! I have made the required changes, please have a look.

mikemccand

Hi @Deepika0510 -- thank you for persisting here!

I added a bunch of comments. Lucene's IndexReader hierarchy is super tricky and I am not even certain about the comments I wrote. But the big picture is I think you can share some of the wrapper classes directly from ExitableDirectoryReader, and then also fork small parts of that class into ExitableIndexReader while switching DirectoryReader to IndexReader?

mikemccand · 2024-02-05T16:09:43Z

lucene/core/src/java/org/apache/lucene/index/ExitableIndexReader.java

+   */
+  public ExitableIndexReader(IndexReader indexReader, QueryTimeout queryTimeout)
+      throws IOException {
+    this.indexReader = new ExitableIndexReaderWrapper((DirectoryReader) indexReader, queryTimeout);


Hmm I think we should not be casting incoming indexReader to a DirectoryReader? The idea of this new class is to enforce timeouts for any IndexReader, not just DirectoryReader (which is a subclass of IndexReader), I think?

In fact, you might be able to start by copying ExitableDirectoryReader.java to ExitableIndexReader.java and then change all DirectoryReader to IndexReader and iterate from there? This way we would also get points timeout implemented. I realize this is all quite tricky!!

mikemccand · 2024-02-05T16:12:04Z

lucene/core/src/java/org/apache/lucene/index/ExitableIndexReader.java

+
+  /** Thrown when elapsed search time exceeds allowed search time. */
+  @SuppressWarnings("serial")
+  static class TimeExceededException extends RuntimeException {


I think we should have both ExitableDirectoryReader and this class to use a single class (the existing ExitingReaderException) when timeout happens?

mikemccand · 2024-02-05T16:15:12Z

lucene/core/src/java/org/apache/lucene/index/ExitableIndexReader.java

+
+  @Override
+  public StoredFields storedFields() throws IOException {
+    if (queryTimeout.shouldExit()) {


Can we factor out a private helper method that does this if and the throw? Just like ExitableDirectoryReader.checkAndThrow.

mikemccand · 2024-02-05T16:15:32Z

lucene/core/src/java/org/apache/lucene/index/ExitableIndexReader.java

+
+  @Override
+  public int docFreq(Term term) throws IOException {
+


Maybe remove this newline at the top of each of these methods?

mikemccand · 2024-02-05T16:22:12Z

lucene/core/src/java/org/apache/lucene/index/ExitableIndexReader.java

+    return indexReader.getSumTotalTermFreq(field);
+  }
+
+  private static class ExitableIndexReaderWrapper extends FilterDirectoryReader {


We should not be subclassing FilterDirectoryReader here since we want to work with any IndexReader.

mikemccand · 2024-02-05T16:24:58Z

lucene/core/src/java/org/apache/lucene/index/ExitableIndexReader.java

+     * TimeoutLeafReader is wrapper class for FilterLeafReader which is imposing timeout on
+     * different operations of FilterLeafReader
+     */
+    private static class TimeoutLeafReader extends FilterLeafReader {


I think you could reuse the existing ExitableDirectoryReader.ExitableFilterAtomicReader class, instead of making a new class here, maybe? And that class already implements timeouts for points, postings, doc values, etc.

mikemccand · 2024-02-05T16:27:39Z

lucene/core/src/java/org/apache/lucene/index/ExitableIndexReader.java

+
+    private final CacheHelper readerCacheHelper;
+
+    public ExitableIndexReaderWrapper(DirectoryReader in, QueryTimeout queryTimeout)


And maybe reuse the existing ExitableDirectoryReader.ExitableSubReaderWrapper instead of making a new one here?

mikemccand · 2024-02-05T16:31:16Z

lucene/facet/src/test/org/apache/lucene/facet/TestStringValueFacetCounts.java

@@ -77,7 +78,11 @@ public void testBasicSingleValued() throws Exception {
        new StringDocValuesReaderState(searcher.getIndexReader(), "field");
    checkTopNFacetResult(expectedCounts, expectedTotalDocCount, searcher, state, 10, 2, 1, 0);

-    IOUtils.close(searcher.getIndexReader(), dir);
+    IndexReader r = searcher.getIndexReader();
+    if (r instanceof ExitableIndexReader) {


Hmm why was this needed? In general users of ExitableIndexReader should not have to special case which exact instance of IndexReader they have. In this case, maybe ExitableIndexReader is not implementing close correctly?

mikemccand · 2024-02-05T16:38:30Z

lucene/core/src/java/org/apache/lucene/index/ExitableIndexReader.java

+  @Override
+  public IndexReaderContext getContext() {
+
+    return indexReader.getContext();


I think this isn't quite right -- we can't just return the underlying IndexReaderContext. I think instead you need to get this underlying context, and then make a new CompositeReaderContext that wraps each of the leaf readers wrapped in ExitableFilterAtomicReader, and children wrapped with this new class (ExitableIndexReader)?

github-actions · 2024-02-20T00:17:22Z

This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the [email protected] list. Thank you for your contribution!

github-actions · 2025-05-14T12:12:19Z

This PR does not have an entry in lucene/CHANGES.txt. Consider adding one. If the PR doesn't need a changelog entry, then add the skip-changelog-check label to it and you will stop receiving this reminder on future updates to the PR.

github-actions · 2025-05-15T14:50:03Z

This PR does not have an entry in lucene/CHANGES.txt. Consider adding one. If the PR doesn't need a changelog entry, then add the skip-changelog-check label to it and you will stop receiving this reminder on future updates to the PR.

mikemccand reviewed Jun 9, 2023

View reviewed changes

Deepika0510 commented Jul 14, 2023

View reviewed changes

Deepika0510 commented Dec 4, 2023

View reviewed changes

mikemccand reviewed Dec 5, 2023

View reviewed changes

Deepika0510 requested a review from jpountz December 5, 2023 11:58

Deepika0510 commented Dec 12, 2023

View reviewed changes

mikemccand reviewed Jan 2, 2024

View reviewed changes

github-actions bot added the Stale label Jan 17, 2024

Deepika0510 commented Jan 22, 2024

View reviewed changes

github-actions bot removed the Stale label Jan 23, 2024

mikemccand reviewed Feb 5, 2024

View reviewed changes

github-actions bot added the Stale label Feb 20, 2024

github-actions bot added module:core/index module:core/search module:facet labels May 14, 2025

github-actions bot removed the Stale label May 15, 2025

Deepika0510 closed this May 15, 2025

Deepika0510 force-pushed the main branch from 17d8640 to 18b70d2 Compare May 15, 2025 14:49


		private final CacheHelper readerCacheHelper;

		public ExitableIndexReaderWrapper(DirectoryReader in, QueryTimeout queryTimeout)

LUCENE-10641: IndexSearcher#setTimeout should also abort query rewrites, point ranges and vector searches #12345

LUCENE-10641: IndexSearcher#setTimeout should also abort query rewrites, point ranges and vector searches #12345

Conversation

Deepika0510 commented Jun 1, 2023

Description

Solution

Tests

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Deepika0510 left a comment

Choose a reason for hiding this comment

jpountz commented Jul 20, 2023

Deepika0510 commented Sep 17, 2023

mikemccand commented Oct 19, 2023

Deepika0510 commented Oct 29, 2023

mikemccand commented Oct 29, 2023

Deepika0510 commented Nov 2, 2023

Deepika0510 commented Nov 2, 2023

mikemccand commented Nov 2, 2023

Deepika0510 left a comment

Choose a reason for hiding this comment

Deepika0510 left a comment

Choose a reason for hiding this comment

mikemccand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Deepika0510 left a comment

Choose a reason for hiding this comment

mikemccand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 17, 2024

Deepika0510 left a comment

Choose a reason for hiding this comment

mikemccand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 20, 2024

github-actions bot commented May 14, 2025

github-actions bot commented May 15, 2025