Skip to content

Commit 5f0afbc

Browse files
authored
ESQL: Check for errors while loading blocks (#129016) (#129073)
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 b82d55a commit 5f0afbc

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;
@@ -158,12 +157,6 @@ public int get(int i) {
158157
many.run();
159158
}
160159
}
161-
if (Assertions.ENABLED) {
162-
for (int f = 0; f < fields.length; f++) {
163-
assert blocks[f].elementType() == ElementType.NULL || blocks[f].elementType() == fields[f].info.type
164-
: blocks[f].elementType() + " NOT IN (NULL, " + fields[f].info.type + ")";
165-
}
166-
}
167160
success = true;
168161
return page.appendBlocks(blocks);
169162
} catch (IOException e) {
@@ -227,6 +220,7 @@ private void loadFromSingleLeaf(Block[] blocks, int shard, int segment, BlockLoa
227220
BlockLoader.ColumnAtATimeReader columnAtATime = field.columnAtATime(ctx);
228221
if (columnAtATime != null) {
229222
blocks[f] = (Block) columnAtATime.read(loaderBlockFactory, docs);
223+
sanityCheckBlock(columnAtATime, docs.count(), blocks[f], f);
230224
} else {
231225
rowStrideReaders.add(
232226
new RowStrideReaderWork(
@@ -276,6 +270,7 @@ private void loadFromSingleLeaf(Block[] blocks, int shard, int segment, BlockLoa
276270
}
277271
for (RowStrideReaderWork work : rowStrideReaders) {
278272
blocks[work.offset] = work.build();
273+
sanityCheckBlock(work.reader, docs.count(), blocks[work.offset], work.offset);
279274
}
280275
} finally {
281276
Releasables.close(rowStrideReaders);
@@ -379,6 +374,7 @@ void run() throws IOException {
379374
try (Block targetBlock = fieldTypeBuilders[f].build()) {
380375
target[f] = targetBlock.filter(backwards);
381376
}
377+
sanityCheckBlock(rowStride[f], docs.getPositionCount(), target[f], f);
382378
}
383379
}
384380

@@ -555,6 +551,40 @@ protected Status status(long processNanos, int pagesProcessed, long rowsReceived
555551
return new Status(new TreeMap<>(readersBuilt), processNanos, pagesProcessed, rowsReceived, rowsEmitted);
556552
}
557553

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

0 commit comments

Comments
 (0)