KKcorps commented on code in PR #19032:
URL: https://github.com/apache/pinot/pull/19032#discussion_r3680837764
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/invertedindex/H3IndexHandler.java:
##########
@@ -215,7 +214,10 @@ private void
handleDictionaryBasedColumn(SegmentDirectory.Writer segmentWriter,
int numDocs = columnMetadata.getTotalDocs();
for (int i = 0; i < numDocs; i++) {
int dictId = forwardIndexReader.getDictId(i, readerContext);
-
h3IndexCreator.add(GeometrySerializer.deserialize(dictionary.getBytesValue(dictId)));
+ // 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);
Review Comment:
**Critical — null rows can match negative H3 filters**
This change lets the index skip an empty geometry and continue. The skipped
row still keeps its document ID, but it has no H3 posting. When null handling
is enabled, the same row is also marked as null in the null-value vector.
The H3 filter operators do not read that null vector.
`H3InclusionIndexFilterOperator` and `H3IndexFilterOperator` use
`super(numDocs, false)` and build complements or match-all results across every
document ID. Because the skipped null rows have no posting, they can be added
to results for `ST_Contains/Within(...)=false`, lower-bound-only distance
filters, or match-all distance ranges. That is a silent wrong result.
Please limit complement and match-all results to non-null document IDs when
query null handling is enabled. Please also add a query-level test that
compares the H3-index path with the scan path for all-null rows.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/invertedindex/H3IndexHandler.java:
##########
@@ -234,7 +236,8 @@ private void
handleNonDictionaryBasedColumn(SegmentDirectory.Writer segmentWrite
GeoSpatialIndexCreator h3IndexCreator =
StandardIndexes.h3().createIndexCreator(context, config)) {
int numDocs = columnMetadata.getTotalDocs();
for (int i = 0; i < numDocs; i++) {
-
h3IndexCreator.add(GeometrySerializer.deserialize(forwardIndexReader.getBytes(i,
readerContext)));
+ // See handleDictionaryBasedColumn: add(value, dictId) tolerates
empty/default geometry values on reload.
+ h3IndexCreator.add(forwardIndexReader.getBytes(i, readerContext), -1);
Review Comment:
**Major — avoid one exception for every empty row**
In the old-segment case described by this PR, every row can contain the same
empty byte array. `add(value, dictId)` tries to deserialize each value, catches
a `BufferUnderflowException`, and updates the indexing-error metric for each
skipped row. A segment with millions of rows can therefore create millions of
exceptions and metric updates, causing heavy CPU and garbage-collection work
during reload. The dictionary loop has the same problem and may repeat this
work for one dictionary ID.
Please fast-path empty byte arrays before deserialization. For dictionary
columns, cache the decoded or invalid result by dictionary ID. Keep advancing
one document ID for each skipped row, while avoiding repeated exception and
metric-key work. A large all-default test or benchmark would help protect this
path.
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/SegmentPreProcessorTest.java:
##########
@@ -1665,6 +1671,39 @@ public void testV3CleanupH3AndTextIndices()
assertEquals(singleFileIndex.length(), initFileSize);
}
+ /// Regression test for the H3 index builder crashing a segment on
empty/default geometry values.
+ ///
+ /// 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)
+ 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.
+ runPreProcessor(_newColumnsSchemaWithH3EmptyDefault);
+ SegmentMetadataImpl segmentMetadata = new SegmentMetadataImpl(INDEX_DIR);
+ assertNotNull(segmentMetadata.getColumnMetadataFor("newH3Col"));
+
+ // Build the H3 index over the empty/default 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,
Review Comment:
**Minor — also test the raw, no-dictionary path**
The production change updates both the dictionary and raw reload paths, but
this test always uses `EncodingType.DICTIONARY`. This means the changed
`forwardIndexReader.getBytes(...)` path is not covered.
Please extend the test to cover `V1/V3 × DICTIONARY/RAW`. For the raw case,
add `newH3Col` to `_noDictionaryColumns`, use `EncodingType.RAW`, assert that
the column has no dictionary, and verify that the H3 index exists and can be
opened.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
