Jackie-Jiang commented on a change in pull request #7729:
URL: https://github.com/apache/pinot/pull/7729#discussion_r747004938
##########
File path:
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/SegmentColumnarIndexCreator.java
##########
@@ -266,8 +268,17 @@ public void init(SegmentGeneratorConfig
segmentCreationSpec, SegmentIndexCreatio
"FST index is currently only supported on STRING type columns");
Preconditions.checkState(dictEnabledColumn,
"FST index is currently only supported on dictionary-encoded
columns");
- _fstIndexCreatorMap.put(columnName, new
LuceneFSTIndexCreator(_indexDir, columnName,
- (String[]) indexCreationInfo.getSortedUniqueElementsArray()));
+ TextIndexCreator textIndexCreator;
+ if (_config.getFSTIndexType() != null
+ && _config.getFSTIndexType() == FSTType.NATIVE) {
Review comment:
(minor) The first check is redundant
```suggestion
if (_config.getFSTIndexType() == FSTType.NATIVE) {
```
##########
File path:
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/nativefst/NativeFSTIndexCreator.java
##########
@@ -47,7 +47,7 @@
* @throws IOException
*/
public NativeFSTIndexCreator(File indexDir, String columnName, String[]
sortedEntries) {
- _fstIndexFile = new File(indexDir, columnName +
V1Constants.Indexes.NATIVE_FST_INDEX_FILE_EXTENSION);
+ _fstIndexFile = new File(indexDir, columnName +
V1Constants.Indexes.FST_INDEX_FILE_EXTENSION);
Review comment:
(minor) Let's remove `NATIVE_FST_INDEX_FILE_EXTENSION` from
`V1Constants.Indexes` since we have decided to use the same file extension.
##########
File path:
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/column/PhysicalColumnIndexContainer.java
##########
@@ -174,7 +175,14 @@ public
PhysicalColumnIndexContainer(SegmentDirectory.Reader segmentReader, Colum
}
if (loadFSTIndex) {
- _fstIndex = new
LuceneFSTIndexReader(segmentReader.getIndexFor(columnName,
ColumnIndexType.FST_INDEX));
+ PinotDataBuffer buffer = segmentReader.getIndexFor(columnName,
ColumnIndexType.FST_INDEX);
+
+ if (org.apache.pinot.segment.local.utils.nativefst.FSTHeader
Review comment:
(minor) Import the class
##########
File path:
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/column/PhysicalColumnIndexContainer.java
##########
@@ -174,7 +175,14 @@ public
PhysicalColumnIndexContainer(SegmentDirectory.Reader segmentReader, Colum
}
if (loadFSTIndex) {
- _fstIndex = new
LuceneFSTIndexReader(segmentReader.getIndexFor(columnName,
ColumnIndexType.FST_INDEX));
+ PinotDataBuffer buffer = segmentReader.getIndexFor(columnName,
ColumnIndexType.FST_INDEX);
+
+ if (org.apache.pinot.segment.local.utils.nativefst.FSTHeader
+ .isValidFST(buffer.toDirectByteBuffer(0, (int) buffer.size()))) {
Review comment:
Can we directly read the first int and compare it with the magic header?
That way we can avoid creating a `ByteBuffer` out of the existing buffer. Also
in certain extreme case `buffer.size()` could overflow when casting to `int`.
##########
File path:
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/IndexLoadingConfig.java
##########
@@ -56,6 +57,7 @@
private Set<String> _invertedIndexColumns = new HashSet<>();
private Set<String> _rangeIndexColumns = new HashSet<>();
private int _rangeIndexVersion = IndexingConfig.DEFAULT_RANGE_INDEX_VERSION;
+ private FSTType _fstTypeForFSTIndex = FSTType.LUCENE;
Review comment:
Understood. Suggest making it consistent with the getter and setter, and
we can add another one for the text index FSTType if necessary in the future.
We might also decide to use the same one for both text and fst.
##########
File path:
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/IndexLoadingConfig.java
##########
@@ -289,6 +293,10 @@ public int getRangeIndexVersion() {
return _rangeIndexVersion;
}
+ public FSTType getFstIndexType() {
Review comment:
^^
##########
File path:
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/invertedindex/FSTIndexHandler.java
##########
@@ -119,7 +124,8 @@ private void createFSTIndexForColumn(ColumnMetadata
columnMetadata)
String segmentName = _segmentMetadata.getName();
String column = columnMetadata.getColumnName();
File inProgress = new File(_indexDir, column + ".fst.inprogress");
- File fstIndexFile = new File(_indexDir, column + FST_INDEX_FILE_EXTENSION);
+ String fileExtension = FST_INDEX_FILE_EXTENSION;
Review comment:
(minor) Redundant to put it as a local variable (worse readability imo)
--
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
