xiangfu0 commented on code in PR #19687:
URL: https://github.com/apache/pinot/pull/19687#discussion_r4138054498
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/openstruct/OpenStructIndexType.java:
##########
@@ -136,24 +140,101 @@ private void validatePerKeyIndexes(OpenStructIndexConfig
config, String column)
}
/// Validates one per-key [FieldConfig] of OPEN_STRUCT `column`; `target`
names it in error messages.
+ ///
+ /// A materialized key honors only its `encodingType` and its `inverted`,
`range` and `bloom` indexes, both when
+ /// `OpenStructColumnSplitter` builds it and when a reload resolves its
index configs through
+ /// `FieldIndexConfigsUtil.fromFieldConfig`. Every other setting would be
silently dropped, so it is rejected here:
+ /// `indexes.forward` and `indexes.dictionary` may only restate what
`encodingType` decides, and the legacy
+ /// `compressionCodec`, `indexTypes`, `timestampConfig`, `properties` and
`tierOverwrites` fields are never read.
private static void validatePerKeyFieldConfig(FieldConfig fieldConfig,
String column, String target) {
+ checkPerKeySetting(fieldConfig.getCompressionCodec() == null, column,
"compressionCodec", target,
+ FIXED_FORWARD_INDEX);
+ checkPerKeySetting(fieldConfig.getIndexTypes().isEmpty(), column,
"indexTypes", target,
+ "declare per-key indexes under 'indexes'");
+ checkPerKeySetting(fieldConfig.getTimestampConfig() == null, column,
"timestampConfig", target,
+ "materialized keys do not build TIMESTAMP indexes");
+ Map<String, String> properties = fieldConfig.getProperties();
+ checkPerKeySetting(properties == null || properties.isEmpty(), column,
"properties", target,
+ "configure the key through encodingType and 'indexes'");
+ checkPerKeySetting(isUnset(fieldConfig.getTierOverwrites()), column,
"tierOverwrites", target,
+ "per-key configs are not tier-specific");
+
JsonNode indexes = fieldConfig.getIndexes();
- if (indexes == null) {
- return;
- }
JsonNode forwardIndex =
indexes.get(StandardIndexes.forward().getPrettyName());
// The OPEN_STRUCT splitter builds its own per-key forward-index configs
(dict-vs-raw decision plus a
// fixed LZ4 raw compression), so a per-key codecSpec would be silently
discarded. Reject it explicitly.
Preconditions.checkState(forwardIndex == null ||
!forwardIndex.hasNonNull("codecSpec"),
- "OPEN_STRUCT column '%s': codecSpec is not supported for %s;
materialized keys always use a "
- + "dictionary-encoded or LZ4 raw forward index", column, target);
+ "OPEN_STRUCT column '%s': codecSpec is not supported for %s; " +
FIXED_FORWARD_INDEX, column, target);
Iterator<String> indexNames = indexes.fieldNames();
while (indexNames.hasNext()) {
String indexName = indexNames.next();
Preconditions.checkState(OpenStructSupportedIndexes.ALLOWED_PRETTY_NAMES.contains(indexName),
"OPEN_STRUCT key '%s' declares unsupported index '%s'; supported
indexes are %s",
fieldConfig.getName(), indexName,
OpenStructSupportedIndexes.ALLOWED_PRETTY_NAMES);
}
+ FieldConfig.EncodingType encodingType = fieldConfig.getEncodingType();
+ if (forwardIndex != null && !forwardIndex.isNull()) {
+ validatePerKeyForwardIndex(forwardIndex, encodingType, column, target);
+ }
+ // An empty `indexes.dictionary` still enables the dictionary, so only an
absent or null one is skipped.
+ JsonNode dictionary =
indexes.get(StandardIndexes.dictionary().getPrettyName());
+ if (dictionary != null && !dictionary.isNull()) {
+ validatePerKeyDictionary(dictionary, encodingType, column, target);
+ }
+ }
+
+ /// The splitter replaces a key's forward-index config with a
dictionary-encoded or LZ4 raw one chosen by
+ /// `encodingType`, so `indexes.forward` may only restate `encodingType` or
keep the index enabled.
+ private static void validatePerKeyForwardIndex(JsonNode forwardIndex,
FieldConfig.EncodingType encodingType,
+ String column, String target) {
+ Preconditions.checkState(forwardIndex.isObject(), "OPEN_STRUCT column
'%s': indexes.forward of %s must be a JSON "
+ + "object", column, target);
+ for (Map.Entry<String, JsonNode> field : forwardIndex.properties()) {
+ String name = field.getKey();
+ JsonNode value = field.getValue();
+ if (value.isNull()) {
+ continue;
+ }
+ if (name.equals("encodingType")) {
+ Preconditions.checkState(value.asText().equals(encodingType.name()),
+ "OPEN_STRUCT column '%s': indexes.forward.encodingType %s of %s
conflicts with its encodingType %s; set "
+ + "encodingType to choose between a dictionary-encoded and a
raw forward index", column,
+ value.asText(), target, encodingType);
+ } else {
+ checkPerKeySetting(name.equals("disabled") && !value.asBoolean(),
column, "indexes.forward." + name, target,
+ FIXED_FORWARD_INDEX);
+ }
+ }
+ }
+
+ /// A key's per-key dictionary config is never read: the key has a
dictionary unless its `encodingType` is RAW, or
+ /// when an enabled index requires one (such as inverted).
`indexes.dictionary` may only restate that choice.
+ private static void validatePerKeyDictionary(JsonNode dictionary,
FieldConfig.EncodingType encodingType,
Review Comment:
Good catch, thanks. Fixed in fed5f3cef1: both checks now compare against how
the key is actually built, which is dictionary-encoded when `encodingType` is
DICTIONARY or its inverted index is enabled. So your RAW + `inverted` +
`dictionary: {}` example passes, and `dictionary: {disabled: true}` on the same
key is rejected. `indexes.forward.encodingType` had the same flaw and uses the
same rule now. Inverted is the only vetted per-key index that requires a
dictionary, and a new test fails if another one ever starts to.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
--
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]
