xiangfu0 commented on PR #19284:
URL: https://github.com/apache/pinot/pull/19284#issuecomment-5366774581
## 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.
--
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]
