Jackie-Jiang commented on code in PR #18229:
URL: https://github.com/apache/pinot/pull/18229#discussion_r3326313136
##########
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:
Do not maintain 2 index writers in this class. You can create the index
writer and pass it in from the caller
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/FieldConfig.java:
##########
@@ -76,7 +76,12 @@ public class FieldConfig extends BaseJsonConfig {
private final List<IndexType> _indexTypes;
private final JsonNode _indexes;
private final JsonNode _tierOverwrites;
+ /// Raw forward-index compression codec. New raw configs should prefer
[#getCodecSpec()] for
+ /// the codecs the DSL can express (LZ4, SNAPPY, GZIP, ZSTD, DELTA,
DELTADELTA, T64, GORILLA),
+ /// but this field is still load-bearing for `MV_ENTRY_DICT`, the CLP
family, and any existing
+ /// configs on disk — so it is **not** deprecated until those migrate.
private final CompressionCodec _compressionCodec;
+ private final String _codecSpec;
Review Comment:
Do not add it in the top level (this is legacy behavior). Only add it into
`ForwardIndexConfig`
##########
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:
Keeping 2 writers causes tech debts. I assume the new framework covers all
existing codecs. Can we use the same writer to handle both new and legacy
config? Do you see overhead in the new writer?
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/io/codec/CodecPipelineExecutor.java:
##########
@@ -0,0 +1,233 @@
+/**
+ * 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.codec;
+
+import com.google.common.base.Preconditions;
+import java.io.IOException;
+import java.nio.ByteBuffer;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.pinot.segment.spi.codec.ChunkCodecHandler;
+import org.apache.pinot.segment.spi.codec.CodecContext;
+import org.apache.pinot.segment.spi.codec.CodecInvocation;
+import org.apache.pinot.segment.spi.codec.CodecKind;
+import org.apache.pinot.segment.spi.codec.CodecOptions;
+import org.apache.pinot.segment.spi.codec.CodecPipeline;
+import org.apache.pinot.segment.spi.codec.CodecSpecParser;
+
+
+/// Executes a parsed and validated [CodecPipeline] for a single forward-index
chunk.
+///
+/// Write path: values → transforms (in order) → compression → bytes stored on
disk.
+/// Read path: bytes from disk → decompress → reverse transforms (in reverse
order) → values.
+///
+/// The executor is constructed once per column from the canonical `codecSpec`
string
+/// stored in the file header and is thread-safe for concurrent read calls (it
holds no mutable
+/// per-call state).
+///
+/// The executor is codec-agnostic: it holds an ordered list of [BoundStage]
objects,
+/// each pairing a [ChunkCodecHandler] with its parsed [CodecOptions]. Codec
logic
+/// lives entirely in the handler implementations; this class only drives the
pipeline loop.
+///
+/// ### Buffer contract
+///
+/// - [#compress()]: `src` is ready for read (position=0); returns a new
Review Comment:
There are a lot of invalid references. Please fix them
```suggestion
/// - [#compress]: `src` is ready for read (position=0); returns a new
```
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/io/codec/AbstractDeltaCodecDefinition.java:
##########
@@ -0,0 +1,178 @@
+/**
+ * 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.codec;
+
+import java.nio.ByteBuffer;
+import java.util.List;
+import org.apache.pinot.segment.spi.codec.ChunkCodecHandler;
+import org.apache.pinot.segment.spi.codec.CodecContext;
+import org.apache.pinot.segment.spi.codec.CodecKind;
+import org.apache.pinot.segment.spi.codec.CodecOptions;
+import org.apache.pinot.spi.data.FieldSpec.DataType;
+
+
+/// Shared structure for [DeltaCodecDefinition] and
[DeltaDeltaCodecDefinition].
+///
+/// Both codecs: accept no arguments, operate only on INT/LONG, produce the
same header format
+/// (`[flag:byte][count:int]…`), and have the same `maxEncodedSize`. Only the
+/// encode/decode arithmetic differs and is delegated to the four abstract
helper methods.
+///
+/// **Two's-complement wrap-around is intentional.** Deltas are computed using
ordinary Java
+/// `-` on int/long, which silently wraps when the mathematical delta does not
fit in the
+/// value's width (e.g. `Integer.MAX_VALUE - Integer.MIN_VALUE`). Decode
applies the symmetric
+/// `+` so the original sequence is recovered bit-for-bit. Boundary tests in
+/// `CodecPipelineForwardIndexTest` lock in this property — do not switch the
encode/decode
+/// helpers to checked arithmetic (`Math.subtractExact`, `Math.addExact`)
without also
+/// changing the on-disk format.
+abstract class AbstractDeltaCodecDefinition<O extends CodecOptions> implements
ChunkCodecHandler<O> {
Review Comment:
(nit) Use `BaseX` for abstract class
--
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]
