From 9ec88af89c55979c973a922cfa393c45005dc8b3 Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Tue, 6 Aug 2024 13:47:28 -0400 Subject: [PATCH 1/2] Avoid NPEs when making blocks of unsupported values --- .../lucene/UnsupportedValueSource.java | 51 ------------------- .../esql/action/EsqlQueryResponseTests.java | 8 ++- 2 files changed, 3 insertions(+), 56 deletions(-) delete mode 100644 x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/lucene/UnsupportedValueSource.java diff --git a/x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/lucene/UnsupportedValueSource.java b/x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/lucene/UnsupportedValueSource.java deleted file mode 100644 index 3f2632d9a643f..0000000000000 --- a/x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/lucene/UnsupportedValueSource.java +++ /dev/null @@ -1,51 +0,0 @@ -/* - * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one - * or more contributor license agreements. Licensed under the Elastic License - * 2.0; you may not use this file except in compliance with the Elastic License - * 2.0. - */ - -package org.elasticsearch.compute.lucene; - -import org.apache.lucene.index.LeafReaderContext; -import org.elasticsearch.common.Rounding; -import org.elasticsearch.index.fielddata.DocValueBits; -import org.elasticsearch.index.fielddata.FieldData; -import org.elasticsearch.index.fielddata.SortedBinaryDocValues; -import org.elasticsearch.search.aggregations.support.AggregationContext; -import org.elasticsearch.search.aggregations.support.ValuesSource; - -import java.io.IOException; -import java.util.function.Function; - -public class UnsupportedValueSource extends ValuesSource { - - public static final String UNSUPPORTED_OUTPUT = null; - private final ValuesSource originalSource; - - public UnsupportedValueSource(ValuesSource originalSource) { - this.originalSource = originalSource; - } - - @Override - public SortedBinaryDocValues bytesValues(LeafReaderContext context) throws IOException { - if (originalSource != null) { - try { - return originalSource.bytesValues(context); - } catch (Exception e) { - // ignore and fall back to UNSUPPORTED_OUTPUT - } - } - return FieldData.emptySortedBinary(); - } - - @Override - public DocValueBits docsWithValue(LeafReaderContext context) throws IOException { - throw new UnsupportedOperationException(); - } - - @Override - protected Function roundingPreparer(AggregationContext context) throws IOException { - throw new UnsupportedOperationException(); - } -} diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/action/EsqlQueryResponseTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/action/EsqlQueryResponseTests.java index 86610ae923af6..86f313a6eec51 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/action/EsqlQueryResponseTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/action/EsqlQueryResponseTests.java @@ -27,7 +27,6 @@ import org.elasticsearch.compute.data.IntBlock; import org.elasticsearch.compute.data.LongBlock; import org.elasticsearch.compute.data.Page; -import org.elasticsearch.compute.lucene.UnsupportedValueSource; import org.elasticsearch.compute.operator.AbstractPageMappingOperator; import org.elasticsearch.compute.operator.DriverProfile; import org.elasticsearch.compute.operator.DriverStatus; @@ -157,9 +156,7 @@ private Page randomPage(List columns) { ); case DATETIME -> ((LongBlock.Builder) builder).appendLong(randomInstant().toEpochMilli()); case BOOLEAN -> ((BooleanBlock.Builder) builder).appendBoolean(randomBoolean()); - case UNSUPPORTED -> ((BytesRefBlock.Builder) builder).appendBytesRef( - new BytesRef(UnsupportedValueSource.UNSUPPORTED_OUTPUT) - ); + case UNSUPPORTED -> ((BytesRefBlock.Builder) builder).appendNull(); case VERSION -> ((BytesRefBlock.Builder) builder).appendBytesRef(new Version(randomIdentifier()).toBytesRef()); case GEO_POINT -> ((BytesRefBlock.Builder) builder).appendBytesRef(GEO.asWkb(GeometryTestUtils.randomPoint())); case CARTESIAN_POINT -> ((BytesRefBlock.Builder) builder).appendBytesRef(CARTESIAN.asWkb(ShapeTestUtils.randomPoint())); @@ -660,7 +657,8 @@ static Page valuesToPage(BlockFactory blockFactory, List columns case LONG, COUNTER_LONG -> ((LongBlock.Builder) builder).appendLong(((Number) value).longValue()); case INTEGER, COUNTER_INTEGER -> ((IntBlock.Builder) builder).appendInt(((Number) value).intValue()); case DOUBLE, COUNTER_DOUBLE -> ((DoubleBlock.Builder) builder).appendDouble(((Number) value).doubleValue()); - case KEYWORD, TEXT, UNSUPPORTED -> ((BytesRefBlock.Builder) builder).appendBytesRef(new BytesRef(value.toString())); + case KEYWORD, TEXT -> ((BytesRefBlock.Builder) builder).appendBytesRef(new BytesRef(value.toString())); + case UNSUPPORTED -> ((BytesRefBlock.Builder) builder).appendNull(); case IP -> ((BytesRefBlock.Builder) builder).appendBytesRef(stringToIP(value.toString())); case DATETIME -> { long longVal = dateTimeToLong(value.toString()); From 0d86658e28026a89b177199e6ca388983704bee8 Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Tue, 6 Aug 2024 13:47:46 -0400 Subject: [PATCH 2/2] we don't need a whole class to have one named constant null that gets used twice --- .../elasticsearch/xpack/esql/action/PositionToXContent.java | 3 +-- .../elasticsearch/xpack/esql/action/ResponseValueUtils.java | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/PositionToXContent.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/PositionToXContent.java index 0bc1eb46abefe..ac320d4a1058d 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/PositionToXContent.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/PositionToXContent.java @@ -16,7 +16,6 @@ import org.elasticsearch.compute.data.DoubleBlock; import org.elasticsearch.compute.data.IntBlock; import org.elasticsearch.compute.data.LongBlock; -import org.elasticsearch.compute.lucene.UnsupportedValueSource; import org.elasticsearch.xcontent.ToXContent; import org.elasticsearch.xcontent.XContentBuilder; import org.elasticsearch.xcontent.XContentParser; @@ -151,7 +150,7 @@ protected XContentBuilder valueToXContent(XContentBuilder builder, ToXContent.Pa @Override protected XContentBuilder valueToXContent(XContentBuilder builder, ToXContent.Params params, int valueIndex) throws IOException { - return builder.value(UnsupportedValueSource.UNSUPPORTED_OUTPUT); + return builder.value((String) null); } }; case SOURCE -> new PositionToXContent(block) { diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/ResponseValueUtils.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/ResponseValueUtils.java index 67c6e1f48a47a..9c157b088039e 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/ResponseValueUtils.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/ResponseValueUtils.java @@ -18,7 +18,6 @@ import org.elasticsearch.compute.data.IntBlock; import org.elasticsearch.compute.data.LongBlock; import org.elasticsearch.compute.data.Page; -import org.elasticsearch.compute.lucene.UnsupportedValueSource; import org.elasticsearch.xcontent.XContentParser; import org.elasticsearch.xcontent.XContentParserConfiguration; import org.elasticsearch.xpack.esql.EsqlIllegalArgumentException; @@ -128,7 +127,7 @@ private static Object valueAt(DataType dataType, Block block, int offset, BytesR case GEO_POINT, GEO_SHAPE, CARTESIAN_POINT, CARTESIAN_SHAPE -> spatialToString( ((BytesRefBlock) block).getBytesRef(offset, scratch) ); - case UNSUPPORTED -> UnsupportedValueSource.UNSUPPORTED_OUTPUT; + case UNSUPPORTED -> (String) null; case SOURCE -> { BytesRef val = ((BytesRefBlock) block).getBytesRef(offset, scratch); try {