Jackie-Jiang commented on a change in pull request #6409:
URL: https://github.com/apache/incubator-pinot/pull/6409#discussion_r553082363
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/indexsegment/mutable/MutableSegmentImpl.java
##########
@@ -311,6 +315,13 @@ public long getLatestIngestionTimestamp() {
// Inverted index
RealtimeInvertedIndexReader invertedIndexReader =
invertedIndexColumns.contains(column) ? new
RealtimeInvertedIndexReader() : null;
+ RealtimeH3IndexReader h3IndexReader;
+ try {
+ h3IndexReader = h3IndexColumnMap.containsKey(column) ? new
RealtimeH3IndexReader(
+ new
H3IndexResolution(h3IndexColumnMap.get(column).getResolutions())) : null;
+ } catch (IOException e) {
+ throw new RuntimeException(String.format("Failed to initiate H3 index
reader for column %s", column));
Review comment:
Put the exception in the RuntimeException for easier debugging
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/LLRealtimeSegmentDataManager.java
##########
@@ -1187,6 +1189,8 @@ public
LLRealtimeSegmentDataManager(RealtimeSegmentZKMetadata segmentZKMetadata,
Set<String> textIndexColumns = indexLoadingConfig.getTextIndexColumns();
_textIndexColumns = new ArrayList<>(textIndexColumns);
+ _h3IndexColumns = indexLoadingConfig.getH3IndexColumns();
Review comment:
No need for this member variable, you may inline it
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/segment/index/datasource/BaseDataSource.java
##########
@@ -46,19 +48,29 @@ public BaseDataSource(DataSourceMetadata
dataSourceMetadata, ForwardIndexReader<
@Nullable Dictionary dictionary, @Nullable InvertedIndexReader<?>
invertedIndex,
@Nullable InvertedIndexReader<?> rangeIndex, @Nullable TextIndexReader
textIndex,
@Nullable TextIndexReader fstIndex, @Nullable JsonIndexReader jsonIndex,
@Nullable BloomFilterReader bloomFilter,
- @Nullable NullValueVectorReader nullValueVector) {
+ @Nullable NullValueVectorReader nullValueVector, @Nullable H3IndexReader
h3Index) {
_dataSourceMetadata = dataSourceMetadata;
_forwardIndex = forwardIndex;
_dictionary = dictionary;
_invertedIndex = invertedIndex;
_rangeIndex = rangeIndex;
+ _h3Index = h3Index;
_textIndex = textIndex;
_fstIndex = fstIndex;
_jsonIndex = jsonIndex;
_bloomFilter = bloomFilter;
_nullValueVector = nullValueVector;
}
+ public BaseDataSource(DataSourceMetadata dataSourceMetadata,
ForwardIndexReader<?> forwardIndex,
Review comment:
Remove this constructor. We cannot have a separate constructor per
nullable index. We can consider using builder, but out of the scope of this PR
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/segment/index/datasource/BaseDataSource.java
##########
@@ -46,19 +48,29 @@ public BaseDataSource(DataSourceMetadata
dataSourceMetadata, ForwardIndexReader<
@Nullable Dictionary dictionary, @Nullable InvertedIndexReader<?>
invertedIndex,
@Nullable InvertedIndexReader<?> rangeIndex, @Nullable TextIndexReader
textIndex,
@Nullable TextIndexReader fstIndex, @Nullable JsonIndexReader jsonIndex,
@Nullable BloomFilterReader bloomFilter,
- @Nullable NullValueVectorReader nullValueVector) {
+ @Nullable NullValueVectorReader nullValueVector, @Nullable H3IndexReader
h3Index) {
Review comment:
Put argument `h3Index` between `rangeIndex` and `textIndex`
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/segment/index/loader/SegmentPreProcessor.java
##########
@@ -115,7 +116,13 @@ public void process()
new RangeIndexHandler(_indexDir, _segmentMetadata,
_indexLoadingConfig, segmentWriter);
rangeIndexHandler.createRangeIndices();
- // Create text indices according to the index config.
Review comment:
Keep the original comment for text index?
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/geospatial/transform/function/StDistanceFunction.java
##########
@@ -121,7 +121,7 @@ private static void checkLongitude(double longitude) {
.checkArgument(longitude >= MIN_LONGITUDE && longitude <=
MAX_LONGITUDE, "Longitude must be between -180 and 180");
}
- private static double sphericalDistance(Geometry leftGeometry, Geometry
rightGeometry) {
+ public static double sphericalDistance(Geometry leftGeometry, Geometry
rightGeometry) {
Review comment:
I don't see any public usage of these 2 functions. If we need them
public, put it in a util class or as scalar function?
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/segment/index/datasource/BaseDataSource.java
##########
@@ -105,6 +117,10 @@ public JsonIndexReader getJsonIndex() {
return _jsonIndex;
}
+ public H3IndexReader getH3Index() {
Review comment:
Put this before text index for consistency
##########
File path:
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/FieldConfig.java
##########
@@ -67,7 +67,7 @@ public FieldConfig(@JsonProperty(value = "name", required =
true) String name,
// If null, there won't be any index
public enum IndexType {
- INVERTED, SORTED, TEXT, FST
+ INVERTED, SORTED, TEXT, FST, H3
Review comment:
High level question, is it possible to have more than one of these index
types on the same column?
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/common/DataSource.java
##########
@@ -22,12 +22,14 @@
import org.apache.pinot.core.segment.index.readers.BloomFilterReader;
import org.apache.pinot.core.segment.index.readers.Dictionary;
import org.apache.pinot.core.segment.index.readers.ForwardIndexReader;
+import org.apache.pinot.core.segment.index.readers.H3IndexReader;
import org.apache.pinot.core.segment.index.readers.InvertedIndexReader;
import org.apache.pinot.core.segment.index.readers.JsonIndexReader;
import org.apache.pinot.core.segment.index.readers.NullValueVectorReader;
import org.apache.pinot.core.segment.index.readers.TextIndexReader;
+
Review comment:
(nit) remove extra empty line
----------------------------------------------------------------
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.
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
