xiangfu0 commented on code in PR #17998:
URL: https://github.com/apache/pinot/pull/17998#discussion_r3005954670
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/fst/FstIndexType.java:
##########
@@ -98,11 +93,8 @@ public String getPrettyName() {
@Override
protected ColumnConfigDeserializer<FstIndexConfig>
createDeserializerForLegacyConfigs() {
- return IndexConfigDeserializer.fromIndexTypes(FieldConfig.IndexType.FST,
(tableConfig, fieldConfig) -> {
- IndexingConfig indexingConfig = tableConfig.getIndexingConfig();
- FSTType fstIndexType = indexingConfig != null ?
indexingConfig.getFSTIndexType() : null;
- return new FstIndexConfig(fstIndexType);
- });
+ return IndexConfigDeserializer.fromIndexTypes(FieldConfig.IndexType.FST,
+ (tableConfig, fieldConfig) -> new FstIndexConfig(false));
Review Comment:
High: same compatibility problem for FST. This deserializer now treats
legacy/native FST configs as plain enabled FST, and `createIndexCreator()`
always emits Lucene. During a rolling upgrade the identical table config will
still build native FST on old servers but Lucene FST on upgraded servers. That
means replica reloads/rebuilds are no longer deterministic for the table, which
violates our mixed-version compatibility bar. I think the native selector needs
to be rejected up front rather than silently ignored.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/text/TextIndexType.java:
##########
@@ -103,24 +96,16 @@ public String getPrettyName() {
@Override
protected ColumnConfigDeserializer<TextIndexConfig>
createDeserializerForLegacyConfigs() {
- return IndexConfigDeserializer.fromIndexTypes(FieldConfig.IndexType.TEXT,
(tableConfig, fieldConfig) -> {
- Map<String, String> properties = fieldConfig.getProperties();
- FSTType fstType = TextIndexUtils.isFstTypeNative(properties) ?
FSTType.NATIVE : FSTType.LUCENE;
- return new
TextIndexConfigBuilder(fstType).withProperties(properties).build();
- });
+ return IndexConfigDeserializer.fromIndexTypes(FieldConfig.IndexType.TEXT,
+ (tableConfig, fieldConfig) -> new
TextIndexConfigBuilder().withProperties(fieldConfig.getProperties()).build());
Review Comment:
High: this silently reinterprets legacy native text configs as Lucene
instead of rejecting them. `createDeserializerForLegacyConfigs()` now drops
`fstType=native`, while `createIndexCreator()` and `createMutableIndex()`
unconditionally build Lucene. In a rolling upgrade, the same table config will
keep producing `NativeMutableTextIndex` on old servers and
`RealtimeLuceneTextIndex` on upgraded servers, so query behavior diverges
across replicas (`TEXT_CONTAINS` only exists on the old native path;
`TEXT_MATCH` only works on Lucene). We need controller/table-config validation
to fail fast on legacy native text configs before any server starts rebuilding
with a different implementation.
--
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]
