[GitHub] [pinot] Jackie-Jiang commented on a change in pull request #7684: Support FST Index using Native FST Library

[GitBox <gi...@apache.org>]

Jackie-Jiang commented on a change in pull request #7684:
URL: https://github.com/apache/pinot/pull/7684#discussion_r744071304



##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/config/table/FieldConfig.java
##########
@@ -85,7 +85,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, H3, JSON, RANGE
+    INVERTED, SORTED, TEXT, FST, NATIVE_FST, H3, JSON, RANGE

Review comment:
       Suggest treating native FST as FST, and add an enum in `IndexingConfig` for the FST version (`LUCENE` or `NATIVE`)

##########
File path: pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/datasource/DataSource.java
##########
@@ -76,6 +76,12 @@
   @Nullable
   TextIndexReader getFSTIndex();
 
+  /**
+   * Returns the Native FST index for the column if exists, or {@code null} if not.
+   */
+  @Nullable
+  TextIndexReader getNativeFSTIndex();

Review comment:
       Since native FST and lucene FST are both FST, just different implementation, and I don't think they should co-exist, let's reuse the same `getFSTIndex()` for both of them. We shouldn't need to change any code on the query execution side. The loader should be able to tell which version of FST exists and load it as the FST index.

##########
File path: pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/creator/SegmentGeneratorConfig.java
##########
@@ -73,6 +73,7 @@
   private final List<String> _invertedIndexCreationColumns = new ArrayList<>();
   private final List<String> _textIndexCreationColumns = new ArrayList<>();
   private final List<String> _fstIndexCreationColumns = new ArrayList<>();
+  private final List<String> _nativeFSTIndexCreationColumns = new ArrayList<>();

Review comment:
       I'd suggest adding an enum for the FST type (can be `LUCENE` or `NATIVE` instead of keeping separate lists




-- 
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