Add codec spec DSL and configuration plumbing

[xiangfu0]

Context

This is the configuration layer extracted from #18229 and the root of the codec-pipeline PR stack.

What changed

• Adds the wrapper-less codec DSL, for example DELTA,ZSTD(3).
• Adds immutable AST types, bounded parsing, structural normalization, and package-private shared syntax limits.
• Adds nullable indexes.forward.codecSpec support to ForwardIndexConfig, including JSON, builder/copy, equality, RAW-only validation, and mutual exclusion with legacy compressionCodec.
• Reconciles legacy noDictionary and top-level compression settings with the nested forward-index config.
• Keeps the removed CODEC(...) wrapper reserved and rejected with a precise diagnostic.

Stack boundary and compatibility

This PR deliberately does not execute codecs or change on-disk data. codecSpec is accepted and structurally normalized here. #19285-#19306 add the runtime codecs; #19307 is the first slice that consumes codecSpec for forward-index creation/loading; #19308 adds semantic table-config validation and codec-aware reload/rewrite behavior. Do not configure this unreleased property on an intermediate build of the stack.

The dedicated bounded parser is intentional: reusing Pinot's SQL parser would create a module cycle through pinot-common, and Calcite canonicalization is not a suitable dependency for strings later frozen into segment headers.

Verification

• Focused codec AST/parser, ForwardIndexConfig, and forward-index configuration-reconciliation tests.
• Spotless, Checkstyle, license format, and license check on the affected modules.
• Exact parent/child topology and range-diff validation after rebase.

Stack

1. Add codec spec DSL and configuration plumbing #19284 — DSL and configuration plumbing (this PR)
2. Add bounded codec runtime and compression handlers #19285 — bounded runtime and compression handlers
3. Add DELTA and DELTADELTA transform codecs #19305 — DELTA and DELTADELTA
4. Add T64 and GORILLA packing transform codecs #19306 — T64 and GORILLA
5. Add V7 raw forward index format for codec pipelines #19307 — V7 forward-index I/O
6. Validate codecSpec and support codec-aware forward-index reloads #19308 — semantic validation and reload/rewrite support
7. Add codec pipeline integration tests and design doc #19309 — integration coverage and design documentation

Review and merge parent-first; GitHub will retarget each child as its parent merges.

[Copilot]

Pull request overview

Adds the codec-spec DSL and configuration plumbing while keeping codec execution disabled.

Changes:

• Adds bounded parsing and immutable codec AST types.
• Adds codecSpec to forward-index configuration.
• Adds fail-closed validation across creation, loading, preprocessing, and realtime paths.

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
ForwardIndexConfig.java	Adds codec-spec configuration support.
CodecSpecParser.java	Parses and canonicalizes the DSL.
CodecPipeline.java	Represents ordered codec stages.
CodecInvocation.java	Represents individual codec calls.
ForwardIndexType.java	Reconciles configuration and rejects activation.
ForwardIndexReaderFactory.java	Adds reader-side rejection.
ForwardIndexCreatorFactory.java	Adds creator-side rejection.
ForwardIndexHandler.java	Rejects preprocessing with codec specs.
ForwardIndexConfigTest.java	Tests configuration behavior.
CodecSpecParserTest.java	Tests parsing and limits.
CodecPipelineTest.java	Tests pipeline immutability.
CodecInvocationTest.java	Tests invocation validation.
TableConfigUtilsTest.java	Tests table validation.
ForwardIndexHandlerTest.java	Tests preprocessing rejection.
ForwardIndexTypeTest.java	Tests reconciliation and mutable indexes.
ForwardIndexReaderFactoryTest.java	Tests reader rejection.
ForwardIndexCreatorFactoryTest.java	Tests creator rejection.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 88.70056% with 20 lines in your changes missing coverage. Please review.
✅ Project coverage is 67.07%. Comparing base (f8352be) to head (e8dc1c7).
⚠️ Report is 6 commits behind head on master.

Files with missing lines	Patch %	Lines
...pache/pinot/segment/spi/codec/CodecSpecParser.java	93.40%	3 Missing and 3 partials ⚠️
...he/pinot/segment/spi/index/ForwardIndexConfig.java	76.19%	2 Missing and 3 partials ⚠️
...pache/pinot/segment/spi/codec/CodecInvocation.java	80.95%	2 Missing and 2 partials ⚠️
.../apache/pinot/segment/spi/codec/CodecPipeline.java	78.94%	2 Missing and 2 partials ⚠️
...apache/pinot/segment/spi/codec/CodecDslSyntax.java	96.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #19284      +/-   ##
============================================
+ Coverage     67.00%   67.07%   +0.06%     
  Complexity     1424     1424              
============================================
  Files          3463     3467       +4     
  Lines        221671   222321     +650     
  Branches      34954    34994      +40     
============================================
+ Hits         148524   149111     +587     
+ Misses        61310    61304       -6     
- Partials      11837    11906      +69     

Flag	Coverage Δ
integration	100.00% <ø> (ø)
integration1	100.00% <ø> (ø)
integration2	0.00% <ø> (ø)
java-25	67.07% <88.70%> (+0.06%)	⬆️
lane-a	100.00% <ø> (ø)
lane-b	0.00% <ø> (ø)
temurin	67.07% <88.70%> (+0.06%)	⬆️
unittests	67.06% <88.70%> (+0.06%)	⬆️
unittests1	57.82% <88.70%> (+0.11%)	⬆️
unittests2	38.84% <0.00%> (-0.11%)	⬇️

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.

[xiangfu0]

Reviewed against the split plan. One thing worth fixing before this merges, since this PR freezes the codecSpec grammar: CodecSpecParser accepts leading zeros in numeric args and canonicalization preserves them, so ZSTD(03) and ZSTD(3) canonicalize to different strings. The canonical form later gets embedded in V7 segment headers and compared for rewrite detection (upcoming PRs in this stack), so two semantically identical specs would compare as different and could trigger spurious rewrites. Suggest rejecting or normalizing leading-zero args in parseArg.

Minor: MAX_SPEC_LENGTH = 64KB is generous for a string destined for a segment header; a 1–4KB cap would bound header bloat.

[Copilot]

Pull request overview

Copilot reviewed 21 out of 21 changed files in this pull request and generated no new comments.

[xiangfu0]

Pushed `e00e837b5c` addressing review findings on the DSL surface.

Leading-zero arguments are now rejected. `ZSTD(03)` and `ZSTD(3)` previously canonicalized to two different strings. Since the canonical spec is frozen into V7 segment headers and compared by equality on reload, two spellings of the same value would eventually mean spurious rewrites and a parser that has to accept both forever. The grammar is now `arg ::= "0" | [1-9][0-9]*`, enforced in both `CodecSpecParser.parseArg` and the `CodecInvocation` constructor (programmatic construction bypasses the parser). A bare `0` remains valid.

`MAX_SPEC_LENGTH` tightened 64KB to 4KB to bound header bloat. Note this now wins over the combinatorial maximum of the per-stage limits (32 stages x 128-char names is ~4.1KB); any realistic spec is far below it. Tests pin both boundaries — exactly `MAX_SPEC_LENGTH` parses, one over is rejected.

`REMOVED_WRAPPER_NAME` widened to public so every layer enforcing the `CODEC` reservation shares one definition instead of re-spelling the literal (the registry in #19285 now references it).

All of these tighten acceptance before first release, which is the reversible direction — accepting now and rejecting later would not be.

Verification: `CodecSpecParserTest`, `CodecInvocationTest`, `CodecPipelineTest`, `ForwardIndexConfigTest` (pinot-segment-spi) plus `ForwardIndexTypeTest`, `TableConfigUtilsTest`, `ForwardIndexCreatorFactoryTest`, `OpenStructIndexTypeTest` (pinot-segment-local) all green; spotless/checkstyle/license clean on both modules.

[xiangfu0]

Note: pushed a master sync to this branch (merge commit, no code changes of its own) so the downstream stack includes the #19282 chunk-caching fix that the reload tests in #19308 depend on.

The full stack continuing this PR: #19305 (DELTA/DELTADELTA) → #19306 (T64/GORILLA) → #19307 (V7 format) → #19308 (reload + enable) → #19309 (integration tests + docs). Each is based on its predecessor's branch.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Jackie-Jiang]

Let's remove all the unnecessary config check

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[xiangfu0]

Code Review — Codec spec DSL and configuration plumbing

Reviewed the full diff. This is a clean, well-scoped configuration-layer PR that adds the codec DSL AST/parser and threads a nullable codecSpec through ForwardIndexConfig without executing any codecs. Code quality is high: immutable AST nodes, defensive copies, a hand-rolled bounded parser with a clearly documented rationale, and genuinely thorough tests covering canonicalization, structural limits (accept and reject boundaries), mutual exclusion, and the reconciliation paths. The design note in CodecSpecParser explaining why the SQL/Calcite parser was deliberately avoided (module cycle + on-disk stability) is excellent and answers the obvious reviewer question preemptively.

A few observations below — none blocking.

Correctness

• ForwardIndexType disabled-path structure check (ForwardIndexType.java:222-231). The non-disabled path guards with Preconditions.checkState(forwardIndexNode.isObject(), ...) before reading child fields, but the new disabled-path check calls forwardIndexNode.get("codecSpec") without that guard. It's safe today because JsonNode.get(String) returns null on non-object nodes rather than throwing, so a malformed indexes.forward (string/array) silently reads as "no codecSpec" and the config is discarded anyway. Still, a malformed non-object forward node is accepted without diagnostic on the disabled path while it's rejected on the enabled path — a minor asymmetry worth a comment or an isObject() guard for consistency.

• Null-safety of the moved getIndexes().get(...) (ForwardIndexType.java:222). Verified this is safe: FieldConfig._indexes defaults to NullNode.getInstance() (never Java null), and NullNode.get(name) returns null. Moving the call earlier in the disabled branch introduces no new NPE risk. Good.

Serialization

• codecSpec serializes as "codecSpec":null for every existing ForwardIndexConfig, since neither IndexConfig nor the shared JsonUtils mapper sets @JsonInclude(NON_NULL). I confirmed this matches the existing behavior of compressionCodec and the other nullable fields, so it's consistent, not a regression — just noting it in case a future reader expects the field to be omitted when unset.

Parser / DSL

• Grammar bounds are enforced twice — once structurally in the parser and once in CodecInvocation/CodecPipeline constructors — and the tests explicitly assert both entry points agree on the accept boundary (testStructuralLimitsMatchTheParser, testStageLimitMatchesTheParser). This is the right call given codecs will build invocations programmatically, bypassing the parser.
• No DoS surface: length is checked before trim(), MAX_SPEC_LENGTH=4096 bounds all scanning, skipWhitespace is O(n) overall, and only ASCII digits are accepted (the ZSTD(०) unicode-digit rejection test is a nice touch).
• Leading-zero rejection is correctly motivated by canonical-form stability (segment-header string equality). Bare 0 stays valid. Well tested.

Minor / nits

• CodecSpecParser.parse checks spec == null → throws "must not be null or blank", then length, then isBlank(). The ordering means an over-length blank string reports the length error rather than the blank error. Harmless, but the two messages could be unified.
• The public MAX_* constants on CodecSpecParser re-export the package-private CodecDslSyntax values. Fine, but it does mean the limits are named in two public-ish surfaces; worth keeping in mind when these get tuned later in the stack.

Risk assessment

Low. The PR is inert by design — codecSpec is parsed and normalized but never consumed, per the stated stack boundary (#19307 is the first consumer). Mutual exclusion with legacy compressionCodec, the RAW-only requirement, and the reserved CODEC(...) wrapper are all validated at construction and covered by tests. The main thing downstream reviewers should keep an eye on is that the frozen canonical form (toDslString()) stays byte-stable across the stack, since later layers compare it by string equality for rewrite detection — the tests here establish that contract, so any future change to normalization needs to preserve it.

Nice work — happy to see this merged as the stack root once the minor disabled-path asymmetry is considered.