Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] Performance of numeric exact-match queries can be improved #11097

Closed
msfroh opened this issue Nov 5, 2023 · 4 comments · Fixed by #11209
Closed

[BUG] Performance of numeric exact-match queries can be improved #11097

msfroh opened this issue Nov 5, 2023 · 4 comments · Fixed by #11209
Assignees
Labels
bug Something isn't working Search:Performance v2.12.0 Issues and PRs related to version 2.12.0 v3.0.0 Issues and PRs related to version 3.0.0

Comments

@msfroh
Copy link
Collaborator

msfroh commented Nov 5, 2023

Describe the bug
I was recently looking at a query that included a clause like

"term" : {
  "test": 0
}

In this case, test was a long field, indexed as a point. Profiling the query (with "profiler":true) to see why it was slow, I found that the query was parsing to PointRangeQuery(test:[0 TO 0]), without wrapping it in an IndexOrDocValuesQuery. In this case, the PointRangeQuery was taking up a lot of the query time.

I rewrote the query as:

"range": {
  "test": {
    "lte":0,
    "gte": 0
  }
}

That one did do the IndexOrDocValuesQuery wrapping, and it was much faster (latency dropped from 1.2 seconds to 350ms).

Expected behavior
I would expect the the query mapping by NumberFieldMapper.NumberType would do the IndexOrDocValuesQuery wrapping automatically.

The implementation of termQuery should probably delegate to rangeQuery in most cases, just applying the range over the target value. For the various integer types, we can keep the optimization that returns a MatchNoDocsQuery if the value is not an integer. (As a bonus, while we're there, we could return a MatchNoDocsQuery for values outside permitted range of a field, like a negative value for unsigned_long or a number >= 128 for byte.)

At the same time, the implementation of termsQuery for number fields should just be a BooleanQuery over termQuery when the number of terms is small. (For keyword fields, "small" means 16 or fewer clauses.)

@msfroh msfroh added bug Something isn't working untriaged Search:Performance labels Nov 5, 2023
@harshavamsi harshavamsi self-assigned this Nov 6, 2023
@harshavamsi harshavamsi added v2.12.0 Issues and PRs related to version 2.12.0 and removed untriaged labels Nov 6, 2023
@harshavamsi
Copy link
Contributor

Some initial benchmarks:

|                                        50th percentile latency |              simple_term |     4311.71 |     ms |
|                                        90th percentile latency |              simple_term |     4864.34 |     ms |
|                                        99th percentile latency |              simple_term |     4996.19 |     ms |
|                                       100th percentile latency |              simple_term |     5009.17 |     ms |
|                                   50th percentile service time |              simple_term |      32.621 |     ms |
|                                   90th percentile service time |              simple_term |     40.3655 |     ms |
|                                   99th percentile service time |              simple_term |     45.6012 |     ms |
|                                  100th percentile service time |              simple_term |     45.9752 |     ms |
|                                                     error rate |              simple_term |           0 |      % |

Running the http_logs workload with 1 shard and 3 segments per shard.

Query is a simple term query on status:

{
"query":
{
"term":
{
"status": 200
}
}
}

@msfroh
Copy link
Collaborator Author

msfroh commented Nov 14, 2023

Query is a simple term query on status:

This isn't going to be enough. That should behave the same as the index query alone.

The key piece of the IndexOrDocValuesQuery optimization is that it lets a "lead iterator" do the driving, while the IoDVQ "lazily" matches on doc values (when appropriate). In this case, you don't have a lead iterator.

Imagine you have a simple "A AND B" BooleanQuery. Lucene obtains iterators for both A and B (via the createWeight() method on the Query then scorerSupplier() on the Weight). If you imagine these are both simple TermQuery instances, then iteration starts with the one with lower doc frequency (since its iterator is going to be more sparse), calling nextDoc(), then calls advance() on the other one to jump to the first value >= the first iterator's current doc. They leap-frog until they're both pointing to the same doc ID, at which point you have a match. Then it calls nextDoc on the more sparse iterator, and things continue

Now, imagine instead that B is an IndexOrDocValuesQuery. The key point is that the IoDVQ's Weight's scorerSupplier() is called with the cost of A (i.e. the cost of A is the leadCost). If the index query's cost is sufficiently close to the cost of A (i.e. within a factor of 8), then we still run the index query. If the index query cost is higher than that (i.e. it matches a lot of stuff), then we return the doc value query's iterator. The DV query's iterator has an "approximate" advance method that will always jump to whatever you ask it to jump to (whether the value matches or not). It's wrapped in a TwoPhaseIterator, which exposes a matches() method that loads the doc value for the target doc and checks if it matches the constraints of the query. So, A's iterator does all of the stepping through doc IDs, B's iterator always just says, "Sure, whatever", and the matches() call loads the DV and confirms a match.

tl;dr: We need a lead iterator for IndexOrDocValuesQuery to run in doc values mode.

@hdhalter
Copy link

Hi @msfroh , will documentation for this feature be required for 2.12? If so, can you please create the doc issue, let me know who will be creating the initial doc PR, and add this dev issue to the unified tracker project? Thanks!

@msfroh
Copy link
Collaborator Author

msfroh commented Dec 12, 2023

Hi @msfroh , will documentation for this feature be required for 2.12?

No documentation changes will be required. It's just an under-the-hood optimization.

@reta reta added the v3.0.0 Issues and PRs related to version 3.0.0 label Jan 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Search:Performance v2.12.0 Issues and PRs related to version 2.12.0 v3.0.0 Issues and PRs related to version 3.0.0
Projects
Status: 2.12.0 (Launched)
Status: Done
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants