Skip to content

Reject per-key OPEN_STRUCT settings that materialized keys silently ignore - #19687

Merged
xiangfu0 merged 2 commits into
masterfrom
xiangfu0/openstruct-perkey-validation
Sep 30, 2026
Merged

xiangfu0 merged 2 commits into
masterfrom
xiangfu0/openstruct-perkey-validation

Conversation

@xiangfu0

@xiangfu0 xiangfu0 commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor

PR flow

OPEN_STRUCT validation adds checks that reject per‑key settings not honored by materialized keys.

flowchart TD
  N0["validatePerKeyFieldConfig #40;F1#41;"]:::stModified
  N1["checkPerKeySetting #40;F1#41;"]:::stModified
  N2["validatePerKeyForwardIndex #40;F1#41;"]:::stModified
  N3["validatePerKeyDictionary #40;F1#41;"]:::stModified
  N4["isUnset #40;F1#41;"]:::stModified
  N0 -->|"calls"| N1
  N0 -->|"calls"| N4
  N0 -->|"calls"| N2
  N0 -->|"calls"| N3
  N2 -->|"calls"| N1
  N3 -->|"calls"| N1
  classDef stAdded fill:#dafbe1,stroke:#1a7f37,color:#1f2328,stroke-width:2px
  classDef stModified fill:#fff8c5,stroke:#9a6700,color:#1f2328,stroke-width:2px
  classDef stRemoved fill:#ffebe9,stroke:#cf222e,color:#1f2328,stroke-width:2px
  classDef stUnchanged fill:#f6f8fa,stroke:#656d76,color:#1f2328,stroke-width:1px
Loading

AI-generated · Green: added · Yellow: modified · Red: removed · Gray: existing

Diff evidence
  • F1: pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/openstruct/OpenStructIndexType.java — before · after
  • Regenerate PR flow

Context

Follow-up to #19308 (merged), which started rejecting a per-key codecSpec on OPEN_STRUCT keys. Rebased onto master after #19608; its multi-value and nested-key changes apply per-key configs the same way, so the rules below hold for those keys too.

Problem

An OPEN_STRUCT column materializes dense keys as child columns (e.g. payload$clicks), each configured through a per-key FieldConfig in valueFieldConfigs or defaultValueFieldConfig. Only part of that FieldConfig is honored; the rest is silently dropped:

Per-key setting At segment creation and on reload
encodingType Honored. DICTIONARY builds a dictionary-encoded key; RAW builds a raw LZ4 key, unless an enabled index that requires a dictionary (such as inverted) makes it dictionary-encoded
indexes.inverted, indexes.range, indexes.bloom Honored. A per-key config without inverted builds no inverted index
indexes.forward Parsed, then replaced: OpenStructColumnSplitter always builds a dictionary-encoded or LZ4 raw forward index
indexes.dictionary Never read: FieldIndexConfigsUtil.fromFieldConfig derives the dictionary from encodingType
compressionCodec, indexTypes, timestampConfig, properties, tierOverwrites Never read

Traced through OpenStructColumnSplitter.writeDenseKeyColumn / resolveUseDictionary (segment creation) and IndexLoadingConfig.withOpenStructChildConfigs → FieldIndexConfigsUtil.fromFieldConfig (reload, where the inverted, range and bloom handlers act on child columns and ForwardIndexHandler skips them).

Changes

  • OpenStructIndexType table-config validation rejects each ignored setting, naming the OPEN_STRUCT column and the key or defaultValueFieldConfig, for example: 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.
    • indexes.forward and indexes.dictionary may only restate how the key is built: it is dictionary-encoded when encodingType is DICTIONARY or its inverted index is enabled, and raw with LZ4 otherwise. So forward.encodingType must match that encoding, forward.disabled may only be false, and dictionary.disabled must match whether the key has a dictionary (an empty {} enables it).
    • Empty properties / tierOverwrites and JSON nulls count as unset. The codecSpec message from Report V7 compression stats and reject per-key OPEN_STRUCT codecSpec #19308 is unchanged.
  • The OpenStructIndexConfig class Javadoc lists the honored settings, correcting the per-key dictionary claim added in Report V7 compression stats and reject per-key OPEN_STRUCT codecSpec #19308.
  • No new build-time guard in the splitter: validation is the enforcement point, and failing segment builds over an ignored tuning setting would stall ingestion for tables saved with validation skipped. The codecSpec splitter guard from Report V7 compression stats and reject per-key OPEN_STRUCT codecSpec #19308 stays.

Compatibility

Validation tightening only. OPEN_STRUCT has not been released, and no quickstart, test resource, doc or in-repo test config sets a newly rejected setting. A table that already carries one keeps loading and ingesting; its next table-config update fails validation until the setting is removed.

Testing

  • OpenStructIndexTypeTest (14 → 41 tests): 19 rejected-setting cases, honored and restated settings accepted, defaultValueFieldConfig naming, and an end-to-end TableConfigUtils.validate JSON round trip.
  • In-reactor with JDK 25: *OpenStruct*Test, TableConfigUtilsTest and SegmentMetadataImplTest across pinot-spi and pinot-segment-local, 349 tests, 0 failures.
  • Spotless, license and checkstyle clean on pinot-spi and pinot-segment-local.

@xiangfu0 xiangfu0 added bug Something is not working as expected configuration Config changes (addition/deletion/change in behavior) index Related to indexing (general) labels Sep 28, 2026
Base automatically changed from xiangfu0/codex/codec-stack/06-reload-enable to master September 28, 2026 16:12
…gnore

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.
@xiangfu0
xiangfu0 force-pushed the xiangfu0/openstruct-perkey-validation branch from b88a7bb to e001205 Compare September 29, 2026 13:00
@codecov-commenter

codecov-commenter commented Sep 29, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 95.12195% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.09%. Comparing base (2cd9a2f) to head (fed5f3c).
⚠️ Report is 4 commits behind head on master.

Files with missing lines Patch % Lines
.../segment/index/openstruct/OpenStructIndexType.java 95.12% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##             master   #19687       +/-   ##
=============================================
+ Coverage     39.77%   68.09%   +28.31%     
- Complexity     1449     1450        +1     
=============================================
  Files          3518     3518               
  Lines        228478   228516       +38     
  Branches      36201    36216       +15     
=============================================
+ Hits          90881   155601    +64720     
+ Misses       129364    60694    -68670     
- Partials       8233    12221     +3988     
Flag Coverage Δ
integration 100.00% <ø> (ø)
integration1 100.00% <ø> (ø)
integration2 0.00% <ø> (ø)
java-25 68.09% <95.12%> (+28.31%) ⬆️
lane-a 100.00% <ø> (ø)
lane-b 0.00% <ø> (ø)
temurin 68.09% <95.12%> (+28.31%) ⬆️
unittests 68.08% <95.12%> (+28.31%) ⬆️
unittests1 58.07% <0.00%> (?)
unittests2 39.79% <95.12%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

…the key is built

An enabled inverted index requires a dictionary, so a RAW key with one is
built dictionary-encoded. validatePerKeyDictionary compared
indexes.dictionary only against encodingType, so it rejected an accurate
`dictionary: {}` on such a key and accepted `dictionary: {disabled: true}`;
indexes.forward.encodingType had the same flaw. Both now compare against
the built outcome: dictionary-encoded when encodingType is DICTIONARY or
the inverted index is enabled.

A test pins inverted as the only vetted per-key index that requires a
dictionary, and new cases cover JSON-null fields and non-object forward and
dictionary blocks.
@xiangfu0
xiangfu0 merged commit 9e5a41f into master Sep 30, 2026
16 of 18 checks passed
@xiangfu0
xiangfu0 deleted the xiangfu0/openstruct-perkey-validation branch September 30, 2026 04:40
xiangfu0 added a commit to pinot-contrib/pinot-docs that referenced this pull request Sep 30, 2026
Clarifies honored OPEN_STRUCT per-key encoding and index settings, newly
rejected ignored settings, and the effect on existing table-config
updates. Follows apache/pinot#19687.

Co-authored-by: Xiang Fu <xiangfu@Xiang-mac-mtv-2.local>
@xiangfu0

Copy link
Copy Markdown
Contributor Author

Documentation follow-up: pinot-contrib/pinot-docs#1076 (merged). This clarifies supported per-key OPEN_STRUCT settings and table-config validation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something is not working as expected configuration Config changes (addition/deletion/change in behavior) index Related to indexing (general)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants