xiangfu0 commented on code in PR #18668:
URL: https://github.com/apache/pinot/pull/18668#discussion_r4397759594
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/FieldConfig.java:
##########
@@ -242,6 +272,7 @@ public JsonNode getTierOverwrites() {
/// Returns the raw forward-index compression codec, or `null` when using
`codecSpec` or the
/// default. Still the only way to express `MV_ENTRY_DICT` and the CLP
family — not deprecated.
@Nullable
+ @Deprecated
Review Comment:
Minor: the doc two lines up still says "Still the only way to express
`MV_ENTRY_DICT` and the CLP family — not deprecated", which now contradicts
this `@Deprecated` and the validation reject.
`indexes.forward.compressionCodec` accepts the full `CompressionCodec` enum
(see `ForwardIndexConfig`'s ctor), so the claim is stale — worth rewording to
point there.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:
##########
@@ -2047,11 +2047,16 @@ private static void
validateIndexingConfigAndFieldConfigList(TableConfig tableCo
String column = fieldConfig.getName();
Preconditions.checkState(seenColumns.add(column), "Duplicate
FieldConfig for column: %s", column);
Preconditions.checkState(schema.hasColumn(column), "Failed to find
column: %s in schema", column);
-
- // Validate DELTA / DELTADELTA compression codecs compatibility
- validateGorillaCompressionCodecIfPresent(fieldConfig,
schema.getFieldSpecFor(column));
+ Preconditions.checkState(!fieldConfig.hasFieldLevelEncodingType(),
+ "FieldConfig.encodingType is deprecated for column: %s. Use
fieldConfigList[].indexes.forward.encodingType "
Review Comment:
**MAJOR — this breaks config updates for every pre-upgrade table with a
`fieldConfigList`, not just ones that used field-level encoding.** Verified
locally (pinot-spi built at base `b58776b` vs this head): the old `FieldConfig`
ctor defaulted null→`DICTIONARY` and serialized the getter, so every persisted
config carries serializer-injected `"encodingType":"DICTIONARY"`; on this head
that deserializes to `hasFieldLevelEncodingType()==true` and any update (even
GET→edit→PUT of an unrelated field) fails here until the operator hand-strips
the field.
Suggest tolerating `DICTIONARY` when `indexes.forward.encodingType` is
absent or agrees (reject only `RAW`/true conflicts), or auto-rewriting on read
as Jackie suggested — a bare reject needs at least a release-note-level
migration callout.
--
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]
