-
Notifications
You must be signed in to change notification settings - Fork 25.6k
[ES|QL] ToAggregateMetricDouble function #124595
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
Changes from 10 commits
2f64c49
7a49ca5
efab117
58104fb
61c3fef
a65a1d3
227de7d
2ccfd46
5c4d965
0594990
d2531de
ce814cf
804e980
ec2862b
4749325
b5afb7f
9b47fc3
a2613f3
9e4f3b4
e469df8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,9 +7,17 @@ | |
|
|
||
| package org.elasticsearch.compute.data; | ||
|
|
||
| import org.elasticsearch.TransportVersion; | ||
| import org.elasticsearch.TransportVersions; | ||
| import org.elasticsearch.common.io.stream.GenericNamedWriteable; | ||
| import org.elasticsearch.common.io.stream.NamedWriteableRegistry; | ||
| import org.elasticsearch.common.io.stream.StreamInput; | ||
| import org.elasticsearch.common.io.stream.StreamOutput; | ||
| import org.elasticsearch.core.Releasables; | ||
| import org.elasticsearch.index.mapper.BlockLoader; | ||
|
|
||
| import java.io.IOException; | ||
|
|
||
| public class AggregateMetricDoubleBlockBuilder extends AbstractBlockBuilder implements BlockLoader.AggregateMetricDoubleBuilder { | ||
|
|
||
| private DoubleBlockBuilder minBuilder; | ||
|
|
@@ -161,11 +169,107 @@ public String getLabel() { | |
| } | ||
| } | ||
|
|
||
| public record AggregateMetricDoubleLiteral(Double min, Double max, Double sum, Integer count) { | ||
| public record AggregateMetricDoubleLiteral(Double min, Double max, Double sum, Integer count) implements GenericNamedWriteable { | ||
| public AggregateMetricDoubleLiteral { | ||
| min = min.isNaN() ? null : min; | ||
| max = max.isNaN() ? null : max; | ||
| sum = sum.isNaN() ? null : sum; | ||
| } | ||
|
|
||
| public static final NamedWriteableRegistry.Entry ENTRY = new NamedWriteableRegistry.Entry( | ||
| GenericNamedWriteable.class, | ||
| "AggregateMetricDoubleLiteral", | ||
| AggregateMetricDoubleLiteral::new | ||
| ); | ||
|
|
||
| @Override | ||
| public String getWriteableName() { | ||
| return "AggregateMetricDoubleLiteral"; | ||
| } | ||
|
|
||
| public AggregateMetricDoubleLiteral(StreamInput input) throws IOException { | ||
| this(input.readOptionalDouble(), input.readOptionalDouble(), input.readOptionalDouble(), input.readOptionalInt()); | ||
| } | ||
|
|
||
| @Override | ||
| public void writeTo(StreamOutput out) throws IOException { | ||
| out.writeOptionalDouble(min); | ||
| out.writeOptionalDouble(max); | ||
| out.writeOptionalDouble(sum); | ||
| out.writeOptionalInt(count); | ||
| } | ||
|
|
||
| @Override | ||
| public TransportVersion getMinimalSupportedVersion() { | ||
| return TransportVersions.ESQL_AGGREGATE_METRIC_DOUBLE_LITERAL; | ||
| } | ||
|
|
||
| } | ||
|
|
||
| public static class AggregateMetricDoubleVectorBuilder extends AbstractBlockBuilder { | ||
| private final DoubleBlockBuilder valuesBuilder; | ||
|
|
||
| public AggregateMetricDoubleVectorBuilder(int estimatedSize, BlockFactory blockFactory) { | ||
| super(blockFactory); | ||
| valuesBuilder = new DoubleBlockBuilder(estimatedSize, blockFactory); | ||
limotova marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| @Override | ||
| protected int valuesLength() { | ||
|
||
| throw new UnsupportedOperationException("Not available on aggregate_metric_double"); | ||
| } | ||
|
|
||
| @Override | ||
| protected void growValuesArray(int newSize) { | ||
| throw new UnsupportedOperationException("Not available on aggregate_metric_double"); | ||
| } | ||
|
|
||
| @Override | ||
| protected int elementSize() { | ||
| throw new UnsupportedOperationException("Not available on aggregate_metric_double"); | ||
| } | ||
|
|
||
| @Override | ||
| public Block.Builder copyFrom(Block block, int beginInclusive, int endExclusive) { | ||
| // TODO | ||
| return null; | ||
| } | ||
|
|
||
| @Override | ||
| public Block.Builder mvOrdering(Block.MvOrdering mvOrdering) { | ||
| // TODO | ||
| return null; | ||
| } | ||
|
|
||
| public void appendValue(double value) { | ||
| valuesBuilder.appendDouble(value); | ||
| } | ||
|
|
||
| @Override | ||
| public Block build() { | ||
| Block[] blocks = new Block[4]; | ||
| Block block = null; | ||
| IntBlock countVector = null; | ||
| boolean success = false; | ||
| try { | ||
| finish(); | ||
| block = valuesBuilder.build(); | ||
| countVector = blockFactory.newConstantIntBlockWith(1, block.getPositionCount()); | ||
| blocks[Metric.MIN.getIndex()] = block; | ||
| blocks[Metric.MAX.getIndex()] = block; | ||
| block.incRef(); | ||
| blocks[Metric.SUM.getIndex()] = block; | ||
| block.incRef(); | ||
| blocks[Metric.COUNT.getIndex()] = countVector; | ||
| CompositeBlock compositeBlock = new CompositeBlock(blocks); | ||
| success = true; | ||
| return compositeBlock; | ||
| } finally { | ||
| if (success == false) { | ||
| Releasables.closeExpectNoException(blocks); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -285,7 +285,19 @@ private static Object valueAtOffset(Block block, int offset) { | |
| DocVector v = ((DocBlock) block).asVector(); | ||
| yield new Doc(v.shards().getInt(offset), v.segments().getInt(offset), v.docs().getInt(offset)); | ||
| } | ||
| case COMPOSITE -> throw new IllegalArgumentException("can't read values from composite blocks"); | ||
| case COMPOSITE -> { | ||
| CompositeBlock compositeBlock = (CompositeBlock) block; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can in a follow up the usage of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure I understand? There's no block creation here, just fetching the value from the block that already exists, so no building is involved There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, this is comment doesn't make sense here. I think I was more wondering about whether in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could if we wanted, but I'm not sure it's all that important. The entire compute engine is pretty ok with runtime types and treating blocks in specific ways based on the input. We don't have a special type for utf-8 text - just bytes - so I'm not sure we'd need a special type for these blocks. If we want it we can have it for sure, but I don't think it's all that important. |
||
| var minBlock = (DoubleBlock) compositeBlock.getBlock(AggregateMetricDoubleBlockBuilder.Metric.MIN.getIndex()); | ||
| var maxBlock = (DoubleBlock) compositeBlock.getBlock(AggregateMetricDoubleBlockBuilder.Metric.MAX.getIndex()); | ||
| var sumBlock = (DoubleBlock) compositeBlock.getBlock(AggregateMetricDoubleBlockBuilder.Metric.SUM.getIndex()); | ||
| var countBlock = (IntBlock) compositeBlock.getBlock(AggregateMetricDoubleBlockBuilder.Metric.COUNT.getIndex()); | ||
| yield new AggregateMetricDoubleLiteral( | ||
| minBlock.getDouble(offset), | ||
| maxBlock.getDouble(offset), | ||
| sumBlock.getDouble(offset), | ||
| countBlock.getInt(offset) | ||
| ); | ||
| } | ||
| case UNKNOWN -> throw new IllegalArgumentException("can't read values from [" + block + "]"); | ||
| }; | ||
| } | ||
|
|
||
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 we not extend from BlockBuilder but just a Releasable here. Also should we move this to ToAggregateMetricDouble class 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.
Yeah I think keeping it in ToAggregateMetricDouble makes sense, I moved it there, but that class is getting quite heavy, I wonder if the factories should be moved into a separate file?