-
Notifications
You must be signed in to change notification settings - Fork 25.7k
Skip iterating DISI when reading metric values #133365
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
Conversation
server/src/main/java/org/elasticsearch/index/codec/tsdb/es819/ES819TSDBDocValuesProducer.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/index/codec/tsdb/es819/ES819TSDBDocValuesProducer.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/index/codec/tsdb/es819/ES819TSDBDocValuesProducer.java
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/index/mapper/BlockDocValuesReader.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/index/mapper/BlockDocValuesReader.java
Outdated
Show resolved
Hide resolved
kkrik-es
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, I'm glad my draft has some overlap :)
martijnvg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good. I left minor comments and we need a test for tryReadDoubles() in ES819TSDBDocValuesFormatTests
server/src/main/java/org/elasticsearch/index/codec/tsdb/es819/ES819TSDBDocValuesProducer.java
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/index/codec/tsdb/es819/ES819TSDBDocValuesProducer.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/index/mapper/BlockDocValuesReader.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/index/codec/tsdb/es819/ES819TSDBDocValuesProducer.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/index/mapper/BlockDocValuesReader.java
Outdated
Show resolved
Hide resolved
23874ce to
46085ab
Compare
46085ab to
6bb2a4a
Compare
43831ab to
9fd96a3
Compare
|
@martijnvg @kkrik-es I've updated this with Kostas's PR. I think it looks better. Can you take a look? Thanks! |
|
Pinging @elastic/es-storage-engine (Team:StorageEngine) |
|
Hi @dnhatn, I've created a changelog YAML for you. |
martijnvg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks Nhat!
server/src/main/java/org/elasticsearch/index/mapper/BlockLoader.java
Outdated
Show resolved
Hide resolved
kkrik-es
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice and clean!
|
@martijnvg @kkrik-es Thank you! |
Avoids iterating the DISI when reading a sparse metric field if its nulls were already filtered by a WHERE clause (e.g., metrics != NULL). Iterating the DISI on a sparse field can be expensive. From my local benchmark, the query time of
decreased from 35ms to 27ms (20%).
Before:

After: