Skip to content

Commit

Permalink
Addressed code review comments to use getVersion() in forward index…
Browse files Browse the repository at this point in the history
… reader to fetch version number.
  • Loading branch information
jackluo923 committed Oct 16, 2024
1 parent 0da0ca7 commit 89ec8af
Show file tree
Hide file tree
Showing 4 changed files with 22 additions and 8 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -107,15 +107,21 @@ 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;
}

private void writeHeader(ChunkCompressionType compressionType, int targetDecompressedChunkSize)
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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,12 +73,12 @@
* <p>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.</p>
*
* @see VarByteChunkForwardIndexWriterV4
* @see VarByteChunkForwardIndexWriterV5#getVersion()
* @see VarByteChunkForwardIndexWriterV5#getConcreteClassVersion()
*/
@NotThreadSafe
public class VarByteChunkForwardIndexWriterV5 extends VarByteChunkForwardIndexWriterV4 {
Expand All @@ -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;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 89ec8af

Please sign in to comment.