-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Some optimizations for constant blocks #132456
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
Here is the benchmark with this change: Before:
After:
|
try { | ||
bytes = blockFactory.newConstantBytesRefVector(v, 1); | ||
ordinals = blockFactory.newConstantIntVector(0, ords.length); | ||
final var result = new OrdinalBytesRefBlock(ordinals.asBlock(), bytes); |
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.
Here, we choose to return an ordinal constant block instead of a direct constant block to ensure that ordinal optimizations are applied if the constant optimization is not available.
Hi @dnhatn, I've created a changelog YAML for you. |
This speedup is amazing! |
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 two comments.
...ql/compute/src/main/java/org/elasticsearch/compute/lucene/read/SingletonOrdinalsBuilder.java
Outdated
Show resolved
Hide resolved
...mpute/src/test/java/org/elasticsearch/compute/lucene/read/SingletonOrdinalsBuilderTests.java
Show resolved
Hide resolved
Pinging @elastic/es-analytical-engine (Team:Analytics) |
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 👍
try { | ||
bytes = blockFactory.newConstantBytesRefVector(v, 1); | ||
ordinals = blockFactory.newConstantIntVector(0, ords.length); | ||
final var result = new OrdinalBytesRefBlock(ordinals.asBlock(), bytes); |
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.
Should this be a ConstantBytesRefBlock
?
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.
Yeah, it should be but here we need to return an ordinal constant block instead of a direct constant block to ensure that ordinal optimizations are applied if the constant optimization is not available: #132456 (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.
Got it. It's worth adding a comment about why that's not a ConstantBlock I think.
++, I added a comment in 276759a. |
@martijnvg @nik9000 Thank you! |
This change introduces several optimizations for constant blocks:
When reading keyword fields that are index-sorted and the query range is large enough, we can return a constant block of BytesRef values to enable downstream optimizations.
Enable shortcuts for BytesRefBlockHash and BytesRefLongBlockHash when handling constant blocks. These optimizations are quick wins for time-series aggregations.
Enable shortcuts for CountAggregator and ValuesAggregator for constant blocks.