xiangfu0 commented on PR #19308:
URL: https://github.com/apache/pinot/pull/19308#issuecomment-5340441439
Pushed `aafe8c6fd7` correcting the `CompressionCodec` Javadoc this PR adds.
Two issues, both found while assessing whether `compressionCodec` could be
deprecated in favour of `codecSpec`:
**1. The DELTA/DELTADELTA guidance described a migration that is not
possible.** The previous wording ("For new SV INT/LONG columns, prefer
`codecSpec=...`" / "migrating to codecSpec changes on-disk semantics") implied
users have a working `compressionCodec: DELTA` config to migrate from. They do
not: both constants are declared `(false, false)`, so
`ForwardIndexType.validateForwardIndexEnabled` rejects them on the dictionary
branch (`isApplicableToDictEncodedIndex()`) and on the raw branch
(`isApplicableToRawIndex()`) alike. I verified this empirically rather than by
inspection — a table config with `compressionCodec: DELTA` on a RAW
**single-value INT** column, the exact shape the codec targets, fails
validation:
```
IllegalStateException: Compression codec: DELTA is not applicable to raw
column: intCol
```
Same for DELTADELTA and for LONG. Note this makes
`TableConfigUtils.validateGorillaCompressionCodecIfPresent` (the SV / INT-LONG
checks for DELTA and DELTADELTA) effectively unreachable on the accept path —
the applicability check fires first. That is pre-existing and out of scope
here, but worth knowing.
The Javadoc now states plainly that these values are rejected by
table-config validation and points to `codecSpec="DELTA,LZ4"` as the supported
route, rather than framing it as a migration.
**2. The six constants with no `codecSpec` equivalent had no Javadoc at
all**, which read as an oversight next to the four annotated ones.
PASS_THROUGH, MV_ENTRY_DICT and the CLP family now each say so explicitly and
why:
- **PASS_THROUGH** — no equivalent; the DSL has no identity codec and
rejects a blank spec, and this is the default for METRIC columns, so it stays
the only way to state "uncompressed".
- **MV_ENTRY_DICT** — no equivalent; `codecSpec` applies only to RAW forward
indexes.
- **CLP family** — no equivalent; whole-index formats for STRING columns
rather than chunk codecs, validated against stored type instead of the
applicability flags.
I also fixed the malformed `{@link CLPForwardIndexCreatorV2)` reference in
that comment (mismatched brace/paren, and those classes are not reachable from
`pinot-spi` anyway) by using code spans.
Net effect: the guidance now covers all 12 constants and matches what
validation actually does. Comment-only change, no behavior difference.
Verification: `pinot-spi` compiles; `IndexCombinationValidationTest`,
`TableConfigUtilsTest`, `ForwardIndexTypeTest` all green (169 tests);
spotless/checkstyle/license clean. #19309 has been rebased onto the new head so
the chain stays intact.
--
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]
