Copilot commented on code in PR #19282:
URL: https://github.com/apache/pinot/pull/19282#discussion_r3799923008
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/forward/FixedByteChunkSVForwardIndexTest.java:
##########
@@ -60,6 +60,73 @@ public static Object[][] combinations() {
.toArray(Object[][]::new);
}
+ @DataProvider(name = "deltaCompressions")
+ public static Object[][] deltaCompressions() {
+ return new Object[][]{
+ {ChunkCompressionType.DELTA},
+ {ChunkCompressionType.DELTADELTA}
+ };
+ }
+
+ @Test(dataProvider = "deltaCompressions")
+ public void testDeltaIntChunkCaching(ChunkCompressionType compressionType)
+ throws Exception {
+ int[] expected = {101, 103, 107, 109, 211, 223, 227, 229, 307};
+ File outputFile = new File(TEST_FILE + "-int-" + compressionType);
+ FileUtils.deleteQuietly(outputFile);
+
+ try {
+ try (FixedByteChunkForwardIndexWriter writer = new
FixedByteChunkForwardIndexWriter(outputFile,
+ compressionType, expected.length, 4, Integer.BYTES,
FixedBytePower2ChunkSVForwardIndexReader.VERSION)) {
Review Comment:
These regression tests hard-code format version 4 and therefore exercise
only `FixedBytePower2ChunkSVForwardIndexReader`. The corrected base method is
also used by `FixedByteChunkSVForwardIndexReader` for versions 2 and 3, while
the existing `combinations` provider explicitly excludes both delta codecs, so
those legacy reader paths still have no coverage. Parameterize these tests
across versions 2, 3, and 4 and select the corresponding reader, as the
existing tests below do.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/forward/BaseChunkForwardIndexReader.java:
##########
@@ -211,17 +211,17 @@ protected ByteBuffer decompressChunk(int chunkId,
ChunkReaderContext context) {
ByteBuffer decompressedBuffer = context.getChunkBuffer();
decompressedBuffer.clear();
+ // Invalidate the cached chunk before decoding. If decompression fails, a
subsequent read must
+ // retry instead of returning a partially-mutated buffer as a cache hit.
+ context.setChunkId(-1);
Review Comment:
The new failure-safety behavior is not covered by the added tests, which
only perform successful decompressions. Add a regression that first caches a
valid chunk, attempts to decode a malformed/failing chunk, and then rereads the
prior chunk; this verifies that a failed decode cannot leave a false cache hit
returning the partially mutated context buffer.
--
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]
