Skip to content

Commit 4be381f

Browse files
authored
ESQL: Avoid null in intermediate exponential_histogram merge state (#138511)
1 parent 979d92e commit 4be381f

File tree

10 files changed

+142
-37
lines changed

10 files changed

+142
-37
lines changed

x-pack/plugin/esql/compute/src/main/generated/org/elasticsearch/compute/aggregation/HistogramMergeExponentialHistogramAggregatorFunction.java

Lines changed: 10 additions & 2 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

x-pack/plugin/esql/compute/src/main/generated/org/elasticsearch/compute/aggregation/HistogramMergeExponentialHistogramGroupingAggregatorFunction.java

Lines changed: 25 additions & 4 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/aggregation/ExponentialHistogramStates.java

Lines changed: 29 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -64,8 +64,16 @@ public void add(ExponentialHistogram histogram, boolean allowUpscale) {
6464

6565
@Override
6666
public void toIntermediate(Block[] blocks, int offset, DriverContext driverContext) {
67-
assert blocks.length >= offset + 1;
68-
blocks[offset] = evaluateFinal(driverContext);
67+
assert blocks.length >= offset + 2;
68+
BlockFactory blockFactory = driverContext.blockFactory();
69+
// in case of error, the blocks are closed by the caller
70+
if (merger == null) {
71+
blocks[offset] = blockFactory.newConstantExponentialHistogramBlock(ExponentialHistogram.empty(), 1);
72+
blocks[offset + 1] = blockFactory.newConstantBooleanBlockWith(false, 1);
73+
} else {
74+
blocks[offset] = blockFactory.newConstantExponentialHistogramBlock(merger.get(), 1);
75+
blocks[offset + 1] = blockFactory.newConstantBooleanBlockWith(true, 1);
76+
}
6977
}
7078

7179
@Override
@@ -127,8 +135,25 @@ private void ensureCapacity(int groupId) {
127135

128136
@Override
129137
public void toIntermediate(Block[] blocks, int offset, IntVector selected, DriverContext driverContext) {
130-
assert blocks.length >= offset + 1 : "blocks=" + blocks.length + ",offset=" + offset;
131-
blocks[offset] = evaluateFinal(selected, driverContext);
138+
assert blocks.length >= offset + 2 : "blocks=" + blocks.length + ",offset=" + offset;
139+
try (
140+
var histoBuilder = driverContext.blockFactory().newExponentialHistogramBlockBuilder(selected.getPositionCount());
141+
var seenBuilder = driverContext.blockFactory().newBooleanBlockBuilder(selected.getPositionCount());
142+
) {
143+
for (int i = 0; i < selected.getPositionCount(); i++) {
144+
int groupId = selected.getInt(i);
145+
ExponentialHistogramMerger state = getOrNull(groupId);
146+
if (state != null) {
147+
seenBuilder.appendBoolean(true);
148+
histoBuilder.append(state.get());
149+
} else {
150+
seenBuilder.appendBoolean(false);
151+
histoBuilder.append(ExponentialHistogram.empty());
152+
}
153+
}
154+
blocks[offset] = histoBuilder.build();
155+
blocks[offset + 1] = seenBuilder.build();
156+
}
132157
}
133158

134159
public Block evaluateFinal(IntVector selected, DriverContext driverContext) {

x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/aggregation/HistogramMergeExponentialHistogramAggregator.java

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@
1616
import org.elasticsearch.compute.operator.DriverContext;
1717
import org.elasticsearch.exponentialhistogram.ExponentialHistogram;
1818

19-
@Aggregator({ @IntermediateState(name = "value", type = "EXPONENTIAL_HISTOGRAM"), })
19+
@Aggregator({ @IntermediateState(name = "value", type = "EXPONENTIAL_HISTOGRAM"), @IntermediateState(name = "seen", type = "BOOLEAN") })
2020
@GroupingAggregator
2121
public class HistogramMergeExponentialHistogramAggregator {
2222

@@ -28,8 +28,10 @@ public static void combine(ExponentialHistogramStates.SingleState state, Exponen
2828
state.add(value, true);
2929
}
3030

31-
public static void combineIntermediate(ExponentialHistogramStates.SingleState state, ExponentialHistogram value) {
32-
state.add(value, false);
31+
public static void combineIntermediate(ExponentialHistogramStates.SingleState state, ExponentialHistogram value, boolean seen) {
32+
if (seen) {
33+
state.add(value, false);
34+
}
3335
}
3436

3537
public static Block evaluateFinal(ExponentialHistogramStates.SingleState state, DriverContext driverContext) {
@@ -44,8 +46,15 @@ public static void combine(ExponentialHistogramStates.GroupingState current, int
4446
current.add(groupId, value, true);
4547
}
4648

47-
public static void combineIntermediate(ExponentialHistogramStates.GroupingState state, int groupId, ExponentialHistogram value) {
48-
state.add(groupId, value, false);
49+
public static void combineIntermediate(
50+
ExponentialHistogramStates.GroupingState state,
51+
int groupId,
52+
ExponentialHistogram value,
53+
boolean seen
54+
) {
55+
if (seen) {
56+
state.add(groupId, value, false);
57+
}
4958
}
5059

5160
public static Block evaluateFinal(

x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/data/ExponentialHistogramArrayBlock.java

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,7 @@ private List<Block> getSubBlocks() {
108108

109109
@Override
110110
public ExponentialHistogram getExponentialHistogram(int valueIndex, ExponentialHistogramScratch scratch) {
111+
assert isNull(valueIndex) == false : "tried to get histogram at null position " + valueIndex;
111112
BytesRef bytes = encodedHistograms.getBytesRef(encodedHistograms.getFirstValueIndex(valueIndex), scratch.bytesRefScratch);
112113
double zeroThreshold = zeroThresholds.getDouble(zeroThresholds.getFirstValueIndex(valueIndex));
113114
double valueCount = valueCounts.getDouble(valueCounts.getFirstValueIndex(valueIndex));

x-pack/plugin/esql/qa/server/multi-clusters/src/javaRestTest/java/org/elasticsearch/xpack/esql/ccq/MultiClusterSpecIT.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -430,11 +430,11 @@ protected boolean supportsExponentialHistograms() {
430430
try {
431431
return RestEsqlTestCase.hasCapabilities(
432432
client(),
433-
List.of(EsqlCapabilities.Cap.EXPONENTIAL_HISTOGRAM_PRE_TECH_PREVIEW_V3.capabilityName())
433+
List.of(EsqlCapabilities.Cap.EXPONENTIAL_HISTOGRAM_PRE_TECH_PREVIEW_V4.capabilityName())
434434
)
435435
&& RestEsqlTestCase.hasCapabilities(
436436
remoteClusterClient(),
437-
List.of(EsqlCapabilities.Cap.EXPONENTIAL_HISTOGRAM_PRE_TECH_PREVIEW_V3.capabilityName())
437+
List.of(EsqlCapabilities.Cap.EXPONENTIAL_HISTOGRAM_PRE_TECH_PREVIEW_V4.capabilityName())
438438
);
439439
} catch (IOException e) {
440440
throw new RuntimeException(e);

x-pack/plugin/esql/qa/server/single-node/src/javaRestTest/java/org/elasticsearch/xpack/esql/qa/single_node/EsqlSpecIT.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ protected boolean supportsSourceFieldMapping() {
5858
protected boolean supportsExponentialHistograms() {
5959
return RestEsqlTestCase.hasCapabilities(
6060
client(),
61-
List.of(EsqlCapabilities.Cap.EXPONENTIAL_HISTOGRAM_PRE_TECH_PREVIEW_V3.capabilityName())
61+
List.of(EsqlCapabilities.Cap.EXPONENTIAL_HISTOGRAM_PRE_TECH_PREVIEW_V4.capabilityName())
6262
);
6363
}
6464

x-pack/plugin/esql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/EsqlSpecTestCase.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -289,7 +289,7 @@ protected boolean supportsSourceFieldMapping() throws IOException {
289289
protected boolean supportsExponentialHistograms() {
290290
return RestEsqlTestCase.hasCapabilities(
291291
client(),
292-
List.of(EsqlCapabilities.Cap.EXPONENTIAL_HISTOGRAM_PRE_TECH_PREVIEW_V3.capabilityName())
292+
List.of(EsqlCapabilities.Cap.EXPONENTIAL_HISTOGRAM_PRE_TECH_PREVIEW_V4.capabilityName())
293293
);
294294
}
295295

0 commit comments

Comments
 (0)