Jackie-Jiang commented on code in PR #18470:
URL: https://github.com/apache/pinot/pull/18470#discussion_r3223638576
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/fst/IFSTIndexType.java:
##########
@@ -96,13 +97,19 @@ protected ColumnConfigDeserializer<FstIndexConfig>
createDeserializerForLegacyCo
@Override
public FSTIndexCreator createIndexCreator(IndexCreationContext context,
FstIndexConfig indexConfig)
throws IOException {
- Preconditions.checkState(context.getFieldSpec().isSingleValueField(),
+ FieldSpec fieldSpec = context.getFieldSpec();
+ Preconditions.checkState(fieldSpec.isSingleValueField(),
"IFST index is currently only supported on single-value columns");
-
Preconditions.checkState(context.getFieldSpec().getDataType().getStoredType()
== FieldSpec.DataType.STRING,
+ Preconditions.checkState(fieldSpec.getDataType().getStoredType() ==
FieldSpec.DataType.STRING,
"IFST index is currently only supported on STRING type columns");
Preconditions.checkState(context.hasDictionary(),
"IFST index is currently only supported on dictionary-encoded
columns");
- return new LuceneIFSTIndexCreator(context);
+ ColumnStatistics columnStatistics = context.getColumnStatistics();
+ Preconditions.checkState(columnStatistics != null, "ColumnStatistics must
be provided");
+ Object uniqueValuesSet = columnStatistics.getUniqueValuesSet();
+ Preconditions.checkState(uniqueValuesSet instanceof String[], "Unique
values set must be of type String[]");
+ return new LuceneIFSTIndexCreator(context.getIndexDir(),
fieldSpec.getName(), context.getTableNameWithType(),
+ context.isContinueOnError(), (String[]) uniqueValuesSet);
Review Comment:
Good catch — `IFSTIndexHandler` calls this with a metadata-only context, so
the strict null-check broke reload. Aligned with `FSTIndexType` to pass `null`
when `ColumnStatistics` is absent; `LuceneIFSTIndexCreator` handles null
`sortedEntries` and the handler then streams dictionary values via `add()`.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/fst/FstIndexType.java:
##########
@@ -100,13 +101,17 @@ protected ColumnConfigDeserializer<FstIndexConfig>
createDeserializerForLegacyCo
@Override
public FSTIndexCreator createIndexCreator(IndexCreationContext context,
FstIndexConfig indexConfig)
throws IOException {
- Preconditions.checkState(context.getFieldSpec().isSingleValueField(),
+ FieldSpec fieldSpec = context.getFieldSpec();
+ Preconditions.checkState(fieldSpec.isSingleValueField(),
"FST index is currently only supported on single-value columns");
-
Preconditions.checkState(context.getFieldSpec().getDataType().getStoredType()
== FieldSpec.DataType.STRING,
+ Preconditions.checkState(fieldSpec.getDataType().getStoredType() ==
FieldSpec.DataType.STRING,
"FST index is currently only supported on STRING type columns");
Preconditions.checkState(context.hasDictionary(),
"FST index is currently only supported on dictionary-encoded columns");
- return new LuceneFSTIndexCreator(context);
+ ColumnStatistics columnStatistics = context.getColumnStatistics();
+ String[] sortedEntries = columnStatistics != null ? (String[])
columnStatistics.getUniqueValuesSet() : null;
Review Comment:
The cast is gated by the upstream `getStoredType() == STRING` +
`hasDictionary()` checks, which is the documented contract for
`ColumnStatistics.getUniqueValuesSet()` on `STRING` dict columns. After fixing
`IFSTIndexType` to mirror this null-tolerant pattern, the two are aligned.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
