xiangfu0 commented on PR #19285: URL: https://github.com/apache/pinot/pull/19285#issuecomment-5336117392
Pushed \`bfe108c3d9\` (rebased onto the updated #19284 head) addressing review findings on the runtime. **Snappy decode size cap, fixed as a pattern rather than a fourth copy.** \`SnappyCodecDefinition.decode\` allocated from the header-declared decompressed size with only a negative check, while LZ4/GZIP/ZSTD each carried a 1 GiB sanity cap. That gap is exactly the drift a per-codec hand-rolled constant produces, so instead of adding a fourth private copy I hoisted it into \`CodecBufferUtils.checkDeclaredDecompressedSize(declared, codec, source)\` and routed all four codecs through it. A future codec cannot omit the bound, and the policy is single-sourced. New corrupt-input tests cover both Snappy paths — including \`decodeInto\`, which is the one the bounded executor actually uses in production — at the boundary (cap + 1), plus a garbage-input test Snappy was missing. **\`CodecRegistry.register()\` rejects the reserved name \`CODEC\`** as defense in depth alongside the parser and \`CodecInvocation\` reservations, referencing \`CodecSpecParser.REMOVED_WRAPPER_NAME\` (made public in #19284) rather than re-spelling the literal. **\`ZSTD(0)\` is now rejected**; the accepted range is \`[1, maxCompressionLevel]\`. zstd treats level 0 as "use the default level", so \`ZSTD(0)\` and \`ZSTD(3)\` would be identical behavior under two canonical spellings — the same one-spelling-per-behavior rule that motivates the leading-zero fix, and worth settling before the header format freezes. Use bare \`ZSTD\` for the default. One follow-up worth filing separately (deliberately not fixed here, since it is a different long-shipped subsystem): the legacy \`SnappyDecompressor.decompressedLength()\` in \`io/compression\` has the same unbounded-header shape, and shipped callers such as \`VarByteChunkForwardIndexReaderV4\` allocate from it. Verification: 53 codec tests plus 177 downstream gate/validation/handler tests green (\`CodecPipelineExecutorTest\`, \`CodecPipelineValidatorTest\`, \`CodecRegistryTest\`, \`CompressionCodecCorruptInputTest\`, \`ZstdCodecDefinitionTest\`, \`ForwardIndexTypeTest\`, \`TableConfigUtilsTest\`, \`ForwardIndexCreatorFactoryTest\`, \`OpenStructIndexTypeTest\`, \`ForwardIndexHandlerTest\`); spotless/checkstyle/license clean. -- 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]
