xiangfu0 commented on code in PR #18229:
URL: https://github.com/apache/pinot/pull/18229#discussion_r3330767403
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/fwd/SingleValueFixedByteRawIndexCreator.java:
##########
@@ -20,57 +20,71 @@
import java.io.File;
import java.io.IOException;
+import java.util.Objects;
+import javax.annotation.Nullable;
+import org.apache.pinot.segment.local.io.codec.CodecPipelineExecutor;
import
org.apache.pinot.segment.local.io.writer.impl.FixedByteChunkForwardIndexWriter;
+import
org.apache.pinot.segment.local.io.writer.impl.FixedByteChunkForwardIndexWriterV7;
import org.apache.pinot.segment.spi.V1Constants;
import org.apache.pinot.segment.spi.compression.ChunkCompressionType;
import org.apache.pinot.segment.spi.index.ForwardIndexConfig;
import org.apache.pinot.segment.spi.index.creator.ForwardIndexCreator;
import org.apache.pinot.spi.data.FieldSpec.DataType;
-/**
- * Raw (non-dictionary-encoded) forward index creator for single-value column
of fixed length data type (INT, LONG,
- * FLOAT, DOUBLE).
- */
+/// Raw (non-dictionary-encoded) forward index creator for single-value column
of fixed length
+/// data type (`INT`, `LONG`, `FLOAT`, `DOUBLE`).
public class SingleValueFixedByteRawIndexCreator implements
ForwardIndexCreator {
+ @Nullable
Review Comment:
Done — the creator now holds a single `FixedByteValueWriter` reference and
delegates `put*`/`close` with no branching. The factory creates the concrete
writer (legacy vs V7) and the creator no longer maintains two writers.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/io/writer/impl/FixedByteChunkForwardIndexWriterV7.java:
##########
@@ -0,0 +1,277 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.segment.local.io.writer.impl;
+
+import java.io.Closeable;
+import java.io.File;
+import java.io.IOException;
+import java.io.RandomAccessFile;
+import java.io.UncheckedIOException;
+import java.nio.ByteBuffer;
+import java.nio.channels.FileChannel;
+import java.nio.charset.StandardCharsets;
+import javax.annotation.concurrent.NotThreadSafe;
+import org.apache.pinot.segment.local.io.codec.CodecPipelineExecutor;
+import org.apache.pinot.segment.spi.index.ForwardIndexConfig;
+
+
+/// Chunk-based raw (non-dictionary-encoded) forward index writer for
single-value fixed-width
+/// columns (INT, LONG) that uses a [CodecPipelineExecutor] for encoding.
+///
+/// This writer introduces **version 7** of the fixed-byte chunk raw forward
index
+/// format. The on-disk layout is:
+///
+/// ```
+/// File header:
+/// version (int, value = 7)
+/// numChunks (int)
+/// numDocsPerChunk (int, normalised to power-of-2)
+/// sizeOfEntry (int, bytes per logical value, e.g. 4 for INT)
+/// totalDocs (int)
+/// codecSpecLength (int, byte length of the UTF-8 encoded canonical
codec spec)
+/// dataHeaderStart (int, byte offset from file start where
chunk-offset table begins)
+/// codecSpec (byte[], UTF-8 encoded canonical spec, length =
codecSpecLength)
+/// chunkOffsets (long[numChunks], absolute byte offset of each
chunk's per-chunk header)
+/// Data (per chunk):
+/// compressedSize (int, byte length of the encoded payload that
follows)
+/// uncompressedSize (int, byte length of the original decoded chunk
data)
+/// payload (byte[], encoded chunk data, length =
compressedSize)
+/// ```
+///
+/// Each chunk contains `numDocsPerChunk` values encoded by the pipeline.
Chunk offsets
+/// are 8-byte longs to support files larger than 2 GB. The per-chunk size
header allows readers
+/// to verify decompression output and to skip/read chunks without scanning
adjacent offsets.
+///
+/// This class is *not* thread-safe.
+@NotThreadSafe
+public class FixedByteChunkForwardIndexWriterV7 implements Closeable {
Review Comment:
Both writers now implement a shared `FixedByteValueWriter` interface, so
callers hold one writer reference regardless of on-disk format. On the overhead
question: we intentionally kept two on-disk formats. Plain
`compressionCodec`/compression-only `codecSpec` columns still use the unchanged
legacy `FixedByteChunkForwardIndexWriter` (byte-identical output, zero added
overhead, and readable by pre-V7 servers). The V7 writer is used only where the
legacy chunk header cannot represent the spec — transforms
(DELTA/DELTADELTA/T64/GORILLA) and non-default compression args (e.g. ZSTD(5)).
Routing all raw columns through V7 would have changed every existing raw
column's on-disk format and broken pre-V7 server reads, so unification was done
at the writer-interface level rather than by collapsing the formats.
--
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]
