Skip to content

Commit 3f067db

Browse files
xiangfu0claude
andcommitted
Address PR review comments
- Allow transformCodec=NONE on dict-encoded columns (treat NONE same as null in validation) - Use explicit value-based lookup in TransformCodec.valueOf(int) instead of ordinal to prevent silent corruption if enum is reordered - Fix empty-array tests to actually exercise encode/decode with numBytes=0 instead of returning early - Fix misleading comment in DoubleDeltaTransform.encodeInts() that said "reverse order" when the code encodes forward Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent b84269a commit 3f067db

4 files changed

Lines changed: 33 additions & 19 deletions

File tree

pinot-segment-local/src/main/java/org/apache/pinot/segment/local/io/compression/DoubleDeltaTransform.java

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -68,9 +68,8 @@ private void encodeInts(ByteBuffer buffer, int numBytes) {
6868
}
6969
int pos = buffer.position();
7070

71-
// Encode in reverse to avoid overwriting values we still need.
72-
// First pass: compute all original values we need (they're in the buffer).
73-
// We encode from the end to index 2, then handle index 1.
71+
// Encode in forward order, keeping the previous original value and delta in local variables
72+
// so we can safely overwrite each position in-place as we advance from index 2 onward.
7473
int prevPrevVal = buffer.getInt(pos);
7574
int prevVal = buffer.getInt(pos + Integer.BYTES);
7675
int prevDelta = prevVal - prevPrevVal;

pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/forward/ForwardIndexType.java

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -113,7 +113,11 @@ private void validateForwardIndexEnabled(ForwardIndexConfig forwardIndexConfig,
113113
if (dictionaryConfig.isEnabled()) {
114114
Preconditions.checkState(compressionCodec == null || compressionCodec.isApplicableToDictEncodedIndex(),
115115
"Compression codec: %s is not applicable to dictionary encoded column: %s", compressionCodec, column);
116-
Preconditions.checkState(forwardIndexConfig.getTransformCodec() == null,
116+
org.apache.pinot.segment.spi.compression.TransformCodec dictTransformCodec =
117+
forwardIndexConfig.getTransformCodec();
118+
Preconditions.checkState(
119+
dictTransformCodec == null
120+
|| dictTransformCodec == org.apache.pinot.segment.spi.compression.TransformCodec.NONE,
117121
"Transform codec is not supported for dictionary-encoded column: %s", column);
118122
} else {
119123
boolean isCLPCodec = compressionCodec == CompressionCodec.CLP || compressionCodec == CompressionCodec.CLPV2

pinot-segment-local/src/test/java/org/apache/pinot/segment/local/io/compression/ChunkTransformTest.java

Lines changed: 6 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -191,16 +191,14 @@ public void testDoubleDeltaRandomLongs() {
191191
// --- Helpers ---
192192

193193
private void assertRoundTripInts(ChunkTransform transform, int[] original) {
194-
if (original.length == 0) {
195-
return;
196-
}
197-
ByteBuffer buffer = ByteBuffer.allocate(original.length * Integer.BYTES);
194+
int numBytes = original.length * Integer.BYTES;
195+
ByteBuffer buffer = ByteBuffer.allocate(Math.max(numBytes, 1));
198196
for (int v : original) {
199197
buffer.putInt(v);
200198
}
201199
buffer.flip();
202200

203-
int numBytes = original.length * Integer.BYTES;
201+
// Exercise encode/decode even on empty buffers to verify no-throw behavior
204202
transform.encode(buffer, numBytes, Integer.BYTES);
205203
transform.decode(buffer, numBytes, Integer.BYTES);
206204

@@ -211,16 +209,14 @@ private void assertRoundTripInts(ChunkTransform transform, int[] original) {
211209
}
212210

213211
private void assertRoundTripLongs(ChunkTransform transform, long[] original) {
214-
if (original.length == 0) {
215-
return;
216-
}
217-
ByteBuffer buffer = ByteBuffer.allocate(original.length * Long.BYTES);
212+
int numBytes = original.length * Long.BYTES;
213+
ByteBuffer buffer = ByteBuffer.allocate(Math.max(numBytes, 1));
218214
for (long v : original) {
219215
buffer.putLong(v);
220216
}
221217
buffer.flip();
222218

223-
int numBytes = original.length * Long.BYTES;
219+
// Exercise encode/decode even on empty buffers to verify no-throw behavior
224220
transform.encode(buffer, numBytes, Long.BYTES);
225221
transform.decode(buffer, numBytes, Long.BYTES);
226222

pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/compression/TransformCodec.java

Lines changed: 20 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,19 @@ public enum TransformCodec {
4646
*/
4747
DOUBLE_DELTA(2);
4848

49-
private static final TransformCodec[] VALUES = values();
49+
private static final TransformCodec[] BY_VALUE;
50+
51+
static {
52+
TransformCodec[] all = values();
53+
int maxValue = 0;
54+
for (TransformCodec tc : all) {
55+
maxValue = Math.max(maxValue, tc._value);
56+
}
57+
BY_VALUE = new TransformCodec[maxValue + 1];
58+
for (TransformCodec tc : all) {
59+
BY_VALUE[tc._value] = tc;
60+
}
61+
}
5062

5163
private final int _value;
5264

@@ -58,10 +70,13 @@ public int getValue() {
5870
return _value;
5971
}
6072

61-
public static TransformCodec valueOf(int ordinal) {
62-
if (ordinal < 0 || ordinal >= VALUES.length) {
63-
throw new IllegalArgumentException("Invalid TransformCodec ordinal: " + ordinal);
73+
/**
74+
* Look up a TransformCodec by its on-disk int value (not ordinal).
75+
*/
76+
public static TransformCodec valueOf(int value) {
77+
if (value < 0 || value >= BY_VALUE.length || BY_VALUE[value] == null) {
78+
throw new IllegalArgumentException("Invalid TransformCodec value: " + value);
6479
}
65-
return VALUES[ordinal];
80+
return BY_VALUE[value];
6681
}
6782
}

0 commit comments

Comments
 (0)