gortiz commented on code in PR #16002:
URL: https://github.com/apache/pinot/pull/16002#discussion_r2180025998
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/invertedindex/RangeIndexHandler.java:
##########
@@ -256,10 +256,13 @@ private void
handleNonDictionaryBasedColumn(SegmentDirectory.Writer segmentWrite
private CombinedInvertedIndexCreator newRangeIndexCreator(ColumnMetadata
columnMetadata)
throws Exception {
File indexDir = _segmentDirectory.getSegmentMetadata().getIndexDir();
- IndexCreationContext context = IndexCreationContext.builder()
+ IndexCreationContext.Builder builder = IndexCreationContext.builder()
.withIndexDir(indexDir)
- .withColumnMetadata(columnMetadata)
- .build();
+ .withColumnMetadata(columnMetadata);
+ if (_tableConfig != null) {
+ builder.withTableNameWithType(_tableConfig.getTableName());
+ }
Review Comment:
This pattern is repeated _a lot_ in the code... shouldn't be better to add a
`withTableNameWithType(TableConfig)` method in `IndexCreationContext.Builder`?
In that method we could check nullability and set tableNameWithType in case
table config is not null
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/inv/geospatial/BaseH3IndexCreator.java:
##########
@@ -83,8 +88,9 @@ public abstract class BaseH3IndexCreator implements
GeoSpatialIndexCreator {
int _nextDocId;
- BaseH3IndexCreator(File indexDir, String columnName, H3IndexResolution
resolution)
+ BaseH3IndexCreator(File indexDir, String columnName, String
tableNameWithType, H3IndexResolution resolution)
Review Comment:
If tableNameWithType can be null, shouldn't be annotated as such?
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/H3IndexTest.java:
##########
@@ -126,6 +126,35 @@ public void testH3Index()
}
}
+ @Test
+ public void testSkipNullOrInvalidGeometry()
+ throws Exception {
+ String columnName = "skipInvalid";
+ int res = 5;
+ H3IndexResolution resolution = new
H3IndexResolution(Collections.singletonList(res));
+
+ try (GeoSpatialIndexCreator creator = new OnHeapH3IndexCreator(TEMP_DIR,
columnName, "myTable_OFFLINE",
+ resolution)) {
+ Point point = GeometryUtils.GEOMETRY_FACTORY.createPoint(new
Coordinate(10, 20));
+ creator.add(point);
+
+ // Invalid serialized bytes should be skipped without throwing exception
+ creator.add(new byte[]{1, 2, 3}, -1);
+
+ // Explicit null geometry should also be skipped
+ creator.add(null);
+
+ creator.seal();
+ }
+
+ File indexFile = new File(TEMP_DIR, columnName +
V1Constants.Indexes.H3_INDEX_FILE_EXTENSION);
+ try (PinotDataBuffer buffer =
PinotDataBuffer.mapReadOnlyBigEndianFile(indexFile);
+ H3IndexReader reader = new ImmutableH3IndexReader(buffer)) {
+ long h3Id = H3Utils.H3_CORE.latLngToCell(20, 10, res);
+ Assert.assertEquals(reader.getDocIds(h3Id).getCardinality(), 1);
+ }
+ }
Review Comment:
Ideally we should have one test for null and another for invalid. That way
if one or the other fail, we could find the problem faster
--
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: commits-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
