Skip to content

Commit f9107e3

Browse files
nik9000nik9000
authored
ESQL: Disable optimizations with bad null handling (#99434)
* ESQL: Disable optimizations with bad null handling We have optimizations that kick in when aggregating on the following pairs of field types: * `long`, `long` * `keyword`, `long` * `long`, `keyword` These optimizations don't have proper support for `null` valued fields but will grow that after #98749. In the mean time this disables them in a way that prevents them from bit-rotting. * Update docs/changelog/99434.yaml
1 parent 6ab6b23 commit f9107e3

File tree

10 files changed

+79
-41
lines changed

10 files changed

+79
-41
lines changed

‎benchmarks/src/main/java/org/elasticsearch/benchmark/compute/operator/AggregatorBenchmark.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -141,7 +141,7 @@ private static Operator operator(String grouping, String op, String dataType) {
141141
};
142142
return new HashAggregationOperator(
143143
List.of(supplier(op, dataType, groups.size()).groupingAggregatorFactory(AggregatorMode.SINGLE)),
144-
() -> BlockHash.build(groups, BIG_ARRAYS, 16 * 1024),
144+
() -> BlockHash.build(groups, BIG_ARRAYS, 16 * 1024, false),
145145
new DriverContext()
146146
);
147147
}

‎docs/changelog/99434.yaml‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
pr: 99434
2+
summary: "ESQL: Disable optimizations with bad null handling"
3+
area: ES|QL
4+
type: bug
5+
issues: []

‎x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/aggregation/blockhash/BlockHash.java‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -62,12 +62,21 @@ public abstract sealed class BlockHash implements Releasable, SeenGroupIds //
6262
* Creates a specialized hash table that maps one or more {@link Block}s to ids.
6363
* @param emitBatchSize maximum batch size to be emitted when handling combinatorial
6464
* explosion of groups caused by multivalued fields
65+
* @param allowBrokenOptimizations true ot allow optimizations with bad null handling. We will fix their
66+
* null handling and remove this flag, but we need to disable these in
67+
* production until we can. And this lets us continue to compile and
68+
* test them.
6569
*/
66-
public static BlockHash build(List<HashAggregationOperator.GroupSpec> groups, BigArrays bigArrays, int emitBatchSize) {
70+
public static BlockHash build(
71+
List<HashAggregationOperator.GroupSpec> groups,
72+
BigArrays bigArrays,
73+
int emitBatchSize,
74+
boolean allowBrokenOptimizations
75+
) {
6776
if (groups.size() == 1) {
6877
return newForElementType(groups.get(0).channel(), groups.get(0).elementType(), bigArrays);
6978
}
70-
if (groups.size() == 2) {
79+
if (allowBrokenOptimizations && groups.size() == 2) {
7180
var g1 = groups.get(0);
7281
var g2 = groups.get(1);
7382
if (g1.elementType() == ElementType.LONG && g2.elementType() == ElementType.LONG) {

‎x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/operator/HashAggregationOperator.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ public record HashAggregationOperatorFactory(
3939
) implements OperatorFactory {
4040
@Override
4141
public Operator get(DriverContext driverContext) {
42-
return new HashAggregationOperator(aggregators, () -> BlockHash.build(groups, bigArrays, maxPageSize), driverContext);
42+
return new HashAggregationOperator(aggregators, () -> BlockHash.build(groups, bigArrays, maxPageSize, false), driverContext);
4343
}
4444

4545
@Override

‎x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/operator/OrdinalsGroupingOperator.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -422,7 +422,7 @@ private static class ValuesAggregator implements Releasable {
422422
this.extractor = new ValuesSourceReaderOperator(sources, docChannel, groupingField);
423423
this.aggregator = new HashAggregationOperator(
424424
aggregatorFactories,
425-
() -> BlockHash.build(List.of(new GroupSpec(channelIndex, sources.get(0).elementType())), bigArrays, maxPageSize),
425+
() -> BlockHash.build(List.of(new GroupSpec(channelIndex, sources.get(0).elementType())), bigArrays, maxPageSize, false),
426426
driverContext
427427
);
428428
}

‎x-pack/plugin/esql/compute/src/test/java/org/elasticsearch/compute/OperatorTests.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -247,7 +247,8 @@ public String toString() {
247247
() -> BlockHash.build(
248248
List.of(new HashAggregationOperator.GroupSpec(0, ElementType.BYTES_REF)),
249249
bigArrays,
250-
randomPageSize()
250+
randomPageSize(),
251+
false
251252
),
252253
driverContext
253254
)

‎x-pack/plugin/esql/compute/src/test/java/org/elasticsearch/compute/aggregation/blockhash/BlockHashRandomizedTests.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -168,7 +168,7 @@ private BlockHash newBlockHash(int emitBatchSize, List<ElementType> types) {
168168
MockBigArrays bigArrays = new MockBigArrays(PageCacheRecycler.NON_RECYCLING_INSTANCE, new NoneCircuitBreakerService());
169169
return forcePackedHash
170170
? new PackedValuesBlockHash(specs, bigArrays, emitBatchSize)
171-
: BlockHash.build(specs, bigArrays, emitBatchSize);
171+
: BlockHash.build(specs, bigArrays, emitBatchSize, true);
172172
}
173173

174174
private static class KeyComparator implements Comparator<List<?>> {

‎x-pack/plugin/esql/compute/src/test/java/org/elasticsearch/compute/aggregation/blockhash/BlockHashTests.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1032,7 +1032,7 @@ private void hash(Consumer<OrdsAndKeys> callback, int emitBatchSize, Block... va
10321032
try (
10331033
BlockHash blockHash = forcePackedHash
10341034
? new PackedValuesBlockHash(specs, bigArrays, emitBatchSize)
1035-
: BlockHash.build(specs, bigArrays, emitBatchSize)
1035+
: BlockHash.build(specs, bigArrays, emitBatchSize, true)
10361036
) {
10371037
hash(true, blockHash, callback, values);
10381038
}

‎x-pack/plugin/esql/qa/testFixtures/src/main/resources/stats.csv-spec‎

Lines changed: 54 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -290,23 +290,35 @@ null
290290
;
291291

292292
byStringAndLong
293-
from employees | eval trunk_worked_seconds = avg_worked_seconds / 100000000 * 100000000 | stats c = count(gender) by gender, trunk_worked_seconds | sort c desc;
293+
FROM employees
294+
| EVAL trunk_worked_seconds = avg_worked_seconds / 100000000 * 100000000
295+
| STATS c = COUNT(gender) by gender, trunk_worked_seconds
296+
| SORT c desc;
294297

295298
c:long | gender:keyword | trunk_worked_seconds:long
296-
30 | M | 300000000
297-
27 | M | 200000000
298-
22 | F | 300000000
299-
11 | F | 200000000
299+
30 | M | 300000000
300+
27 | M | 200000000
301+
22 | F | 300000000
302+
11 | F | 200000000
303+
0 | null | 200000000
304+
0 | null | 300000000
300305
;
301306

302307
byStringAndLongWithAlias
303-
from employees | eval trunk_worked_seconds = avg_worked_seconds / 100000000 * 100000000 | rename gender as g, trunk_worked_seconds as tws | keep g, tws | stats c = count(g) by g, tws | sort c desc;
308+
FROM employees
309+
| EVAL trunk_worked_seconds = avg_worked_seconds / 100000000 * 100000000
310+
| RENAME gender as g, trunk_worked_seconds as tws
311+
| KEEP g, tws
312+
| STATS c = count(g) by g, tws
313+
| SORT c desc;
304314

305315
c:long | g:keyword | tws:long
306-
30 | M | 300000000
307-
27 | M | 200000000
308-
22 | F | 300000000
309-
11 | F | 200000000
316+
30 | M | 300000000
317+
27 | M | 200000000
318+
22 | F | 300000000
319+
11 | F | 200000000
320+
0 | null | 200000000
321+
0 | null | 300000000
310322
;
311323

312324
byStringAndString
@@ -324,35 +336,45 @@ c:long | gender:keyword | hire_year_str:keyword
324336
;
325337

326338
byLongAndLong
327-
from employees | eval trunk_worked_seconds = avg_worked_seconds / 100000000 * 100000000 | stats c = count(languages.long) by languages.long, trunk_worked_seconds | sort c desc, languages.long, trunk_worked_seconds;
339+
FROM employees
340+
| EVAL trunk_worked_seconds = avg_worked_seconds / 100000000 * 100000000
341+
| STATS c = COUNT(languages.long) BY languages.long, trunk_worked_seconds
342+
| SORT c DESC, languages.long, trunk_worked_seconds;
328343

329344
c:long | languages.long:long | trunk_worked_seconds:long
330-
15 |5 |300000000
331-
11 |2 |300000000
332-
10 |4 |300000000
333-
9 |3 |200000000
334-
8 |1 |200000000
335-
8 |2 |200000000
336-
8 |3 |300000000
337-
8 |4 |200000000
338-
7 |1 |300000000
339-
6 |5 |200000000
345+
15 |5 |300000000
346+
11 |2 |300000000
347+
10 |4 |300000000
348+
9 |3 |200000000
349+
8 |1 |200000000
350+
8 |2 |200000000
351+
8 |3 |300000000
352+
8 |4 |200000000
353+
7 |1 |300000000
354+
6 |5 |200000000
355+
0 |null |200000000
356+
0 |null |300000000
340357
;
341358

342359
byUnmentionedLongAndLong
343-
from employees | eval trunk_worked_seconds = avg_worked_seconds / 100000000 * 100000000 | stats c = count(gender) by languages.long, trunk_worked_seconds | sort c desc, trunk_worked_seconds;
360+
FROM employees
361+
| EVAL trunk_worked_seconds = avg_worked_seconds / 100000000 * 100000000
362+
| STATS c = count(gender) by languages.long, trunk_worked_seconds
363+
| SORT c desc, trunk_worked_seconds;
344364

345365
c:long | languages.long:long | trunk_worked_seconds:long
346-
13 |5 |300000000
347-
10 |2 |300000000
348-
9 |3 |200000000
349-
9 |4 |300000000
350-
8 |4 |200000000
351-
8 |3 |300000000
352-
7 |1 |200000000
353-
6 |2 |200000000
354-
6 |1 |300000000
355-
4 |5 |200000000
366+
13 |5 |300000000
367+
10 |2 |300000000
368+
9 |3 |200000000
369+
9 |4 |300000000
370+
8 |4 |200000000
371+
8 |3 |300000000
372+
7 |1 |200000000
373+
6 |2 |200000000
374+
6 |null |300000000
375+
6 |1 |300000000
376+
4 |null |200000000
377+
4 |5 |200000000
356378
;
357379

358380
byUnmentionedIntAndLong

‎x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/planner/TestPhysicalOperationProviders.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -256,7 +256,8 @@ public Operator get(DriverContext driverContext) {
256256
() -> BlockHash.build(
257257
List.of(new HashAggregationOperator.GroupSpec(groupByChannel, groupElementType)),
258258
bigArrays,
259-
pageSize
259+
pageSize,
260+
false
260261
),
261262
columnName,
262263
driverContext

0 commit comments

Comments
 (0)