This is an automated email from the ASF dual-hosted git repository. xiangfu0 pushed a commit to branch xiangfu0/openstruct-perkey-validation in repository https://gitbox.apache.org/repos/asf/pinot.git
commit b88a7bbcf3f0b5db3d4004e5eb74ce5c9b715c6f Author: Xiang Fu <[email protected]> AuthorDate: Mon Sep 28 15:15:39 2026 +0530 Reject per-key OPEN_STRUCT settings that materialized keys silently ignore OPEN_STRUCT materialized keys honor only the per-key encodingType and the inverted, range and bloom indexes. The splitter replaces the per-key forward-index config with a dictionary-encoded or LZ4 raw one, FieldIndexConfigsUtil.fromFieldConfig skips the per-key dictionary config, and the compressionCodec, indexTypes, timestampConfig, properties and tierOverwrites fields are never read, at segment creation or on reload. Table-config validation now rejects each of those settings instead of dropping it silently, naming the OPEN_STRUCT column and the key or defaultValueFieldConfig. indexes.forward and indexes.dictionary may only restate what encodingType decides. The OpenStructIndexConfig Javadoc now lists the honored settings. --- .../index/openstruct/OpenStructIndexType.java | 91 ++++++++++++- .../openstruct/OpenStructSupportedIndexes.java | 3 +- .../index/openstruct/OpenStructIndexTypeTest.java | 142 +++++++++++++++++++-- .../spi/config/table/OpenStructIndexConfig.java | 14 +- 4 files changed, 232 insertions(+), 18 deletions(-) diff --git a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/openstruct/OpenStructIndexType.java b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/openstruct/OpenStructIndexType.java index 5dfcd8c72e8..3f8462301c3 100644 --- a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/openstruct/OpenStructIndexType.java +++ b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/openstruct/OpenStructIndexType.java @@ -58,6 +58,10 @@ public class OpenStructIndexType public static final String INDEX_DISPLAY_NAME = "open_struct"; private static final List<String> EXTENSIONS = List.of(".open_struct.idx"); + private static final String FIXED_FORWARD_INDEX = + "materialized keys always use a dictionary-encoded or LZ4 raw forward index"; + private static final String DICTIONARY_FOLLOWS_ENCODING = + "a key has a dictionary unless its encodingType is RAW, or when an enabled index (such as inverted) requires one"; protected OpenStructIndexType() { super(StandardIndexes.OPEN_STRUCT_ID); @@ -136,17 +140,31 @@ public class OpenStructIndexType } /// 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(); @@ -154,6 +172,69 @@ public class OpenStructIndexType "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, + String column, String target) { + Preconditions.checkState(dictionary.isObject(), "OPEN_STRUCT column '%s': indexes.dictionary of %s must be a " + + "JSON object", column, target); + for (Map.Entry<String, JsonNode> field : dictionary.properties()) { + if (!field.getKey().equals("disabled")) { + checkPerKeySetting(field.getValue().isNull(), column, "indexes.dictionary." + field.getKey(), target, + DICTIONARY_FOLLOWS_ENCODING); + } + } + boolean disabled = dictionary.path("disabled").asBoolean(false); + boolean raw = encodingType == FieldConfig.EncodingType.RAW; + Preconditions.checkState(disabled == raw, + "OPEN_STRUCT column '%s': indexes.dictionary of %s %s the dictionary, which conflicts with its encodingType " + + "%s; %s", column, target, disabled ? "disables" : "enables", encodingType, DICTIONARY_FOLLOWS_ENCODING); + } + + private static void checkPerKeySetting(boolean supported, String column, String setting, String target, + String reason) { + Preconditions.checkState(supported, "OPEN_STRUCT column '%s': %s is not supported for %s; %s", column, setting, + target, reason); + } + + /// Returns `true` for an absent, JSON null or empty JSON value. + private static boolean isUnset(@Nullable JsonNode node) { + return node == null || node.isNull() || node.isMissingNode() || (node.isContainerNode() && node.size() == 0); } private void validateIgnoredKeys(OpenStructIndexConfig config, FieldSpec fieldSpec) { diff --git a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/openstruct/OpenStructSupportedIndexes.java b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/openstruct/OpenStructSupportedIndexes.java index cf23f188319..1ee0df49285 100644 --- a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/openstruct/OpenStructSupportedIndexes.java +++ b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/openstruct/OpenStructSupportedIndexes.java @@ -24,7 +24,8 @@ import org.apache.pinot.segment.spi.index.StandardIndexes; /// The set of index types (by pretty name) supported on OPEN_STRUCT materialized child columns. A key's /// `FieldConfig` may declare only these; non-vetted indexes are rejected at table-config validation. -/// `dictionary` is built structurally (lifecycle CUSTOM); `forward` is always written. +/// `dictionary` is built structurally (lifecycle CUSTOM) and `forward` is always written, so their per-key configs +/// may only restate the key's `encodingType` (see `OpenStructIndexType`). public final class OpenStructSupportedIndexes { private OpenStructSupportedIndexes() { } diff --git a/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/openstruct/OpenStructIndexTypeTest.java b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/openstruct/OpenStructIndexTypeTest.java index 8baa99d66c5..334ffeb10e0 100644 --- a/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/openstruct/OpenStructIndexTypeTest.java +++ b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/openstruct/OpenStructIndexTypeTest.java @@ -23,6 +23,7 @@ import com.fasterxml.jackson.databind.node.ObjectNode; import java.util.List; import java.util.Map; import java.util.Set; +import javax.annotation.Nullable; import org.apache.pinot.segment.local.utils.TableConfigUtils; import org.apache.pinot.segment.spi.index.FieldIndexConfigs; import org.apache.pinot.segment.spi.index.StandardIndexes; @@ -43,6 +44,7 @@ import static org.testng.Assert.assertEquals; import static org.testng.Assert.assertNotNull; import static org.testng.Assert.assertSame; import static org.testng.Assert.assertThrows; +import static org.testng.Assert.assertTrue; import static org.testng.Assert.expectThrows; @@ -205,22 +207,144 @@ public class OpenStructIndexTypeTest { @Test(dataProvider = "codecSpecOpenStructConfigs") public void testTableValidationRejectsCodecSpecForMaterializedChildren(OpenStructIndexConfig openStructConfig, String expectedTarget) { + IllegalStateException exception = expectThrows(IllegalStateException.class, + () -> validateTableWithOpenStruct(openStructConfig)); + assertEquals(exception.getMessage(), "OPEN_STRUCT column 'payload': codecSpec is not supported for " + + expectedTarget + "; materialized keys always use a dictionary-encoded or LZ4 raw forward index"); + } + + /// Per-key settings that a materialized key would silently ignore. JSON uses single quotes for readability. + @DataProvider(name = "ignoredPerKeySettings") + public Object[][] ignoredPerKeySettings() { + String unsupported = "OPEN_STRUCT column 'payload': %s is not supported for key 'clicks'; "; + return new Object[][]{ + {"{'name': 'clicks', 'encodingType': 'RAW', 'compressionCodec': 'ZSTANDARD'}", + String.format(unsupported, "compressionCodec")}, + {"{'name': 'clicks', 'indexTypes': ['INVERTED']}", String.format(unsupported, "indexTypes")}, + {"{'name': 'clicks', 'timestampConfig': {'granularities': ['DAY']}}", + String.format(unsupported, "timestampConfig")}, + {"{'name': 'clicks', 'properties': {'forwardIndexDisabled': 'true'}}", + String.format(unsupported, "properties")}, + {"{'name': 'clicks', 'tierOverwrites': {'hotTier': {'encodingType': 'RAW'}}}", + String.format(unsupported, "tierOverwrites")}, + {rawKeyWithForward("'compressionCodec': 'ZSTANDARD'"), + String.format(unsupported, "indexes.forward.compressionCodec")}, + {rawKeyWithForward("'chunkCompressionType': 'ZSTANDARD'"), + String.format(unsupported, "indexes.forward.chunkCompressionType")}, + {"{'name': 'clicks', 'indexes': {'forward': {'dictIdCompressionType': 'MV_ENTRY_DICT'}}}", + String.format(unsupported, "indexes.forward.dictIdCompressionType")}, + {rawKeyWithForward("'targetDocsPerChunk': 2048"), + String.format(unsupported, "indexes.forward.targetDocsPerChunk")}, + {rawKeyWithForward("'targetMaxChunkSize': '512K'"), + String.format(unsupported, "indexes.forward.targetMaxChunkSize")}, + {rawKeyWithForward("'rawIndexWriterVersion': 4"), + String.format(unsupported, "indexes.forward.rawIndexWriterVersion")}, + {rawKeyWithForward("'deriveNumDocsPerChunk': true"), + String.format(unsupported, "indexes.forward.deriveNumDocsPerChunk")}, + {rawKeyWithForward("'configs': {'key': 'value'}"), String.format(unsupported, "indexes.forward.configs")}, + {rawKeyWithForward("'disabled': true"), String.format(unsupported, "indexes.forward.disabled")}, + {"{'name': 'clicks', 'indexes': {'dictionary': {'onHeap': true}}}", + String.format(unsupported, "indexes.dictionary.onHeap")}, + {"{'name': 'clicks', 'indexes': {'dictionary': {'useVarLengthDictionary': true}}}", + String.format(unsupported, "indexes.dictionary.useVarLengthDictionary")}, + // The dictionary vs raw choice comes from the FieldConfig encodingType; the per-key forward and dictionary + // configs may only restate it. + {"{'name': 'clicks', 'indexes': {'forward': {'encodingType': 'RAW'}}}", + "OPEN_STRUCT column 'payload': indexes.forward.encodingType RAW of key 'clicks' conflicts with its " + + "encodingType DICTIONARY; "}, + {"{'name': 'clicks', 'indexes': {'dictionary': {'disabled': true}}}", + "OPEN_STRUCT column 'payload': indexes.dictionary of key 'clicks' disables the dictionary, which conflicts " + + "with its encodingType DICTIONARY; "}, + {"{'name': 'clicks', 'encodingType': 'RAW', 'indexes': {'dictionary': {}}}", + "OPEN_STRUCT column 'payload': indexes.dictionary of key 'clicks' enables the dictionary, which conflicts " + + "with its encodingType RAW; "} + }; + } + + @Test(dataProvider = "ignoredPerKeySettings") + public void testValidateRejectsIgnoredPerKeySettings(String keyConfigJson, String expectedMessagePrefix) + throws Exception { + FieldConfig keyConfig = parseFieldConfig(keyConfigJson); + IllegalStateException exception = expectThrows(IllegalStateException.class, + () -> validatePerKeyConfigs(null, List.of(keyConfig))); + assertTrue(exception.getMessage().startsWith(expectedMessagePrefix), exception.getMessage()); + } + + @Test + public void testValidateNamesDefaultValueFieldConfigInPerKeyErrors() + throws Exception { + FieldConfig defaultConfig = parseFieldConfig("{'name': 'default', 'properties': {'forwardIndexDisabled': 'true'}}"); + IllegalStateException exception = expectThrows(IllegalStateException.class, + () -> validatePerKeyConfigs(defaultConfig, null)); + assertEquals(exception.getMessage(), "OPEN_STRUCT column 'payload': properties is not supported for " + + "defaultValueFieldConfig; configure the key through encodingType and 'indexes'"); + } + + /// Settings a materialized key honors, and forward/dictionary entries that only restate its encodingType, pass. + @Test + public void testValidateAllowsHonoredAndRestatedPerKeySettings() + throws Exception { + FieldConfig rawKey = parseFieldConfig("{'name': 'clicks', 'encodingType': 'RAW', 'indexes': {" + + "'forward': {'encodingType': 'RAW', 'disabled': false}, 'dictionary': {'disabled': true}, " + + "'range': {'version': 2}, 'bloom': {'fpp': 0.01}}}"); + FieldConfig dictionaryKey = parseFieldConfig("{'name': 'views', 'properties': {}, 'tierOverwrites': {}, " + + "'indexes': {'forward': {}, 'dictionary': {'disabled': false}, 'inverted': {}}}"); + FieldConfig defaultConfig = parseFieldConfig("{'name': 'default', 'encodingType': 'RAW', " + + "'indexes': {'forward': null, 'dictionary': null, 'bloom': {}}}"); + validatePerKeyConfigs(defaultConfig, List.of(rawKey, dictionaryKey)); + } + + /// The table-config path deserializes per-key configs from JSON; a round trip must neither lose a rejected setting + /// nor turn an unset field into a rejected one. + @Test + public void testTableValidationChecksIgnoredPerKeySettings() + throws Exception { + FieldConfig honoredKey = parseFieldConfig("{'name': 'clicks', 'encodingType': 'RAW', " + + "'indexes': {'forward': {'encodingType': 'RAW'}, 'range': {}}}"); + validateTableWithOpenStruct( + new OpenStructIndexConfig(false, null, -1, null, 0.5, List.of(honoredKey), null, null, null)); + + FieldConfig ignoredKey = parseFieldConfig("{'name': 'clicks', 'encodingType': 'RAW', " + + "'indexes': {'forward': {'targetDocsPerChunk': 2048}}}"); + IllegalStateException exception = expectThrows(IllegalStateException.class, () -> validateTableWithOpenStruct( + new OpenStructIndexConfig(false, null, -1, null, 0.5, List.of(ignoredKey), null, null, null))); + assertEquals(exception.getMessage(), "OPEN_STRUCT column 'payload': indexes.forward.targetDocsPerChunk is not " + + "supported for key 'clicks'; materialized keys always use a dictionary-encoded or LZ4 raw forward index"); + } + + private static FieldConfig parseFieldConfig(String singleQuotedJson) + throws Exception { + return JsonUtils.stringToObject(singleQuotedJson.replace('\'', '"'), FieldConfig.class); + } + + private static String rawKeyWithForward(String forwardFields) { + return "{'name': 'clicks', 'encodingType': 'RAW', 'indexes': {'forward': {" + forwardFields + "}}}"; + } + + /// Validates an OPEN_STRUCT column `payload` whose index config has the given per-key configs. + private static void validatePerKeyConfigs(@Nullable FieldConfig defaultValueFieldConfig, + @Nullable List<FieldConfig> valueFieldConfigs) { + OpenStructIndexConfig config = new OpenStructIndexConfig(false, defaultValueFieldConfig, -1, null, 0.5, + valueFieldConfigs, null, null, null); + FieldIndexConfigs fieldIndexConfigs = + new FieldIndexConfigs.Builder().add(StandardIndexes.openStruct(), config).build(); + StandardIndexes.openStruct().validate(fieldIndexConfigs, openStructSchema().getFieldSpecFor("payload"), null); + } + + /// Runs full table-config validation, which reads the OPEN_STRUCT config back from JSON. + private static void validateTableWithOpenStruct(OpenStructIndexConfig openStructConfig) { ObjectNode indexes = JsonUtils.newObjectNode(); indexes.set(StandardIndexes.openStruct().getPrettyName(), JsonUtils.objectToJsonNode(openStructConfig)); FieldConfig parentFieldConfig = new FieldConfig.Builder("payload").withIndexes(indexes).build(); TableConfig tableConfig = new TableConfigBuilder(TableType.OFFLINE) - .setTableName("openStructCodecSpecTest") + .setTableName("openStructPerKeyTest") .setFieldConfigList(List.of(parentFieldConfig)) .build(); - Schema schema = new Schema.SchemaBuilder() - .setSchemaName("openStructCodecSpecTest") - .addOpenStruct("payload", Map.of()) - .build(); + TableConfigUtils.validate(tableConfig, openStructSchema()); + } - IllegalStateException exception = expectThrows(IllegalStateException.class, - () -> TableConfigUtils.validate(tableConfig, schema)); - assertEquals(exception.getMessage(), "OPEN_STRUCT column 'payload': codecSpec is not supported for " - + expectedTarget + "; materialized keys always use a dictionary-encoded or LZ4 raw forward index"); + private static Schema openStructSchema() { + return new Schema.SchemaBuilder().setSchemaName("openStructPerKeyTest").addOpenStruct("payload", Map.of()).build(); } private static FieldConfig rawCodecSpecFieldConfig(String name) { diff --git a/pinot-spi/src/main/java/org/apache/pinot/spi/config/table/OpenStructIndexConfig.java b/pinot-spi/src/main/java/org/apache/pinot/spi/config/table/OpenStructIndexConfig.java index 6f3f4732ac3..0eac621e7e3 100644 --- a/pinot-spi/src/main/java/org/apache/pinot/spi/config/table/OpenStructIndexConfig.java +++ b/pinot-spi/src/main/java/org/apache/pinot/spi/config/table/OpenStructIndexConfig.java @@ -46,9 +46,17 @@ import org.apache.pinot.spi.utils.JsonUtils; /// **Per-key index settings** are specified via `valueFieldConfigs` — each entry is a standard /// [FieldConfig] (modern `indexes` format) for one materialized OPEN_STRUCT key. Keys without an /// entry fall back to `defaultValueFieldConfig`. When neither is set, the built-in default is -/// DICTIONARY encoding with an inverted index. A per-key config selects the dictionary vs raw -/// encoding and the dictionary, inverted, range and bloom filter indexes; forward-index tuning is not -/// configurable per key (a raw key always uses LZ4 compression), and a per-key `codecSpec` is rejected. +/// DICTIONARY encoding with an inverted index. A per-key config honors only: +/// - `encodingType`: DICTIONARY (the default) builds a dictionary-encoded key, and RAW builds a raw +/// forward index with LZ4 compression, unless an enabled index that requires a dictionary (such as +/// inverted) makes the key dictionary-encoded. +/// - The `inverted`, `range` and `bloom` entries under `indexes`. A per-key config without an +/// `inverted` entry builds no inverted index. +/// +/// `indexes.forward` and `indexes.dictionary` may only restate what `encodingType` decides. Table-config +/// validation rejects every other per-key setting, which a materialized key would silently ignore: +/// forward-index tuning (such as `codecSpec`, `compressionCodec` or chunk sizes), dictionary tuning, and +/// the `indexTypes`, `properties`, `timestampConfig` and `tierOverwrites` fields. public class OpenStructIndexConfig extends IndexConfig { public static final OpenStructIndexConfig DISABLED = new OpenStructIndexConfig(false); public static final OpenStructIndexConfig DEFAULT = new OpenStructIndexConfig(true); --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
