This is an automated email from the ASF dual-hosted git repository. xiangfu0 pushed a commit to branch xiangfu0/codec-stack/01-config in repository https://gitbox.apache.org/repos/asf/pinot.git
commit b0846dad5b73b7e3dabf411f336b42f1461ba913 Author: Xiang Fu <[email protected]> AuthorDate: Wed Aug 19 12:50:57 2026 -0700 Pin codec DSL equality and limit symmetry with tests The canonical spec produced here is frozen into segment headers and compared for rewrite detection by later layers in this stack, but CodecInvocation and CodecPipeline had no equals/hashCode coverage, and the documented structural limits were only exercised on the reject side. Add tests that pin the frozen contract: - a parsed invocation/pipeline equals the programmatically built equivalent and agrees on hashCode, argument-less invocations canonicalize alike, and stage order is significant - every documented limit (identifier length, argument length, arguments per invocation, pipeline stages) is checked at its accept boundary as well as one over, through both the parser and the AST constructors, which are independent entry points Test-only; no production behavior changes. --- .../segment/spi/codec/CodecInvocationTest.java | 46 ++++++++++++++++++++++ .../pinot/segment/spi/codec/CodecPipelineTest.java | 36 +++++++++++++++++ .../segment/spi/codec/CodecSpecParserTest.java | 10 +++-- 3 files changed, 89 insertions(+), 3 deletions(-) diff --git a/pinot-segment-spi/src/test/java/org/apache/pinot/segment/spi/codec/CodecInvocationTest.java b/pinot-segment-spi/src/test/java/org/apache/pinot/segment/spi/codec/CodecInvocationTest.java index 3803aa4838b..bcebce831a1 100644 --- a/pinot-segment-spi/src/test/java/org/apache/pinot/segment/spi/codec/CodecInvocationTest.java +++ b/pinot-segment-spi/src/test/java/org/apache/pinot/segment/spi/codec/CodecInvocationTest.java @@ -20,10 +20,12 @@ package org.apache.pinot.segment.spi.codec; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collections; import java.util.List; import org.testng.annotations.Test; import static org.testng.Assert.assertEquals; +import static org.testng.Assert.assertNotEquals; import static org.testng.Assert.assertThrows; @@ -51,4 +53,48 @@ public class CodecInvocationTest { assertThrows(IllegalArgumentException.class, () -> new CodecInvocation("ZSTD", List.of("-1"))); assertThrows(IllegalArgumentException.class, () -> new CodecInvocation("ZSTD", List.of("03"))); } + + @Test + public void testEqualityFollowsTheCanonicalForm() { + // Later layers freeze the canonical spec into segment headers and compare it to detect rewrites, so two + // spellings of the same invocation must be equal and agree on their hash code. + CodecInvocation parsed = CodecSpecParser.parse("zstd( 3 )").stages().get(0); + CodecInvocation built = new CodecInvocation("ZSTD", List.of("3")); + assertEquals(built, parsed); + assertEquals(built.hashCode(), parsed.hashCode()); + assertEquals(built.toString(), "ZSTD(3)"); + + assertNotEquals(built, new CodecInvocation("ZSTD", List.of("4"))); + assertNotEquals(built, new CodecInvocation("LZ4", List.of("3"))); + assertNotEquals(built, new CodecInvocation("ZSTD", List.of())); + assertNotEquals(built, "ZSTD(3)"); + + // An argument-less invocation renders without parentheses, so "DELTA()" and "DELTA" canonicalize alike. + CodecInvocation noArgs = new CodecInvocation("DELTA", List.of()); + assertEquals(noArgs.toString(), "DELTA"); + assertEquals(CodecSpecParser.parse("DELTA()").stages().get(0), noArgs); + assertEquals(CodecSpecParser.parse("DELTA").stages().get(0), noArgs); + } + + @Test + public void testStructuralLimitsMatchTheParser() { + // Codec definitions build invocations programmatically, bypassing the parser, so both entry points must + // enforce the same bounds — on the accept side as well as the reject side. + String maxName = "A".repeat(CodecSpecParser.MAX_IDENTIFIER_LENGTH); + assertEquals(new CodecInvocation(maxName, List.of()).name(), maxName); + assertThrows(IllegalArgumentException.class, () -> new CodecInvocation(maxName + "A", List.of())); + assertThrows(IllegalArgumentException.class, () -> new CodecInvocation("", List.of())); + + String maxArg = "1".repeat(CodecSpecParser.MAX_ARGUMENT_LENGTH); + assertEquals(new CodecInvocation("ZSTD", List.of(maxArg)).args(), List.of(maxArg)); + assertThrows(IllegalArgumentException.class, () -> new CodecInvocation("ZSTD", List.of(maxArg + "1"))); + assertThrows(IllegalArgumentException.class, () -> new CodecInvocation("ZSTD", List.of(""))); + + List<String> maxArgs = Collections.nCopies(CodecSpecParser.MAX_ARGS_PER_INVOCATION, "1"); + assertEquals(new CodecInvocation("ZSTD", maxArgs).args().size(), CodecSpecParser.MAX_ARGS_PER_INVOCATION); + List<String> tooManyArgs = Collections.nCopies(CodecSpecParser.MAX_ARGS_PER_INVOCATION + 1, "1"); + assertThrows(IllegalArgumentException.class, () -> new CodecInvocation("ZSTD", tooManyArgs)); + + assertThrows(NullPointerException.class, () -> new CodecInvocation("ZSTD", null)); + } } diff --git a/pinot-segment-spi/src/test/java/org/apache/pinot/segment/spi/codec/CodecPipelineTest.java b/pinot-segment-spi/src/test/java/org/apache/pinot/segment/spi/codec/CodecPipelineTest.java index cc1d7adf5e1..2f167b7d570 100644 --- a/pinot-segment-spi/src/test/java/org/apache/pinot/segment/spi/codec/CodecPipelineTest.java +++ b/pinot-segment-spi/src/test/java/org/apache/pinot/segment/spi/codec/CodecPipelineTest.java @@ -20,10 +20,12 @@ package org.apache.pinot.segment.spi.codec; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collections; import java.util.List; import org.testng.annotations.Test; import static org.testng.Assert.assertEquals; +import static org.testng.Assert.assertNotEquals; import static org.testng.Assert.assertSame; import static org.testng.Assert.assertThrows; @@ -50,4 +52,38 @@ public class CodecPipelineTest { assertThrows(NullPointerException.class, () -> new CodecPipeline(Arrays.asList((CodecInvocation) null))); } + + @Test + public void testEqualityFollowsTheCanonicalForm() { + // Rewrite detection in later layers compares canonical specs, so a parsed pipeline and the equivalent + // programmatically built one must be equal and agree on their hash code. + CodecPipeline parsed = CodecSpecParser.parse(" delta , zstd( 3 ) "); + CodecPipeline built = new CodecPipeline( + List.of(new CodecInvocation("DELTA", List.of()), new CodecInvocation("ZSTD", List.of("3")))); + assertEquals(built, parsed); + assertEquals(built.hashCode(), parsed.hashCode()); + assertEquals(built.toDslString(), "DELTA,ZSTD(3)"); + assertEquals(built.toString(), built.toDslString()); + + // Stage order is significant: a pipeline is an ordered list, not a set. + assertNotEquals(built, new CodecPipeline( + List.of(new CodecInvocation("ZSTD", List.of("3")), new CodecInvocation("DELTA", List.of())))); + assertNotEquals(built, CodecSpecParser.parse("DELTA")); + assertNotEquals(built, "DELTA,ZSTD(3)"); + } + + @Test + public void testStageLimitMatchesTheParser() { + // The parser and the constructor are independent entry points and must agree on the accept boundary. + List<CodecInvocation> maxStages = + Collections.nCopies(CodecSpecParser.MAX_PIPELINE_STAGES, new CodecInvocation("LZ4", List.of())); + assertEquals(new CodecPipeline(maxStages).stages().size(), CodecSpecParser.MAX_PIPELINE_STAGES); + assertEquals( + CodecSpecParser.parse(String.join(",", Collections.nCopies(CodecSpecParser.MAX_PIPELINE_STAGES, "LZ4"))) + .stages().size(), CodecSpecParser.MAX_PIPELINE_STAGES); + + List<CodecInvocation> tooManyStages = + Collections.nCopies(CodecSpecParser.MAX_PIPELINE_STAGES + 1, new CodecInvocation("LZ4", List.of())); + assertThrows(IllegalArgumentException.class, () -> new CodecPipeline(tooManyStages)); + } } diff --git a/pinot-segment-spi/src/test/java/org/apache/pinot/segment/spi/codec/CodecSpecParserTest.java b/pinot-segment-spi/src/test/java/org/apache/pinot/segment/spi/codec/CodecSpecParserTest.java index 6c649b01ac5..7bb8a23f549 100644 --- a/pinot-segment-spi/src/test/java/org/apache/pinot/segment/spi/codec/CodecSpecParserTest.java +++ b/pinot-segment-spi/src/test/java/org/apache/pinot/segment/spi/codec/CodecSpecParserTest.java @@ -93,10 +93,14 @@ public class CodecSpecParserTest { // still parses. assertEquals( CodecSpecParser.parse("LZ4" + " ".repeat(CodecSpecParser.MAX_SPEC_LENGTH - 3)).toDslString(), "LZ4"); + String maximumName = "A".repeat(CodecSpecParser.MAX_IDENTIFIER_LENGTH); + assertEquals(CodecSpecParser.parse(maximumName).stages().get(0).name(), maximumName); + assertThrows(IllegalArgumentException.class, () -> CodecSpecParser.parse(maximumName + "A")); + String maximumArgument = "1".repeat(CodecSpecParser.MAX_ARGUMENT_LENGTH); + assertEquals(CodecSpecParser.parse("ZSTD(" + maximumArgument + ")").stages().get(0).args(), + List.of(maximumArgument)); assertThrows(IllegalArgumentException.class, - () -> CodecSpecParser.parse("A".repeat(CodecSpecParser.MAX_IDENTIFIER_LENGTH + 1))); - assertThrows(IllegalArgumentException.class, - () -> CodecSpecParser.parse("ZSTD(" + "1".repeat(CodecSpecParser.MAX_ARGUMENT_LENGTH + 1) + ")")); + () -> CodecSpecParser.parse("ZSTD(" + maximumArgument + "1)")); String maximumArguments = String.join(",", Collections.nCopies(CodecSpecParser.MAX_ARGS_PER_INVOCATION, "1")); assertEquals(CodecSpecParser.parse("TEST(" + maximumArguments + ")").stages().get(0).args().size(), --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
