xiangfu0 opened a new pull request, #19307:
URL: https://github.com/apache/pinot/pull/19307
Stacked on #19306 (`xiangfu0/codex/codec-stack/04-t64-gorilla`). Part of the
split of #18229.
## Summary
Adds the **V7 on-disk raw forward index format** — the first format that can
persist a codec
pipeline — plus the factory routing that decides, per `codecSpec`, whether a
column can keep
using the legacy chunk formats or needs V7:
- **`FixedByteChunkForwardIndexWriterV7`** (new, `io.writer.impl`): chunked
SV fixed-byte writer
for INT/LONG. File header carries `version=7`, an explicit `FORMAT_MAGIC`
(`0xC0DEC0DE`)
discriminator, chunk geometry, and the **canonical codec spec** (UTF-8)
followed by a long[]
chunk-offset table; each chunk stores `encodedSize`/`decodedSize` and the
pipeline-encoded
payload. The writer validates the pipeline's composed encoded-size bound
and cumulative work
bound up front so it never emits a file it could not read back.
- **`FixedByteChunkSVForwardIndexReaderV7`** (new, `readers.forward`):
self-describing reader
that re-creates the `CodecPipelineExecutor` **from the header string** and
fully validates the
header (chunk geometry consistency, spec length, offset-table
monotonicity/bounds, per-chunk
sizes against the composed pipeline bound) before any allocation sized
from file data.
- **`FixedByteValueWriter`** (new interface) + retrofit on
`FixedByteChunkForwardIndexWriter`, so
`SingleValueFixedByteRawIndexCreator` holds a single writer reference and
gains a V7
constructor taking a `CodecPipelineExecutor`.
- **`CodecSpecUtils`** (new, public, `io.codec`): classification helper —
`toLegacyChunkCompressionType(...)` (compression-only specs that map 1:1
to a legacy
`ChunkCompressionType`, e.g. `LZ4`, `ZSTD(3)`) and `hasTransform(...)`.
Alongside
`CodecPipelineExecutor` this is the only public entry into the
package-private codec runtime.
- **`ForwardIndexCreatorFactory`**: `codecSpec` routing — legacy-mappable
compression-only specs
go to the existing raw writers (byte-identical files to a legacy
`compressionCodec` config);
transforms/chains/non-default options go to the V7 writer (SV INT/LONG
only, with
defense-in-depth checks).
- **`ForwardIndexReaderFactory`**: V7 dispatch keyed on the **second-int
`FORMAT_MAGIC` check
BEFORE the `version >= 4` power-of-2 fallback** (legacy fixed-byte writers
accept arbitrary
versions ≥ 4, so the version integer alone can never identify V7), plus a
truncation check
before reading the version int.
- **SPI**: `ForwardIndexConfig.CODEC_PIPELINE_WRITER_VERSION = 7` and
`ForwardIndexReader.getCodecSpec()` default method (V7 readers return the
canonical header
spec; every other reader returns `null`).
- **`ForwardIndexHandler` guard (~6 lines)**: in
`shouldChangeRawCompressionType`, if the
existing reader reports `getCodecSpec() != null`, return `false`
**before** the
`existingCompressionType != null` precondition. V7 segments are never
rewritten yet (the next
PR owns reload/rewrite). The `computeOperations` gate already rejects
codecSpec **configs**;
this guard protects against V7 **segments** on disk (e.g. tooling-created)
paired with a
non-codec config transition.
### Canonicalization (frozen into on-disk headers)
The V7 writer embeds `executor.getCanonicalSpec()` — never
`ForwardIndexConfig`'s stored string,
which is only a structural normalization (`toDslString`) and can differ
(e.g. `ZSTANDARD` vs
`ZSTD(3)`). The reader re-creates the executor from the header string, so
the header is always
in executor-canonical form (covered by `testCanonicalSpecStoredInHeader`).
## Gate relocation (deliberate, explicit)
The codec stack's closed feature gate (`codecSpec is not supported yet for
column: %s`) was
planted at every config/creator/reader/reload surface in #19284. **This PR
removes exactly two
factory-level gates** because it makes those code paths real:
- `ForwardIndexCreatorFactory.createIndexCreator` — now routes instead of
throwing.
- `ForwardIndexReaderFactory.createIndexReader` (both overrides) — now reads
instead of throwing.
**All config-surface gates remain:** `ForwardIndexType.validate`,
`ForwardIndexType.shouldCreateIndex`, `ForwardIndexType.createMutableIndex`,
`ForwardIndexHandler.computeOperations` (`rejectUnsupportedCodecSpecs`), and
the OPEN_STRUCT
sites. Net effect: **no table config carrying `codecSpec` can validate,
build a segment, create
a mutable index, or reload** — so no V7 segment is creatable from config —
but the format code
itself and its direct-construction tests work end to end.
## Deliberately excluded (next PR: 06-reload-enable)
- Opening the feature gate (removing the remaining
`rejectUnsupportedCodecSpec` sites) and
table-config validation of codecSpec column types (`validateCodecSpec`).
- `ForwardIndexHandler` rewrite support for codec-pipeline segments
(codecSpec change
detection via canonical-spec comparison, legacy↔V7 conversions on reload).
- Mutable/realtime forward index support for codecSpec.
## Why master stays safe with only this merged
- The gate keeps `codecSpec` unusable from any table config, so the new
writer/reader can only
be reached by direct construction (tests/tools). No production config can
produce a V7 file.
- V7 reader dispatch triggers only on the `0xC0DEC0DE` second-int magic,
which no legacy writer
emits; all legacy version ≥ 4 files keep their exact previous routing
(locked by
`ForwardIndexReaderFactoryBackwardCompatTest`, including golden base64
fixtures produced
before this code existed).
- The handler guard only short-circuits columns whose on-disk index already
reports a codec
spec, i.e. V7 segments that could not exist via config today; legacy
columns follow the exact
previous logic.
## Verification
```
./mvnw -q -T 1C install -DskipTests -Ppinot-fastdev -pl
pinot-segment-spi,pinot-segment-local -am
./mvnw -q test -Ppinot-fastdev -pl pinot-segment-local \
-Dtest=CodecPipelineForwardIndexTest,FixedByteChunkSVForwardIndexReaderV7CorruptionTest,\
ForwardIndexCreatorFactoryTest,ForwardIndexReaderFactoryTest,ForwardIndexReaderFactoryBackwardCompatTest,\
CodecSpecUtilsTest,CompressionCodecCorruptInputTest,ForwardIndexHandlerTest \
-Dsurefire.failIfNoSpecifiedTests=false
# precommit
./mvnw spotless:apply license:format -pl
pinot-segment-spi,pinot-segment-local
./mvnw checkstyle:check license:check -pl
pinot-segment-spi,pinot-segment-local
```
--
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]
