Normalize legacy FieldConfig indexes into indexes#18412
Normalize legacy FieldConfig indexes into indexes#18412xiangfu0 wants to merge 1 commit intoapache:masterfrom
Conversation
81e5521 to
c6440c8
Compare
c6440c8 to
53d023b
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #18412 +/- ##
==============================================
+ Coverage 63.40% 100.00% +36.59%
+ Complexity 1668 6 -1662
==============================================
Files 3252 3 -3249
Lines 198661 6 -198655
Branches 30770 0 -30770
==============================================
- Hits 125965 6 -125959
+ Misses 62632 0 -62632
+ Partials 10064 0 -10064
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
xiangfu0
left a comment
There was a problem hiding this comment.
Found one high-signal backward-compatibility issue; see inline comment.
| } | ||
|
|
||
| @Deprecated | ||
| @JsonIgnore |
There was a problem hiding this comment.
Adding @JsonIgnore here removes indexType / indexTypes from every FieldConfig serialized through controller APIs and TableConfig#toJsonString(). Those fields are already part of the public JSON contract, so older clients that still round-trip or inspect them will see a backward-incompatible response-shape change. Please keep emitting the legacy fields for now, even if you normalize internally to indexes, and deprecate them over a compatibility window.
Jackie-Jiang
left a comment
There was a problem hiding this comment.
We should remove "disabled": false. It is always implicit when an index is configured
| _compressionCodec = compressionCodec; | ||
| _timestampConfig = timestampConfig; | ||
| _properties = properties; | ||
| _indexes = indexes == null ? NullNode.getInstance() : indexes; | ||
| _indexes = normalizeIndexes(name, indexType, indexTypes, indexes); |
There was a problem hiding this comment.
Normalizing in constructor could be potentially expensive. We load TableConfig everywhere, but only very few uses FieldConfig. Normalizing during usage should be better.
For new/updated table configs, we can normalize before writing to ZK
Summary
_indexTypesas storedFieldConfigstate and normalize legacyindexType/indexTypesinto genericindexesentries at construction timeFieldConfigUser Manual
Pinot still accepts legacy
fieldConfigListentries that useindexTypeorindexTypes.After this change:
indexTypeandindexTypesare converted intoindexesduring deserializationFieldConfigonly records generic enabled index membership inindexes; each index type remains responsible for interpreting any legacy per-index propertiesindexes; Pinot no longer writes backindexTypeorindexTypesgetIndexType()andgetIndexTypes()continue to work as derived compatibility accessors backed by normalizedindexesSample Table Config
Legacy input:
{ "fieldConfigList": [ { "name": "message", "encodingType": "RAW", "indexType": "TEXT", "properties": { "stopWordInclude": "pinot,apache", "enableQueryCacheForTextIndex": "true" } }, { "name": "cityId", "indexTypes": ["INVERTED", "RANGE"] } ] }Normalized form written back by Pinot:
{ "fieldConfigList": [ { "name": "message", "encodingType": "RAW", "indexes": { "text": { "disabled": false, "queryCache": true, "stopWordsInclude": ["pinot", "apache"] } } }, { "name": "cityId", "encodingType": "DICTIONARY", "indexes": { "inverted": { "disabled": false }, "range": { "disabled": false } } } ] }Testing
./mvnw spotless:apply -pl pinot-spi,pinot-segment-spi,pinot-common,pinot-segment-local -am./mvnw checkstyle:check -pl pinot-spi,pinot-segment-spi,pinot-common,pinot-segment-local -am./mvnw license:format -pl pinot-spi,pinot-segment-spi,pinot-common,pinot-segment-local -am./mvnw license:check -pl pinot-spi,pinot-segment-spi,pinot-common,pinot-segment-local -am./mvnw -pl pinot-common,pinot-segment-local,pinot-core -am -Dtest=TableConfigSerDeUtilsTest,TextIndexTest,RangeIndexTest,InvertedIndexTypeTest,H3IndexTest,VectorIndexTest,JsonIndexTest,FstIndexTypeTest,TextSearchQueriesTest,H3IndexQueriesTest -Dsurefire.failIfNoSpecifiedTests=false test