xiangfu0 commented on code in PR #17998:
URL: https://github.com/apache/pinot/pull/17998#discussion_r3005975113
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/fst/FstIndexTypeTest.java:
##########
@@ -66,35 +68,25 @@ public void oldFieldConfigFstNoType()
+ " \"indexTypes\" : [\"FST\"]\n"
+ " }");
- assertEquals(new FstIndexConfig(null));
+ assertEquals(new FstIndexConfig());
}
@Test
- public void oldFieldConfigFstExplicitLuceneType()
+ public void newNativeTypeIsIgnored()
Review Comment:
Removed the dead compatibility tests (`newNativeTypeIsIgnored`,
`newNativeTypeStillValid`, `newLuceneTypeIsIgnored`) since `FstIndexConfig` no
longer has a `type` field and these were only verifying that Jackson ignores
unknown JSON properties.
##########
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:
Since `FSTType` has been removed and `IndexingConfig.getFSTIndexType()` no
longer exists, legacy configs with `fstIndexType=NATIVE` will be silently
ignored by Jackson deserialization. The `FSTIndexHandler` detects and replaces
legacy native FST indexes on segment reload, which is the migration safety net.
##########
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:
Same approach as FST: the legacy native text config properties
(`fst_type=native`) are now silently ignored during deserialization, and the
`TextIndexHandler` detects and replaces legacy native text indexes on segment
reload.
--
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]
