You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "KKcorps (via GitHub)" <gi...@apache.org> on 2025/06/05 01:04:10 UTC
[PR] Handle null Geometry gracefully In H3Index [pinot]
KKcorps opened a new pull request, #16002:
URL: https://github.com/apache/pinot/pull/16002
This pull request introduces enhancements to the H3 indexing functionality, focusing on improved error handling for invalid or null geometries and adding corresponding test coverage. The key changes include updates to the `BaseH3IndexCreator` and `GeoSpatialIndexCreator` classes, as well as the addition of a new test case to validate the behavior.
### Enhancements to error handling for geometries:
* [`BaseH3IndexCreator.java`](diffhunk://#diff-fc821d06c7c68283deebde35f8066ec59f1a7027a7e6ea011a45b6743858be70R109-R127): Modified the `deserialize` method to catch exceptions during deserialization and return `null` for invalid serialized geometries. Updated the `add` method to skip indexing for `null` geometries while incrementing the document ID and logging the failure using `ServerMetrics`.
* [`GeoSpatialIndexCreator.java`](diffhunk://#diff-e17cfcd9e4693b850863f267d618f8cb8804d64fe13e1ebd3787bb5c9b01b87dL38-R45): Updated the default `add` method to handle exceptions during deserialization gracefully by treating invalid geometries as `null` and passing them to the `add` method.
### Test coverage for new functionality:
* [`H3IndexTest.java`](diffhunk://#diff-0ea94f95fe6ca53944a51cccd85dca927ba21c670efcae330397f9d4d4c44ce7R126-R153): Added a new test case, `testSkipNullOrInvalidGeometry`, to verify that invalid serialized geometries and explicit `null` geometries are skipped without throwing exceptions. The test ensures that valid geometries are still indexed correctly.
### Dependency updates:
* [`BaseH3IndexCreator.java`](diffhunk://#diff-fc821d06c7c68283deebde35f8066ec59f1a7027a7e6ea011a45b6743858be70R33-R34): Added imports for `ServerMeter` and `ServerMetrics` to facilitate logging of indexing failures.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Handle null Geometry gracefully In H3Index [pinot]
Posted by "somandal (via GitHub)" <gi...@apache.org>.
somandal commented on code in PR #16002:
URL: https://github.com/apache/pinot/pull/16002#discussion_r2139082669
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/inv/geospatial/BaseH3IndexCreator.java:
##########
@@ -104,12 +106,25 @@ public abstract class BaseH3IndexCreator implements GeoSpatialIndexCreator {
@Override
public Geometry deserialize(byte[] bytes) {
- return GeometrySerializer.deserialize(bytes);
+ try {
+ return GeometrySerializer.deserialize(bytes);
+ } catch (Exception e) {
+ // For invalid serialized geometry, return null so that the doc can be skipped
+ return null;
+ }
}
@Override
public void add(Geometry geometry)
throws IOException {
+ if (geometry == null) {
+ ServerMetrics metrics = ServerMetrics.get();
+ if (metrics != null) {
+ metrics.addMeteredGlobalValue(ServerMeter.INDEXING_FAILURES, 1);
Review Comment:
Can the `defaultNullValue` also be passed in? For some indexes like `LuceneTextIndexCreator` we probably want to add the `defaultNullValue` rather than skipping because the IndexReader takes the numDocs as input (and for this one it seems to fail if the numDocs don't match what we actually added). This strategy might not work for all scenarios though
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Handle null Geometry gracefully In H3Index [pinot]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #16002:
URL: https://github.com/apache/pinot/pull/16002#discussion_r2127752443
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/creator/GeoSpatialIndexCreator.java:
##########
@@ -35,7 +35,14 @@ public interface GeoSpatialIndexCreator extends IndexCreator {
@Override
default void add(@Nonnull Object value, int dictId)
throws IOException {
- add(deserialize((byte[]) value));
+ Geometry geometry = null;
+ try {
+ geometry = deserialize((byte[]) value);
Review Comment:
Consider logging the caught exception (even at a debug level) in the try-catch block to facilitate troubleshooting while still treating the geometry as null.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Handle null Geometry gracefully In H3Index [pinot]
Posted by "somandal (via GitHub)" <gi...@apache.org>.
somandal commented on PR #16002:
URL: https://github.com/apache/pinot/pull/16002#issuecomment-3002281049
@Jackie-Jiang @KKcorps couldn't figure out a good way to push changes directly to this PR, so opened a new one: https://github.com/apache/pinot/pull/16197
please take a look, it addresses the review comments @Jackie-Jiang left
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Handle null Geometry gracefully In H3Index [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #16002:
URL: https://github.com/apache/pinot/pull/16002#discussion_r2181183713
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/creator/IndexCreationContext.java:
##########
@@ -308,14 +323,29 @@ final class Common implements IndexCreationContext {
private final boolean _realtimeConversion;
private final File _consumerDir;
private final int[] _immutableToMutableIdMap;
+ private final String _tableNameWithType;
+ @Deprecated
Review Comment:
We don't need to handle backward compatible here since this is an inner class
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/creator/IndexCreationContext.java:
##########
@@ -266,12 +271,22 @@ public Builder withImmutableToMutableIdMap(int[] immutableToMutableIdMap) {
return this;
}
+ public Builder withTableNameWithType(@Nullable String tableNameWithType) {
+ _tableNameWithType = tableNameWithType;
+ return this;
+ }
+
+ public Builder withTableNameWithType(@Nullable TableConfig tableConfig) {
Review Comment:
It is a little bit strange to pass `TableConfig` into `withTableNameWithType()`. Can you take a look at #16267 and see if the logic can be simplified with it?
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Handle null Geometry gracefully In H3Index [pinot]
Posted by "somandal (via GitHub)" <gi...@apache.org>.
somandal commented on code in PR #16002:
URL: https://github.com/apache/pinot/pull/16002#discussion_r2178584430
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/ForwardIndexHandler.java:
##########
@@ -477,6 +477,9 @@ private void rewriteForwardIndexForCompressionChange(String column, ColumnMetada
try (ForwardIndexReader<?> reader = ForwardIndexType.read(segmentWriter, columnMetadata)) {
IndexCreationContext.Builder builder =
IndexCreationContext.builder().withIndexDir(indexDir).withColumnMetadata(columnMetadata);
+ if (_tableConfig != null) {
Review Comment:
So the issue is that all the interfaces mark the `tableConfig` as `@Nullable`, e.g:
```
public BaseIndexHandler(SegmentDirectory segmentDirectory, Map<String, FieldIndexConfigs> fieldIndexConfigs,
@Nullable TableConfig tableConfig)
```
Similarly:
```
IndexHandler createIndexHandler(SegmentDirectory segmentDirectory, Map<String, FieldIndexConfigs> configsByCol,
@Nullable Schema schema, @Nullable TableConfig tableConfig);
```
And some code paths that are just meant to read and tests can set the `tableConfig` as null:
```
/**
* Loads the segment with empty schema and IndexLoadingConfig. This method is used to
* access the segment without modifying it, i.e. in read-only mode.
*/
public static ImmutableSegment load(File indexDir, ReadMode readMode)
throws Exception {
IndexLoadingConfig defaultIndexLoadingConfig = new IndexLoadingConfig();
defaultIndexLoadingConfig.setReadMode(readMode);
return load(indexDir, defaultIndexLoadingConfig, false);
}
/**
* NOTE: Can be used in production code when we want to load a segment as is without any modifications.
*/
public IndexLoadingConfig() {
this(null, null, null); // null tableConfig
}
public IndexLoadingConfig(@Nullable InstanceDataManagerConfig instanceDataManagerConfig,
@Nullable TableConfig tableConfig, @Nullable Schema schema) {
_instanceDataManagerConfig = instanceDataManagerConfig;
_tableConfig = tableConfig;
_schema = schema;
init();
}
```
So that's why I decided to add handling throughout to prevent weird issues with tests and all. I don't think for the code paths where we actually intend to create segments we'll come across a null tableConfig. Even the tableNameWithType null checks are mostly for this kind of compliance. Let me know if you think we should remove this null handling and try it out?
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Handle null Geometry gracefully In H3Index [pinot]
Posted by "somandal (via GitHub)" <gi...@apache.org>.
somandal commented on PR #16002:
URL: https://github.com/apache/pinot/pull/16002#issuecomment-3032487253
The compatibility check is expected to fail as based on discussion it is okay to add a new field to `IndexCreationContext::Common`'s constructor and not create a deprecated version. I have also marked it as private to prevent future compatibility failures. cc @Jackie-Jiang @gortiz
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Handle null Geometry gracefully In H3Index [pinot]
Posted by "somandal (via GitHub)" <gi...@apache.org>.
somandal commented on code in PR #16002:
URL: https://github.com/apache/pinot/pull/16002#discussion_r2180402949
##########
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:
good catch, updated
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Handle null Geometry gracefully In H3Index [pinot]
Posted by "somandal (via GitHub)" <gi...@apache.org>.
somandal commented on code in PR #16002:
URL: https://github.com/apache/pinot/pull/16002#discussion_r2165756966
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/inv/geospatial/BaseH3IndexCreator.java:
##########
@@ -104,12 +106,25 @@ public abstract class BaseH3IndexCreator implements GeoSpatialIndexCreator {
@Override
public Geometry deserialize(byte[] bytes) {
- return GeometrySerializer.deserialize(bytes);
+ try {
+ return GeometrySerializer.deserialize(bytes);
+ } catch (Exception e) {
+ // For invalid serialized geometry, return null so that the doc can be skipped
+ return null;
+ }
}
@Override
public void add(Geometry geometry)
throws IOException {
+ if (geometry == null) {
+ ServerMetrics metrics = ServerMetrics.get();
+ if (metrics != null) {
+ metrics.addMeteredGlobalValue(ServerMeter.INDEXING_FAILURES, 1);
+ }
+ _nextDocId++;
+ return;
+ }
Preconditions.checkState(geometry instanceof Point, "H3 index can only be applied to Point, got: %s",
Review Comment:
done - have not added a log though (as that would flood logs)
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/inv/geospatial/BaseH3IndexCreator.java:
##########
@@ -104,12 +106,25 @@ public abstract class BaseH3IndexCreator implements GeoSpatialIndexCreator {
@Override
public Geometry deserialize(byte[] bytes) {
- return GeometrySerializer.deserialize(bytes);
+ try {
+ return GeometrySerializer.deserialize(bytes);
+ } catch (Exception e) {
+ // For invalid serialized geometry, return null so that the doc can be skipped
+ return null;
+ }
}
@Override
public void add(Geometry geometry)
Review Comment:
done
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/inv/geospatial/BaseH3IndexCreator.java:
##########
@@ -104,12 +106,25 @@ public abstract class BaseH3IndexCreator implements GeoSpatialIndexCreator {
@Override
public Geometry deserialize(byte[] bytes) {
- return GeometrySerializer.deserialize(bytes);
+ try {
+ return GeometrySerializer.deserialize(bytes);
+ } catch (Exception e) {
+ // For invalid serialized geometry, return null so that the doc can be skipped
+ return null;
+ }
}
@Override
public void add(Geometry geometry)
throws IOException {
+ if (geometry == null) {
+ ServerMetrics metrics = ServerMetrics.get();
+ if (metrics != null) {
Review Comment:
done
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Handle null Geometry gracefully In H3Index [pinot]
Posted by "somandal (via GitHub)" <gi...@apache.org>.
somandal commented on PR #16002:
URL: https://github.com/apache/pinot/pull/16002#issuecomment-3003272826
@Jackie-Jiang @KKcorps the compatibility failure is due to adding the `tableNameWithType` to `IndexCreationContext::Common`. What's the recommendation here? Do we need to create a new constructor to add a new variable? We do want the `tableNameWithType` to be passed in though, right?
```
caused by these superclasses and interfaces are not reflected in the output.
***! MODIFIED CLASS: PUBLIC STATIC FINAL org.apache.pinot.segment.spi.creator.IndexCreationContext$Common (not serializable)
=== CLASS FILE FORMAT VERSION: 55.0 <- 55.0
=== UNCHANGED SUPERCLASS: java.lang.Object (<- java.lang.Object)
---! REMOVED CONSTRUCTOR: PUBLIC(-) IndexCreationContext$Common(java.io.File, int, int, int, boolean, org.apache.pinot.spi.data.FieldSpec, boolean, int, int, int, boolean, java.lang.Comparable<?>, java.lang.Comparable<?>, boolean, java.lang.Object, boolean, boolean, boolean, org.apache.pinot.segment.spi.creator.ColumnStatistics, boolean, java.io.File, int[])
E: There is at least one incompatibility: org.apache.pinot.segment.spi.creator.IndexCreationContext$Common.IndexCreationContext$Common(java.io.File,int,int,int,boolean,org.apache.pinot.spi.data.FieldSpec,boolean,int,int,int,boolean,java.lang.Comparable,java.lang.Comparable,boolean,java.lang.Object,boolean,boolean,boolean,org.apache.pinot.segment.spi.creator.ColumnStatistics,boolean,java.io.File,int[]):CONSTRUCTOR_REMOVED
```
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Handle null Geometry gracefully In H3Index [pinot]
Posted by "somandal (via GitHub)" <gi...@apache.org>.
somandal commented on code in PR #16002:
URL: https://github.com/apache/pinot/pull/16002#discussion_r2178586213
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/inv/geospatial/BaseH3IndexCreator.java:
##########
@@ -108,10 +114,19 @@ public Geometry deserialize(byte[] bytes) {
}
@Override
- public void add(Geometry geometry)
+ public void add(@Nullable Geometry geometry)
throws IOException {
- Preconditions.checkState(geometry instanceof Point, "H3 index can only be applied to Point, got: %s",
- geometry.getGeometryType());
+ if (geometry == null || !(geometry instanceof Point)) {
+ if (_tableNameWithType != null) {
+ String metricKeyName =
+ _tableNameWithType + "-" + H3IndexType.INDEX_DISPLAY_NAME.toUpperCase(Locale.US) + "-indexingError";
+ ServerMetrics.get().addMeteredTableValue(metricKeyName, ServerMeter.INDEXING_FAILURES, 1);
+ } else {
Review Comment:
I've mostly added this due to cases where tableConfig might be null, etc:
```
if (_tableConfig != null) {
builder.withTableNameWithType(_tableConfig.getTableName());
}
```
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Handle null Geometry gracefully In H3Index [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #16002:
URL: https://github.com/apache/pinot/pull/16002#discussion_r2178427998
##########
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, H3IndexResolution resolution, String tableNameWithType)
Review Comment:
Same for other constructors
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/inv/geospatial/BaseH3IndexCreator.java:
##########
@@ -108,10 +114,19 @@ public Geometry deserialize(byte[] bytes) {
}
@Override
- public void add(Geometry geometry)
+ public void add(@Nullable Geometry geometry)
throws IOException {
- Preconditions.checkState(geometry instanceof Point, "H3 index can only be applied to Point, got: %s",
- geometry.getGeometryType());
+ if (geometry == null || !(geometry instanceof Point)) {
+ if (_tableNameWithType != null) {
+ String metricKeyName =
+ _tableNameWithType + "-" + H3IndexType.INDEX_DISPLAY_NAME.toUpperCase(Locale.US) + "-indexingError";
+ ServerMetrics.get().addMeteredTableValue(metricKeyName, ServerMeter.INDEXING_FAILURES, 1);
+ } else {
Review Comment:
Can `_tableNameWithType` ever be `null`?
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/ForwardIndexHandler.java:
##########
@@ -477,6 +477,9 @@ private void rewriteForwardIndexForCompressionChange(String column, ColumnMetada
try (ForwardIndexReader<?> reader = ForwardIndexType.read(segmentWriter, columnMetadata)) {
IndexCreationContext.Builder builder =
IndexCreationContext.builder().withIndexDir(indexDir).withColumnMetadata(columnMetadata);
+ if (_tableConfig != null) {
Review Comment:
We shouldn't create index handler when table config is unavailable. If `_tableConfig` can be null here, we should check the code path and prevent index handler being created
##########
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, H3IndexResolution resolution, String tableNameWithType)
Review Comment:
(minor) Put `tableNameWithType` in front of `columnName`
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Handle null Geometry gracefully In H3Index [pinot]
Posted by "somandal (via GitHub)" <gi...@apache.org>.
somandal commented on code in PR #16002:
URL: https://github.com/apache/pinot/pull/16002#discussion_r2182934378
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/creator/IndexCreationContext.java:
##########
@@ -308,14 +323,29 @@ final class Common implements IndexCreationContext {
private final boolean _realtimeConversion;
private final File _consumerDir;
private final int[] _immutableToMutableIdMap;
+ private final String _tableNameWithType;
+ @Deprecated
Review Comment:
done, marked it as private based on @gortiz's recommendation as well so that in the future we don't have compatibility failures
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/creator/IndexCreationContext.java:
##########
@@ -266,12 +271,22 @@ public Builder withImmutableToMutableIdMap(int[] immutableToMutableIdMap) {
return this;
}
+ public Builder withTableNameWithType(@Nullable String tableNameWithType) {
+ _tableNameWithType = tableNameWithType;
+ return this;
+ }
+
+ public Builder withTableNameWithType(@Nullable TableConfig tableConfig) {
Review Comment:
done - removed and updated the code
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Handle null Geometry gracefully In H3Index [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #16002:
URL: https://github.com/apache/pinot/pull/16002#issuecomment-2942452679
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/16002?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `6.66667%` with `14 lines` in your changes missing coverage. Please review.
> Project coverage is 56.48%. Comparing base [(`1a476de`)](https://app.codecov.io/gh/apache/pinot/commit/1a476dea72ebf1c15e14ded28ae60c385f71b2c3?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`38a897a`)](https://app.codecov.io/gh/apache/pinot/commit/38a897a1d39d42a65255eda080c8675d83c172fb?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 201 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/16002?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...reator/impl/inv/geospatial/BaseH3IndexCreator.java](https://app.codecov.io/gh/apache/pinot/pull/16002?src=pr&el=tree&filepath=pinot-segment-local%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Flocal%2Fsegment%2Fcreator%2Fimpl%2Finv%2Fgeospatial%2FBaseH3IndexCreator.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2NyZWF0b3IvaW1wbC9pbnYvZ2Vvc3BhdGlhbC9CYXNlSDNJbmRleENyZWF0b3IuamF2YQ==) | 11.11% | [7 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/pinot/pull/16002?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...ment/spi/index/creator/GeoSpatialIndexCreator.java](https://app.codecov.io/gh/apache/pinot/pull/16002?src=pr&el=tree&filepath=pinot-segment-spi%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Fspi%2Findex%2Fcreator%2FGeoSpatialIndexCreator.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1zcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3NlZ21lbnQvc3BpL2luZGV4L2NyZWF0b3IvR2VvU3BhdGlhbEluZGV4Q3JlYXRvci5qYXZh) | 0.00% | [6 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/16002?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (1a476de) and HEAD (38a897a). Click for more details.
>
> <details><summary>HEAD has 68 uploads less than BASE</summary>
>
>| Flag | BASE (1a476de) | HEAD (38a897a) |
>|------|------|------|
>|java-21|10|1|
>|unittests|6|1|
>|unittests1|3|1|
>|skip-bytebuffers-true|4|0|
>|temurin|15|1|
>|unittests2|3|0|
>|skip-bytebuffers-false|8|0|
>|java-11|5|0|
>|integration|9|0|
>|integration2|3|0|
>|integration1|3|0|
>|custom-integration1|3|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #16002 +/- ##
============================================
- Coverage 62.90% 56.48% -6.42%
+ Complexity 1386 690 -696
============================================
Files 2867 2274 -593
Lines 163354 124240 -39114
Branches 24952 19800 -5152
============================================
- Hits 102755 70181 -32574
+ Misses 52847 48252 -4595
+ Partials 7752 5807 -1945
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/16002/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/16002/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/16002/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/16002/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/16002/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/16002/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/16002/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.48% <6.66%> (-6.34%)` | :arrow_down: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/16002/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/16002/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/16002/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.48% <6.66%> (-6.42%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/16002/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.48% <6.66%> (-6.42%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/16002/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.48% <6.66%> (+0.66%)` | :arrow_up: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/16002/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/16002?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Handle null Geometry gracefully In H3Index [pinot]
Posted by "somandal (via GitHub)" <gi...@apache.org>.
somandal commented on code in PR #16002:
URL: https://github.com/apache/pinot/pull/16002#discussion_r2178628191
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/creator/GeoSpatialIndexCreator.java:
##########
@@ -35,7 +35,14 @@ public interface GeoSpatialIndexCreator extends IndexCreator {
@Override
default void add(@Nonnull Object value, int dictId)
throws IOException {
- add(deserialize((byte[]) value));
+ Geometry geometry = null;
+ try {
+ geometry = deserialize((byte[]) value);
Review Comment:
this will result in too much logging, so ignoring this comment
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Handle null Geometry gracefully In H3Index [pinot]
Posted by "somandal (via GitHub)" <gi...@apache.org>.
somandal commented on PR #16002:
URL: https://github.com/apache/pinot/pull/16002#issuecomment-3005243962
> As you said, in order to solve the compatibility failure we need to add deprecated a constructor without the new param. But that may be problematic if the new attribute is not nullable (which I'm assume it is not because it is not marked as such, but looks like it is set on an optional builder method, so I'm not sure).
>
> Alternatively, we can break backward compatibility here. But in that case I would suggest to make the single constructor of Common private, making it clear that the only correct way to instantiate it is by using the Builder. That is obviously a breaking change, but will prevent future breaking changes if we need to add new attributes in the future (because changing a private constructor is not a breaking change)
Thanks for the suggestions @gortiz! I've gone ahead and added a deprecated constructor for now and marked the field as Nullable for now.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Handle null Geometry gracefully In H3Index [pinot]
Posted by "somandal (via GitHub)" <gi...@apache.org>.
somandal commented on code in PR #16002:
URL: https://github.com/apache/pinot/pull/16002#discussion_r2180403748
##########
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:
makes sense, updated
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Handle null Geometry gracefully In H3Index [pinot]
Posted by "somandal (via GitHub)" <gi...@apache.org>.
somandal commented on code in PR #16002:
URL: https://github.com/apache/pinot/pull/16002#discussion_r2178576433
##########
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, H3IndexResolution resolution, String tableNameWithType)
Review Comment:
done
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Handle null Geometry gracefully In H3Index [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on PR #16002:
URL: https://github.com/apache/pinot/pull/16002#issuecomment-3003545061
As you said, in order to solve the compatibility failure we need to add deprecated a constructor without the new param. But that may be problematic if the new attribute is not nullable (which I'm assume it is not because it is not marked as such, but looks like it is set on an optional builder method, so I'm not sure).
Alternatively, we can break backward compatibility here. But in that case I would suggest to make the single constructor of Common private, making it clear that the only correct way to instantiate it is by using the Builder. That is obviously a breaking change, but will prevent future breaking changes if we need to add new attributes in the future (because changing a private constructor is not a breaking change)
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Handle null Geometry gracefully In H3Index [pinot]
Posted by "somandal (via GitHub)" <gi...@apache.org>.
somandal commented on code in PR #16002:
URL: https://github.com/apache/pinot/pull/16002#discussion_r2139082669
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/inv/geospatial/BaseH3IndexCreator.java:
##########
@@ -104,12 +106,25 @@ public abstract class BaseH3IndexCreator implements GeoSpatialIndexCreator {
@Override
public Geometry deserialize(byte[] bytes) {
- return GeometrySerializer.deserialize(bytes);
+ try {
+ return GeometrySerializer.deserialize(bytes);
+ } catch (Exception e) {
+ // For invalid serialized geometry, return null so that the doc can be skipped
+ return null;
+ }
}
@Override
public void add(Geometry geometry)
throws IOException {
+ if (geometry == null) {
+ ServerMetrics metrics = ServerMetrics.get();
+ if (metrics != null) {
+ metrics.addMeteredGlobalValue(ServerMeter.INDEXING_FAILURES, 1);
Review Comment:
~~Can the `defaultNullValue` also be passed in? For some indexes like `LuceneTextIndexCreator` we probably want to add the `defaultNullValue` rather than skipping because the IndexReader takes the numDocs as input (and for this one it seems to fail if the numDocs don't match what we actually added). This strategy might not work for all scenarios though~~
Never mind the above - looks like the context contains the FieldSpec, which should have the defaultNullValue set - I'm guessing this can be used as is (will try it out)
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Handle null Geometry gracefully In H3Index [pinot]
Posted by "somandal (via GitHub)" <gi...@apache.org>.
somandal merged PR #16002:
URL: https://github.com/apache/pinot/pull/16002
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Handle null Geometry gracefully In H3Index [pinot]
Posted by "somandal (via GitHub)" <gi...@apache.org>.
somandal commented on code in PR #16002:
URL: https://github.com/apache/pinot/pull/16002#discussion_r2180403330
##########
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:
thanks for the suggestion, updated
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Handle null Geometry gracefully In H3Index [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Handle null Geometry gracefully In H3Index [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #16002:
URL: https://github.com/apache/pinot/pull/16002#discussion_r2136632610
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/inv/geospatial/BaseH3IndexCreator.java:
##########
@@ -104,12 +106,25 @@ public abstract class BaseH3IndexCreator implements GeoSpatialIndexCreator {
@Override
public Geometry deserialize(byte[] bytes) {
- return GeometrySerializer.deserialize(bytes);
+ try {
+ return GeometrySerializer.deserialize(bytes);
+ } catch (Exception e) {
+ // For invalid serialized geometry, return null so that the doc can be skipped
+ return null;
+ }
}
@Override
public void add(Geometry geometry)
Review Comment:
Add `@Nullable` to the argument, same for interface
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/inv/geospatial/BaseH3IndexCreator.java:
##########
@@ -104,12 +106,25 @@ public abstract class BaseH3IndexCreator implements GeoSpatialIndexCreator {
@Override
public Geometry deserialize(byte[] bytes) {
- return GeometrySerializer.deserialize(bytes);
+ try {
+ return GeometrySerializer.deserialize(bytes);
+ } catch (Exception e) {
+ // For invalid serialized geometry, return null so that the doc can be skipped
+ return null;
+ }
}
@Override
public void add(Geometry geometry)
throws IOException {
+ if (geometry == null) {
+ ServerMetrics metrics = ServerMetrics.get();
+ if (metrics != null) {
+ metrics.addMeteredGlobalValue(ServerMeter.INDEXING_FAILURES, 1);
+ }
+ _nextDocId++;
+ return;
+ }
Preconditions.checkState(geometry instanceof Point, "H3 index can only be applied to Point, got: %s",
Review Comment:
Let's change this `checkState` into a if check. Currently the code cannot handle bytes representing anything other than `Point`.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/inv/geospatial/BaseH3IndexCreator.java:
##########
@@ -104,12 +106,25 @@ public abstract class BaseH3IndexCreator implements GeoSpatialIndexCreator {
@Override
public Geometry deserialize(byte[] bytes) {
- return GeometrySerializer.deserialize(bytes);
+ try {
+ return GeometrySerializer.deserialize(bytes);
Review Comment:
We are capturing the exception from the caller side, no need to do another try-catch here
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/inv/geospatial/BaseH3IndexCreator.java:
##########
@@ -104,12 +106,25 @@ public abstract class BaseH3IndexCreator implements GeoSpatialIndexCreator {
@Override
public Geometry deserialize(byte[] bytes) {
- return GeometrySerializer.deserialize(bytes);
+ try {
+ return GeometrySerializer.deserialize(bytes);
+ } catch (Exception e) {
+ // For invalid serialized geometry, return null so that the doc can be skipped
+ return null;
+ }
}
@Override
public void add(Geometry geometry)
throws IOException {
+ if (geometry == null) {
+ ServerMetrics metrics = ServerMetrics.get();
+ if (metrics != null) {
Review Comment:
(minor) This check is redundant
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/inv/geospatial/BaseH3IndexCreator.java:
##########
@@ -104,12 +106,25 @@ public abstract class BaseH3IndexCreator implements GeoSpatialIndexCreator {
@Override
public Geometry deserialize(byte[] bytes) {
- return GeometrySerializer.deserialize(bytes);
+ try {
+ return GeometrySerializer.deserialize(bytes);
+ } catch (Exception e) {
+ // For invalid serialized geometry, return null so that the doc can be skipped
+ return null;
+ }
}
@Override
public void add(Geometry geometry)
throws IOException {
+ if (geometry == null) {
+ ServerMetrics metrics = ServerMetrics.get();
+ if (metrics != null) {
+ metrics.addMeteredGlobalValue(ServerMeter.INDEXING_FAILURES, 1);
Review Comment:
Can we include the table name and index type similar to how this meter is emitted in `MutableSegmentImpl`? We should be able to include the table name within `IndexCreationContext` and pass it in
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Handle null Geometry gracefully In H3Index [pinot]
Posted by "somandal (via GitHub)" <gi...@apache.org>.
somandal commented on code in PR #16002:
URL: https://github.com/apache/pinot/pull/16002#discussion_r2165758499
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/inv/geospatial/BaseH3IndexCreator.java:
##########
@@ -104,12 +106,25 @@ public abstract class BaseH3IndexCreator implements GeoSpatialIndexCreator {
@Override
public Geometry deserialize(byte[] bytes) {
- return GeometrySerializer.deserialize(bytes);
+ try {
+ return GeometrySerializer.deserialize(bytes);
+ } catch (Exception e) {
+ // For invalid serialized geometry, return null so that the doc can be skipped
+ return null;
+ }
}
@Override
public void add(Geometry geometry)
throws IOException {
+ if (geometry == null) {
+ ServerMetrics metrics = ServerMetrics.get();
+ if (metrics != null) {
+ metrics.addMeteredGlobalValue(ServerMeter.INDEXING_FAILURES, 1);
Review Comment:
done - please take a look. this is going to cause compatibility issues since a new parameter `tableNameWithType` is added to the constructor
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/inv/geospatial/BaseH3IndexCreator.java:
##########
@@ -104,12 +106,25 @@ public abstract class BaseH3IndexCreator implements GeoSpatialIndexCreator {
@Override
public Geometry deserialize(byte[] bytes) {
- return GeometrySerializer.deserialize(bytes);
+ try {
+ return GeometrySerializer.deserialize(bytes);
Review Comment:
done
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org