-
Notifications
You must be signed in to change notification settings - Fork 25.6k
ES|QL block type for exponential histograms #133393
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
1a4c296 to
65b3d0e
Compare
bc159b1 to
b991e5b
Compare
ℹ️ Important: Docs version tagging👋 Thanks for updating the docs! Just a friendly reminder that our docs are now cumulative. This means all 9.x versions are documented on the same page and published off of the main branch, instead of creating separate pages for each minor version. We use applies_to tags to mark version-specific features and changes. Expand for a quick overviewWhen to use applies_to tags:✅ At the page level to indicate which products/deployments the content applies to (mandatory) What NOT to do:❌ Don't remove or replace information that applies to an older version 🤔 Need help?
|
07c9955 to
8a0a14f
Compare
x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/data/BlockUtils.java
Show resolved
Hide resolved
...l/compute/src/main/java/org/elasticsearch/compute/data/ExponentialHistogramBlockBuilder.java
Show resolved
Hide resolved
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/AggregateMapper.java
Outdated
Show resolved
Hide resolved
|
Pinging @elastic/es-storage-engine (Team:StorageEngine) |
x-pack/plugin/esql/compute/test/src/main/java/org/elasticsearch/compute/test/RandomBlock.java
Outdated
Show resolved
Hide resolved
.../testFixtures/src/main/java/org/elasticsearch/xpack/esql/JsonBackedExponentialHistogram.java
Outdated
Show resolved
Hide resolved
...l/compute/src/main/java/org/elasticsearch/compute/data/ExponentialHistogramBlockBuilder.java
Outdated
Show resolved
Hide resolved
...sql/compute/src/main/java/org/elasticsearch/compute/data/ExponentialHistogramArrayBlock.java
Outdated
Show resolved
Hide resolved
...sql/compute/src/main/java/org/elasticsearch/compute/data/ExponentialHistogramArrayBlock.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.
I'll leave it to the esql experts to approve.
…h/compute/test/RandomBlock.java Co-authored-by: Kostas Krikellas <[email protected]>
# Conflicts: # benchmarks/src/main/java/org/elasticsearch/benchmark/exponentialhistogram/ExponentialHistogramMergeBench.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java # x-pack/plugin/mapper-exponential-histogram/src/main/java/org/elasticsearch/xpack/exponentialhistogram/ExponentialHistogramFieldMapper.java
14d8614 to
f491a55
Compare
f491a55 to
621b472
Compare
dnhatn
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.
Thank you for all the iterations. There are several things we need to harden for this block, but since it's guarded with the feature flag, let's get it in so you can start integrating. We can improve or fix these before removing the feature flag.
|
|
||
| @Override | ||
| public int getValueCount(int position) { | ||
| return isNull(position) ? 0 : 1; |
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.
Can we delegate to encodedHistograms instead?
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.
Based on the mapping of positions -> valueIndices -> subBlock-positions (see #133393 (comment)), imo the current implementation is clearer, as it delegates the details of this mapping to the isNull method?
Why would you want to delegate to delegate to encodedHistograms here?
|
|
||
| @Override | ||
| public int getFirstValueIndex(int position) { | ||
| return position; |
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.
Can we delegate to encodedHistograms instead? I think, eventually, we should throw UnsupportedOperationException for all these operations, but it's okay for now.
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.
I think delegating to encodedHistograms.getFirstValueIndex() would be wrong here: E.g. in theory encodedHistograms could be a BytesRefOrdinalBlock returning the same value index for different positions. However, we need to be able to load the other components (e.g. min, max) given just a valueIndex, which then becomes impossible.
The current implementation uses the following model/semantics for positions and value indices:
- We use the sub-blocks as plain storage without multi-value support, but nullability. E.g. they are effectively one-dimensional arrays to us, where each position either contains a single or no value.
- We don't make any assumptions about the value-indices of the sub-blocks. We always access them via
subBlock.getValue(subBlock.getFirstValueIndex(subBlockPosition)). This means we adhere to theBlockinterface guarantees. - Given a
valueIndexwithin aExponentialHistogramBlockswe need to be able to load all components (= the values from the subblocks) for that given valueIndex. We need to do a "array lookup" at the given index into the subblocks. We do by usingvalueIndexin the context ofExponentialHistogramBlockas positions into the subblocks (= basically one-dimensional arrays). - For now,
ExponentialHistogramBlocksdon't support multi-values. Therefore we use the simple mapping of thevalueIndexof aExponentialHistogramBlocksbeing exactly theposition. We omit the firstValueIndexes indirection which otherwise would be needed. We can later add multi-value support easily by adding such afirstValueIndexesarray to theExponentialHistogramBlocks. Note that this won't change the fact that the sub-blocks are non-multivalued, we will still continue to use them as one-dimensional arrays. We will just usefirstValueIndexesto change the mapping fromExponentialHistogramBlock-positions tovalueIndexes. AvalueIndexwill still correspond to a position in a sub-block.
...ck/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/LocalExecutionPlanner.java
Outdated
Show resolved
Hide resolved
...sql/compute/src/main/java/org/elasticsearch/compute/data/ExponentialHistogramArrayBlock.java
Show resolved
Hide resolved
nik9000
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.
Sorry to block you. Happy hunting on the next bits.
Adds a barebones ES|QL and block type for exponential histograms.
To keep this PR as small as possible I've reduced the type to just storing thescaleof a histogram and nothing else.Everything else will be added in follow-up PRs.
Update: To better get the bigger picture, the block has been now fully fleshed out with all sub-blocks in this PR.
In this PR I've marked the shortcuts / disabled tests that definitely need work before we eventually can put this into tech-preview with
TODO(b/133393).Note that I've also excluded the type from some tests (e.g. TopN) without a
TODO(b/133393): These are tests which cover functionality which I think won't be needed, at least for a tech-preview. Please carefully review them and let me know if those cover functionality which should work, so that I can add theTODO(b/133393)there aswell.Initally this PR also included some CSV-tests. But I decided to remove them for now, as they would require implementing a blockloader, increasing the size of the PR unnecessarily. I'll add them back together with the blockloader in a followup PR.