This is an automated email from the ASF dual-hosted git repository.
swaminathanmanish pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/pinot.git
The following commit(s) were added to refs/heads/master by this push:
new 7ef952d8342 Address review on H3 empty-default fix: null-filter
correctness, reload perf, raw-path test (#19128)
7ef952d8342 is described below
commit 7ef952d8342834ea2d4e3d42be34aa87b2558b09
Author: Arunkumar Saravanan <[email protected]>
AuthorDate: Fri Jul 31 16:09:20 2026 +0530
Address review on H3 empty-default fix: null-filter correctness, reload
perf, raw-path test (#19128)
* Address review on H3 empty-default fix: null-filter correctness, reload
perf, raw-path test
Follow-up to #19032.
- Correctness: H3IndexFilterOperator and H3InclusionIndexFilterOperator
excluded
null rows (which have no H3 posting) from match-all, lower-bound-flip and
negative-check complement results, so they no longer leak into
ST_Contains/Within(...)=false and lower-bound/match-all distance filters
when
query null handling is enabled.
- Performance: GeoSpatialIndexCreator.toGeometry() fast-paths the empty
default-null byte array to null instead of throwing (and catching) a
BufferUnderflowException per row on reload; H3IndexHandler decodes each
dictionary id at most once instead of per doc.
- Tests: SegmentPreProcessorTest now covers V1/V3 x DICTIONARY/RAW reload
paths;
H3IndexQueriesTest adds a null-handling query test asserting null rows are
excluded from match-all / lower-bound / negative-ST_Within results.
Co-Authored-By: Claude Opus 4.8 <[email protected]>
Signed-off-by: Arunkumar Saravanan
<[email protected]>
* Address Copilot review: bound H3 reload decode cache to avoid large
retained heap
Instead of caching one decoded Geometry per dict id (which for a
high-cardinality
geo column retains ~one Geometry per doc and risks OOM during reload), only
reuse
the single decoded value for the cardinality-1 empty-default case;
higher-cardinality
columns decode per doc via toGeometry(), which still fast-paths the empty
default
value so no per-row exception is thrown.
Co-Authored-By: Claude Opus 4.8 <[email protected]>
Signed-off-by: Arunkumar Saravanan
<[email protected]>
* Fix checkstyle: use /// markdown doc comments instead of Javadoc
Master added a checkstyle rule (JEP 467) banning /** */ Javadoc in favor of
/// markdown doc comments. Convert the doc comments added in this PR
(GeoSpatialIndexCreator.toGeometry, the two H3 filter operators'
getNullDocIds,
and the null-handling query test) to /// style.
Co-Authored-By: Claude Opus 4.8 <[email protected]>
Signed-off-by: Arunkumar Saravanan
<[email protected]>
---------
Signed-off-by: Arunkumar Saravanan
<[email protected]>
Co-authored-by: Claude Opus 4.8 <[email protected]>
---
.../filter/H3InclusionIndexFilterOperator.java | 30 ++++++-
.../operator/filter/H3IndexFilterOperator.java | 43 +++++++++-
.../apache/pinot/queries/H3IndexQueriesTest.java | 93 ++++++++++++++++++++--
.../index/loader/invertedindex/H3IndexHandler.java | 30 +++++--
.../index/loader/SegmentPreProcessorTest.java | 55 ++++++++++---
.../spi/index/creator/GeoSpatialIndexCreator.java | 24 ++++--
6 files changed, 238 insertions(+), 37 deletions(-)
diff --git
a/pinot-core/src/main/java/org/apache/pinot/core/operator/filter/H3InclusionIndexFilterOperator.java
b/pinot-core/src/main/java/org/apache/pinot/core/operator/filter/H3InclusionIndexFilterOperator.java
index 880c534a0f4..15d7f26610c 100644
---
a/pinot-core/src/main/java/org/apache/pinot/core/operator/filter/H3InclusionIndexFilterOperator.java
+++
b/pinot-core/src/main/java/org/apache/pinot/core/operator/filter/H3InclusionIndexFilterOperator.java
@@ -22,6 +22,7 @@ import com.google.common.base.CaseFormat;
import it.unimi.dsi.fastutil.longs.LongIterator;
import it.unimi.dsi.fastutil.longs.LongSet;
import java.util.List;
+import javax.annotation.Nullable;
import org.apache.commons.lang3.tuple.Pair;
import org.apache.pinot.common.request.context.ExpressionContext;
import org.apache.pinot.common.request.context.predicate.EqPredicate;
@@ -36,6 +37,7 @@ import
org.apache.pinot.segment.local.utils.GeometrySerializer;
import org.apache.pinot.segment.local.utils.H3Utils;
import org.apache.pinot.segment.spi.IndexSegment;
import org.apache.pinot.segment.spi.index.reader.H3IndexReader;
+import org.apache.pinot.segment.spi.index.reader.NullValueVectorReader;
import org.apache.pinot.spi.utils.BooleanUtils;
import org.locationtech.jts.geom.Geometry;
import org.roaringbitmap.buffer.BufferFastAggregation;
@@ -51,6 +53,7 @@ public class H3InclusionIndexFilterOperator extends
BaseFilterOperator {
private final IndexSegment _segment;
private final QueryContext _queryContext;
private final Predicate _predicate;
+ private final String _column;
private final H3IndexReader _h3IndexReader;
private final Geometry _geometry;
private final boolean _isPositiveCheck;
@@ -67,12 +70,13 @@ public class H3InclusionIndexFilterOperator extends
BaseFilterOperator {
_isPositiveCheck = BooleanUtils.toBoolean(eqPredicate.getValue());
if (arguments.get(0).getType() == ExpressionContext.Type.IDENTIFIER) {
- _h3IndexReader =
segment.getDataSource(arguments.get(0).getIdentifier()).getH3Index();
+ _column = arguments.get(0).getIdentifier();
_geometry =
GeometrySerializer.deserialize(arguments.get(1).getLiteral().getBytesValue());
} else {
- _h3IndexReader =
segment.getDataSource(arguments.get(1).getIdentifier()).getH3Index();
+ _column = arguments.get(1).getIdentifier();
_geometry =
GeometrySerializer.deserialize(arguments.get(0).getLiteral().getBytesValue());
}
+ _h3IndexReader = segment.getDataSource(_column).getH3Index();
// must be some h3 index
assert _h3IndexReader != null : "the column must have H3 index setup.";
}
@@ -111,6 +115,12 @@ public class H3InclusionIndexFilterOperator extends
BaseFilterOperator {
MutableRoaringBitmap fullNotMatch = potentialMatch.clone();
fullNotMatch.flip(0L, _numDocs);
fullNotMatch.andNot(fullMatch);
+ // The flip turns non-matching docs into full not-matches, but null
geometries have no posting and must not be
+ // reported as matches for the negative check when query null handling
is enabled, so exclude them.
+ ImmutableRoaringBitmap nullDocIds = getNullDocIds();
+ if (nullDocIds != null) {
+ fullNotMatch.andNot(nullDocIds);
+ }
return getFilterBlock(fullNotMatch, potentialMatch);
}
}
@@ -130,6 +140,22 @@ public class H3InclusionIndexFilterOperator extends
BaseFilterOperator {
};
}
+ /// Returns the null document IDs for the indexed column when query null
handling is enabled and the column has a
+ /// non-empty null-value vector, otherwise `null`. Used to exclude null rows
from the negative-check complement
+ /// result, which is built across all document IDs and would otherwise
include null rows that have no H3 posting.
+ @Nullable
+ private ImmutableRoaringBitmap getNullDocIds() {
+ if (!_queryContext.isNullHandlingEnabled()) {
+ return null;
+ }
+ NullValueVectorReader nullValueVector =
_segment.getDataSource(_column).getNullValueVector();
+ if (nullValueVector == null) {
+ return null;
+ }
+ ImmutableRoaringBitmap nullDocIds = nullValueVector.getNullBitmap();
+ return nullDocIds != null && !nullDocIds.isEmpty() ? nullDocIds : null;
+ }
+
@Override
public List<Operator> getChildOperators() {
return List.of();
diff --git
a/pinot-core/src/main/java/org/apache/pinot/core/operator/filter/H3IndexFilterOperator.java
b/pinot-core/src/main/java/org/apache/pinot/core/operator/filter/H3IndexFilterOperator.java
index 931f10072d0..e9d20aa49c3 100644
---
a/pinot-core/src/main/java/org/apache/pinot/core/operator/filter/H3IndexFilterOperator.java
+++
b/pinot-core/src/main/java/org/apache/pinot/core/operator/filter/H3IndexFilterOperator.java
@@ -24,6 +24,7 @@ import com.uber.h3core.LengthUnit;
import java.util.HashSet;
import java.util.List;
import java.util.Set;
+import javax.annotation.Nullable;
import org.apache.pinot.common.request.context.ExpressionContext;
import org.apache.pinot.common.request.context.predicate.Predicate;
import org.apache.pinot.common.request.context.predicate.RangePredicate;
@@ -39,7 +40,9 @@ import
org.apache.pinot.segment.local.utils.GeometrySerializer;
import org.apache.pinot.segment.local.utils.H3Utils;
import org.apache.pinot.segment.spi.IndexSegment;
import org.apache.pinot.segment.spi.index.reader.H3IndexReader;
+import org.apache.pinot.segment.spi.index.reader.NullValueVectorReader;
import org.locationtech.jts.geom.Coordinate;
+import org.roaringbitmap.buffer.ImmutableRoaringBitmap;
import org.roaringbitmap.buffer.MutableRoaringBitmap;
@@ -50,6 +53,7 @@ public class H3IndexFilterOperator extends BaseFilterOperator
{
private final IndexSegment _segment;
private final QueryContext _queryContext;
private final Predicate _predicate;
+ private final String _column;
private final H3IndexReader _h3IndexReader;
private final long _h3Id;
private final double _edgeLength;
@@ -66,12 +70,13 @@ public class H3IndexFilterOperator extends
BaseFilterOperator {
List<ExpressionContext> arguments =
predicate.getLhs().getFunction().getArguments();
Coordinate coordinate;
if (arguments.get(0).getType() == ExpressionContext.Type.IDENTIFIER) {
- _h3IndexReader =
segment.getDataSource(arguments.get(0).getIdentifier()).getH3Index();
+ _column = arguments.get(0).getIdentifier();
coordinate =
GeometrySerializer.deserialize(arguments.get(1).getLiteral().getBytesValue()).getCoordinate();
} else {
- _h3IndexReader =
segment.getDataSource(arguments.get(1).getIdentifier()).getH3Index();
+ _column = arguments.get(1).getIdentifier();
coordinate =
GeometrySerializer.deserialize(arguments.get(0).getLiteral().getBytesValue()).getCoordinate();
}
+ _h3IndexReader = segment.getDataSource(_column).getH3Index();
assert _h3IndexReader != null;
int resolution =
_h3IndexReader.getH3IndexResolution().getLowestResolution();
_h3Id = H3Utils.H3_CORE.latLngToCell(coordinate.y, coordinate.x,
resolution);
@@ -102,8 +107,16 @@ public class H3IndexFilterOperator extends
BaseFilterOperator {
// No lower bound
if (Double.isNaN(_upperBound)) {
- // No bound, return a match-all block
- return new MatchAllDocIdSet(_numDocs);
+ // No bound, return a match-all block. Null geometries have no H3
posting, so with query null handling
+ // enabled they must be excluded from the match-all result instead
of being reported as matches.
+ ImmutableRoaringBitmap nullDocIds = getNullDocIds();
+ if (nullDocIds == null) {
+ return new MatchAllDocIdSet(_numDocs);
+ }
+ MutableRoaringBitmap matchAllDocIds = new MutableRoaringBitmap();
+ matchAllDocIds.add(0L, _numDocs);
+ matchAllDocIds.andNot(nullDocIds);
+ return new BitmapDocIdSet(matchAllDocIds, _numDocs);
}
// Upper bound only
@@ -136,6 +149,12 @@ public class H3IndexFilterOperator extends
BaseFilterOperator {
fullMatchDocIds.or(_h3IndexReader.getDocIds(partialMatchH3Id));
}
fullMatchDocIds.flip(0L, _numDocs);
+ // The flip turns non-matching docs into full matches, but null
geometries have no posting and must not be
+ // reported as matches when query null handling is enabled, so exclude
them.
+ ImmutableRoaringBitmap nullDocIds = getNullDocIds();
+ if (nullDocIds != null) {
+ fullMatchDocIds.andNot(nullDocIds);
+ }
// Remove the always not match H3 ids from possible not match H3 ids
to get the partial match H3 ids
possibleNotMatchH3Ids.removeAll(alwaysNotMatchH3Ids);
@@ -235,6 +254,22 @@ public class H3IndexFilterOperator extends
BaseFilterOperator {
};
}
+ /// Returns the null document IDs for the indexed column when query null
handling is enabled and the column has a
+ /// non-empty null-value vector, otherwise `null`. Used to exclude null rows
from match-all and complement results,
+ /// which are built across all document IDs and would otherwise include null
rows that have no H3 posting.
+ @Nullable
+ private ImmutableRoaringBitmap getNullDocIds() {
+ if (!_queryContext.isNullHandlingEnabled()) {
+ return null;
+ }
+ NullValueVectorReader nullValueVector =
_segment.getDataSource(_column).getNullValueVector();
+ if (nullValueVector == null) {
+ return null;
+ }
+ ImmutableRoaringBitmap nullDocIds = nullValueVector.getNullBitmap();
+ return nullDocIds != null && !nullDocIds.isEmpty() ? nullDocIds : null;
+ }
+
@Override
public List<Operator> getChildOperators() {
return List.of();
diff --git
a/pinot-core/src/test/java/org/apache/pinot/queries/H3IndexQueriesTest.java
b/pinot-core/src/test/java/org/apache/pinot/queries/H3IndexQueriesTest.java
index f4457ebb8e7..32af1e46509 100644
--- a/pinot-core/src/test/java/org/apache/pinot/queries/H3IndexQueriesTest.java
+++ b/pinot-core/src/test/java/org/apache/pinot/queries/H3IndexQueriesTest.java
@@ -39,6 +39,7 @@ import
org.apache.pinot.segment.spi.creator.SegmentGeneratorConfig;
import org.apache.pinot.spi.config.table.FieldConfig;
import org.apache.pinot.spi.config.table.TableConfig;
import org.apache.pinot.spi.config.table.TableType;
+import org.apache.pinot.spi.config.table.ingestion.IngestionConfig;
import org.apache.pinot.spi.data.FieldSpec.DataType;
import org.apache.pinot.spi.data.Schema;
import org.apache.pinot.spi.data.readers.GenericRow;
@@ -68,12 +69,24 @@ public class H3IndexQueriesTest extends BaseQueriesTest {
.addSingleValueDimension(H3_INDEX_GEOMETRY_COLUMN, DataType.BYTES)
.addSingleValueDimension(NON_H3_INDEX_GEOMETRY_COLUMN,
DataType.BYTES).build();
private static final Map<String, String> H3_INDEX_PROPERTIES =
Map.of("resolutions", "5");
+ private static final List<FieldConfig> H3_FIELD_CONFIGS = List.of(
+ new FieldConfig(H3_INDEX_COLUMN, FieldConfig.EncodingType.DICTIONARY,
FieldConfig.IndexType.H3, null,
+ H3_INDEX_PROPERTIES),
+ new FieldConfig(H3_INDEX_GEOMETRY_COLUMN,
FieldConfig.EncodingType.DICTIONARY, FieldConfig.IndexType.H3, null,
+ H3_INDEX_PROPERTIES));
private static final TableConfig TABLE_CONFIG = new
TableConfigBuilder(TableType.OFFLINE).setTableName(RAW_TABLE_NAME)
- .setFieldConfigList(List.of(
- new FieldConfig(H3_INDEX_COLUMN,
FieldConfig.EncodingType.DICTIONARY, FieldConfig.IndexType.H3, null,
- H3_INDEX_PROPERTIES),
- new FieldConfig(H3_INDEX_GEOMETRY_COLUMN,
FieldConfig.EncodingType.DICTIONARY, FieldConfig.IndexType.H3, null,
- H3_INDEX_PROPERTIES))).build();
+ .setFieldConfigList(H3_FIELD_CONFIGS).build();
+ // Null handling enabled so the segment stores a null-value vector, and
continueOnError enabled so the H3 index
+ // creator skips (rather than fails on) the null geometries during segment
creation.
+ private static final TableConfig NULL_ENABLED_TABLE_CONFIG =
+ new
TableConfigBuilder(TableType.OFFLINE).setTableName(RAW_TABLE_NAME).setFieldConfigList(H3_FIELD_CONFIGS)
+
.setNullHandlingEnabled(true).setIngestionConfig(continueOnErrorIngestionConfig()).build();
+
+ private static IngestionConfig continueOnErrorIngestionConfig() {
+ IngestionConfig ingestionConfig = new IngestionConfig();
+ ingestionConfig.setContinueOnError(true);
+ return ingestionConfig;
+ }
private IndexSegment _indexSegment;
@@ -94,18 +107,24 @@ public class H3IndexQueriesTest extends BaseQueriesTest {
public void setUp(List<GenericRow> records)
throws Exception {
+ setUp(records, TABLE_CONFIG, false);
+ }
+
+ private void setUp(List<GenericRow> records, TableConfig tableConfig,
boolean nullHandlingEnabled)
+ throws Exception {
FileUtils.deleteDirectory(INDEX_DIR);
- SegmentGeneratorConfig segmentGeneratorConfig = new
SegmentGeneratorConfig(TABLE_CONFIG, SCHEMA);
+ SegmentGeneratorConfig segmentGeneratorConfig = new
SegmentGeneratorConfig(tableConfig, SCHEMA);
segmentGeneratorConfig.setTableName(RAW_TABLE_NAME);
segmentGeneratorConfig.setSegmentName(SEGMENT_NAME);
+ segmentGeneratorConfig.setDefaultNullHandlingEnabled(nullHandlingEnabled);
segmentGeneratorConfig.setOutDir(INDEX_DIR.getPath());
SegmentIndexCreationDriverImpl driver = new
SegmentIndexCreationDriverImpl();
driver.init(segmentGeneratorConfig, new GenericRowRecordReader(records));
driver.build();
- IndexLoadingConfig indexLoadingConfig = new
IndexLoadingConfig(TABLE_CONFIG, SCHEMA);
+ IndexLoadingConfig indexLoadingConfig = new
IndexLoadingConfig(tableConfig, SCHEMA);
_indexSegment = ImmutableSegmentLoader.load(new File(INDEX_DIR,
SEGMENT_NAME), indexLoadingConfig);
}
@@ -122,6 +141,15 @@ public class H3IndexQueriesTest extends BaseQueriesTest {
records.add(record);
}
+ private void addNullRecord(List<GenericRow> records) {
+ GenericRow record = new GenericRow();
+ record.putValue(H3_INDEX_COLUMN, null);
+ record.putValue(NON_H3_INDEX_COLUMN, null);
+ record.putValue(H3_INDEX_GEOMETRY_COLUMN, null);
+ record.putValue(NON_H3_INDEX_GEOMETRY_COLUMN, null);
+ records.add(record);
+ }
+
@Test
public void testH3Index()
throws Exception {
@@ -291,6 +319,57 @@ public class H3IndexQueriesTest extends BaseQueriesTest {
}
}
+ /// With query null handling enabled, rows whose geometry is null have no H3
posting and must not leak into
+ /// match-all, complement (negative check), or lower-bound H3-index results.
Each query below matches every non-null
+ /// row, so the correct answer is the non-null record count; before the fix
the null rows were wrongly counted too.
+ ///
+ /// Note: this compares against the expected count rather than the scan
path, because the geospatial transform
+ /// functions do not tolerate the null/empty geometry input on the scan path
(they throw a BufferUnderflowException),
+ /// so the scan path cannot serve as an oracle for all-null rows here. The
H3-index path never deserializes the
+ /// column values, so it is unaffected.
+ @Test
+ public void testH3IndexWithNullHandling()
+ throws Exception {
+ int numNonNullRecords = 0;
+ List<GenericRow> records = new ArrayList<>(NUM_RECORDS);
+ for (int i = 0; i < NUM_RECORDS; i++) {
+ if (i % 2 == 0) {
+ double longitude = -122.5 + RANDOM.nextDouble();
+ double latitude = 37 + RANDOM.nextDouble();
+ addRecord(records, longitude, latitude);
+ numNonNullRecords++;
+ } else {
+ addNullRecord(records);
+ }
+ }
+ setUp(records, NULL_ENABLED_TABLE_CONFIG, true);
+
+ // Match-all (no bound): every non-null row matches, null rows are
excluded.
+ validateNullHandlingResult("SELECT COUNT(*) FROM testTable WHERE
ST_Distance(%s, ST_Point(-122, 37.5, 1)) > -1",
+ H3_INDEX_COLUMN, numNonNullRecords);
+ // Lower bound only: distance is always positive for non-null rows, null
rows are excluded.
+ validateNullHandlingResult("SELECT COUNT(*) FROM testTable WHERE
ST_Distance(%s, ST_Point(-122, 37.5, 1)) > 0",
+ H3_INDEX_COLUMN, numNonNullRecords);
+ // Negative ST_Within against a polygon that contains none of the points:
every non-null row satisfies "= 0",
+ // exercising the inclusion operator complement; null rows are excluded.
+ validateNullHandlingResult(
+ "SELECT COUNT(*) FROM testTable WHERE ST_Within(%s,
ST_GeomFromText('POLYGON ((\n"
+ + " 122.0008564 -37.5004316, \n"
+ + " 121.9991291 -37.5005168, \n"
+ + " 121.9990325 -37.4995294, \n"
+ + " 122.0001268 -37.4993506, \n"
+ + " 122.0008564 -37.5004316))')) = 0",
+ H3_INDEX_GEOMETRY_COLUMN, numNonNullRecords);
+ }
+
+ private void validateNullHandlingResult(String queryTemplate, String
h3Column, long expectedCount) {
+ String h3IndexQuery = "SET enableNullHandling=true;\n" +
String.format(queryTemplate, h3Column);
+ AggregationOperator h3IndexOperator = getOperator(h3IndexQuery);
+ long h3IndexCount = (long) h3IndexOperator.nextBlock().getResults().get(0);
+ // The H3-index path must exclude the null rows (which have no posting)
from the match-all / complement result.
+ Assert.assertEquals(h3IndexCount, expectedCount);
+ }
+
@Test
public void stContainPointVeryCloseToBorderTest()
throws Exception {
diff --git
a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/invertedindex/H3IndexHandler.java
b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/invertedindex/H3IndexHandler.java
index b0fae087d5f..e7d03c288a5 100644
---
a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/invertedindex/H3IndexHandler.java
+++
b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/invertedindex/H3IndexHandler.java
@@ -46,6 +46,7 @@ import org.apache.pinot.segment.spi.store.SegmentDirectory;
import org.apache.pinot.spi.config.table.TableConfig;
import org.apache.pinot.spi.data.FieldSpec.DataType;
import org.apache.pinot.spi.data.Schema;
+import org.locationtech.jts.geom.Geometry;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -212,12 +213,23 @@ public class H3IndexHandler extends BaseIndexHandler {
.createIndexReader(segmentWriter, colIndexConf, columnMetadata);
GeoSpatialIndexCreator h3IndexCreator =
StandardIndexes.h3().createIndexCreator(context, config)) {
int numDocs = columnMetadata.getTotalDocs();
- for (int i = 0; i < numDocs; i++) {
- int dictId = forwardIndexReader.getDictId(i, readerContext);
- // Route through add(value, dictId) so that empty/default geometry
values (e.g. old segments reloaded after
- // the geo column was added, which have no source data to build a
Point from) are tolerated the same way the
- // segment-creation path tolerates them, instead of failing the whole
reload with a BufferUnderflowException.
- h3IndexCreator.add(dictionary.getBytesValue(dictId), dictId);
+ // Old segments reloaded after a geo column was added hold a single
empty default value (cardinality 1). Decode
+ // that one value once and reuse it for every doc, so the empty default
is decoded a single time rather than per
+ // doc. Decoding through the creator's toGeometry() fast-paths the empty
default value to null instead of failing
+ // the whole reload with a BufferUnderflowException, tolerating it the
same way the segment-creation path does.
+ // For higher-cardinality columns, decode per doc instead of retaining
one Geometry per dict id (which for a
+ // high-cardinality geo column would be roughly one per doc and risk a
large heap during reload); the per-doc
+ // path still goes through toGeometry(), so the empty default value
never throws.
+ if (dictionary.length() == 1) {
+ Geometry geometry =
h3IndexCreator.toGeometry(dictionary.getBytesValue(0));
+ for (int i = 0; i < numDocs; i++) {
+ h3IndexCreator.add(geometry);
+ }
+ } else {
+ for (int i = 0; i < numDocs; i++) {
+ int dictId = forwardIndexReader.getDictId(i, readerContext);
+
h3IndexCreator.add(h3IndexCreator.toGeometry(dictionary.getBytesValue(dictId)));
+ }
}
h3IndexCreator.seal();
}
@@ -236,8 +248,10 @@ public class H3IndexHandler extends BaseIndexHandler {
GeoSpatialIndexCreator h3IndexCreator =
StandardIndexes.h3().createIndexCreator(context, config)) {
int numDocs = columnMetadata.getTotalDocs();
for (int i = 0; i < numDocs; i++) {
- // See handleDictionaryBasedColumn: add(value, dictId) tolerates
empty/default geometry values on reload.
- h3IndexCreator.add(forwardIndexReader.getBytes(i, readerContext), -1);
+ // See handleDictionaryBasedColumn: toGeometry() fast-paths
empty/default geometry values to null (no
+ // per-row exception) and add(Geometry) tolerates them on reload the
same way segment creation does. The raw
+ // path has no dictionary, so values cannot be cached across docs.
+
h3IndexCreator.add(h3IndexCreator.toGeometry(forwardIndexReader.getBytes(i,
readerContext)));
}
h3IndexCreator.seal();
}
diff --git
a/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/SegmentPreProcessorTest.java
b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/SegmentPreProcessorTest.java
index 3db15e33406..05234c1f2a8 100644
---
a/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/SegmentPreProcessorTest.java
+++
b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/SegmentPreProcessorTest.java
@@ -343,6 +343,19 @@ public class SegmentPreProcessorTest implements
PinotBuffersAfterClassCheckRule
return new SegmentVersion[][]{{SegmentVersion.v1}, {SegmentVersion.v3}};
}
+ /// Cartesian product of segment version (v1/v3) and geo-column encoding
(DICTIONARY/RAW), so the H3 reload test
+ /// exercises both
[org.apache.pinot.segment.local.segment.index.loader.invertedindex.H3IndexHandler]
+ /// paths: the dictionary path and the raw
`forwardIndexReader.getBytes(...)` path.
+ @DataProvider(name = "h3VersionAndEncoding")
+ public Object[][] h3VersionAndEncoding() {
+ return new Object[][]{
+ {SegmentVersion.v1, FieldConfig.EncodingType.DICTIONARY},
+ {SegmentVersion.v3, FieldConfig.EncodingType.DICTIONARY},
+ {SegmentVersion.v1, FieldConfig.EncodingType.RAW},
+ {SegmentVersion.v3, FieldConfig.EncodingType.RAW}
+ };
+ }
+
/// Test to check for default column handling and text index creation during
/// segment load after a new raw column is added to the schema with text
index
/// creation enabled.
@@ -1663,36 +1676,56 @@ public class SegmentPreProcessorTest implements
PinotBuffersAfterClassCheckRule
assertEquals(singleFileIndex.length(), initFileSize);
}
- /// Regression test for the H3 index builder crashing a segment on
empty/default geometry values.
+ /// Regression test for the H3 index builder crashing a segment on
empty/default geometry values,
+ /// covering both the dictionary and the raw reload paths across v1/v3
segments.
///
/// When a geo column is added to the schema after a segment was built, old
segments have no source
/// data to derive it from, so the derived BYTES column is filled with its
default null value -- the
/// empty byte array. Building an H3 index over those rows used to call
///
[org.apache.pinot.segment.local.utils.GeometrySerializer#deserialize(byte\[\])]
directly on the
/// empty bytes, throwing a `BufferUnderflowException` that propagated out
of the reload and parked
- /// the segment in an ERROR state. The handler now routes through the
creator's tolerant add path,
- /// which skips undeserializable/default values when `continueOnError` is
enabled (set by
- /// [#resetIndexConfigs()]), exactly like the segment-creation path.
- @Test(dataProvider = "bothV1AndV3")
- public void testH3IndexCreationOnEmptyDefaultValue(SegmentVersion
segmentVersion)
+ /// the segment in an ERROR state. The handler now routes through the
creator's tolerant path, which
+ /// fast-paths the empty default value and skips undeserializable values
when `continueOnError` is
+ /// enabled (set by [#resetIndexConfigs()]), exactly like the
segment-creation path.
+ ///
+ /// The `DICTIONARY` case is the empty-default column itself (auto-generated
columns are always
+ /// dictionary-encoded, so their `cardinality == 1` value cannot be stored
raw). The `RAW` case
+ /// derives a non-dictionary BYTES column holding values that are not
decodable as geometry, so the
+ /// raw `forwardIndexReader.getBytes(...)` path is exercised and its values
are tolerated (skipped)
+ /// the same way empty defaults are.
+ @Test(dataProvider = "h3VersionAndEncoding")
+ public void testH3IndexCreationOnEmptyDefaultValue(SegmentVersion
segmentVersion,
+ FieldConfig.EncodingType encodingType)
throws Exception {
buildSegment(segmentVersion);
- // Add newH3Col as a derived column whose default null value is the empty
byte array (no explicit
- // defaultNullValue in the schema), mirroring old segments reloaded after
a geo column was added.
+ boolean rawEncoding = encodingType == FieldConfig.EncodingType.RAW;
+ if (rawEncoding) {
+ // Auto-generated empty-default columns are always dictionary-encoded,
so to exercise the raw reload path
+ // derive newH3Col from an existing column as raw BYTES. The values are
not valid serialized geometry, so the
+ // handler must skip them (like empty defaults) rather than fail.
+ _noDictionaryColumns.add("newH3Col");
+ _ingestionConfig.setTransformConfigs(List.of(new
TransformConfig("newH3Col", "toUtf8(column3)")));
+ }
+
+ // Add newH3Col. For DICTIONARY it is a pure default column whose default
null value is the empty byte array (no
+ // explicit defaultNullValue in the schema), mirroring old segments
reloaded after a geo column was added.
runPreProcessor(_newColumnsSchemaWithH3EmptyDefault);
SegmentMetadataImpl segmentMetadata = new SegmentMetadataImpl(INDEX_DIR);
- assertNotNull(segmentMetadata.getColumnMetadataFor("newH3Col"));
+ ColumnMetadata newH3ColMetadata =
segmentMetadata.getColumnMetadataFor("newH3Col");
+ assertNotNull(newH3ColMetadata);
+ assertEquals(newH3ColMetadata.hasDictionary(), !rawEncoding);
- // Build the H3 index over the empty/default values. This must not throw
and must produce the index.
+ // Build the H3 index over the values. This must not throw and must
produce the index.
_fieldConfigMap.put("newH3Col",
- new FieldConfig("newH3Col", FieldConfig.EncodingType.DICTIONARY,
List.of(FieldConfig.IndexType.H3), null,
+ new FieldConfig("newH3Col", encodingType,
List.of(FieldConfig.IndexType.H3), null,
Map.of("resolutions", "5")));
runPreProcessor(_newColumnsSchemaWithH3EmptyDefault);
try (SegmentDirectory segmentDirectory = new
SegmentLocalFSDirectory(INDEX_DIR, ReadMode.mmap);
SegmentDirectory.Reader reader = segmentDirectory.createReader()) {
assertTrue(reader.hasIndexFor("newH3Col", StandardIndexes.h3()));
+ assertEquals(reader.hasIndexFor("newH3Col",
StandardIndexes.dictionary()), !rawEncoding);
}
}
diff --git
a/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/creator/GeoSpatialIndexCreator.java
b/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/creator/GeoSpatialIndexCreator.java
index 87e4795e26f..d573f173145 100644
---
a/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/creator/GeoSpatialIndexCreator.java
+++
b/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/creator/GeoSpatialIndexCreator.java
@@ -32,14 +32,28 @@ public interface GeoSpatialIndexCreator extends
IndexCreator {
@Override
default void add(Object value, int dictId)
throws IOException {
- Geometry geometry;
+ add(toGeometry((byte[]) value));
+ }
+
+ /// Converts a serialized geometry value into a [Geometry], returning `null`
for values that cannot be decoded into
+ /// a geometry.
+ ///
+ /// The empty byte array is fast-pathed to `null` without attempting
deserialization: it is the BYTES default-null
+ /// value assigned to a derived geo column when it is added to the schema of
segments that predate its source data,
+ /// so on reload every such row would otherwise throw a
`BufferUnderflowException`. Skipping the deserialization
+ /// avoids one exception (and its stack-trace capture) per row for these
all-default segments. Any other undecodable
+ /// value is also mapped to `null`; [#add(Geometry)] then decides whether to
skip or fail it based on whether
+ /// `continueOnError` is enabled.
+ @Nullable
+ default Geometry toGeometry(@Nullable byte[] value) {
+ if (value == null || value.length == 0) {
+ return null;
+ }
try {
- geometry = deserialize((byte[]) value);
+ return deserialize(value);
} catch (Exception e) {
- // Swallow the exception and treat the geometry as null
- geometry = null;
+ return null;
}
- add(geometry);
}
@Override
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
