Skip to content

Commit f2e4201

Browse files
authored
ESQL: Check for errors while loading blocks (#129016)
Runs a sanity check after loading a block of values. Previously we were doing a quick check if assertions were enabled. Now we do two quick checks all the time. Better - we attach information about how a block was loaded when there's a problem. Relates to #128959
1 parent 3420831 commit f2e4201

File tree

2 files changed

+38
-8
lines changed

2 files changed

+38
-8
lines changed

test/framework/src/main/java/org/elasticsearch/index/mapper/MapperServiceTestCase.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,7 @@
103103
import static org.mockito.Mockito.mock;
104104

105105
/**
106-
* Test case that lets you easilly build {@link MapperService} based on some
106+
* Test case that lets you easily build {@link MapperService} based on some
107107
* mapping. Useful when you don't need to spin up an entire index but do
108108
* need most of the trapping of the mapping.
109109
*/

x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/lucene/ValuesSourceReaderOperator.java

Lines changed: 37 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,6 @@
2727
import org.elasticsearch.compute.operator.AbstractPageMappingOperator;
2828
import org.elasticsearch.compute.operator.DriverContext;
2929
import org.elasticsearch.compute.operator.Operator;
30-
import org.elasticsearch.core.Assertions;
3130
import org.elasticsearch.core.Releasable;
3231
import org.elasticsearch.core.Releasables;
3332
import org.elasticsearch.index.fieldvisitor.StoredFieldLoader;
@@ -161,12 +160,6 @@ public int get(int i) {
161160
many.run();
162161
}
163162
}
164-
if (Assertions.ENABLED) {
165-
for (int f = 0; f < fields.length; f++) {
166-
assert blocks[f].elementType() == ElementType.NULL || blocks[f].elementType() == fields[f].info.type
167-
: blocks[f].elementType() + " NOT IN (NULL, " + fields[f].info.type + ")";
168-
}
169-
}
170163
success = true;
171164
for (Block b : blocks) {
172165
valuesLoaded += b.getTotalValueCount();
@@ -233,6 +226,7 @@ private void loadFromSingleLeaf(Block[] blocks, int shard, int segment, BlockLoa
233226
BlockLoader.ColumnAtATimeReader columnAtATime = field.columnAtATime(ctx);
234227
if (columnAtATime != null) {
235228
blocks[f] = (Block) columnAtATime.read(loaderBlockFactory, docs);
229+
sanityCheckBlock(columnAtATime, docs.count(), blocks[f], f);
236230
} else {
237231
rowStrideReaders.add(
238232
new RowStrideReaderWork(
@@ -282,6 +276,7 @@ private void loadFromSingleLeaf(Block[] blocks, int shard, int segment, BlockLoa
282276
}
283277
for (RowStrideReaderWork work : rowStrideReaders) {
284278
blocks[work.offset] = work.build();
279+
sanityCheckBlock(work.reader, docs.count(), blocks[work.offset], work.offset);
285280
}
286281
} finally {
287282
Releasables.close(rowStrideReaders);
@@ -385,6 +380,7 @@ void run() throws IOException {
385380
try (Block targetBlock = fieldTypeBuilders[f].build()) {
386381
target[f] = targetBlock.filter(backwards);
387382
}
383+
sanityCheckBlock(rowStride[f], docs.getPositionCount(), target[f], f);
388384
}
389385
}
390386

@@ -561,6 +557,40 @@ protected Status status(long processNanos, int pagesProcessed, long rowsReceived
561557
return new Status(new TreeMap<>(readersBuilt), processNanos, pagesProcessed, rowsReceived, rowsEmitted, valuesLoaded);
562558
}
563559

560+
/**
561+
* Quick checks for on the loaded block to make sure it looks reasonable.
562+
* @param loader the object that did the loading - we use it to make error messages if the block is busted
563+
* @param expectedPositions how many positions the block should have - it's as many as the incoming {@link Page} has
564+
* @param block the block to sanity check
565+
* @param field offset into the {@link #fields} array for the block being loaded
566+
*/
567+
private void sanityCheckBlock(Object loader, int expectedPositions, Block block, int field) {
568+
if (block.getPositionCount() != expectedPositions) {
569+
throw new IllegalStateException(
570+
sanityCheckBlockErrorPrefix(loader, block, field)
571+
+ " has ["
572+
+ block.getPositionCount()
573+
+ "] positions instead of ["
574+
+ expectedPositions
575+
+ "]"
576+
);
577+
}
578+
if (block.elementType() != ElementType.NULL && block.elementType() != fields[field].info.type) {
579+
throw new IllegalStateException(
580+
sanityCheckBlockErrorPrefix(loader, block, field)
581+
+ "'s element_type ["
582+
+ block.elementType()
583+
+ "] NOT IN (NULL, "
584+
+ fields[field].info.type
585+
+ ")"
586+
);
587+
}
588+
}
589+
590+
private String sanityCheckBlockErrorPrefix(Object loader, Block block, int field) {
591+
return fields[field].info.name + "[" + loader + "]: " + block;
592+
}
593+
564594
public static class Status extends AbstractPageMappingOperator.Status {
565595
public static final NamedWriteableRegistry.Entry ENTRY = new NamedWriteableRegistry.Entry(
566596
Operator.Status.class,

0 commit comments

Comments
 (0)