From 89ec8af7bacc19d9ac6415968036ee81588bcb2f Mon Sep 17 00:00:00 2001 From: Jack Luo Date: Thu, 17 Oct 2024 02:44:14 +0800 Subject: [PATCH] Addressed code review comments to use `getVersion()` in forward index reader to fetch version number. --- .../impl/VarByteChunkForwardIndexWriterV4.java | 10 ++++++++-- .../impl/VarByteChunkForwardIndexWriterV5.java | 14 ++++++++++---- .../forward/VarByteChunkForwardIndexReaderV4.java | 3 ++- .../forward/VarByteChunkForwardIndexReaderV5.java | 3 ++- 4 files changed, 22 insertions(+), 8 deletions(-) diff --git a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/io/writer/impl/VarByteChunkForwardIndexWriterV4.java b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/io/writer/impl/VarByteChunkForwardIndexWriterV4.java index f94fd64cee7..52e6cb45c6e 100644 --- a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/io/writer/impl/VarByteChunkForwardIndexWriterV4.java +++ b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/io/writer/impl/VarByteChunkForwardIndexWriterV4.java @@ -107,7 +107,13 @@ public VarByteChunkForwardIndexWriterV4(File file, ChunkCompressionType compress writeHeader(_chunkCompressor.compressionType(), chunkSize); } - public int getVersion() { + // Child class must shadow this static method + public static int getVersion() { + return VERSION; + } + + // Child class must override this class instance method + protected int getConcreteClassVersion() { return VERSION; } @@ -115,7 +121,7 @@ private void writeHeader(ChunkCompressionType compressionType, int targetDecompr throws IOException { // keep metadata BE for backwards compatibility // (e.g. the version needs to be read by a factory which assumes BE) - _output.writeInt(getVersion()); + _output.writeInt(getConcreteClassVersion()); _output.writeInt(targetDecompressedChunkSize); _output.writeInt(compressionType.getValue()); // reserve a slot to write the data offset into diff --git a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/io/writer/impl/VarByteChunkForwardIndexWriterV5.java b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/io/writer/impl/VarByteChunkForwardIndexWriterV5.java index a9f6fad975b..35b7b70f1f5 100644 --- a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/io/writer/impl/VarByteChunkForwardIndexWriterV5.java +++ b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/io/writer/impl/VarByteChunkForwardIndexWriterV5.java @@ -73,12 +73,12 @@ *

Note that the {@code VERSION} tag is a {@code static final} class variable set to {@code 5}. Since static * variables are shadowed in the child class thus associated with the class that defines them, care must be taken to * ensure that the parent class can correctly observe the child class's {@code VERSION} value at runtime. To handle - * this cleanly and correctly, the {@code getVersion()} method is overridden to return the concrete subclass's - * {@code VERSION} value, ensuring that the correct version number is returned even when using a reference + * this cleanly and correctly, the {@code getConcreteClassVersion()} method is overridden to return the concrete + * subclass's {@code VERSION} value, ensuring that the correct version number is returned even when using a reference * to the parent class.

* * @see VarByteChunkForwardIndexWriterV4 - * @see VarByteChunkForwardIndexWriterV5#getVersion() + * @see VarByteChunkForwardIndexWriterV5#getConcreteClassVersion() */ @NotThreadSafe public class VarByteChunkForwardIndexWriterV5 extends VarByteChunkForwardIndexWriterV4 { @@ -89,8 +89,14 @@ public VarByteChunkForwardIndexWriterV5(File file, ChunkCompressionType compress super(file, compressionType, chunkSize); } + // Hide the parent class getVersion() + public static int getVersion() { + return VERSION; + } + + // Override the parent class getConcreteClassVersion(); @Override - public int getVersion() { + public int getConcreteClassVersion() { return VERSION; } diff --git a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/forward/VarByteChunkForwardIndexReaderV4.java b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/forward/VarByteChunkForwardIndexReaderV4.java index b8b007ec8dd..ac1e739cb9f 100644 --- a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/forward/VarByteChunkForwardIndexReaderV4.java +++ b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/forward/VarByteChunkForwardIndexReaderV4.java @@ -82,7 +82,8 @@ public VarByteChunkForwardIndexReaderV4(PinotDataBuffer dataBuffer, FieldSpec.Da public void validateIndexVersion(PinotDataBuffer dataBuffer) { int version = dataBuffer.getInt(0); - Preconditions.checkState(version == VarByteChunkForwardIndexWriterV4.VERSION, "Illegal index version: %s", version); + Preconditions.checkState(version == VarByteChunkForwardIndexWriterV4.getVersion(), "Illegal index version: %s", + version); } @Override diff --git a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/forward/VarByteChunkForwardIndexReaderV5.java b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/forward/VarByteChunkForwardIndexReaderV5.java index 3221479c9f9..a3c7ba86074 100644 --- a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/forward/VarByteChunkForwardIndexReaderV5.java +++ b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/forward/VarByteChunkForwardIndexReaderV5.java @@ -40,7 +40,8 @@ public VarByteChunkForwardIndexReaderV5(PinotDataBuffer dataBuffer, FieldSpec.Da @Override public void validateIndexVersion(PinotDataBuffer dataBuffer) { int version = dataBuffer.getInt(0); - Preconditions.checkState(version == VarByteChunkForwardIndexWriterV5.VERSION, "Illegal index version: %s", version); + Preconditions.checkState(version == VarByteChunkForwardIndexWriterV5.getVersion(), "Illegal index version: %s", + version); } @Override