Add codec spec DSL and configuration plumbing - #19284
Conversation
xiangfu0
added
feature
There was a problem hiding this comment.
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
codecSpecto 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 Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #19284 +/- ##
============================================
- Coverage 67.18% 67.13% -0.06%
Complexity 1424 1424
============================================
Files 3462 3465 +3
Lines 220361 220870 +509
Branches 35147 35307 +160
============================================
+ Hits 148045 148274 +229
- Misses 60506 60749 +243
- Partials 11810 11847 +37
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
e83ed0e to
1c5df46
Compare
1c5df46 to
ad52533
Compare
|
Reviewed against the split plan. One thing worth fixing before this merges, since this PR freezes the codecSpec grammar: Minor: |
The canonical codec spec is later frozen into V7 segment headers, so every argument value must have exactly one spelling: ZSTD(03) and ZSTD(3) must not produce different canonical strings. Reject leading zeros in both the parser and the CodecInvocation constructor (programmatic construction bypasses the parser). Also tighten MAX_SPEC_LENGTH from 64KB to 4KB to bound V7 header bloat; the cap still admits any realistic spec, and where it intersects the combinatorial maximum of the per-stage limits, the length cap wins. Widen REMOVED_WRAPPER_NAME to public so every layer enforcing the reservation shares one definition.
|
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. |
|
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. |
The canonical spec produced here is frozen into segment headers and compared for rewrite detection by later layers in this stack, but CodecInvocation and CodecPipeline had no equals/hashCode coverage, and the documented structural limits were only exercised on the reject side. Add tests that pin the frozen contract: - a parsed invocation/pipeline equals the programmatically built equivalent and agrees on hashCode, argument-less invocations canonicalize alike, and stage order is significant - every documented limit (identifier length, argument length, arguments per invocation, pipeline stages) is checked at its accept boundary as well as one over, through both the parser and the AST constructors, which are independent entry points Test-only; no production behavior changes.
The DSL is function-call shaped, so reviewers reasonably ask why it does not reuse Pinot's expression parsing. Record the answer where it is discoverable rather than leaving it to be re-derived. CalciteSqlParser is unreachable from pinot-segment-spi: it lives in pinot-common, which already depends on this module, so calling into it would close a module cycle. Calcite's own SqlParser is on the classpath but is deliberately unused, chiefly because the canonical form produced here is frozen into segment headers and compared by string equality, which would tie on-disk segment compatibility to Calcite's casing, quoting, and literal formatting across upgrades. Also records the cost of that choice: unsigned integer arguments only, so negative and keyword arguments are not expressible today. Javadoc-only; no behavior change.
xiangfu0
added
the
configuration
Context
This is the bottom layer extracted from #18229 after the request to split the codec-pipeline work and land the DSL plus configuration wiring first.
Native GitHub stack:
What changed
CODEC(...)form with an ordered invocation list such asDELTA,ZSTD(3).indexes.forward.codecSpecsupport toForwardIndexConfig, including JSON, builder, copy, equality, RAW-only validation, and mutual exclusion with legacycompressionCodec.noDictionaryand top-level compression signals with the nested forward-index config.Safety and compatibility
This layer does not activate
codecSpecand does not change any on-disk format. Every non-null spec is deliberately rejected before a reader, writer, or mutable index can silently ignore it. Activation will be a later stacked PR after the runtime and reader/writer lifecycle are reviewable.The unreleased
CODEC(...)syntax is intentionally not retained as an alias.Why a dedicated parser rather than the SQL expression parser
The DSL is function-call shaped, so reusing Pinot's expression parsing is a fair question. Two separate answers:
CalciteSqlParseris unreachable frompinot-segment-spi. It lives inpinot-common, andpinot-commonalready declares a dependency onpinot-segment-spi, so calling into it would close a module cycle.Calcite's own
SqlParseris on the classpath — this module already uses Calcite for type inference — but is deliberately not used either:DELTA,ZSTD(3)is not a valid statement, so it would have to be wrapped and then walked to reject arithmetic, nested calls, string literals, aliases, and qualified names — larger than the parser here, and failing open as new node types appear in later Calcite versions, in a path that otherwise fails closed.ForwardIndexConfigconstructor, on controller, server, and minion.The cost of that choice, stated plainly: arguments are unsigned integers only, so neither negative arguments (ZSTD fast-mode levels) nor keyword arguments (
LZ4(HIGH)) are expressible today. Widening the grammar later is additive and contained; removing a Calcite dependency from segment headers would not be. The rationale is recorded in theCodecSpecParserjavadoc.Tests
ForwardIndexConfigtestshashCodepinning forCodecInvocationandCodecPipeline, plus accept-boundary coverage for every documented structural limit through both the parser and the AST constructorspinot-segment-spiandpinot-segment-local