xiangfu0 opened a new pull request, #19308:
URL: https://github.com/apache/pinot/pull/19308
Stacked on #19307 (`xiangfu0/codex/codec-stack/05-v7-format`). Part of the
split of #18229.
## Summary
This is the enabling PR of the series: it removes every remaining
`codecSpec` feature-gate site and
replaces the gates with real behavior — semantic table-config-time
validation, reload/rewrite support in
`ForwardIndexHandler`, and rollback back to the legacy formats.
### Reload / rewrite (`ForwardIndexHandler`)
`shouldChangeRawCompressionType` is replaced by
`shouldRewriteRawForwardIndex`, which handles every
combination of existing on-disk format × new config:
| Existing on disk | New config | Decision |
|---|---|---|
| Legacy raw (compression C1) | no codecSpec, compression C2 | rewrite iff
C1 != C2 (unchanged legacy behavior) |
| Legacy raw (compression C1) | codecSpec mapping to legacy C2 | rewrite iff
C1 != C2 (stays in legacy format) |
| Legacy raw | codecSpec needing V7 (transform/chain/non-default options) |
rewrite to V7 codec-pipeline format |
| V7 (stored canonical spec S1) | codecSpec needing V7, canonical S2 |
rewrite iff S1 != S2 |
| V7 | codecSpec mapping to a legacy compression | rewrite back to the
legacy raw format |
| V7 | no codecSpec, explicit legacy `compressionCodec` (incl.
`PASS_THROUGH`) | rewrite back to the legacy format (**rollback path** —
`isLegacyRevertTargetForFixedByteSv`) |
| V7 | no codecSpec, no compressionCodec | no-op |
Change detection compares **canonical** specs on both sides: the stored side
is already canonical (V7
headers persist `CodecPipelineExecutor.getCanonicalSpec()`), and the
configured side is canonicalized by
running it through `CodecPipelineExecutor.create(...)` — never
`toDslString()` or the raw config string —
so semantically identical specs (e.g. differently-spelled defaults) never
trigger a rewrite, and an
unchanged config is always a no-op.
V7 detection is positive (`reader.getCodecSpec() != null`), not inferred
from a null compression type; a
legacy raw reader returning a null `ChunkCompressionType` now fails loudly
instead of silently skipping.
### Gate removal (all remaining sites)
- `ForwardIndexType.validate` — gate replaced by `validateCodecSpec(...)`,
run only for enabled forward
indexes: parses/validates the pipeline via the public
`CodecPipelineExecutor.create` entry point
(unknown codec, transform ordering, per-codec type compatibility) and
enforces the V7 writer's shape
constraints (single-value INT/LONG) for specs the legacy raw formats
cannot serve. Precise errors
replace the generic gate message.
- `ForwardIndexType.shouldCreateIndex` — gate removed; codecSpec configs
flow to the creator factory
(wired in the previous PR).
- `ForwardIndexType.createMutableIndex` — gate removed. Matching the
original #18229 behavior, the
mutable (consuming) forward index builds the standard in-memory format and
deliberately ignores
codecSpec: the spec applies when the consuming segment is
converted/committed to an immutable
segment. Realtime tables with a codecSpec consume normally.
- `ForwardIndexHandler.computeOperations` — the fail-fast guard from the
previous PR is replaced by the
real decision logic above.
- `OpenStructIndexType` / `OpenStructColumnSplitter` — **kept as a
deliberate rejection**, reworded from
the temporary gate message to `"codecSpec is not supported for OPEN_STRUCT
key: %s"`. The original
#18229 never wired codecSpec into OPEN_STRUCT per-key child columns (the
splitter builds its own child
forward configs with a fixed LZ4 raw compression and would silently
discard a per-key codecSpec), so an
explicit rejection is the honest behavior rather than silent config loss.
### Docs
- `FieldConfig.CompressionCodec` constants get "prefer codecSpec" guidance
in the new comma-list syntax
(`SNAPPY`→`"SNAPPY"`, `ZSTANDARD`→`"ZSTD(3)"`, `DELTA`→`"DELTA,LZ4"` with
the on-disk-semantics
caveat, etc.); `getCompressionCodec()` documents that it remains the only
way to express
`MV_ENTRY_DICT` and the CLP family.
- `TableConfigUtils` comment updated to the final behavior (codecSpec is
validated on the resolved
`ForwardIndexConfig` via `IndexType.validate`, so
`noDictionaryColumns`-resolved RAW columns work).
## Tests
- `ForwardIndexHandlerTest`: V7 no-op on unchanged canonical spec; V7
spec-change rewrite; V7
compression-only spec rewritten back to legacy raw; V7→legacy
`compressionCodec` rollback with full
value verification; legacy→equivalent codecSpec no-op; legacy→different
compression-only codecSpec
rewrite; legacy `DELTA`/`DELTADELTA` → V7 `DELTA,LZ4`/`DELTADELTA,LZ4`
migration reload preserving all
values across chunks.
- `TableConfigUtilsTest`: gate-rejection test replaced by
`testCodecSpecValidation` — accepts valid specs
(compression-only on any RAW column incl. STRING/MV, transform chains on
SV INT/LONG,
`noDictionaryColumns`-resolved RAW), rejects invalid ones with precise
errors (unknown codec, transform
after packing transform, V7-requiring specs on MV or non-INT/LONG columns).
- `ForwardIndexTypeTest`: mutable-index gate test replaced — a codecSpec
config now builds the standard
mutable forward index for realtime consumption.
- `ForwardIndexCreatorFactoryTest`: `shouldCreateIndex` gate test flipped to
acceptance.
- `OpenStructIndexTypeTest` / `OpenStructColumnSplitterTest`: assertions
updated to the permanent
rejection message.
## Deliberately excluded
- `CompressionCodecMigrator` and its tests (present in the original #18229)
are dropped from the series
entirely — legacy `DELTA`/`DELTADELTA` columns migrate through the normal
reload path instead, covered
by `testLegacyTransformMigrationReloadPreservesValues`.
- codecSpec on OPEN_STRUCT per-key child columns is explicitly rejected (see
above); no later PR in this
series adds it.
The final slice (integration tests + design doc) follows this PR but adds no
production code.
## Why master stays safe with only this merged
This PR is the gate-opener, so unlike the earlier slices it *does* activate
the feature — safety comes
from the pipeline being complete underneath it:
- All runtime pieces (DSL, validator, executor, transforms, V7 format,
creator/reader wiring) merged in
the previous five PRs; this PR only removes the gates and adds the reload
logic.
- No existing config changes behavior: columns without `codecSpec` follow
the exact pre-series code
paths (the legacy compression-change comparison is byte-for-byte the old
logic), and reload of a
segment with an unchanged config is a no-op.
- Rollback is first-class: reverting a table config from `codecSpec` to a
legacy `compressionCodec`
(or an equivalent compression-only spec) rewrites segments back to the
legacy formats readable by
pre-V7 servers.
- Realtime ingestion is unaffected: the mutable index ignores codecSpec; it
only applies at segment
conversion.
- Mixed-version caution still applies as designed for the series: V7
segments are only produced when an
operator explicitly configures a V7-requiring codecSpec, and older servers
cannot read them — the
standard guidance (finish the server rollout before adopting V7-requiring
specs) is unchanged from
#18229.
## Verification
```
./mvnw -q -T 1C install -DskipTests -Ppinot-fastdev -pl
pinot-spi,pinot-segment-spi,pinot-segment-local -am
./mvnw -q test -Ppinot-fastdev -pl pinot-segment-local \
-Dtest=ForwardIndexHandlerTest,TableConfigUtilsTest,ForwardIndexTypeTest,CodecPipelineForwardIndexTest,OpenStructIndexTypeTest,OpenStructColumnSplitterTest,ForwardIndexCreatorFactoryTest
\
-Dsurefire.failIfNoSpecifiedTests=false
./mvnw spotless:apply license:format -pl pinot-spi,pinot-segment-local
./mvnw checkstyle:check license:check -pl pinot-spi,pinot-segment-local
```
Result: all tests pass. The stack root is synced with master, so this branch
includes the #19282
chunk-caching fix that `testLegacyTransformMigrationReloadPreservesValues`
depends on (verified: a
523-test batch across the codec suite at the chain tip runs green, including
both migration variants
and `ForwardIndexHandlerContextTest`).
--
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]
