From 73a59738b020bf5e06b868c2f989be1a94968e18 Mon Sep 17 00:00:00 2001 From: Xiang Fu Date: Sun, 30 Aug 2026 13:55:03 -0700 Subject: [PATCH 1/5] Add V7 raw forward index format for codec pipelines Persist codec pipelines in a self-describing V7 format and route every codecSpec through it. Reject unsupported table configurations before segment creation, and reconcile codec changes during reload using header-only inspection. Keep codec runtime changes in the prerequisite commit. Cover real INT/LONG round trips, malformed files, compression-only routing, table validation, reload transitions and queries in both engines. Enable codecSpec only after segment readers/writers have been upgraded; remove it and rebuild or reload affected indexes before rolling binaries back. --- .../RawForwardIndexWithDictionaryTest.java | 39 +- .../FixedByteChunkForwardIndexWriter.java | 7 +- .../FixedByteChunkForwardIndexWriterV7.java | 380 +++++++++++++++ .../io/writer/impl/FixedByteChunkWriter.java | 47 ++ .../SingleValueFixedByteRawIndexCreator.java | 42 +- .../forward/ForwardIndexCreatorFactory.java | 27 +- .../forward/ForwardIndexReaderFactory.java | 30 +- .../index/forward/ForwardIndexType.java | 47 +- .../index/loader/ForwardIndexHandler.java | 80 ++-- .../invertedindex/TextIndexHandler.java | 2 +- .../FixedByteChunkSVForwardIndexReaderV7.java | 453 ++++++++++++++++++ .../ForwardIndexCreatorFactoryTest.java | 268 ++++++++++- ...rwardIndexHandlerCompressionStatsTest.java | 2 +- .../index/loader/ForwardIndexHandlerTest.java | 101 +++- .../local/utils/TableConfigUtilsTest.java | 102 ++++ .../segment/spi/index/ForwardIndexConfig.java | 4 + 16 files changed, 1568 insertions(+), 63 deletions(-) create mode 100644 pinot-segment-local/src/main/java/org/apache/pinot/segment/local/io/writer/impl/FixedByteChunkForwardIndexWriterV7.java create mode 100644 pinot-segment-local/src/main/java/org/apache/pinot/segment/local/io/writer/impl/FixedByteChunkWriter.java create mode 100644 pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/forward/FixedByteChunkSVForwardIndexReaderV7.java diff --git a/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/custom/RawForwardIndexWithDictionaryTest.java b/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/custom/RawForwardIndexWithDictionaryTest.java index 62e3672b5277..fc39bae091f0 100644 --- a/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/custom/RawForwardIndexWithDictionaryTest.java +++ b/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/custom/RawForwardIndexWithDictionaryTest.java @@ -55,6 +55,7 @@ /// /// All three columns get identical values so query outputs must match exactly across filters, aggregations, /// GROUP BY, DISTINCT, IN, and REGEXP_LIKE predicates. +/// Additional INT/LONG columns exercise V7 codec pipelines through the same shared cluster. @Test(suiteName = "CustomClusterIntegrationTest") public class RawForwardIndexWithDictionaryTest extends CustomDataQueryClusterIntegrationTest { private static final String TABLE_NAME = "RawForwardIndexWithDictionaryTest"; @@ -65,6 +66,9 @@ public class RawForwardIndexWithDictionaryTest extends CustomDataQueryClusterInt private static final String RAW_DICT_INT_DIMENSION = "rawDictIntDim"; private static final String RAW_DICT_INV_INT_DIMENSION = "rawDictInvIntDim"; private static final String RAW_DICT_RANGE_INT_DIMENSION = "rawDictRangeIntDim"; + private static final String CODEC_INT_DIMENSION = "codecIntDim"; + private static final String CODEC_LONG_DIMENSION = "codecLongDim"; + private static final long CODEC_LONG_BASE = (long) Integer.MAX_VALUE + 1; private static final String METRIC_COLUMN = "metric"; private static final int ROW_COUNT = 1000; private static final int UNIQUE_DIMENSION_VALUES = 20; @@ -108,6 +112,8 @@ public Schema createSchema() { .addSingleValueDimension(RAW_DICT_INT_DIMENSION, FieldSpec.DataType.INT) .addSingleValueDimension(RAW_DICT_INV_INT_DIMENSION, FieldSpec.DataType.INT) .addSingleValueDimension(RAW_DICT_RANGE_INT_DIMENSION, FieldSpec.DataType.INT) + .addSingleValueDimension(CODEC_INT_DIMENSION, FieldSpec.DataType.INT) + .addSingleValueDimension(CODEC_LONG_DIMENSION, FieldSpec.DataType.LONG) .addMetric(METRIC_COLUMN, FieldSpec.DataType.LONG) .addDateTime(TIMESTAMP_FIELD_NAME, FieldSpec.DataType.LONG, "1:MILLISECONDS:EPOCH", "1:MILLISECONDS") .build(); @@ -124,6 +130,8 @@ public List createAvroFiles() .requiredInt(RAW_DICT_INT_DIMENSION) .requiredInt(RAW_DICT_INV_INT_DIMENSION) .requiredInt(RAW_DICT_RANGE_INT_DIMENSION) + .requiredInt(CODEC_INT_DIMENSION) + .requiredLong(CODEC_LONG_DIMENSION) .requiredLong(METRIC_COLUMN) .requiredLong(TIMESTAMP_FIELD_NAME) .endRecord(); @@ -145,6 +153,8 @@ public List createAvroFiles() record.put(RAW_DICT_INT_DIMENSION, intValue); record.put(RAW_DICT_INV_INT_DIMENSION, intValue); record.put(RAW_DICT_RANGE_INT_DIMENSION, intValue); + record.put(CODEC_INT_DIMENSION, intValue); + record.put(CODEC_LONG_DIMENSION, CODEC_LONG_BASE + i); record.put(METRIC_COLUMN, random.nextInt(10_000)); record.put(TIMESTAMP_FIELD_NAME, currentTimeMillis + i); fileWriter.append(record); @@ -178,7 +188,34 @@ protected List getFieldConfigs() { FieldConfig rawDictRangeInt = new FieldConfig(RAW_DICT_RANGE_INT_DIMENSION, FieldConfig.EncodingType.RAW, null, null, null, null, dictionaryIndex.deepCopy(), null, null); - return List.of(rawDictString, rawDictInvString, rawDictInt, rawDictInvInt, rawDictRangeInt); + return List.of(rawDictString, rawDictInvString, rawDictInt, rawDictInvInt, rawDictRangeInt, + codecFieldConfig(CODEC_INT_DIMENSION, "DELTA,T64,LZ4"), + codecFieldConfig(CODEC_LONG_DIMENSION, "ZSTD(3)")); + } + + private static FieldConfig codecFieldConfig(String column, String codecSpec) { + ObjectNode forward = JsonUtils.newObjectNode().put("codecSpec", codecSpec).put("targetDocsPerChunk", 128); + ObjectNode indexes = JsonUtils.newObjectNode(); + indexes.set("forward", forward); + return new FieldConfig.Builder(column).withEncodingType(FieldConfig.EncodingType.RAW).withIndexes(indexes).build(); + } + + /// Exercises persisted INT/LONG pipelines through ingestion and both query engines, across chunk boundaries. + @Test(dataProvider = "useBothQueryEngines") + public void testCodecPipelineQueries(boolean useMultiStageQueryEngine) + throws Exception { + setUseMultiStageQueryEngine(useMultiStageQueryEngine); + JsonNode response = postQuery("SELECT " + CODEC_INT_DIMENSION + ", " + CODEC_LONG_DIMENSION + " FROM " + + getTableName() + " WHERE " + CODEC_INT_DIMENSION + " = 7 ORDER BY " + CODEC_LONG_DIMENSION + " LIMIT 1000"); + assertTrue(response.path("exceptions").isEmpty(), response.toString()); + JsonNode rows = response.path("resultTable").path("rows"); + assertEquals(rows.size(), ROW_COUNT / UNIQUE_DIMENSION_VALUES); + for (int i = 0; i < rows.size(); i++) { + assertEquals(rows.get(i).get(0).asInt(), 7); + assertEquals(rows.get(i).get(1).asLong(), CODEC_LONG_BASE + 7 + (long) i * UNIQUE_DIMENSION_VALUES); + } + assertEquals(scalarLong("SELECT SUM(" + CODEC_INT_DIMENSION + ") FROM " + getTableName()), + (long) ROW_COUNT * (UNIQUE_DIMENSION_VALUES - 1) / 2); } @Override diff --git a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/io/writer/impl/FixedByteChunkForwardIndexWriter.java b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/io/writer/impl/FixedByteChunkForwardIndexWriter.java index 449dfa7b6aa5..cbcd40f3dd94 100644 --- a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/io/writer/impl/FixedByteChunkForwardIndexWriter.java +++ b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/io/writer/impl/FixedByteChunkForwardIndexWriter.java @@ -27,7 +27,8 @@ /// Chunk-based raw (non-dictionary-encoded) forward index writer where each chunk contains fixed number of docs, and /// each entry has fixed number of bytes. @NotThreadSafe -public class FixedByteChunkForwardIndexWriter extends BaseChunkForwardIndexWriter { +public class FixedByteChunkForwardIndexWriter extends BaseChunkForwardIndexWriter + implements FixedByteChunkWriter { private int _chunkDataOffset; /// Constructor for the class. @@ -49,24 +50,28 @@ public FixedByteChunkForwardIndexWriter(File file, ChunkCompressionType compress _chunkDataOffset = 0; } + @Override public void putInt(int value) { _chunkBuffer.putInt(value); _chunkDataOffset += Integer.BYTES; flushChunkIfNeeded(); } + @Override public void putLong(long value) { _chunkBuffer.putLong(value); _chunkDataOffset += Long.BYTES; flushChunkIfNeeded(); } + @Override public void putFloat(float value) { _chunkBuffer.putFloat(value); _chunkDataOffset += Float.BYTES; flushChunkIfNeeded(); } + @Override public void putDouble(double value) { _chunkBuffer.putDouble(value); _chunkDataOffset += Double.BYTES; diff --git a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/io/writer/impl/FixedByteChunkForwardIndexWriterV7.java b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/io/writer/impl/FixedByteChunkForwardIndexWriterV7.java new file mode 100644 index 000000000000..2f4c39d3fecf --- /dev/null +++ b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/io/writer/impl/FixedByteChunkForwardIndexWriterV7.java @@ -0,0 +1,380 @@ +/** + * 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.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.codec.CodecSpecParser; +import org.apache.pinot.segment.spi.memory.CleanerUtil; +import org.apache.pinot.spi.data.FieldSpec.DataType; + + +/// 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) +/// formatMagic (int, value = 0xC0DEC0DE) +/// 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): +/// encodedSize (int, byte length of the encoded payload that follows) +/// decodedSize (int, byte length of the original decoded chunk data) +/// payload (byte[], encoded chunk data, length = encodedSize) +/// ``` +/// +/// 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 decoded output and to skip/read chunks without scanning adjacent offsets. +/// +/// This class is *not* thread-safe. +@NotThreadSafe +public class FixedByteChunkForwardIndexWriterV7 implements FixedByteChunkWriter { + + /// Frozen on-disk format version. The version must be paired with [#FORMAT_MAGIC] because legacy + /// fixed-byte writers also accept arbitrary versions greater than or equal to 4. + public static final int VERSION = 7; + public static final int FORMAT_MAGIC = 0xC0DEC0DE; + + /// Upper bound for the canonical, ASCII-only codec spec embedded in the header. Keep the wire + /// limit aligned with the DSL parser so every accepted header is representable by public config. + public static final int MAX_CODEC_SPEC_LENGTH_BYTES = CodecSpecParser.MAX_SPEC_LENGTH; + + /// Maximum decoded bytes in one V7 chunk. The normal Pinot target is 1 MiB; this 64 MiB ceiling + /// bounds per-reader direct scratch and intermediate pipeline buffers for corrupt segments. + public static final int MAX_DECODED_CHUNK_SIZE_BYTES = 64 * 1024 * 1024; + + /// Maximum conservative encoded-size bound for every stage in a V7 pipeline. Writers reject a + /// pipeline/chunk-size combination whose composed bound exceeds this ceiling, so readers can + /// allocate bounded scratch without accepting a file that the same writer could not read back. + public static final int MAX_ENCODED_CHUNK_SIZE_BYTES = 128 * 1024 * 1024; + + /// Maximum sum of all stage-output bounds for one chunk. This prevents a long pipeline of + /// individually bounded transforms from causing unbounded allocation and CPU churn. + public static final long MAX_PIPELINE_WORK_SIZE_BYTES = 256L * 1024 * 1024; + + /// Bytes written before each chunk payload: encodedSize (int) + decodedSize (int). + public static final int CHUNK_HEADER_BYTES = 2 * Integer.BYTES; + + // Number of fixed int fields before the codec spec: version, formatMagic, numChunks, + // numDocsPerChunk, sizeOfEntry, totalDocs, codecSpecLength, dataHeaderStart + private static final int FIXED_HEADER_INT_COUNT = 8; + public static final int FIXED_HEADER_BYTES = FIXED_HEADER_INT_COUNT * Integer.BYTES; + + // Hold both the RAF and its FileChannel: closing the channel closes the underlying FD, but + // some JVM finalizers close the FD when the RAF becomes unreachable. Holding the RAF as a + // field anchors it to the writer's lifetime and removes any reliance on finalizer ordering. + private final RandomAccessFile _raf; + private final FileChannel _dataFile; + private final CodecPipelineExecutor _executor; + private final CodecPipelineExecutor.EncodeScratch _encodeScratch = new CodecPipelineExecutor.EncodeScratch(); + private final int _numDocsPerChunk; + private final int _sizeOfEntry; + private final int _chunkFullBytes; + private final int _maxFullChunkEncodedSize; + private final ByteBuffer _header; + private final ByteBuffer _chunkBuffer; + private final ByteBuffer _chunkHeaderBuffer = ByteBuffer.allocateDirect(CHUNK_HEADER_BYTES); + private final int _numChunks; + private final int _totalDocs; + + private long _dataOffset; + private int _docsWritten; + private int _chunksWritten; + + /// Creates a new writer. + /// + /// @param file output file + /// @param executor pre-validated pipeline executor + /// @param totalDocs total number of documents to write + /// @param numDocsPerChunk target documents per chunk (will be rounded up to power-of-2) + /// @param sizeOfEntry bytes per value (e.g. 4 for INT, 8 for LONG) + public FixedByteChunkForwardIndexWriterV7(File file, CodecPipelineExecutor executor, int totalDocs, + int numDocsPerChunk, int sizeOfEntry) + throws IOException { + if (totalDocs < 0) { + throw new IllegalArgumentException("totalDocs must be non-negative, got: " + totalDocs); + } + _executor = executor; + _numDocsPerChunk = validateAndNormalizeNumDocsPerChunk(executor, sizeOfEntry, numDocsPerChunk); + _sizeOfEntry = sizeOfEntry; + _totalDocs = totalDocs; + long chunkSizeLong = (long) sizeOfEntry * _numDocsPerChunk; + _chunkFullBytes = (int) chunkSizeLong; + _maxFullChunkEncodedSize = executor.maxEncodedSize(_chunkFullBytes, MAX_ENCODED_CHUNK_SIZE_BYTES, + MAX_PIPELINE_WORK_SIZE_BYTES); + _numChunks = (int) (((long) totalDocs + _numDocsPerChunk - 1) / _numDocsPerChunk); + _docsWritten = 0; + _chunksWritten = 0; + + byte[] specBytes = executor.getCanonicalSpec().getBytes(StandardCharsets.UTF_8); + if (specBytes.length > MAX_CODEC_SPEC_LENGTH_BYTES) { + throw new IllegalArgumentException( + "Canonical codec spec is " + specBytes.length + " bytes; maximum is " + MAX_CODEC_SPEC_LENGTH_BYTES); + } + + // Header layout: + // 8 ints of fixed fields + // specBytes.length bytes of codec spec + // numChunks longs of chunk offsets + long fixedHeaderBytesLong = FIXED_HEADER_BYTES; + long dataHeaderStartLong = fixedHeaderBytesLong + specBytes.length; + long chunkOffsetTableBytesLong = (long) _numChunks * Long.BYTES; + long totalHeaderBytesLong = dataHeaderStartLong + chunkOffsetTableBytesLong; + if (totalHeaderBytesLong > Integer.MAX_VALUE) { + throw new IllegalArgumentException( + "Header size " + totalHeaderBytesLong + " bytes exceeds Integer.MAX_VALUE. Reduce totalDocs or" + + " increase numDocsPerChunk."); + } + int dataHeaderStart = (int) dataHeaderStartLong; + int totalHeaderBytes = (int) totalHeaderBytesLong; + + _header = ByteBuffer.allocateDirect(totalHeaderBytes); + _header.putInt(VERSION); + _header.putInt(FORMAT_MAGIC); + _header.putInt(_numChunks); + _header.putInt(_numDocsPerChunk); + _header.putInt(sizeOfEntry); + _header.putInt(totalDocs); + _header.putInt(specBytes.length); + _header.putInt(dataHeaderStart); + _header.put(specBytes); + // chunk offsets will be filled in during writeChunk() calls + + _dataOffset = totalHeaderBytes; + + // Open file first, then allocate the direct buffer under a try/catch so that an OOM during + // allocation closes the already-open file descriptor (the caller has no reference to a + // partially-constructed object and cannot invoke close() itself). + RandomAccessFile raf = new RandomAccessFile(file, "rw"); + FileChannel channel = raf.getChannel(); + try { + raf.setLength(0L); + _chunkBuffer = ByteBuffer.allocateDirect((int) chunkSizeLong); + } catch (Throwable t) { + try { + raf.close(); + } catch (IOException closeEx) { + t.addSuppressed(closeEx); + } + throw t; + } + _raf = raf; + _dataFile = channel; + } + + /// Writes a 4-byte integer value. + @Override + public void putInt(int value) { + if (_sizeOfEntry != Integer.BYTES) { + throw new IllegalStateException("putInt cannot write a LONG V7 forward index"); + } + checkRoomForOneMore(); + _chunkBuffer.putInt(value); + _docsWritten++; + flushIfNeeded(); + } + + /// Writes an 8-byte long value. + @Override + public void putLong(long value) { + if (_sizeOfEntry != Long.BYTES) { + throw new IllegalStateException("putLong cannot write an INT V7 forward index"); + } + checkRoomForOneMore(); + _chunkBuffer.putLong(value); + _docsWritten++; + flushIfNeeded(); + } + + /// The V7 codec-pipeline transforms (DELTA/DELTADELTA/T64/GORILLA) are defined for integral + /// INT/LONG values only, so FLOAT is not supported by this writer. + @Override + public void putFloat(float value) { + throw new UnsupportedOperationException("V7 codec-pipeline writer does not support FLOAT"); + } + + /// See [#putFloat] — DOUBLE is likewise unsupported by the V7 codec-pipeline writer. + @Override + public void putDouble(double value) { + throw new UnsupportedOperationException("V7 codec-pipeline writer does not support DOUBLE"); + } + + /// Fail fast at write time if the caller would exceed the declared `totalDocs`. Without this + /// guard the writer keeps producing chunks past the declared length and only `close()` catches + /// the mismatch, leaving a semantically-invalid partial file behind. + private void checkRoomForOneMore() { + if (_docsWritten >= _totalDocs) { + throw new IllegalStateException( + "Cannot write past declared totalDocs=" + _totalDocs + " (already wrote " + _docsWritten + ")"); + } + } + + private void flushIfNeeded() { + if (_chunkBuffer.position() == _chunkFullBytes) { + writeChunk(); + } + } + + private void writeChunk() { + _chunkBuffer.flip(); + int decodedSize = _chunkBuffer.remaining(); + try { + ByteBuffer encoded = _executor.encode(_chunkBuffer, MAX_ENCODED_CHUNK_SIZE_BYTES, + MAX_PIPELINE_WORK_SIZE_BYTES, _encodeScratch); + int encodedSize = encoded.remaining(); + int maxEncodedSize = decodedSize == _chunkFullBytes ? _maxFullChunkEncodedSize + : _executor.maxEncodedSize(decodedSize, MAX_ENCODED_CHUNK_SIZE_BYTES, MAX_PIPELINE_WORK_SIZE_BYTES); + if (encodedSize > maxEncodedSize) { + throw new IllegalStateException( + "Codec pipeline produced " + encodedSize + " bytes for a " + decodedSize + + "-byte chunk, exceeding its declared bound " + maxEncodedSize); + } + + // Per-chunk header: encodedSize (int) + decodedSize (int) + _chunkHeaderBuffer.clear(); + _chunkHeaderBuffer.putInt(encodedSize); + _chunkHeaderBuffer.putInt(decodedSize); + _chunkHeaderBuffer.flip(); + + // Record chunk's starting offset (points to per-chunk header) in the file header + _header.putLong(_dataOffset); + writeFully(_chunkHeaderBuffer, _dataOffset); + writeFully(encoded, _dataOffset + CHUNK_HEADER_BYTES); + _chunksWritten++; + _dataOffset += CHUNK_HEADER_BYTES + encodedSize; + } catch (IOException e) { + throw new UncheckedIOException("Failed to encode/write data chunk " + _chunksWritten, e); + } + _chunkBuffer.clear(); + } + + @Override + public void close() + throws IOException { + try { + if (_chunkBuffer.position() > 0) { + writeChunk(); + } + if (_docsWritten != _totalDocs) { + throw new IllegalStateException( + "Expected " + _totalDocs + " docs but only " + _docsWritten + " were written"); + } + if (_chunksWritten != _numChunks) { + throw new IllegalStateException( + "Expected " + _numChunks + " chunks but wrote " + _chunksWritten); + } + _header.flip(); + writeFully(_header, 0); + } finally { + // Close the RAF (which closes its FileChannel) so the underlying file descriptor is released + // by an explicit call rather than relying on JVM finalizers. + try { + _raf.close(); + } finally { + try { + _encodeScratch.close(); + } finally { + CleanerUtil.cleanQuietly(_header); + CleanerUtil.cleanQuietly(_chunkBuffer); + CleanerUtil.cleanQuietly(_chunkHeaderBuffer); + } + } + } + } + + /// Writes all remaining bytes from `buf` starting at `position`, looping on short writes. + private void writeFully(ByteBuffer buf, long position) + throws IOException { + long pos = position; + while (buf.hasRemaining()) { + int written = _dataFile.write(buf, pos); + pos += written; + } + } + + // ------------------------------------------------------------------------- + + /// Validates V7 chunk sizing and returns the power-of-two document count stored in the header. + /// Table-config validation calls this same method so invalid sizing fails before segment creation. + public static int validateAndNormalizeNumDocsPerChunk(CodecPipelineExecutor executor, int sizeOfEntry, + int numDocsPerChunk) { + if (sizeOfEntry != Integer.BYTES && sizeOfEntry != Long.BYTES) { + throw new IllegalArgumentException("sizeOfEntry must be 4 (INT) or 8 (LONG), got: " + sizeOfEntry); + } + DataType executorType = executor.getStoredType(); + if (executorType != DataType.INT && executorType != DataType.LONG) { + throw new IllegalArgumentException("V7 writer requires an INT or LONG executor, got: " + executorType); + } + if (sizeOfEntry != executorType.size()) { + throw new IllegalArgumentException( + "sizeOfEntry " + sizeOfEntry + " does not match executor stored type " + executorType + + " (" + executorType.size() + " bytes)"); + } + int normalizedDocsPerChunk = normalizePower2(numDocsPerChunk); + long chunkSize = (long) sizeOfEntry * normalizedDocsPerChunk; + if (chunkSize > MAX_DECODED_CHUNK_SIZE_BYTES) { + throw new IllegalArgumentException( + "Decoded chunk size " + chunkSize + " bytes exceeds V7 limit " + + MAX_DECODED_CHUNK_SIZE_BYTES + ". Reduce numDocsPerChunk."); + } + try { + executor.maxEncodedSize((int) chunkSize, MAX_ENCODED_CHUNK_SIZE_BYTES, MAX_PIPELINE_WORK_SIZE_BYTES); + } catch (RuntimeException e) { + throw new IllegalArgumentException( + "Codec pipeline cannot safely bound a " + chunkSize + "-byte V7 chunk within the " + + MAX_ENCODED_CHUNK_SIZE_BYTES + "-byte encoded/intermediate and " + + MAX_PIPELINE_WORK_SIZE_BYTES + "-byte cumulative-work limits. Reduce numDocsPerChunk or the number" + + " of codec stages.", e); + } + return normalizedDocsPerChunk; + } + + /// Rounds `n` up to the next power of two (or returns `n` if already a power of two). + private static int normalizePower2(int n) { + if (n <= 0) { + throw new IllegalArgumentException("numDocsPerChunk must be positive, got: " + n); + } + if (n > (1 << 30)) { + throw new IllegalArgumentException( + "numDocsPerChunk too large (max 2^30 = " + (1 << 30) + "), got: " + n); + } + if ((n & (n - 1)) == 0) { + return n; + } + return 1 << (32 - Integer.numberOfLeadingZeros(n - 1)); + } +} diff --git a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/io/writer/impl/FixedByteChunkWriter.java b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/io/writer/impl/FixedByteChunkWriter.java new file mode 100644 index 000000000000..da1defd81b7c --- /dev/null +++ b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/io/writer/impl/FixedByteChunkWriter.java @@ -0,0 +1,47 @@ +/** + * 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 javax.annotation.concurrent.NotThreadSafe; + + +/// Common write API for single-value fixed-width raw forward-index chunk writers, letting callers +/// hold a single writer reference regardless of the on-disk format that backs it. +/// +/// Two implementations exist, selected by the configured codec spec: +/// - [FixedByteChunkForwardIndexWriter] — the legacy chunk format (versions 2, 3, and arbitrary +/// fixed-writer tags greater than or equal to 4); used for one plain `ChunkCompressionType`. +/// Supports INT/LONG/FLOAT/DOUBLE. +/// - [FixedByteChunkForwardIndexWriterV7] — the explicitly marked V7 codec-pipeline format; used +/// for every configured `codecSpec`. Supports INT/LONG only; +/// `putFloat`/`putDouble` throw [UnsupportedOperationException]. +/// +/// Instances require serialized access and are not safe for concurrent use. +@NotThreadSafe +public interface FixedByteChunkWriter extends Closeable { + + void putInt(int value); + + void putLong(long value); + + void putFloat(float value); + + void putDouble(double value); +} diff --git a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/fwd/SingleValueFixedByteRawIndexCreator.java b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/fwd/SingleValueFixedByteRawIndexCreator.java index 0c6280d75f86..77bc481de893 100644 --- a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/fwd/SingleValueFixedByteRawIndexCreator.java +++ b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/fwd/SingleValueFixedByteRawIndexCreator.java @@ -20,17 +20,25 @@ import java.io.File; import java.io.IOException; +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.local.io.writer.impl.FixedByteChunkWriter; 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.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`). +/// +/// The creator holds a single [FixedByteChunkWriter]; the concrete writer (legacy chunk format vs +/// V7 codec-pipeline format) is chosen by the constructor and all `put*` calls delegate to it +/// without branching. FLOAT/DOUBLE are supported only by the legacy writer; the V7 writer rejects +/// them at the writer level. public class SingleValueFixedByteRawIndexCreator implements CompressionStatsTrackingForwardIndexCreator { - private final FixedByteChunkForwardIndexWriter _indexWriter; + private final FixedByteChunkWriter _indexWriter; private final DataType _valueType; private final ChunkCompressionType _chunkCompressionType; @@ -69,6 +77,24 @@ public SingleValueFixedByteRawIndexCreator(File baseIndexDir, ChunkCompressionTy _chunkCompressionType = compressionType; } + /// Creates a raw fixed-byte creator backed by the V7 codec-pipeline writer. + /// + /// This path is used for every configured codec pipeline so all `codecSpec` values use V7 format. + public SingleValueFixedByteRawIndexCreator(File baseIndexDir, String column, int totalDocs, DataType valueType, + int targetDocsPerChunk, CodecPipelineExecutor executor) + throws IOException { + if (valueType != executor.getStoredType()) { + throw new IllegalArgumentException( + "Creator value type " + valueType + " does not match codec executor stored type " + + executor.getStoredType() + " for column: " + column); + } + File file = new File(baseIndexDir, column + V1Constants.Indexes.RAW_SV_FORWARD_INDEX_FILE_EXTENSION); + _indexWriter = new FixedByteChunkForwardIndexWriterV7(file, executor, totalDocs, targetDocsPerChunk, + valueType.size()); + _valueType = valueType; + _chunkCompressionType = null; + } + @Override public boolean isDictionaryEncoded() { return false; @@ -112,7 +138,11 @@ public void close() @Override public long getRawForwardIndexUncompressedValueSizeInBytes() { - return _indexWriter.getRawForwardIndexUncompressedValueSizeInBytes(); + // Compression-statistics metadata supports only the legacy single-compressor format. + if (_indexWriter instanceof FixedByteChunkForwardIndexWriter legacyWriter) { + return legacyWriter.getRawForwardIndexUncompressedValueSizeInBytes(); + } + return -1; } @Override @@ -122,6 +152,8 @@ public ChunkCompressionType getRawForwardIndexChunkCompressionType() { @Override public void enableRawForwardIndexUncompressedValueSizeTracking() { - _indexWriter.enableRawForwardIndexUncompressedValueSizeTracking(); + if (_indexWriter instanceof FixedByteChunkForwardIndexWriter legacyWriter) { + legacyWriter.enableRawForwardIndexUncompressedValueSizeTracking(); + } } } diff --git a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/forward/ForwardIndexCreatorFactory.java b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/forward/ForwardIndexCreatorFactory.java index d822c5c2c4bd..0fd5141f39a9 100644 --- a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/forward/ForwardIndexCreatorFactory.java +++ b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/forward/ForwardIndexCreatorFactory.java @@ -19,8 +19,10 @@ package org.apache.pinot.segment.local.segment.index.forward; +import com.google.common.base.Preconditions; import java.io.File; import java.io.IOException; +import org.apache.pinot.segment.local.io.codec.CodecPipelineExecutor; import org.apache.pinot.segment.local.segment.creator.impl.fwd.CLPForwardIndexCreatorV1; import org.apache.pinot.segment.local.segment.creator.impl.fwd.CLPForwardIndexCreatorV2; import org.apache.pinot.segment.local.segment.creator.impl.fwd.CompressionStatsTrackingForwardIndexCreator; @@ -52,6 +54,9 @@ public static ForwardIndexCreator createIndexCreator(IndexCreationContext contex FieldSpec fieldSpec = context.getFieldSpec(); String columnName = fieldSpec.getName(); int numTotalDocs = context.getTotalDocs(); + Preconditions.checkArgument( + indexConfig.getCodecSpec() == null || indexConfig.getEncodingType() == FieldConfig.EncodingType.RAW, + "codecSpec requires RAW forward-index encoding for column: %s", columnName); if (indexConfig.getEncodingType() == FieldConfig.EncodingType.DICTIONARY) { // Dictionary-encoded forward index requires a dictionary to translate dict ids to values. @@ -74,8 +79,18 @@ public static ForwardIndexCreator createIndexCreator(IndexCreationContext contex } else { // Raw forward index DataType storedType = fieldSpec.getDataType().getStoredType(); - ForwardIndexCreator creator; - if (indexConfig.getCompressionCodec() == FieldConfig.CompressionCodec.CLP) { + ForwardIndexCreator creator = null; + ChunkCompressionType chunkCompressionType = null; + + // codecSpec always selects the self-describing V7 codec-pipeline format. The legacy raw + // writers remain available only through compressionCodec/chunkCompressionType. + if (indexConfig.getCodecSpec() != null) { + String codecSpec = indexConfig.getCodecSpec(); + ForwardIndexType.validateCodecPipelineShape(codecSpec, fieldSpec); + CodecPipelineExecutor executor = CodecPipelineExecutor.create(codecSpec, storedType); + creator = new SingleValueFixedByteRawIndexCreator(indexDir, columnName, numTotalDocs, storedType, + indexConfig.getTargetDocsPerChunk(), executor); + } else if (indexConfig.getCompressionCodec() == FieldConfig.CompressionCodec.CLP) { // CLP (V1) uses hard-coded chunk compressor which is set to `PassThrough` creator = new CLPForwardIndexCreatorV1(indexDir, columnName, numTotalDocs, context.getColumnStatistics()); } else if (indexConfig.getCompressionCodec() == FieldConfig.CompressionCodec.CLPV2) { @@ -85,8 +100,12 @@ public static ForwardIndexCreator createIndexCreator(IndexCreationContext contex creator = new CLPForwardIndexCreatorV2(indexDir, context.getColumnStatistics(), ChunkCompressionType.ZSTANDARD); } else if (indexConfig.getCompressionCodec() == FieldConfig.CompressionCodec.CLPV2_LZ4) { creator = new CLPForwardIndexCreatorV2(indexDir, context.getColumnStatistics(), ChunkCompressionType.LZ4); - } else { - ChunkCompressionType chunkCompressionType = indexConfig.getChunkCompressionType(); + } + + if (creator == null) { + if (chunkCompressionType == null) { + chunkCompressionType = indexConfig.getChunkCompressionType(); + } if (chunkCompressionType == null) { chunkCompressionType = ForwardIndexType.getDefaultCompressionType(fieldSpec.getFieldType()); } diff --git a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/forward/ForwardIndexReaderFactory.java b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/forward/ForwardIndexReaderFactory.java index 28615c02f218..65dba745c06d 100644 --- a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/forward/ForwardIndexReaderFactory.java +++ b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/forward/ForwardIndexReaderFactory.java @@ -32,6 +32,7 @@ import org.apache.pinot.segment.local.segment.index.readers.forward.FixedBitSVForwardIndexReaderV2; import org.apache.pinot.segment.local.segment.index.readers.forward.FixedByteChunkMVForwardIndexReader; import org.apache.pinot.segment.local.segment.index.readers.forward.FixedByteChunkSVForwardIndexReader; +import org.apache.pinot.segment.local.segment.index.readers.forward.FixedByteChunkSVForwardIndexReaderV7; import org.apache.pinot.segment.local.segment.index.readers.forward.FixedBytePower2ChunkSVForwardIndexReader; import org.apache.pinot.segment.local.segment.index.readers.forward.VarByteChunkForwardIndexReaderV4; import org.apache.pinot.segment.local.segment.index.readers.forward.VarByteChunkForwardIndexReaderV5; @@ -107,17 +108,38 @@ public ForwardIndexReader createIndexReader(PinotDataBuffer dataBuffer, ColumnMe return new CLPForwardIndexReaderV2(dataBuffer, metadata.getTotalDocs()); } } - return createRawIndexReader(dataBuffer, metadata.getDataType().getStoredType(), metadata.isSingleValue()); + return createRawIndexReader(dataBuffer, metadata.getDataType().getStoredType(), metadata.isSingleValue(), + metadata.getTotalDocs()); } } public ForwardIndexReader createRawIndexReader(PinotDataBuffer dataBuffer, DataType storedType, boolean isSingleValue) { + return createRawIndexReader(dataBuffer, storedType, isSingleValue, -1); + } + + private ForwardIndexReader createRawIndexReader(PinotDataBuffer dataBuffer, DataType storedType, + boolean isSingleValue, int expectedTotalDocs) { + if (dataBuffer.size() < Integer.BYTES) { + throw new IllegalArgumentException( + "Raw forward index is truncated: " + dataBuffer.size() + " bytes; cannot read format version"); + } int version = dataBuffer.getInt(0); if (isSingleValue && storedType.isFixedWidth()) { - return version >= FixedBytePower2ChunkSVForwardIndexReader.VERSION - ? new FixedBytePower2ChunkSVForwardIndexReader(dataBuffer, storedType) - : new FixedByteChunkSVForwardIndexReader(dataBuffer, storedType); + // The codec-pipeline V7 format is discriminated by its explicit header magic, not by the + // version integer: legacy fixed-byte writers accept arbitrary versions >= 4, so the + // power-of-2 fallback below must only be reached when the V7 marker is absent. + if (FixedByteChunkSVForwardIndexReaderV7.hasCodecPipelineHeader(dataBuffer)) { + if (storedType != DataType.INT && storedType != DataType.LONG) { + throw new UnsupportedOperationException( + "V7 codec pipeline does not yet support " + storedType + " columns"); + } + return new FixedByteChunkSVForwardIndexReaderV7(dataBuffer, storedType, expectedTotalDocs); + } + if (version >= FixedBytePower2ChunkSVForwardIndexReader.VERSION) { + return new FixedBytePower2ChunkSVForwardIndexReader(dataBuffer, storedType); + } + return new FixedByteChunkSVForwardIndexReader(dataBuffer, storedType); } if (version == VarByteChunkForwardIndexWriterV6.VERSION) { diff --git a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/forward/ForwardIndexType.java b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/forward/ForwardIndexType.java index 47d1ea02c2e7..59d65bf89b79 100644 --- a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/forward/ForwardIndexType.java +++ b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/forward/ForwardIndexType.java @@ -31,6 +31,8 @@ import java.util.Map; import java.util.Set; import javax.annotation.Nullable; +import org.apache.pinot.segment.local.io.codec.CodecPipelineExecutor; +import org.apache.pinot.segment.local.io.writer.impl.FixedByteChunkForwardIndexWriterV7; import org.apache.pinot.segment.local.realtime.impl.forward.CLPMutableForwardIndexV2; import org.apache.pinot.segment.local.realtime.impl.forward.FixedByteMVMutableForwardIndex; import org.apache.pinot.segment.local.realtime.impl.forward.FixedByteSVMutableForwardIndex; @@ -113,6 +115,10 @@ private void validateForwardIndexEnabled(ForwardIndexConfig forwardIndexConfig, String column = fieldSpec.getName(); CompressionCodec compressionCodec = forwardIndexConfig.getCompressionCodec(); DictionaryIndexConfig dictionaryConfig = indexConfigs.getConfig(StandardIndexes.dictionary()); + String codecSpec = forwardIndexConfig.getCodecSpec(); + if (codecSpec != null) { + validateCodecSpec(codecSpec, forwardIndexConfig, fieldSpec); + } // Dictionary-encoded forward index requires a dictionary to translate dict ids back to values. if (forwardIndexConfig.getEncodingType() == FieldConfig.EncodingType.DICTIONARY) { Preconditions.checkState(dictionaryConfig.isEnabled(), @@ -134,9 +140,44 @@ private void validateForwardIndexEnabled(ForwardIndexConfig forwardIndexConfig, } } + /// Validates a codec pipeline against the effective table config and schema before segment creation. + private static void validateCodecSpec(String codecSpec, ForwardIndexConfig forwardIndexConfig, + FieldSpec fieldSpec) { + String column = fieldSpec.getName(); + Preconditions.checkState(forwardIndexConfig.getEncodingType() == FieldConfig.EncodingType.RAW, + "codecSpec requires RAW forward-index encoding for column: %s", column); + validateCodecPipelineShape(codecSpec, fieldSpec); + try { + FieldSpec.DataType storedType = fieldSpec.getDataType().getStoredType(); + CodecPipelineExecutor executor = CodecPipelineExecutor.create(codecSpec, storedType); + FixedByteChunkForwardIndexWriterV7.validateAndNormalizeNumDocsPerChunk(executor, storedType.size(), + forwardIndexConfig.getTargetDocsPerChunk()); + } catch (IllegalArgumentException e) { + throw new IllegalStateException( + "Codec pipeline validation failed for column '" + column + "' (codecSpec='" + codecSpec + "'): " + + e.getMessage(), e); + } + } + + /// Enforces the shape supported by the V7 codec-pipeline writer. The factory calls this as + /// defense in depth for direct callers that bypass table-config validation. + static void validateCodecPipelineShape(String codecSpec, FieldSpec fieldSpec) { + String column = fieldSpec.getName(); + Preconditions.checkArgument(fieldSpec.isSingleValueField(), + "codecSpec '%s' uses the V7 codec-pipeline writer, which only supports single-value columns. " + + "Column '%s' is multi-value.", codecSpec, column); + FieldSpec.DataType storedType = fieldSpec.getDataType().getStoredType(); + Preconditions.checkArgument(storedType == FieldSpec.DataType.INT || storedType == FieldSpec.DataType.LONG, + "codecSpec '%s' uses the V7 codec-pipeline writer, which only supports INT and LONG columns. " + + "Column '%s' has type: %s.", codecSpec, column, storedType); + } + private void validateForwardIndexDisabled(FieldIndexConfigs indexConfigs, FieldSpec fieldSpec, TableConfig tableConfig) { String column = fieldSpec.getName(); + ForwardIndexConfig forwardIndexConfig = indexConfigs.getConfig(StandardIndexes.forward()); + Preconditions.checkState(forwardIndexConfig.getCodecSpec() == null, + "codecSpec cannot be configured when the forward index is disabled for column: %s", column); // TODO: Revisit this. We should allow dropping forward index after segment is sealed. Preconditions.checkState(tableConfig.getTableType() != TableType.REALTIME, @@ -201,9 +242,14 @@ protected ColumnConfigDeserializer createDeserializer() { // Pop processed columns so the post-loop scan only emits defaults for columns with no FieldConfig at all. boolean inNoDictionaryList = noDictionaryColumns.remove(column); + JsonNode forwardIndexNode = fieldConfig.getIndexes().get(INDEX_DISPLAY_NAME); + // `forwardIndexDisabled` short-circuits everything else. Map properties = fieldConfig.getProperties(); if (properties != null && isDisabled(properties)) { + JsonNode codecSpecNode = forwardIndexNode != null ? forwardIndexNode.get("codecSpec") : null; + Preconditions.checkState(codecSpecNode == null || codecSpecNode.isNull(), + "codecSpec cannot be configured when the forward index is disabled for column: %s", column); result.put(column, ForwardIndexConfig.getDisabled()); continue; } @@ -216,7 +262,6 @@ protected ColumnConfigDeserializer createDeserializer() { FieldConfig.EncodingType encodingType = inNoDictionaryList ? FieldConfig.EncodingType.RAW : fieldConfig.getEncodingType(); - JsonNode forwardIndexNode = fieldConfig.getIndexes().get(INDEX_DISPLAY_NAME); if (forwardIndexNode != null) { Preconditions.checkState(forwardIndexNode.isObject(), "Invalid forward index config for column: %s", column); diff --git a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/ForwardIndexHandler.java b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/ForwardIndexHandler.java index 9c53152cdee3..01a21486ba76 100644 --- a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/ForwardIndexHandler.java +++ b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/ForwardIndexHandler.java @@ -31,6 +31,7 @@ import java.util.Set; import javax.annotation.Nullable; import org.apache.commons.io.FileUtils; +import org.apache.pinot.segment.local.io.codec.CodecPipelineExecutor; import org.apache.pinot.segment.local.io.util.PinotDataBitSet; import org.apache.pinot.segment.local.segment.creator.impl.SegmentDictionaryCreator; import org.apache.pinot.segment.local.segment.creator.impl.stats.AbstractColumnStatisticsCollector; @@ -45,6 +46,7 @@ import org.apache.pinot.segment.local.segment.creator.impl.stats.StringColumnPreIndexStatsCollector; import org.apache.pinot.segment.local.segment.index.dictionary.DictionaryIndexType; import org.apache.pinot.segment.local.segment.index.forward.CompressionStatsMetadata; +import org.apache.pinot.segment.local.segment.index.readers.forward.FixedByteChunkSVForwardIndexReaderV7; import org.apache.pinot.segment.local.segment.readers.PinotSegmentColumnReader; import org.apache.pinot.segment.local.utils.ClusterConfigForTable; import org.apache.pinot.segment.spi.ColumnMetadata; @@ -66,6 +68,7 @@ import org.apache.pinot.segment.spi.index.reader.Dictionary; import org.apache.pinot.segment.spi.index.reader.ForwardIndexReader; import org.apache.pinot.segment.spi.index.reader.ForwardIndexReaderContext; +import org.apache.pinot.segment.spi.memory.PinotDataBuffer; import org.apache.pinot.segment.spi.store.SegmentDirectory; import org.apache.pinot.segment.spi.utils.SegmentMetadataUtils; import org.apache.pinot.spi.config.table.FieldConfig.EncodingType; @@ -86,7 +89,7 @@ /// that this handler only works for segment versions >= 3.0. Support for segment version < 3.0 is not added because /// majority of the usecases are in versions >= 3.0 and this avoids adding tech debt. The currently supported /// operations are: -/// 1. Change compression type for a raw column +/// 1. Change compression type or codecSpec for a raw column, including legacy/V7 transitions /// 2. Enable dictionary /// 3. Disable dictionary /// 4. Disable forward index @@ -100,6 +103,7 @@ public class ForwardIndexHandler extends BaseIndexHandler { // This should contain a list of all indexes that need to be rewritten if the dictionary is enabled or disabled private static final List> DICTIONARY_BASED_INDEXES_TO_REWRITE = Arrays.asList(StandardIndexes.range(), StandardIndexes.fst(), StandardIndexes.inverted()); + private final Map _configuredCodecExecutors = new HashMap<>(); /// Re-enable operations are split by target encoding so the intent is explicit at the operation level: a /// `forwardIndex.disabled` column being re-enabled may want to come back as either dict-encoded or raw, @@ -108,7 +112,7 @@ public class ForwardIndexHandler extends BaseIndexHandler { /// and the test assertions specific. protected enum Operation { DISABLE_FORWARD_INDEX, ENABLE_DICT_FORWARD_INDEX, ENABLE_RAW_FORWARD_INDEX, DISABLE_DICTIONARY, - ENABLE_DICTIONARY, CHANGE_INDEX_COMPRESSION_TYPE + ENABLE_DICTIONARY, REWRITE_FORWARD_INDEX } @VisibleForTesting @@ -146,7 +150,7 @@ public void updateIndices(SegmentDirectory.Writer segmentWriter) boolean needsShapeMetadata = operations.contains(Operation.ENABLE_DICT_FORWARD_INDEX) || operations.contains(Operation.ENABLE_RAW_FORWARD_INDEX) - || operations.contains(Operation.CHANGE_INDEX_COMPRESSION_TYPE) + || operations.contains(Operation.REWRITE_FORWARD_INDEX) || (operations.contains(Operation.DISABLE_DICTIONARY) && !forwardIndexDisabledColumns.contains(column) && isForwardIndexDictionaryEncoded(column)); if (needsShapeMetadata) { @@ -214,8 +218,8 @@ && isForwardIndexDictionaryEncoded(column)) { throw new IllegalStateException(String.format("Forward index was not created for column: %s", column)); } break; - case CHANGE_INDEX_COMPRESSION_TYPE: - rewriteForwardIndexForCompressionChange(column, segmentWriter); + case REWRITE_FORWARD_INDEX: + rewriteForwardIndex(column, segmentWriter); break; default: throw new IllegalStateException("Unsupported operation for column " + column); @@ -405,17 +409,17 @@ private List computeColumnOperations(String column, FieldSpec fieldSp } } - // 3. Compression-type change (only when no encoding change happened). - if (ops.isEmpty() && existingFwdEncoding != null && existingFwdEncoding == newFwdEncoding - && existingHasDict == desiredDict) { - if (existingFwdEncoding == EncodingType.RAW) { - // TODO: Also check if raw index version needs to be changed - if (shouldChangeRawCompressionType(column, segmentReader)) { - ops.add(Operation.CHANGE_INDEX_COMPRESSION_TYPE); - } - } else if (shouldChangeDictIdCompressionType(column, segmentReader)) { - ops.add(Operation.CHANGE_INDEX_COMPRESSION_TYPE); + // 3. Raw format/codec change. Adding or removing a standalone dictionary preserves a RAW + // forward index, so codec reconciliation must run independently of dictionary operations. + // Encoding conversions recreate the forward index with the new config and need no second rewrite. + if (existingFwdEncoding == EncodingType.RAW && newFwdEncoding == EncodingType.RAW) { + // TODO: Also check if raw index version needs to be changed + if (shouldRewriteRawForwardIndex(column, segmentReader)) { + ops.add(Operation.REWRITE_FORWARD_INDEX); } + } else if (ops.isEmpty() && existingFwdEncoding == EncodingType.DICTIONARY + && newFwdEncoding == EncodingType.DICTIONARY && shouldChangeDictIdCompressionType(column, segmentReader)) { + ops.add(Operation.REWRITE_FORWARD_INDEX); } return ops; @@ -513,24 +517,44 @@ private boolean shouldDisableDictionary(String column, ColumnMetadata existingCo return true; } - private boolean shouldChangeRawCompressionType(String column, SegmentDirectory.Reader segmentReader) + private boolean shouldRewriteRawForwardIndex(String column, SegmentDirectory.Reader segmentReader) throws Exception { - // The compression type for an existing segment can only be determined by reading the forward index header. + // The persisted compression type / codec spec can only be determined from the forward-index header. ColumnMetadata existingColMetadata = _segmentDirectory.getSegmentMetadata().getColumnMetadataFor(column); ChunkCompressionType existingCompressionType; + String existingCodecSpec; - // Get the forward index reader factory and create a reader - IndexReaderFactory readerFactory = StandardIndexes.forward().getReaderFactory(); - try (ForwardIndexReader fwdIndexReader = readerFactory.createIndexReader(segmentReader, - _fieldIndexConfigs.get(column), existingColMetadata)) { - existingCompressionType = fwdIndexReader.getCompressionType(); - Preconditions.checkState(existingCompressionType != null, - "Existing compressionType cannot be null for raw forward index column=" + column); + PinotDataBuffer forwardIndexBuffer = segmentReader.getIndexFor(column, StandardIndexes.forward()); + if (FixedByteChunkSVForwardIndexReaderV7.hasCodecPipelineHeader(forwardIndexBuffer)) { + existingCompressionType = null; + existingCodecSpec = FixedByteChunkSVForwardIndexReaderV7.readCodecSpec(forwardIndexBuffer); + } else { + IndexReaderFactory readerFactory = StandardIndexes.forward().getReaderFactory(); + try (ForwardIndexReader fwdIndexReader = readerFactory.createIndexReader(segmentReader, + _fieldIndexConfigs.get(column), existingColMetadata)) { + existingCompressionType = fwdIndexReader.getCompressionType(); + existingCodecSpec = null; + } } - // Get the new compression type. - ChunkCompressionType newCompressionType = - _fieldIndexConfigs.get(column).getConfig(StandardIndexes.forward()).getChunkCompressionType(); + ForwardIndexConfig newConfig = _fieldIndexConfigs.get(column).getConfig(StandardIndexes.forward()); + String newCodecSpec = newConfig.getCodecSpec(); + if (newCodecSpec != null) { + CodecPipelineExecutor configuredExecutor = _configuredCodecExecutors.computeIfAbsent(column, + ignored -> CodecPipelineExecutor.create(newCodecSpec, existingColMetadata.getDataType().getStoredType())); + String canonicalNewSpec = configuredExecutor.getCanonicalSpec(); + return !canonicalNewSpec.equals(existingCodecSpec); + } + + // Removing codecSpec always means leaving V7. Even without an explicit compressionCodec, the + // creator rewrites to the column's legacy default, which restores rollback compatibility. + if (existingCodecSpec != null) { + return true; + } + + Preconditions.checkState(existingCompressionType != null, + "Legacy raw forward index for column=%s returned null ChunkCompressionType", column); + ChunkCompressionType newCompressionType = newConfig.getChunkCompressionType(); // Note that default compression type (PASS_THROUGH for metric and LZ4 for dimension) is not considered if the // compressionType is not explicitly provided in tableConfig. This is to avoid incorrectly rewriting all the @@ -560,7 +584,7 @@ private boolean shouldChangeDictIdCompressionType(String column, SegmentDirector return existingCompressionType != newCompressionType; } - private void rewriteForwardIndexForCompressionChange(String column, SegmentDirectory.Writer segmentWriter) + private void rewriteForwardIndex(String column, SegmentDirectory.Writer segmentWriter) throws Exception { ColumnMetadata columnMetadata = _segmentDirectory.getSegmentMetadata().getColumnMetadataFor(column); boolean isSingleValue = columnMetadata.isSingleValue(); diff --git a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/invertedindex/TextIndexHandler.java b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/invertedindex/TextIndexHandler.java index 4273dfbe4249..d94571b667bd 100644 --- a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/invertedindex/TextIndexHandler.java +++ b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/invertedindex/TextIndexHandler.java @@ -373,7 +373,7 @@ private void convertTextIndexToV3Format(SegmentDirectory.Writer segmentWriter, S return; } - // Write the combined file to V3 format (similar to rewriteForwardIndexForCompressionChange) + // Write the combined file to V3 format (similar to ForwardIndexHandler.rewriteForwardIndex) LoaderUtils.writeIndexToV3Format(segmentWriter, columnName, combinedTextIndexFile, StandardIndexes.text()); LOGGER.info("Successfully converted text index to V3 combined format for column: {}", columnName); diff --git a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/forward/FixedByteChunkSVForwardIndexReaderV7.java b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/forward/FixedByteChunkSVForwardIndexReaderV7.java new file mode 100644 index 000000000000..9dd06364b57f --- /dev/null +++ b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/forward/FixedByteChunkSVForwardIndexReaderV7.java @@ -0,0 +1,453 @@ +/** + * 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.segment.index.readers.forward; + +import java.io.IOException; +import java.io.UncheckedIOException; +import java.nio.ByteBuffer; +import java.nio.charset.StandardCharsets; +import org.apache.pinot.segment.local.io.codec.CodecPipelineExecutor; +import org.apache.pinot.segment.local.io.writer.impl.FixedByteChunkForwardIndexWriterV7; +import org.apache.pinot.segment.spi.index.reader.ForwardIndexReader; +import org.apache.pinot.segment.spi.index.reader.ForwardIndexReaderContext; +import org.apache.pinot.segment.spi.memory.CleanerUtil; +import org.apache.pinot.segment.spi.memory.PinotDataBuffer; +import org.apache.pinot.spi.data.FieldSpec.DataType; + + +/// Chunk-based single-value raw forward index reader for version-7 files written by +/// [FixedByteChunkForwardIndexWriterV7]. +/// +/// Reads the canonical codec spec from the file header, instantiates a +/// [CodecPipelineExecutor], and uses it to decode each chunk on demand. +/// +/// Supported data types: INT, LONG. +/// +/// **Threading:** the reader instance itself is immutable after construction and is safe to +/// share across threads, but each [Context] is single-threaded. The +/// [ByteBuffer] returned by `getChunkBuffer` is the context's reusable scratch buffer — +/// its position/limit are mutated on every chunk transition and it must not be retained across +/// subsequent `getInt`/`getLong` calls. +public final class FixedByteChunkSVForwardIndexReaderV7 + implements ForwardIndexReader { + + /// Caller-owned V7 read context. It keeps both the decoded chunk and codec scratch local to the + /// V7 format, is single-threaded, and releases all direct buffers when closed. + public static final class Context implements ForwardIndexReaderContext { + private final ByteBuffer _chunkBuffer; + private int _chunkId = -1; + private CodecPipelineExecutor.DecodeScratch _decodeScratch; + private boolean _closed; + + private Context(int maxChunkSize) { + _chunkBuffer = ByteBuffer.allocateDirect(maxChunkSize); + } + + @Override + public void close() { + if (_closed) { + return; + } + _closed = true; + if (_decodeScratch != null) { + _decodeScratch.close(); + _decodeScratch = null; + } + CleanerUtil.cleanQuietly(_chunkBuffer); + } + + private CodecPipelineExecutor.DecodeScratch getOrCreateDecodeScratch() { + if (_closed) { + throw new IllegalStateException("V7 forward-index reader context is closed"); + } + if (_decodeScratch == null) { + _decodeScratch = new CodecPipelineExecutor.DecodeScratch(); + } + return _decodeScratch; + } + } + + public static final int VERSION = FixedByteChunkForwardIndexWriterV7.VERSION; + + private final PinotDataBuffer _dataBuffer; + private final DataType _storedType; + private final int _numChunks; + private final int _numDocsPerChunk; + private final int _shift; // log2(numDocsPerChunk) for fast chunk id calc + private final int _totalDocs; + private final int _dataHeaderStart; + private final int _chunkCapacityBytes; + private final CodecPipelineExecutor _executor; + private final String _canonicalSpec; + /// Composed pipeline encoded-size bound for a full chunk, computed once at construction; every + /// chunk except a partial final chunk reuses it instead of re-walking the pipeline stages. + private final int _maxFullChunkEncodedSize; + + /// Returns whether the buffer has the explicit header discriminator for the codec-pipeline V7 + /// format. Keep this predicate limited to the marker: once recognized, the constructor must see + /// and reject every corrupt structural field instead of letting the factory fall back to the + /// legacy version-7 reader. + public static boolean hasCodecPipelineHeader(PinotDataBuffer dataBuffer) { + return dataBuffer.size() >= 2L * Integer.BYTES && dataBuffer.getInt(0) == VERSION + && dataBuffer.getInt(Integer.BYTES) == FixedByteChunkForwardIndexWriterV7.FORMAT_MAGIC; + } + + /// Reads only the canonical codec specification from a V7 header. Unlike constructing a full + /// reader, this method does not resolve the codec plan or walk the chunk-offset table and data + /// frames, so segment reload can compare format metadata in constant time. + public static String readCodecSpec(PinotDataBuffer dataBuffer) { + long bufferSize = dataBuffer.size(); + if (bufferSize < FixedByteChunkForwardIndexWriterV7.FIXED_HEADER_BYTES) { + throw new IllegalArgumentException( + "V7 forward index is truncated: " + bufferSize + " bytes; minimum header is " + + FixedByteChunkForwardIndexWriterV7.FIXED_HEADER_BYTES + " bytes"); + } + if (!hasCodecPipelineHeader(dataBuffer)) { + throw new IllegalArgumentException("Forward index does not contain the V7 codec-pipeline header"); + } + int specLength = dataBuffer.getInt(6L * Integer.BYTES); + int dataHeaderStart = dataBuffer.getInt(7L * Integer.BYTES); + if (specLength <= 0 || specLength > FixedByteChunkForwardIndexWriterV7.MAX_CODEC_SPEC_LENGTH_BYTES + || dataHeaderStart != FixedByteChunkForwardIndexWriterV7.FIXED_HEADER_BYTES + specLength + || dataHeaderStart > bufferSize) { + throw new IllegalArgumentException( + "Invalid V7 codec header: specLength=" + specLength + ", dataHeaderStart=" + dataHeaderStart + + ", bufferSize=" + bufferSize); + } + byte[] specBytes = new byte[specLength]; + dataBuffer.copyTo(FixedByteChunkForwardIndexWriterV7.FIXED_HEADER_BYTES, specBytes, 0, specLength); + return new String(specBytes, StandardCharsets.UTF_8); + } + + public FixedByteChunkSVForwardIndexReaderV7(PinotDataBuffer dataBuffer, DataType storedType) { + this(dataBuffer, storedType, -1); + } + + /// Creates a V7 reader and, when `expectedTotalDocs` is non-negative, verifies that the index + /// belongs to segment metadata with the same document count. The two-argument constructor keeps + /// standalone fixture and StarTree helper reads available when no segment metadata is present. + public FixedByteChunkSVForwardIndexReaderV7(PinotDataBuffer dataBuffer, DataType storedType, + int expectedTotalDocs) { + _dataBuffer = dataBuffer; + _storedType = storedType; + + long bufferSize = dataBuffer.size(); + if (bufferSize < FixedByteChunkForwardIndexWriterV7.FIXED_HEADER_BYTES) { + throw new IllegalArgumentException( + "V7 forward index is truncated: " + bufferSize + " bytes; minimum header is " + + FixedByteChunkForwardIndexWriterV7.FIXED_HEADER_BYTES + " bytes"); + } + + if (storedType != DataType.INT && storedType != DataType.LONG) { + throw new IllegalArgumentException( + "FixedByteChunkSVForwardIndexReaderV7 only supports INT and LONG, got: " + storedType); + } + + int offset = 0; + int version = dataBuffer.getInt(offset); + if (version != VERSION) { + throw new IllegalArgumentException("Expected version " + VERSION + " but got " + version); + } + if (!hasCodecPipelineHeader(dataBuffer)) { + throw new IllegalArgumentException( + "Version " + VERSION + " buffer does not contain a valid codec-pipeline header discriminator"); + } + offset += Integer.BYTES; + + int formatMagic = dataBuffer.getInt(offset); + offset += Integer.BYTES; + if (formatMagic != FixedByteChunkForwardIndexWriterV7.FORMAT_MAGIC) { + throw new IllegalArgumentException("Invalid codec-pipeline format magic: " + formatMagic); + } + + _numChunks = dataBuffer.getInt(offset); + offset += Integer.BYTES; + if (_numChunks < 0) { + throw new IllegalArgumentException("Invalid numChunks in forward index header: " + _numChunks); + } + + _numDocsPerChunk = dataBuffer.getInt(offset); + offset += Integer.BYTES; + if (_numDocsPerChunk <= 0 || (_numDocsPerChunk & (_numDocsPerChunk - 1)) != 0) { + throw new IllegalArgumentException( + "Invalid numDocsPerChunk in forward index header: " + _numDocsPerChunk + + ". Expected a positive power of two."); + } + _shift = Integer.numberOfTrailingZeros(_numDocsPerChunk); + + int sizeOfEntry = dataBuffer.getInt(offset); + offset += Integer.BYTES; + if (sizeOfEntry != storedType.size()) { + throw new IllegalArgumentException( + "Header sizeOfEntry=" + sizeOfEntry + " does not match storedType=" + storedType + + " (expected " + storedType.size() + " bytes). Written for a different data type?"); + } + long chunkCapacity = (long) _numDocsPerChunk * sizeOfEntry; + if (chunkCapacity > FixedByteChunkForwardIndexWriterV7.MAX_DECODED_CHUNK_SIZE_BYTES) { + throw new IllegalArgumentException( + "Decoded chunk capacity " + chunkCapacity + " bytes exceeds V7 limit " + + FixedByteChunkForwardIndexWriterV7.MAX_DECODED_CHUNK_SIZE_BYTES + ". Segment may be corrupt."); + } + _chunkCapacityBytes = (int) chunkCapacity; + + _totalDocs = dataBuffer.getInt(offset); + offset += Integer.BYTES; + if (_totalDocs < 0) { + throw new IllegalArgumentException("Invalid totalDocs in forward index header: " + _totalDocs); + } + if (expectedTotalDocs >= 0 && _totalDocs != expectedTotalDocs) { + throw new IllegalArgumentException( + "V7 forward index totalDocs=" + _totalDocs + " does not match segment metadata totalDocs=" + + expectedTotalDocs); + } + + // Validate numChunks/totalDocs/numDocsPerChunk are mutually consistent. A corrupt header + // with mismatched values would otherwise let getChunkOffset() read past the chunk-offset table. + int expectedNumChunks = (int) (((long) _totalDocs + _numDocsPerChunk - 1) / _numDocsPerChunk); + if (_numChunks != expectedNumChunks) { + throw new IllegalArgumentException( + "Inconsistent header: numChunks=" + _numChunks + " but totalDocs=" + _totalDocs + " / numDocsPerChunk=" + + _numDocsPerChunk + " => expected " + expectedNumChunks + ". Segment may be corrupt."); + } + + int specLength = dataBuffer.getInt(offset); + offset += Integer.BYTES; + if (specLength <= 0 || specLength > FixedByteChunkForwardIndexWriterV7.MAX_CODEC_SPEC_LENGTH_BYTES) { + // V7 segments always embed a non-empty canonical codec spec; zero or negative is corruption. + throw new IllegalArgumentException( + "Invalid specLength in forward index header: " + specLength + "; expected [1, " + + FixedByteChunkForwardIndexWriterV7.MAX_CODEC_SPEC_LENGTH_BYTES + "]. Segment may be corrupt."); + } + + _dataHeaderStart = dataBuffer.getInt(offset); + offset += Integer.BYTES; + + // Validate dataHeaderStart, specLength, and the chunk-offset table bounds before using them. + long expectedSpecEnd = (long) offset + specLength; + long chunkOffsetTableEnd = _dataHeaderStart + (long) _numChunks * Long.BYTES; + if (specLength > bufferSize || _dataHeaderStart < 0 || _dataHeaderStart > bufferSize + || expectedSpecEnd != _dataHeaderStart || chunkOffsetTableEnd > bufferSize) { + throw new IllegalArgumentException( + "Forward index header is corrupt: specLength=" + specLength + ", dataHeaderStart=" + _dataHeaderStart + + ", numChunks=" + _numChunks + ", bufferSize=" + bufferSize); + } + + _canonicalSpec = readCodecSpec(dataBuffer); + + try { + _executor = CodecPipelineExecutor.create(_canonicalSpec, storedType); + } catch (RuntimeException e) { + // Catch RuntimeException (not just IllegalArgumentException) so unmapped codec names, + // native-library load failures, and any other unexpected runtime errors surface as a + // typed segment-corruption error rather than propagating as a raw RuntimeException. + throw new IllegalStateException( + "Failed to initialize codec pipeline from spec '" + _canonicalSpec + "' for storedType " + storedType + ": " + + e.getMessage(), e); + } + try { + _maxFullChunkEncodedSize = _executor.maxEncodedSize(_chunkCapacityBytes, + FixedByteChunkForwardIndexWriterV7.MAX_ENCODED_CHUNK_SIZE_BYTES, + FixedByteChunkForwardIndexWriterV7.MAX_PIPELINE_WORK_SIZE_BYTES); + } catch (RuntimeException e) { + throw new IllegalArgumentException( + "Codec pipeline cannot safely bound a " + _chunkCapacityBytes + "-byte V7 chunk within the " + + FixedByteChunkForwardIndexWriterV7.MAX_ENCODED_CHUNK_SIZE_BYTES + + "-byte encoded/intermediate and " + + FixedByteChunkForwardIndexWriterV7.MAX_PIPELINE_WORK_SIZE_BYTES + + "-byte cumulative-work limits. Segment may be corrupt.", e); + } + + // Validate the complete data section up front. Chunk frames must be contiguous: permitting a + // gap before/between/after frames would make those bytes unauthenticated trailing data and + // could hide a partially overwritten or concatenated segment. + long dataSectionStart = _dataHeaderStart + (long) _numChunks * Long.BYTES; + long maxChunkOffset = bufferSize - FixedByteChunkForwardIndexWriterV7.CHUNK_HEADER_BYTES; + long expectedChunkOffset = dataSectionStart; + for (int i = 0; i < _numChunks; i++) { + long chunkOffset = dataBuffer.getLong(_dataHeaderStart + (long) i * Long.BYTES); + if (chunkOffset != expectedChunkOffset || chunkOffset > maxChunkOffset) { + throw new IllegalArgumentException( + "Corrupt chunkOffsets[" + i + "]=" + chunkOffset + ": expected contiguous frame at " + + expectedChunkOffset + " within [" + dataSectionStart + ", " + maxChunkOffset + "]"); + } + int encodedSize = dataBuffer.getInt(chunkOffset); + long maxPayloadBytes = bufferSize - chunkOffset - FixedByteChunkForwardIndexWriterV7.CHUNK_HEADER_BYTES; + if (encodedSize < 0 || encodedSize > maxPayloadBytes) { + throw new IllegalArgumentException( + "Corrupt per-chunk header for chunk " + i + ": encodedSize=" + encodedSize + + ", remainingBuffer=" + maxPayloadBytes); + } + expectedChunkOffset = chunkOffset + FixedByteChunkForwardIndexWriterV7.CHUNK_HEADER_BYTES + encodedSize; + } + if (expectedChunkOffset != bufferSize) { + throw new IllegalArgumentException( + "Corrupt V7 data section: chunk frames end at " + expectedChunkOffset + " but file size is " + + bufferSize); + } + } + + @Override + public Context createContext() { + // Context holds the decoded chunk; initial capacity = one full chunk + return new Context(_chunkCapacityBytes); + } + + @Override + public boolean isDictionaryEncoded() { + return false; + } + + @Override + public boolean isSingleValue() { + return true; + } + + @Override + public DataType getStoredType() { + return _storedType; + } + + @Override + public int getInt(int docId, Context context) { + int chunkRowId = docId & (_numDocsPerChunk - 1); + ByteBuffer chunk = getChunkBuffer(docId, context); + return chunk.getInt(chunkRowId * Integer.BYTES); + } + + @Override + public long getLong(int docId, Context context) { + int chunkRowId = docId & (_numDocsPerChunk - 1); + ByteBuffer chunk = getChunkBuffer(docId, context); + return chunk.getLong(chunkRowId * Long.BYTES); + } + + @Override + public void close() + throws IOException { + // PinotDataBuffer is managed by the caller; nothing to close here. + } + + // ------------------------------------------------------------------------- + + private ByteBuffer getChunkBuffer(int docId, Context context) { + if (docId < 0 || docId >= _totalDocs) { + throw new IndexOutOfBoundsException("docId " + docId + " is out of bounds [0, " + _totalDocs + ")"); + } + int chunkId = docId >>> _shift; + if (context._chunkId == chunkId) { + return context._chunkBuffer; + } + return loadChunk(chunkId, context); + } + + private ByteBuffer loadChunk(int chunkId, Context context) { + long chunkStart = getChunkOffset(chunkId); + // Validate the chunk offset before using it as a buffer index. Constructor validates the + // chunk-offset table extents, but each entry's value is a per-chunk file offset that must + // (1) point past the end of the chunk-offset table (i.e. into the data section), and + // (2) leave room for the per-chunk header before the buffer end. + long bufferSize = _dataBuffer.size(); + long dataSectionStart = _dataHeaderStart + (long) _numChunks * Long.BYTES; + if (chunkStart < dataSectionStart + || chunkStart > bufferSize - FixedByteChunkForwardIndexWriterV7.CHUNK_HEADER_BYTES) { + throw new IllegalStateException( + "Corrupt chunkOffsets[" + chunkId + "]: " + chunkStart + " is outside the data section [" + + dataSectionStart + ", " + bufferSize + "]. Segment may be corrupt."); + } + + // Read per-chunk header: encodedSize (int) + decodedSize (int) + int encodedSize = _dataBuffer.getInt(chunkStart); + int decodedSize = _dataBuffer.getInt(chunkStart + Integer.BYTES); + long chunkEnd = chunkId == _numChunks - 1 ? bufferSize : getChunkOffset(chunkId + 1); + long maxEncodedRoom = chunkEnd - chunkStart - FixedByteChunkForwardIndexWriterV7.CHUNK_HEADER_BYTES; + if (encodedSize < 0 || decodedSize < 0 || encodedSize != maxEncodedRoom) { + throw new IllegalStateException( + "Corrupt per-chunk header for chunk " + chunkId + ": encodedSize=" + encodedSize + + ", decodedSize=" + decodedSize + ", exactPayloadBytes=" + maxEncodedRoom); + } + + long firstDocId = (long) chunkId * _numDocsPerChunk; + int docsInChunk = (int) Math.min(_numDocsPerChunk, _totalDocs - firstDocId); + int expectedDecodedSize = docsInChunk * _storedType.size(); + if (decodedSize != expectedDecodedSize) { + throw new IllegalStateException( + "Corrupt per-chunk header for chunk " + chunkId + ": decodedSize=" + decodedSize + + " but expected " + expectedDecodedSize + " for " + docsInChunk + " " + _storedType + + " values"); + } + + int maxEncodedSize; + if (expectedDecodedSize == _chunkCapacityBytes) { + // Full chunk: reuse the bound computed (and validated) in the constructor. + maxEncodedSize = _maxFullChunkEncodedSize; + } else { + try { + maxEncodedSize = _executor.maxEncodedSize(expectedDecodedSize, + FixedByteChunkForwardIndexWriterV7.MAX_ENCODED_CHUNK_SIZE_BYTES, + FixedByteChunkForwardIndexWriterV7.MAX_PIPELINE_WORK_SIZE_BYTES); + } catch (IllegalArgumentException e) { + throw new IllegalStateException( + "Codec pipeline cannot represent a bounded chunk of " + expectedDecodedSize + " bytes", e); + } + } + if (encodedSize > maxEncodedSize) { + throw new IllegalStateException( + "Corrupt per-chunk header for chunk " + chunkId + ": encodedSize=" + encodedSize + + " exceeds pipeline bound " + maxEncodedSize + " for " + expectedDecodedSize + + " decoded bytes"); + } + + ByteBuffer encoded = _dataBuffer.toDirectByteBuffer( + chunkStart + FixedByteChunkForwardIndexWriterV7.CHUNK_HEADER_BYTES, encodedSize); + ByteBuffer contextBuf = context._chunkBuffer; + if (decodedSize > contextBuf.capacity()) { + throw new IllegalStateException( + "Decoded chunk " + chunkId + " is " + decodedSize + " bytes but context buffer capacity is " + + contextBuf.capacity()); + } + // Invalidate chunkId BEFORE decode — if decoding or the size assertion throws, + // contextBuf may be partially mutated. The next getChunkBuffer call must re-load this chunk + // rather than seeing a stale cache hit on the partial state. + context._chunkId = -1; + try { + // Decode directly into the reusable context buffer — avoids intermediate allocation + copy. + _executor.decode(encoded, contextBuf, decodedSize, + FixedByteChunkForwardIndexWriterV7.MAX_ENCODED_CHUNK_SIZE_BYTES, + FixedByteChunkForwardIndexWriterV7.MAX_PIPELINE_WORK_SIZE_BYTES, + context.getOrCreateDecodeScratch()); + } catch (IOException e) { + throw new UncheckedIOException( + "Failed to decode chunk " + chunkId + " with spec '" + _canonicalSpec + "'", e); + } + if (contextBuf.remaining() != decodedSize) { + throw new IllegalStateException( + "Chunk " + chunkId + ": decoded size " + contextBuf.remaining() + " does not match header value " + + decodedSize); + } + context._chunkId = chunkId; + // All callers (getInt/getLong) use absolute ByteBuffer.getXxx(int) which ignores position, + // so the reusable contextBuf can be returned directly without a per-chunk duplicate(). + return contextBuf; + } + + private long getChunkOffset(int chunkId) { + // Each chunk offset is a long (8 bytes) + return _dataBuffer.getLong(_dataHeaderStart + (long) chunkId * Long.BYTES); + } +} diff --git a/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/forward/ForwardIndexCreatorFactoryTest.java b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/forward/ForwardIndexCreatorFactoryTest.java index c044a82f5d08..089c85bdc9d6 100644 --- a/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/forward/ForwardIndexCreatorFactoryTest.java +++ b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/forward/ForwardIndexCreatorFactoryTest.java @@ -20,13 +20,23 @@ package org.apache.pinot.segment.local.segment.index.forward; import java.io.File; +import java.nio.ByteBuffer; import java.nio.file.Files; +import java.util.Arrays; import org.apache.commons.io.FileUtils; -import org.apache.pinot.segment.spi.ColumnMetadata; +import org.apache.pinot.segment.local.io.writer.impl.FixedByteChunkForwardIndexWriter; +import org.apache.pinot.segment.local.segment.creator.impl.fwd.CompressionStatsTrackingForwardIndexCreator; +import org.apache.pinot.segment.local.segment.index.readers.forward.ChunkReaderContext; +import org.apache.pinot.segment.local.segment.index.readers.forward.FixedByteChunkSVForwardIndexReaderV7; +import org.apache.pinot.segment.local.segment.index.readers.forward.FixedBytePower2ChunkSVForwardIndexReader; +import org.apache.pinot.segment.spi.V1Constants; import org.apache.pinot.segment.spi.compression.ChunkCompressionType; import org.apache.pinot.segment.spi.creator.IndexCreationContext; import org.apache.pinot.segment.spi.index.ForwardIndexConfig; import org.apache.pinot.segment.spi.index.creator.ForwardIndexCreator; +import org.apache.pinot.segment.spi.index.metadata.ColumnMetadataImpl; +import org.apache.pinot.segment.spi.index.reader.ForwardIndexReader; +import org.apache.pinot.segment.spi.memory.PinotDataBuffer; import org.apache.pinot.spi.config.table.FieldConfig; import org.apache.pinot.spi.config.table.TableConfig; import org.apache.pinot.spi.config.table.TableType; @@ -34,12 +44,14 @@ import org.apache.pinot.spi.data.FieldSpec; import org.apache.pinot.spi.data.FieldSpec.DataType; import org.apache.pinot.spi.utils.builder.TableConfigBuilder; -import org.mockito.Mockito; +import org.testng.annotations.DataProvider; import org.testng.annotations.Test; import static org.testng.Assert.assertEquals; import static org.testng.Assert.assertFalse; +import static org.testng.Assert.assertNull; import static org.testng.Assert.assertTrue; +import static org.testng.Assert.expectThrows; /// Tests [ForwardIndexCreatorFactory]'s forward-index encoding branch selection. Each test uses an isolated @@ -105,21 +117,253 @@ public void testTableConfigConstructorPropagatesCompressionStatsFlag() } } + @DataProvider(name = "v7CodecSpecs") + public Object[][] v7CodecSpecs() { + return new Object[][]{ + {"LZ4", DataType.INT, false}, {"LZ4", DataType.INT, true}, + {"DELTA,LZ4", DataType.INT, false}, {"DELTA,LZ4", DataType.INT, true}, + {"ZSTD(3)", DataType.LONG, false}, {"ZSTD(3)", DataType.LONG, true}, + {"DELTADELTA,GORILLA,ZSTD(3)", DataType.LONG, false}, + {"DELTADELTA,GORILLA,ZSTD(3)", DataType.LONG, true} + }; + } + + /// Both compression-only and transform pipelines use V7, including a partial final chunk. + @Test(dataProvider = "v7CodecSpecs") + public void testCodecSpecRoundTripUsesV7Format(String codecSpec, DataType storedType, boolean compressionStatsEnabled) + throws Exception { + File indexDir = Files.createTempDirectory("ForwardIndexCreatorFactoryTest").toFile(); + try { + ForwardIndexConfig config = new ForwardIndexConfig.Builder(FieldConfig.EncodingType.RAW) + .withCodecSpec(codecSpec) + .withTargetDocsPerChunk(2) + .build(); + TableConfig tableConfig = new TableConfigBuilder(TableType.OFFLINE).setTableName("testTable").build(); + tableConfig.getIndexingConfig().setCompressionStatsEnabled(compressionStatsEnabled); + long[] values = storedType == DataType.INT ? new long[]{11, 13, 21} + : new long[]{Long.MIN_VALUE, (long) Integer.MAX_VALUE + 1, Long.MAX_VALUE}; + try (ForwardIndexCreator creator = ForwardIndexCreatorFactory.createIndexCreator( + newContext(indexDir, false, tableConfig, values.length, storedType), config)) { + assertFalse(creator.isDictionaryEncoded()); + assertNull(creator.getRawForwardIndexChunkCompressionType()); + assertEquals(creator.getRawForwardIndexUncompressedValueSizeInBytes(), -1L); + for (long value : values) { + if (storedType == DataType.INT) { + creator.putInt((int) value); + } else { + creator.putLong(value); + } + assertEquals(creator.getRawForwardIndexUncompressedValueSizeInBytes(), -1L); + } + creator.seal(); + assertEquals(creator.getRawForwardIndexUncompressedValueSizeInBytes(), -1L); + } + File indexFile = new File(indexDir, COLUMN_NAME + V1Constants.Indexes.RAW_SV_FORWARD_INDEX_FILE_EXTENSION); + assertTrue(indexFile.exists()); + try (PinotDataBuffer buffer = PinotDataBuffer.mapReadOnlyBigEndianFile(indexFile); + ForwardIndexReader reader = + ForwardIndexReaderFactory.getInstance().createRawIndexReader(buffer, storedType, true)) { + assertTrue(reader instanceof FixedByteChunkSVForwardIndexReaderV7, + "codecSpec was routed to " + reader.getClass().getSimpleName()); + assertEquals(buffer.getInt(0), FixedByteChunkSVForwardIndexReaderV7.VERSION); + assertEquals(FixedByteChunkSVForwardIndexReaderV7.readCodecSpec(buffer), codecSpec); + FixedByteChunkSVForwardIndexReaderV7 v7Reader = (FixedByteChunkSVForwardIndexReaderV7) reader; + try (FixedByteChunkSVForwardIndexReaderV7.Context context = v7Reader.createContext()) { + for (int i = 0; i < values.length; i++) { + assertEquals(storedType == DataType.INT ? v7Reader.getInt(i, context) : v7Reader.getLong(i, context), + values[i]); + } + } + } + } finally { + FileUtils.deleteQuietly(indexDir); + } + } + + @DataProvider(name = "compressionStatsEnabled") + public Object[][] compressionStatsEnabled() { + return new Object[][]{{false}, {true}}; + } + + @Test(dataProvider = "compressionStatsEnabled") + public void testLegacyCompressionStatsRemainOptIn(boolean compressionStatsEnabled) + throws Exception { + File indexDir = Files.createTempDirectory("ForwardIndexCreatorFactoryTest").toFile(); + try { + int[] values = {11, 13, 21}; + TableConfig tableConfig = new TableConfigBuilder(TableType.OFFLINE).setTableName("testTable").build(); + tableConfig.getIndexingConfig().setCompressionStatsEnabled(compressionStatsEnabled); + ForwardIndexConfig config = new ForwardIndexConfig.Builder(FieldConfig.EncodingType.RAW) + .withCompressionType(ChunkCompressionType.LZ4).withRawIndexWriterVersion(4).withTargetDocsPerChunk(2).build(); + try (ForwardIndexCreator creator = ForwardIndexCreatorFactory.createIndexCreator( + newContext(indexDir, false, tableConfig, values.length), config)) { + CompressionStatsTrackingForwardIndexCreator trackingCreator = + (CompressionStatsTrackingForwardIndexCreator) creator; + assertEquals(creator.getRawForwardIndexChunkCompressionType(), ChunkCompressionType.LZ4); + assertEquals(creator.getRawForwardIndexUncompressedValueSizeInBytes(), compressionStatsEnabled ? 0L : -1L); + for (int i = 0; i < values.length; i++) { + creator.putInt(values[i]); + assertEquals(creator.getRawForwardIndexUncompressedValueSizeInBytes(), + compressionStatsEnabled ? (long) (i + 1) * Integer.BYTES : -1L); + expectThrows(IllegalStateException.class, + trackingCreator::enableRawForwardIndexUncompressedValueSizeTracking); + } + creator.seal(); + assertEquals(creator.getRawForwardIndexUncompressedValueSizeInBytes(), + compressionStatsEnabled ? (long) values.length * Integer.BYTES : -1L); + } + File indexFile = new File(indexDir, COLUMN_NAME + V1Constants.Indexes.RAW_SV_FORWARD_INDEX_FILE_EXTENSION); + try (PinotDataBuffer buffer = PinotDataBuffer.mapReadOnlyBigEndianFile(indexFile); + FixedBytePower2ChunkSVForwardIndexReader reader = new FixedBytePower2ChunkSVForwardIndexReader(buffer, + DataType.INT); + ChunkReaderContext context = reader.createContext()) { + for (int i = 0; i < values.length; i++) { + assertEquals(reader.getInt(i, context), values[i]); + } + } + } finally { + FileUtils.deleteQuietly(indexDir); + } + } + + @DataProvider(name = "malformedV7Files") + public Object[][] malformedV7Files() { + return new Object[][]{ + {"metadata", IllegalArgumentException.class, "does not match segment metadata"}, + {"chunkSize", IllegalArgumentException.class, "positive power of two"}, + {"specLength", IllegalArgumentException.class, "Invalid specLength"}, + {"offset", IllegalArgumentException.class, "Corrupt chunkOffsets"}, + {"truncated", IllegalArgumentException.class, "Corrupt per-chunk header"}, + {"trailing", IllegalArgumentException.class, "Corrupt V7 data section"}, + {"encodedSize", IllegalArgumentException.class, "Corrupt per-chunk header"}, + {"decodedSize", IllegalStateException.class, "decodedSize"} + }; + } + + @Test(dataProvider = "malformedV7Files") + public void testMalformedV7FileIsRejected(String mutation, Class failureType, + String message) throws Exception { + File indexDir = Files.createTempDirectory("ForwardIndexCreatorFactoryTest").toFile(); + try { + ForwardIndexConfig config = new ForwardIndexConfig.Builder(FieldConfig.EncodingType.RAW) + .withCodecSpec("LZ4").withTargetDocsPerChunk(2).build(); + try (ForwardIndexCreator creator = + ForwardIndexCreatorFactory.createIndexCreator(newContext(indexDir, false, 3), config)) { + creator.putInt(11); + creator.putInt(13); + creator.putInt(21); + creator.seal(); + } + File indexFile = new File(indexDir, COLUMN_NAME + V1Constants.Indexes.RAW_SV_FORWARD_INDEX_FILE_EXTENSION); + byte[] bytes = Files.readAllBytes(indexFile.toPath()); + ByteBuffer header = ByteBuffer.wrap(bytes); + int offsetTable = header.getInt(7 * Integer.BYTES); + int firstFrame = Math.toIntExact(header.getLong(offsetTable)); + switch (mutation) { + case "metadata": + break; + case "chunkSize": + header.putInt(3 * Integer.BYTES, 3); + break; + case "specLength": + header.putInt(6 * Integer.BYTES, 0); + break; + case "offset": + header.putLong(offsetTable, bytes.length); + break; + case "truncated": + bytes = Arrays.copyOf(bytes, bytes.length - 1); + break; + case "trailing": + bytes = Arrays.copyOf(bytes, bytes.length + 1); + break; + case "encodedSize": + header.putInt(firstFrame, -1); + break; + case "decodedSize": + header.putInt(firstFrame + Integer.BYTES, Integer.MAX_VALUE); + break; + default: + throw new AssertionError(mutation); + } + Files.write(indexFile.toPath(), bytes); + ColumnMetadataImpl metadata = new ColumnMetadataImpl.Builder() + .setFieldSpec(new DimensionFieldSpec(COLUMN_NAME, DataType.INT, true)) + .setTotalDocs(mutation.equals("metadata") ? 4 : 3).setHasDictionary(false).build(); + RuntimeException failure = expectThrows(failureType, () -> { + try (PinotDataBuffer buffer = PinotDataBuffer.mapReadOnlyBigEndianFile(indexFile); + ForwardIndexReader reader = + ForwardIndexReaderFactory.getInstance().createIndexReader(buffer, metadata)) { + FixedByteChunkSVForwardIndexReaderV7 v7Reader = (FixedByteChunkSVForwardIndexReaderV7) reader; + try (FixedByteChunkSVForwardIndexReaderV7.Context context = v7Reader.createContext()) { + v7Reader.getInt(0, context); + } + } + }); + assertTrue(failure.getMessage().contains(message), failure.getMessage()); + } finally { + FileUtils.deleteQuietly(indexDir); + } + } + + /// Version 7 alone does not select the codec-pipeline reader: legacy fixed-byte writers accept + /// arbitrary versions greater than or equal to 4 and lack the V7 format magic. + @Test + public void testLegacyVersionSevenStillUsesLegacyReader() + throws Exception { + File indexDir = Files.createTempDirectory("ForwardIndexCreatorFactoryTest").toFile(); + try { + File indexFile = new File(indexDir, COLUMN_NAME + V1Constants.Indexes.RAW_SV_FORWARD_INDEX_FILE_EXTENSION); + try (FixedByteChunkForwardIndexWriter writer = new FixedByteChunkForwardIndexWriter(indexFile, + ChunkCompressionType.LZ4, 3, 2, Integer.BYTES, FixedByteChunkSVForwardIndexReaderV7.VERSION)) { + writer.putInt(11); + writer.putInt(13); + writer.putInt(21); + } + try (PinotDataBuffer buffer = PinotDataBuffer.mapReadOnlyBigEndianFile(indexFile); + ForwardIndexReader reader = + ForwardIndexReaderFactory.getInstance().createRawIndexReader(buffer, DataType.INT, true)) { + assertTrue(reader instanceof FixedBytePower2ChunkSVForwardIndexReader, + "legacy version 7 was routed to " + reader.getClass().getSimpleName()); + } + } finally { + FileUtils.deleteQuietly(indexDir); + } + } + private static IndexCreationContext newContext(File indexDir, boolean hasDictionary) { + return newContext(indexDir, hasDictionary, 1); + } + + private static IndexCreationContext newContext(File indexDir, boolean hasDictionary, int totalDocs) { TableConfig tableConfig = new TableConfigBuilder(TableType.OFFLINE).setTableName("testTable").build(); - return newContext(indexDir, hasDictionary, tableConfig); + return newContext(indexDir, hasDictionary, tableConfig, totalDocs); } private static IndexCreationContext newContext(File indexDir, boolean hasDictionary, TableConfig tableConfig) { - FieldSpec fieldSpec = new DimensionFieldSpec(COLUMN_NAME, DataType.INT, true); - ColumnMetadata metadata = Mockito.mock(ColumnMetadata.class); - Mockito.when(metadata.getFieldSpec()).thenReturn(fieldSpec); - Mockito.when(metadata.getCardinality()).thenReturn(2); - Mockito.when(metadata.getTotalDocs()).thenReturn(1); - Mockito.when(metadata.hasDictionary()).thenReturn(hasDictionary); - Mockito.when(metadata.getLengthOfShortestElement()).thenReturn(Integer.BYTES); - Mockito.when(metadata.getLengthOfLongestElement()).thenReturn(Integer.BYTES); - Mockito.when(metadata.isFixedLength()).thenReturn(true); + return newContext(indexDir, hasDictionary, tableConfig, 1); + } + + private static IndexCreationContext newContext(File indexDir, boolean hasDictionary, TableConfig tableConfig, + int totalDocs) { + return newContext(indexDir, hasDictionary, tableConfig, totalDocs, DataType.INT); + } + + private static IndexCreationContext newContext(File indexDir, boolean hasDictionary, TableConfig tableConfig, + int totalDocs, DataType dataType) { + FieldSpec fieldSpec = new DimensionFieldSpec(COLUMN_NAME, dataType, true); + DataType storedType = fieldSpec.getDataType().getStoredType(); + int elementSize = storedType.isFixedWidth() ? storedType.size() : 8; + ColumnMetadataImpl metadata = new ColumnMetadataImpl.Builder() + .setFieldSpec(fieldSpec) + .setTotalDocs(totalDocs) + .setCardinality(2) + .setHasDictionary(hasDictionary) + .setLengthOfShortestElement(storedType.isFixedWidth() ? elementSize : 1) + .setLengthOfLongestElement(elementSize) + .setTotalNumberOfEntries(totalDocs) + .setMaxNumberOfMultiValues(fieldSpec.isSingleValueField() ? 0 : 1) + .build(); return new IndexCreationContext.Builder(indexDir, tableConfig, metadata).build(); } } diff --git a/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/ForwardIndexHandlerCompressionStatsTest.java b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/ForwardIndexHandlerCompressionStatsTest.java index d692d761e852..369bb5400db1 100644 --- a/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/ForwardIndexHandlerCompressionStatsTest.java +++ b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/ForwardIndexHandlerCompressionStatsTest.java @@ -442,7 +442,7 @@ public void testDictIdCompressionRewriteBackfillsDictionaryStats() SegmentDirectory.Writer writer = segmentDirectory.createWriter()) { ForwardIndexHandler handler = new ForwardIndexHandler(segmentDirectory, createIndexLoadingConfig()); assertEquals(handler.computeOperations(writer), - Map.of(DICT_INT_MV_COL, List.of(ForwardIndexHandler.Operation.CHANGE_INDEX_COMPRESSION_TYPE))); + Map.of(DICT_INT_MV_COL, List.of(ForwardIndexHandler.Operation.REWRITE_FORWARD_INDEX))); handler.updateIndices(writer); handler.postUpdateIndicesCleanup(writer); } diff --git a/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/ForwardIndexHandlerTest.java b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/ForwardIndexHandlerTest.java index 2687709f63a6..ddb47f0afd20 100644 --- a/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/ForwardIndexHandlerTest.java +++ b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/ForwardIndexHandlerTest.java @@ -43,6 +43,7 @@ import org.apache.pinot.segment.local.segment.index.loader.invertedindex.InvertedIndexHandler; import org.apache.pinot.segment.local.segment.index.loader.invertedindex.RangeIndexHandler; import org.apache.pinot.segment.local.segment.index.readers.BitmapInvertedIndexReader; +import org.apache.pinot.segment.local.segment.index.readers.forward.FixedByteChunkSVForwardIndexReaderV7; import org.apache.pinot.segment.local.segment.readers.GenericRowRecordReader; import org.apache.pinot.segment.local.segment.readers.PinotSegmentColumnReader; import org.apache.pinot.segment.local.segment.store.SegmentLocalFSDirectory; @@ -61,7 +62,9 @@ import org.apache.pinot.segment.spi.index.metadata.SegmentMetadataImpl; import org.apache.pinot.segment.spi.index.reader.Dictionary; import org.apache.pinot.segment.spi.index.reader.ForwardIndexReader; +import org.apache.pinot.segment.spi.index.reader.ForwardIndexReaderContext; import org.apache.pinot.segment.spi.index.reader.InvertedIndexReader; +import org.apache.pinot.segment.spi.memory.PinotDataBuffer; import org.apache.pinot.segment.spi.store.SegmentDirectory; import org.apache.pinot.segment.spi.utils.SegmentMetadataUtils; import org.apache.pinot.spi.config.table.FieldConfig; @@ -726,7 +729,7 @@ public void testComputeOperationChangeCompression() _fieldConfigMap.put(column, new FieldConfig(column, FieldConfig.EncodingType.RAW, List.of(), newCompressionCodec, null)); assertEquals(computeOperations(), - Map.of(column, List.of(ForwardIndexHandler.Operation.CHANGE_INDEX_COMPRESSION_TYPE))); + Map.of(column, List.of(ForwardIndexHandler.Operation.REWRITE_FORWARD_INDEX))); // TEST2: Change compression and add index. Change compressionType for more than 1 column. resetIndexConfigs(); @@ -738,8 +741,8 @@ public void testComputeOperationChangeCompression() new FieldConfig(DIM_SNAPPY_STRING, FieldConfig.EncodingType.RAW, List.of(FieldConfig.IndexType.TEXT), CompressionCodec.ZSTANDARD, null)); assertEquals(computeOperations(), - Map.of(DIM_SNAPPY_INTEGER, List.of(ForwardIndexHandler.Operation.CHANGE_INDEX_COMPRESSION_TYPE), - DIM_SNAPPY_STRING, List.of(ForwardIndexHandler.Operation.CHANGE_INDEX_COMPRESSION_TYPE))); + Map.of(DIM_SNAPPY_INTEGER, List.of(ForwardIndexHandler.Operation.REWRITE_FORWARD_INDEX), + DIM_SNAPPY_STRING, List.of(ForwardIndexHandler.Operation.REWRITE_FORWARD_INDEX))); } } @@ -1129,7 +1132,7 @@ public void testChangeDictCompression() ForwardIndexHandler handler = createForwardIndexHandler(); assertEquals(handler.computeOperations(writer), - Map.of(column, List.of(ForwardIndexHandler.Operation.CHANGE_INDEX_COMPRESSION_TYPE))); + Map.of(column, List.of(ForwardIndexHandler.Operation.REWRITE_FORWARD_INDEX))); assertTrue(handler.needUpdateIndices(writer)); handler.updateIndices(writer); handler.postUpdateIndicesCleanup(writer); @@ -1159,7 +1162,7 @@ public void testChangeDictCompression() ForwardIndexHandler handler = createForwardIndexHandler(); assertEquals(handler.computeOperations(writer), - Map.of(column, List.of(ForwardIndexHandler.Operation.CHANGE_INDEX_COMPRESSION_TYPE))); + Map.of(column, List.of(ForwardIndexHandler.Operation.REWRITE_FORWARD_INDEX))); assertTrue(handler.needUpdateIndices(writer)); handler.updateIndices(writer); handler.postUpdateIndicesCleanup(writer); @@ -2834,6 +2837,94 @@ public void testBackfillFromInvertedIndexRebuild() } } + /// Exercises a real legacy-to-V7 reload and the remove-only rollback path. Removing `codecSpec` + /// without specifying `compressionCodec` must restore the field-type legacy default. + @Test + public void testCodecSpecRemoveOnlyRollback() + throws Exception { + _fieldConfigMap.put(DIM_LZ4_INTEGER, rawFieldConfigWithCodecSpec(DIM_LZ4_INTEGER, "DELTA,ZSTD")); + applyCodecRewrite(DIM_LZ4_INTEGER); + assertRawForwardIndexState(DIM_LZ4_INTEGER, "DELTA,ZSTD(3)", null); + + _fieldConfigMap.put(DIM_LZ4_INTEGER, rawFieldConfigWithCodecSpec(DIM_LZ4_INTEGER, "delta,zstd(3)")); + assertNoCodecRewrite(DIM_LZ4_INTEGER); + + _fieldConfigMap.put(DIM_LZ4_INTEGER, rawFieldConfigWithCodecSpec(DIM_LZ4_INTEGER, "DELTA,LZ4")); + applyCodecRewrite(DIM_LZ4_INTEGER); + assertRawForwardIndexState(DIM_LZ4_INTEGER, "DELTA,LZ4", null); + + _fieldConfigMap.put(DIM_LZ4_INTEGER, new FieldConfig.Builder(DIM_LZ4_INTEGER) + .withEncodingType(FieldConfig.EncodingType.RAW) + .build()); + applyCodecRewrite(DIM_LZ4_INTEGER); + assertRawForwardIndexState(DIM_LZ4_INTEGER, null, ChunkCompressionType.LZ4); + } + + private void assertNoCodecRewrite(String column) + throws Exception { + try (SegmentDirectory segmentDirectory = new SegmentLocalFSDirectory(INDEX_DIR, ReadMode.mmap); + SegmentDirectory.Writer writer = segmentDirectory.createWriter()) { + _segmentDirectory = segmentDirectory; + _writer = writer; + assertTrue(createForwardIndexHandler().computeOperations(writer).isEmpty(), + "Equivalent canonical codecSpec should not rewrite column " + column); + } + } + + private void applyCodecRewrite(String column) + throws Exception { + try (SegmentDirectory segmentDirectory = new SegmentLocalFSDirectory(INDEX_DIR, ReadMode.mmap); + SegmentDirectory.Writer writer = segmentDirectory.createWriter()) { + _segmentDirectory = segmentDirectory; + _writer = writer; + ForwardIndexHandler handler = createForwardIndexHandler(); + assertEquals(handler.computeOperations(writer), + Map.of(column, List.of(ForwardIndexHandler.Operation.REWRITE_FORWARD_INDEX))); + handler.updateIndices(writer); + handler.postUpdateIndicesCleanup(writer); + } + } + + @SuppressWarnings("unchecked") + private void assertRawForwardIndexState(String column, @Nullable String codecSpec, + @Nullable ChunkCompressionType compressionType) + throws Exception { + try (SegmentDirectory segmentDirectory = new SegmentLocalFSDirectory(INDEX_DIR, ReadMode.mmap); + SegmentDirectory.Reader segmentReader = segmentDirectory.createReader()) { + ColumnMetadata metadata = segmentDirectory.getSegmentMetadata().getColumnMetadataFor(column); + PinotDataBuffer forwardIndexBuffer = segmentReader.getIndexFor(column, StandardIndexes.forward()); + IndexReaderFactory readerFactory = StandardIndexes.forward().getReaderFactory(); + try (ForwardIndexReader forwardReader = readerFactory.createIndexReader(segmentReader, + createFieldIndexConfigsFromMetadata(metadata), metadata); + ForwardIndexReaderContext context = forwardReader.createContext()) { + if (codecSpec != null) { + assertTrue(forwardReader instanceof FixedByteChunkSVForwardIndexReaderV7); + assertEquals(FixedByteChunkSVForwardIndexReaderV7.readCodecSpec(forwardIndexBuffer), codecSpec); + } else { + assertFalse(forwardReader instanceof FixedByteChunkSVForwardIndexReaderV7); + assertFalse(FixedByteChunkSVForwardIndexReaderV7.hasCodecPipelineHeader(forwardIndexBuffer)); + } + assertEquals(forwardReader.getCompressionType(), compressionType); + for (int docId = 0; docId < TEST_DATA.size(); docId++) { + int expected = ((Number) TEST_DATA.get(docId).getValue(column)).intValue(); + assertEquals(forwardReader.getInt(docId, context), expected, + "Value changed during codec reload at docId " + docId); + } + } + } + } + + private static FieldConfig rawFieldConfigWithCodecSpec(String column, String codecSpec) { + ObjectNode forward = JsonUtils.newObjectNode(); + forward.put("codecSpec", codecSpec); + ObjectNode indexes = JsonUtils.newObjectNode(); + indexes.set("forward", forward); + return new FieldConfig.Builder(column) + .withEncodingType(FieldConfig.EncodingType.RAW) + .withIndexes(indexes) + .build(); + } + private FieldIndexConfigs createFieldIndexConfigsFromMetadata(ColumnMetadata columnMetadata) { FieldIndexConfigs.Builder builder = new FieldIndexConfigs.Builder(); diff --git a/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/utils/TableConfigUtilsTest.java b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/utils/TableConfigUtilsTest.java index 29ed034bcfa0..42b2639974b2 100644 --- a/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/utils/TableConfigUtilsTest.java +++ b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/utils/TableConfigUtilsTest.java @@ -20,6 +20,7 @@ import com.fasterxml.jackson.databind.node.JsonNodeFactory; import com.fasterxml.jackson.databind.node.ObjectNode; +import com.google.common.base.Throwables; import java.util.Arrays; import java.util.HashMap; import java.util.List; @@ -1797,6 +1798,107 @@ public void testValidateFieldConfig() { } } + @Test + public void testCodecSpecTableConfigValidation() { + Schema schema = new Schema.SchemaBuilder().setSchemaName(TABLE_NAME) + .addSingleValueDimension("intCol", DataType.INT) + .addSingleValueDimension("longCol", DataType.LONG) + .addSingleValueDimension("stringCol", DataType.STRING) + .addMultiValueDimension("mvIntCol", DataType.INT) + .build(); + + TableConfig tableConfig = new TableConfigBuilder(TableType.OFFLINE).setTableName(TABLE_NAME).build(); + tableConfig.setFieldConfigList(List.of( + fieldConfigWithCodecSpec("intCol", FieldConfig.EncodingType.RAW, "DELTA,LZ4"), + fieldConfigWithCodecSpec("longCol", FieldConfig.EncodingType.RAW, "ZSTD(3)"))); + TableConfigUtils.validate(tableConfig, schema); + + assertCodecSpecValidationFails(schema, "intCol", FieldConfig.EncodingType.RAW, "LZ4,UNKNOWN", "Unknown codec"); + assertCodecSpecValidationFails(schema, "intCol", FieldConfig.EncodingType.RAW, "LZ4,DELTA", + "all transforms must precede any compression stage"); + assertCodecSpecValidationFails(schema, "mvIntCol", FieldConfig.EncodingType.RAW, "LZ4", + "only supports single-value columns"); + assertCodecSpecValidationFails(schema, "stringCol", FieldConfig.EncodingType.RAW, "SNAPPY", + "only supports INT and LONG columns"); + assertCodecSpecChunkSizeValidationFails(schema, -1, "numDocsPerChunk must be positive"); + assertCodecSpecChunkSizeValidationFails(schema, 16_777_217, "exceeds V7 limit"); + + tableConfig = new TableConfigBuilder(TableType.OFFLINE).setTableName(TABLE_NAME).build(); + tableConfig.setFieldConfigList( + List.of(fieldConfigWithCodecSpec("intCol", FieldConfig.EncodingType.DICTIONARY, "LZ4"))); + TableConfig dictionaryTableConfig = tableConfig; + IllegalStateException exception = expectThrows(IllegalStateException.class, + () -> TableConfigUtils.validate(dictionaryTableConfig, schema)); + assertEquals(exception.getMessage(), "Failed to create FieldIndexConfigs"); + assertEquals(Throwables.getRootCause(exception).getMessage(), "codecSpec requires RAW forward-index encoding"); + + ObjectNode disabledForward = JsonUtils.newObjectNode(); + disabledForward.put("disabled", true); + disabledForward.put("codecSpec", "LZ4"); + ObjectNode disabledIndexes = JsonUtils.newObjectNode(); + disabledIndexes.set("forward", disabledForward); + tableConfig = new TableConfigBuilder(TableType.OFFLINE).setTableName(TABLE_NAME).build(); + tableConfig.setFieldConfigList(List.of(new FieldConfig.Builder("intCol") + .withEncodingType(FieldConfig.EncodingType.RAW) + .withIndexes(disabledIndexes) + .build())); + TableConfig disabledTableConfig = tableConfig; + exception = expectThrows(IllegalStateException.class, + () -> TableConfigUtils.validate(disabledTableConfig, schema)); + assertTrue(exception.getMessage().contains("codecSpec cannot be configured when the forward index is disabled"), + exception.getMessage()); + + tableConfig = new TableConfigBuilder(TableType.OFFLINE).setTableName(TABLE_NAME).build(); + tableConfig.setFieldConfigList(List.of(new FieldConfig.Builder("intCol") + .withEncodingType(FieldConfig.EncodingType.RAW) + .withIndexes(disabledIndexes) + .withProperties(Map.of(FieldConfig.FORWARD_INDEX_DISABLED, Boolean.TRUE.toString())) + .build())); + TableConfig legacyDisabledTableConfig = tableConfig; + exception = expectThrows(IllegalStateException.class, + () -> TableConfigUtils.validate(legacyDisabledTableConfig, schema)); + assertTrue(Throwables.getRootCause(exception).getMessage() + .contains("codecSpec cannot be configured when the forward index is disabled"), + exception.getMessage()); + } + + private static void assertCodecSpecValidationFails(Schema schema, String column, + FieldConfig.EncodingType encodingType, String codecSpec, String expectedMessage) { + TableConfig tableConfig = new TableConfigBuilder(TableType.OFFLINE).setTableName(TABLE_NAME).build(); + tableConfig.setFieldConfigList(List.of(fieldConfigWithCodecSpec(column, encodingType, codecSpec))); + Exception exception = expectThrows(Exception.class, () -> TableConfigUtils.validate(tableConfig, schema)); + assertTrue(exception.getMessage().contains(expectedMessage), exception.getMessage()); + } + + private static FieldConfig fieldConfigWithCodecSpec(String column, FieldConfig.EncodingType encodingType, + String codecSpec) { + return fieldConfigWithCodecSpec(column, encodingType, codecSpec, null); + } + + private static void assertCodecSpecChunkSizeValidationFails(Schema schema, int targetDocsPerChunk, + String expectedMessage) { + TableConfig tableConfig = new TableConfigBuilder(TableType.OFFLINE).setTableName(TABLE_NAME).build(); + tableConfig.setFieldConfigList(List.of( + fieldConfigWithCodecSpec("intCol", FieldConfig.EncodingType.RAW, "LZ4", targetDocsPerChunk))); + Exception exception = expectThrows(Exception.class, () -> TableConfigUtils.validate(tableConfig, schema)); + assertTrue(exception.getMessage().contains(expectedMessage), exception.getMessage()); + } + + private static FieldConfig fieldConfigWithCodecSpec(String column, FieldConfig.EncodingType encodingType, + String codecSpec, @Nullable Integer targetDocsPerChunk) { + ObjectNode forward = JsonUtils.newObjectNode(); + forward.put("codecSpec", codecSpec); + if (targetDocsPerChunk != null) { + forward.put("targetDocsPerChunk", targetDocsPerChunk); + } + ObjectNode indexes = JsonUtils.newObjectNode(); + indexes.set("forward", forward); + return new FieldConfig.Builder(column) + .withEncodingType(encodingType) + .withIndexes(indexes) + .build(); + } + @Test public void testValidateFieldConfigDuplicateColumnName() { final Schema schema = new Schema.SchemaBuilder().setSchemaName(TABLE_NAME) diff --git a/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/ForwardIndexConfig.java b/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/ForwardIndexConfig.java index 5a1fea5f3d6b..c23370d8767d 100644 --- a/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/ForwardIndexConfig.java +++ b/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/ForwardIndexConfig.java @@ -225,6 +225,10 @@ public CompressionCodec getCompressionCodec() { } /// Returns the structurally normalized codec specification, or `null` for the legacy compression path. + /// + /// Any non-null specification selects the V7 on-disk format. Before enabling it, upgrade every component that may + /// build or read segments. Before rolling those components back, remove the specification and rewrite affected V7 + /// forward indexes into a legacy format. @Nullable public String getCodecSpec() { return _codecSpec; From 3fa0dede7b3dd556aa5af4edb411aa2acbdfcb0a Mon Sep 17 00:00:00 2001 From: Xiang Fu Date: Fri, 4 Sep 2026 11:42:34 -0700 Subject: [PATCH 2/5] Validate V7 frames lazily and harden the review follow-ups Segment load no longer walks every V7 chunk frame: the reader constructor validates only the header-resident offset table and the final frame's extent, and loadChunk verifies each frame's exact size on first access. Reset the cached chunk id when a read context closes and guard loadChunk against a closed context. Freeze the header codec-spec length limit as a literal and check the canonical spec's byte length during table-config validation. Drop the write-only executor cache in ForwardIndexHandler and document that RAW codec reconciliation runs alongside dictionary toggles. Tests: multi-chunk round trips with a partial final chunk, writer/reader guard rejections, malformed-file cases split by load-time vs read-time detection, and a reload that enables/disables a standalone dictionary while changing codecSpec. --- .../FixedByteChunkForwardIndexWriterV7.java | 13 +- .../index/forward/ForwardIndexType.java | 6 + .../index/loader/ForwardIndexHandler.java | 11 +- .../FixedByteChunkSVForwardIndexReaderV7.java | 71 +++++--- .../ForwardIndexCreatorFactoryTest.java | 164 ++++++++++++++++-- .../index/loader/ForwardIndexHandlerTest.java | 65 ++++++- .../segment/spi/index/ForwardIndexConfig.java | 8 +- 7 files changed, 289 insertions(+), 49 deletions(-) diff --git a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/io/writer/impl/FixedByteChunkForwardIndexWriterV7.java b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/io/writer/impl/FixedByteChunkForwardIndexWriterV7.java index 2f4c39d3fecf..f4facd003ebe 100644 --- a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/io/writer/impl/FixedByteChunkForwardIndexWriterV7.java +++ b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/io/writer/impl/FixedByteChunkForwardIndexWriterV7.java @@ -27,7 +27,6 @@ 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.codec.CodecSpecParser; import org.apache.pinot.segment.spi.memory.CleanerUtil; import org.apache.pinot.spi.data.FieldSpec.DataType; @@ -69,9 +68,12 @@ public class FixedByteChunkForwardIndexWriterV7 implements FixedByteChunkWriter public static final int VERSION = 7; public static final int FORMAT_MAGIC = 0xC0DEC0DE; - /// Upper bound for the canonical, ASCII-only codec spec embedded in the header. Keep the wire - /// limit aligned with the DSL parser so every accepted header is representable by public config. - public static final int MAX_CODEC_SPEC_LENGTH_BYTES = CodecSpecParser.MAX_SPEC_LENGTH; + /// Upper bound for the canonical, ASCII-only codec spec embedded in the header. This is part of + /// the frozen on-disk format and is deliberately an independent literal: raising the DSL parser + /// limit (`CodecSpecParser.MAX_SPEC_LENGTH`) later must not change how existing files are validated. + /// Canonicalization can append default arguments, so table-config validation checks the canonical + /// byte length against this bound rather than relying on the parser limit alone. + public static final int MAX_CODEC_SPEC_LENGTH_BYTES = 4096; /// Maximum decoded bytes in one V7 chunk. The normal Pinot target is 1 MiB; this 64 MiB ceiling /// bounds per-reader direct scratch and intermediate pipeline buffers for corrupt segments. @@ -256,7 +258,8 @@ private void writeChunk() { ByteBuffer encoded = _executor.encode(_chunkBuffer, MAX_ENCODED_CHUNK_SIZE_BYTES, MAX_PIPELINE_WORK_SIZE_BYTES, _encodeScratch); int encodedSize = encoded.remaining(); - int maxEncodedSize = decodedSize == _chunkFullBytes ? _maxFullChunkEncodedSize + int maxEncodedSize = decodedSize == _chunkFullBytes + ? _maxFullChunkEncodedSize : _executor.maxEncodedSize(decodedSize, MAX_ENCODED_CHUNK_SIZE_BYTES, MAX_PIPELINE_WORK_SIZE_BYTES); if (encodedSize > maxEncodedSize) { throw new IllegalStateException( diff --git a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/forward/ForwardIndexType.java b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/forward/ForwardIndexType.java index 59d65bf89b79..b1f48d74c2c6 100644 --- a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/forward/ForwardIndexType.java +++ b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/forward/ForwardIndexType.java @@ -24,6 +24,7 @@ import com.google.common.base.Preconditions; import java.io.IOException; import java.io.UncheckedIOException; +import java.nio.charset.StandardCharsets; import java.util.Collection; import java.util.HashMap; import java.util.HashSet; @@ -150,6 +151,11 @@ private static void validateCodecSpec(String codecSpec, ForwardIndexConfig forwa try { FieldSpec.DataType storedType = fieldSpec.getDataType().getStoredType(); CodecPipelineExecutor executor = CodecPipelineExecutor.create(codecSpec, storedType); + int canonicalSpecBytes = executor.getCanonicalSpec().getBytes(StandardCharsets.UTF_8).length; + Preconditions.checkArgument( + canonicalSpecBytes <= FixedByteChunkForwardIndexWriterV7.MAX_CODEC_SPEC_LENGTH_BYTES, + "Canonical codec spec is %s bytes; the V7 header allows at most %s", canonicalSpecBytes, + FixedByteChunkForwardIndexWriterV7.MAX_CODEC_SPEC_LENGTH_BYTES); FixedByteChunkForwardIndexWriterV7.validateAndNormalizeNumDocsPerChunk(executor, storedType.size(), forwardIndexConfig.getTargetDocsPerChunk()); } catch (IllegalArgumentException e) { diff --git a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/ForwardIndexHandler.java b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/ForwardIndexHandler.java index 01a21486ba76..42b0109090e1 100644 --- a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/ForwardIndexHandler.java +++ b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/ForwardIndexHandler.java @@ -103,7 +103,6 @@ public class ForwardIndexHandler extends BaseIndexHandler { // This should contain a list of all indexes that need to be rewritten if the dictionary is enabled or disabled private static final List> DICTIONARY_BASED_INDEXES_TO_REWRITE = Arrays.asList(StandardIndexes.range(), StandardIndexes.fst(), StandardIndexes.inverted()); - private final Map _configuredCodecExecutors = new HashMap<>(); /// Re-enable operations are split by target encoding so the intent is explicit at the operation level: a /// `forwardIndex.disabled` column being re-enabled may want to come back as either dict-encoded or raw, @@ -302,7 +301,9 @@ Map> computeOperations(SegmentDirectory.Reader segmentRe /// `desiredDict = newIsDict || any-enabled-index-requires-dict`. The "force on if required" rule is the /// only place this method consults other indexes — once `desiredDict` is computed, the rest of the logic /// treats it as the source of truth. - /// 3. **Compression-type change** — only when no encoding change happened (forward + dict both unchanged). + /// 3. **Compression-type change** — whenever an existing RAW forward index remains RAW after the other + /// operations. Adding/removing a standalone dictionary does not recreate that raw index, so a codec change + /// must be queued alongside the dictionary operation. /// 4. **Cross-cutting guards** — sorted columns can't toggle forward; range index format is incompatible /// with disabling the dictionary; enabling forward needs dict + inverted on disk; enabling dict needs /// forward to be on so the dict can be bootstrapped. @@ -540,9 +541,9 @@ private boolean shouldRewriteRawForwardIndex(String column, SegmentDirectory.Rea ForwardIndexConfig newConfig = _fieldIndexConfigs.get(column).getConfig(StandardIndexes.forward()); String newCodecSpec = newConfig.getCodecSpec(); if (newCodecSpec != null) { - CodecPipelineExecutor configuredExecutor = _configuredCodecExecutors.computeIfAbsent(column, - ignored -> CodecPipelineExecutor.create(newCodecSpec, existingColMetadata.getDataType().getStoredType())); - String canonicalNewSpec = configuredExecutor.getCanonicalSpec(); + // Compare canonical forms so equivalent spellings (case, aliases, default arguments) do not trigger a rewrite. + String canonicalNewSpec = CodecPipelineExecutor.create(newCodecSpec, + existingColMetadata.getDataType().getStoredType()).getCanonicalSpec(); return !canonicalNewSpec.equals(existingCodecSpec); } diff --git a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/forward/FixedByteChunkSVForwardIndexReaderV7.java b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/forward/FixedByteChunkSVForwardIndexReaderV7.java index 9dd06364b57f..b997e18a58fc 100644 --- a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/forward/FixedByteChunkSVForwardIndexReaderV7.java +++ b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/forward/FixedByteChunkSVForwardIndexReaderV7.java @@ -65,6 +65,8 @@ public void close() { return; } _closed = true; + // Drop the cached chunk id so a post-close read cannot short-circuit into the freed buffer. + _chunkId = -1; if (_decodeScratch != null) { _decodeScratch.close(); _decodeScratch = null; @@ -273,32 +275,54 @@ public FixedByteChunkSVForwardIndexReaderV7(PinotDataBuffer dataBuffer, DataType + "-byte cumulative-work limits. Segment may be corrupt.", e); } - // Validate the complete data section up front. Chunk frames must be contiguous: permitting a - // gap before/between/after frames would make those bytes unauthenticated trailing data and - // could hide a partially overwritten or concatenated segment. + // Validate only header-resident structure here so segment load does not fault in every data page + // of the index (the legacy readers only read their fixed header). The chunk-offset table must start + // at the data section, each later offset must leave room for its predecessor's header and payload + // bound, and the final frame must end exactly at the file end so truncated or concatenated files are + // rejected at load. Each frame's exact size is checked lazily in loadChunk, which verifies that the + // accessed frame ends exactly where its successor (or the file) begins; corruption confined to a + // frame that is never read is therefore reported at read time, not at load time. long dataSectionStart = _dataHeaderStart + (long) _numChunks * Long.BYTES; long maxChunkOffset = bufferSize - FixedByteChunkForwardIndexWriterV7.CHUNK_HEADER_BYTES; - long expectedChunkOffset = dataSectionStart; + long maxFrameBytes = (long) FixedByteChunkForwardIndexWriterV7.CHUNK_HEADER_BYTES + _maxFullChunkEncodedSize; + long previousChunkOffset = -1; for (int i = 0; i < _numChunks; i++) { long chunkOffset = dataBuffer.getLong(_dataHeaderStart + (long) i * Long.BYTES); - if (chunkOffset != expectedChunkOffset || chunkOffset > maxChunkOffset) { - throw new IllegalArgumentException( - "Corrupt chunkOffsets[" + i + "]=" + chunkOffset + ": expected contiguous frame at " - + expectedChunkOffset + " within [" + dataSectionStart + ", " + maxChunkOffset + "]"); + if (i == 0) { + if (chunkOffset != dataSectionStart || chunkOffset > maxChunkOffset) { + throw new IllegalArgumentException( + "Corrupt chunkOffsets[0]=" + chunkOffset + ": expected the first frame exactly at " + dataSectionStart + + " within [" + dataSectionStart + ", " + maxChunkOffset + "]"); + } + } else { + long previousFrameBytes = chunkOffset - previousChunkOffset; + if (previousFrameBytes < FixedByteChunkForwardIndexWriterV7.CHUNK_HEADER_BYTES + || previousFrameBytes > maxFrameBytes || chunkOffset > maxChunkOffset) { + throw new IllegalArgumentException( + "Corrupt chunkOffsets[" + i + "]=" + chunkOffset + ": expected a frame between " + + (previousChunkOffset + FixedByteChunkForwardIndexWriterV7.CHUNK_HEADER_BYTES) + " and " + + (previousChunkOffset + maxFrameBytes) + " within [" + dataSectionStart + ", " + maxChunkOffset + + "]"); + } } - int encodedSize = dataBuffer.getInt(chunkOffset); - long maxPayloadBytes = bufferSize - chunkOffset - FixedByteChunkForwardIndexWriterV7.CHUNK_HEADER_BYTES; - if (encodedSize < 0 || encodedSize > maxPayloadBytes) { + previousChunkOffset = chunkOffset; + } + long dataSectionEnd = dataSectionStart; + if (_numChunks > 0) { + int lastChunkId = _numChunks - 1; + int lastEncodedSize = dataBuffer.getInt(previousChunkOffset); + long maxPayloadBytes = Math.min(_maxFullChunkEncodedSize, + bufferSize - previousChunkOffset - FixedByteChunkForwardIndexWriterV7.CHUNK_HEADER_BYTES); + if (lastEncodedSize < 0 || lastEncodedSize > maxPayloadBytes) { throw new IllegalArgumentException( - "Corrupt per-chunk header for chunk " + i + ": encodedSize=" + encodedSize - + ", remainingBuffer=" + maxPayloadBytes); + "Corrupt per-chunk header for chunk " + lastChunkId + ": encodedSize=" + lastEncodedSize + + ", maximum payload " + maxPayloadBytes); } - expectedChunkOffset = chunkOffset + FixedByteChunkForwardIndexWriterV7.CHUNK_HEADER_BYTES + encodedSize; + dataSectionEnd = previousChunkOffset + FixedByteChunkForwardIndexWriterV7.CHUNK_HEADER_BYTES + lastEncodedSize; } - if (expectedChunkOffset != bufferSize) { + if (dataSectionEnd != bufferSize) { throw new IllegalArgumentException( - "Corrupt V7 data section: chunk frames end at " + expectedChunkOffset + " but file size is " - + bufferSize); + "Corrupt V7 data section: chunk frames end at " + dataSectionEnd + " but file size is " + bufferSize); } } @@ -357,11 +381,16 @@ private ByteBuffer getChunkBuffer(int docId, Context context) { } private ByteBuffer loadChunk(int chunkId, Context context) { + // Explicit guard: a closed context has released its direct buffer, so decoding into it would corrupt + // native memory. Checked once per chunk transition, so it stays off the per-row read path. + if (context._closed) { + throw new IllegalStateException("V7 forward-index reader context is closed"); + } long chunkStart = getChunkOffset(chunkId); - // Validate the chunk offset before using it as a buffer index. Constructor validates the - // chunk-offset table extents, but each entry's value is a per-chunk file offset that must - // (1) point past the end of the chunk-offset table (i.e. into the data section), and - // (2) leave room for the per-chunk header before the buffer end. + // Validate the chunk offset before using it as a buffer index. The constructor validates the + // chunk-offset table, but per-frame sizes are only verified here, on first access: the frame + // must (1) start inside the data section, (2) leave room for its header before the buffer end, + // and (3) end exactly where the next frame (or the file) begins. long bufferSize = _dataBuffer.size(); long dataSectionStart = _dataHeaderStart + (long) _numChunks * Long.BYTES; if (chunkStart < dataSectionStart diff --git a/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/forward/ForwardIndexCreatorFactoryTest.java b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/forward/ForwardIndexCreatorFactoryTest.java index 089c85bdc9d6..e60a9e81a8e6 100644 --- a/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/forward/ForwardIndexCreatorFactoryTest.java +++ b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/forward/ForwardIndexCreatorFactoryTest.java @@ -23,13 +23,17 @@ import java.nio.ByteBuffer; import java.nio.file.Files; import java.util.Arrays; +import java.util.Random; import org.apache.commons.io.FileUtils; +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.local.segment.creator.impl.fwd.CompressionStatsTrackingForwardIndexCreator; import org.apache.pinot.segment.local.segment.index.readers.forward.ChunkReaderContext; import org.apache.pinot.segment.local.segment.index.readers.forward.FixedByteChunkSVForwardIndexReaderV7; import org.apache.pinot.segment.local.segment.index.readers.forward.FixedBytePower2ChunkSVForwardIndexReader; import org.apache.pinot.segment.spi.V1Constants; +import org.apache.pinot.segment.spi.codec.CodecSpecParser; import org.apache.pinot.segment.spi.compression.ChunkCompressionType; import org.apache.pinot.segment.spi.creator.IndexCreationContext; import org.apache.pinot.segment.spi.index.ForwardIndexConfig; @@ -140,7 +144,8 @@ public void testCodecSpecRoundTripUsesV7Format(String codecSpec, DataType stored .build(); TableConfig tableConfig = new TableConfigBuilder(TableType.OFFLINE).setTableName("testTable").build(); tableConfig.getIndexingConfig().setCompressionStatsEnabled(compressionStatsEnabled); - long[] values = storedType == DataType.INT ? new long[]{11, 13, 21} + long[] values = storedType == DataType.INT + ? new long[]{11, 13, 21} : new long[]{Long.MIN_VALUE, (long) Integer.MAX_VALUE + 1, Long.MAX_VALUE}; try (ForwardIndexCreator creator = ForwardIndexCreatorFactory.createIndexCreator( newContext(indexDir, false, tableConfig, values.length, storedType), config)) { @@ -180,6 +185,116 @@ public void testCodecSpecRoundTripUsesV7Format(String codecSpec, DataType stored } } + /// Realistic-volume round trip: many chunks, a non-power-of-two target normalized to 1024 docs per + /// chunk, a partial final chunk, and reads through both a sequential and a random-access context. + @Test(dataProvider = "v7CodecSpecs") + public void testCodecSpecMultiChunkRoundTrip(String codecSpec, DataType storedType, boolean compressionStatsEnabled) + throws Exception { + File indexDir = Files.createTempDirectory("ForwardIndexCreatorFactoryTest").toFile(); + try { + int numDocs = 10009; + Random random = new Random(42); + long[] values = new long[numDocs]; + for (int i = 0; i < numDocs; i++) { + values[i] = storedType == DataType.INT ? random.nextInt() : random.nextLong(); + } + ForwardIndexConfig config = new ForwardIndexConfig.Builder(FieldConfig.EncodingType.RAW) + .withCodecSpec(codecSpec) + .withTargetDocsPerChunk(1000) + .build(); + TableConfig tableConfig = new TableConfigBuilder(TableType.OFFLINE).setTableName("testTable").build(); + tableConfig.getIndexingConfig().setCompressionStatsEnabled(compressionStatsEnabled); + try (ForwardIndexCreator creator = ForwardIndexCreatorFactory.createIndexCreator( + newContext(indexDir, false, tableConfig, numDocs, storedType), config)) { + for (long value : values) { + if (storedType == DataType.INT) { + creator.putInt((int) value); + } else { + creator.putLong(value); + } + } + creator.seal(); + } + File indexFile = new File(indexDir, COLUMN_NAME + V1Constants.Indexes.RAW_SV_FORWARD_INDEX_FILE_EXTENSION); + try (PinotDataBuffer buffer = PinotDataBuffer.mapReadOnlyBigEndianFile(indexFile); + ForwardIndexReader reader = + ForwardIndexReaderFactory.getInstance().createRawIndexReader(buffer, storedType, true)) { + FixedByteChunkSVForwardIndexReaderV7 v7Reader = (FixedByteChunkSVForwardIndexReaderV7) reader; + // Header ints: version, magic, numChunks, numDocsPerChunk, sizeOfEntry, totalDocs + assertEquals(buffer.getInt(3 * Integer.BYTES), 1024, "targetDocsPerChunk=1000 should normalize to 1024"); + assertEquals(buffer.getInt(2 * Integer.BYTES), (numDocs + 1023) / 1024); + assertEquals(buffer.getInt(5 * Integer.BYTES), numDocs); + try (FixedByteChunkSVForwardIndexReaderV7.Context sequential = v7Reader.createContext(); + FixedByteChunkSVForwardIndexReaderV7.Context randomAccess = v7Reader.createContext()) { + for (int docId = 0; docId < numDocs; docId++) { + assertEquals(readValue(v7Reader, storedType, docId, sequential), values[docId], "docId " + docId); + } + for (int i = 0; i < 2000; i++) { + int docId = random.nextInt(numDocs); + assertEquals(readValue(v7Reader, storedType, docId, randomAccess), values[docId], "docId " + docId); + } + } + } + } finally { + FileUtils.deleteQuietly(indexDir); + } + } + + private static long readValue(FixedByteChunkSVForwardIndexReaderV7 reader, DataType storedType, int docId, + FixedByteChunkSVForwardIndexReaderV7.Context context) { + return storedType == DataType.INT ? reader.getInt(docId, context) : reader.getLong(docId, context); + } + + /// The V7 writer rejects shapes it cannot represent and refuses to seal a file whose declared document + /// count does not match what was written; the reader rejects a mismatched stored type and reads after + /// its context is closed. + @Test + public void testV7WriterAndReaderGuards() + throws Exception { + assertTrue(CodecSpecParser.MAX_SPEC_LENGTH <= FixedByteChunkForwardIndexWriterV7.MAX_CODEC_SPEC_LENGTH_BYTES, + "The DSL parser limit must not exceed the frozen V7 header limit"); + File indexDir = Files.createTempDirectory("ForwardIndexCreatorFactoryTest").toFile(); + try { + File indexFile = new File(indexDir, COLUMN_NAME + V1Constants.Indexes.RAW_SV_FORWARD_INDEX_FILE_EXTENSION); + CodecPipelineExecutor intExecutor = CodecPipelineExecutor.create("DELTA,LZ4", DataType.INT); + expectThrows(IllegalArgumentException.class, + () -> new FixedByteChunkForwardIndexWriterV7(indexFile, intExecutor, 3, 2, Long.BYTES)); + try (FixedByteChunkForwardIndexWriterV7 writer = + new FixedByteChunkForwardIndexWriterV7(indexFile, intExecutor, 3, 2, Integer.BYTES)) { + expectThrows(UnsupportedOperationException.class, () -> writer.putFloat(1.0f)); + expectThrows(UnsupportedOperationException.class, () -> writer.putDouble(1.0)); + expectThrows(IllegalStateException.class, () -> writer.putLong(1L)); + writer.putInt(1); + writer.putInt(2); + writer.putInt(3); + expectThrows(IllegalStateException.class, () -> writer.putInt(4)); + } + FixedByteChunkForwardIndexWriterV7 shortWriter = + new FixedByteChunkForwardIndexWriterV7(indexFile, intExecutor, 3, 2, Integer.BYTES); + shortWriter.putInt(1); + expectThrows(IllegalStateException.class, shortWriter::close); + + try (FixedByteChunkForwardIndexWriterV7 writer = new FixedByteChunkForwardIndexWriterV7(indexFile, + CodecPipelineExecutor.create("LZ4", DataType.LONG), 2, 2, Long.BYTES)) { + writer.putLong(1L); + writer.putLong(2L); + } + try (PinotDataBuffer buffer = PinotDataBuffer.mapReadOnlyBigEndianFile(indexFile)) { + expectThrows(IllegalArgumentException.class, + () -> new FixedByteChunkSVForwardIndexReaderV7(buffer, DataType.INT)); + expectThrows(UnsupportedOperationException.class, + () -> ForwardIndexReaderFactory.getInstance().createRawIndexReader(buffer, DataType.DOUBLE, true)); + FixedByteChunkSVForwardIndexReaderV7 reader = new FixedByteChunkSVForwardIndexReaderV7(buffer, DataType.LONG); + FixedByteChunkSVForwardIndexReaderV7.Context context = reader.createContext(); + assertEquals(reader.getLong(0, context), 1L); + context.close(); + expectThrows(IllegalStateException.class, () -> reader.getLong(0, context)); + } + } finally { + FileUtils.deleteQuietly(indexDir); + } + } + @DataProvider(name = "compressionStatsEnabled") public Object[][] compressionStatsEnabled() { return new Object[][]{{false}, {true}}; @@ -226,23 +341,28 @@ public void testLegacyCompressionStatsRemainOptIn(boolean compressionStatsEnable } } + /// Columns: mutation, expected failure type, expected message fragment, and whether the corruption is + /// only detected on first read of the affected chunk (header-resident corruption fails at load). @DataProvider(name = "malformedV7Files") public Object[][] malformedV7Files() { return new Object[][]{ - {"metadata", IllegalArgumentException.class, "does not match segment metadata"}, - {"chunkSize", IllegalArgumentException.class, "positive power of two"}, - {"specLength", IllegalArgumentException.class, "Invalid specLength"}, - {"offset", IllegalArgumentException.class, "Corrupt chunkOffsets"}, - {"truncated", IllegalArgumentException.class, "Corrupt per-chunk header"}, - {"trailing", IllegalArgumentException.class, "Corrupt V7 data section"}, - {"encodedSize", IllegalArgumentException.class, "Corrupt per-chunk header"}, - {"decodedSize", IllegalStateException.class, "decodedSize"} + {"metadata", IllegalArgumentException.class, "does not match segment metadata", false}, + {"chunkSize", IllegalArgumentException.class, "positive power of two", false}, + {"specLength", IllegalArgumentException.class, "Invalid specLength", false}, + {"offset", IllegalArgumentException.class, "Corrupt chunkOffsets[0]", false}, + {"offsetOrder", IllegalArgumentException.class, "Corrupt chunkOffsets[1]", false}, + {"truncated", IllegalArgumentException.class, "Corrupt per-chunk header", false}, + {"trailing", IllegalArgumentException.class, "Corrupt V7 data section", false}, + {"encodedSize", IllegalStateException.class, "Corrupt per-chunk header", true}, + {"gap", IllegalStateException.class, "exactPayloadBytes", true}, + {"decodedSize", IllegalStateException.class, "decodedSize", true} }; } @Test(dataProvider = "malformedV7Files") public void testMalformedV7FileIsRejected(String mutation, Class failureType, - String message) throws Exception { + String message, boolean rejectedAtRead) + throws Exception { File indexDir = Files.createTempDirectory("ForwardIndexCreatorFactoryTest").toFile(); try { ForwardIndexConfig config = new ForwardIndexConfig.Builder(FieldConfig.EncodingType.RAW) @@ -271,6 +391,10 @@ public void testMalformedV7FileIsRejected(String mutation, Class { + RuntimeException failure; + if (rejectedAtRead) { + // Construction must succeed: frame-level corruption is only detected when the chunk is read. try (PinotDataBuffer buffer = PinotDataBuffer.mapReadOnlyBigEndianFile(indexFile); ForwardIndexReader reader = ForwardIndexReaderFactory.getInstance().createIndexReader(buffer, metadata)) { FixedByteChunkSVForwardIndexReaderV7 v7Reader = (FixedByteChunkSVForwardIndexReaderV7) reader; try (FixedByteChunkSVForwardIndexReaderV7.Context context = v7Reader.createContext()) { - v7Reader.getInt(0, context); + failure = expectThrows(failureType, () -> v7Reader.getInt(0, context)); } } - }); + } else { + failure = expectThrows(failureType, () -> { + try (PinotDataBuffer buffer = PinotDataBuffer.mapReadOnlyBigEndianFile(indexFile); + ForwardIndexReader reader = + ForwardIndexReaderFactory.getInstance().createIndexReader(buffer, metadata)) { + assertTrue(reader instanceof FixedByteChunkSVForwardIndexReaderV7); + } + }); + } assertTrue(failure.getMessage().contains(message), failure.getMessage()); } finally { FileUtils.deleteQuietly(indexDir); diff --git a/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/ForwardIndexHandlerTest.java b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/ForwardIndexHandlerTest.java index ddb47f0afd20..3fa71c999f2c 100644 --- a/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/ForwardIndexHandlerTest.java +++ b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/ForwardIndexHandlerTest.java @@ -32,6 +32,7 @@ import java.util.Map; import java.util.Random; import java.util.Set; +import java.util.TreeSet; import javax.annotation.Nullable; import org.apache.commons.configuration2.PropertiesConfiguration; import org.apache.commons.configuration2.ex.ConfigurationException; @@ -2871,15 +2872,75 @@ private void assertNoCodecRewrite(String column) } } + /// One config push can toggle a standalone dictionary and change the codec of the same RAW column. + /// Both operations must be queued in a single reload: the dictionary is built from the existing + /// forward index first, then the forward index is rewritten to V7 while staying RAW. The mirror + /// drops the dictionary while changing the codec again. + @Test + public void testDictionaryToggleAndCodecSpecTogether() + throws Exception { + String column = DIM_LZ4_INTEGER; + ObjectNode forward = JsonUtils.newObjectNode(); + forward.put("codecSpec", "DELTA,LZ4"); + ObjectNode indexes = JsonUtils.newObjectNode(); + indexes.set("forward", forward); + indexes.set("dictionary", JsonUtils.newObjectNode()); + _invertedIndexColumns.add(column); + _noDictionaryColumns.remove(column); + _fieldConfigMap.put(column, new FieldConfig(column, FieldConfig.EncodingType.RAW, null, + List.of(FieldConfig.IndexType.INVERTED), null, null, indexes, null, null)); + applyOperations(column, List.of(ForwardIndexHandler.Operation.ENABLE_DICTIONARY, + ForwardIndexHandler.Operation.REWRITE_FORWARD_INDEX)); + assertRawForwardIndexState(column, "DELTA,LZ4", null); + assertStandaloneDictionaryState(column, true); + + _invertedIndexColumns.remove(column); + _noDictionaryColumns.add(column); + _fieldConfigMap.put(column, rawFieldConfigWithCodecSpec(column, "DELTA,ZSTD(3)")); + applyOperations(column, List.of(ForwardIndexHandler.Operation.DISABLE_DICTIONARY, + ForwardIndexHandler.Operation.REWRITE_FORWARD_INDEX)); + assertRawForwardIndexState(column, "DELTA,ZSTD(3)", null); + assertStandaloneDictionaryState(column, false); + } + + private void assertStandaloneDictionaryState(String column, boolean expectDictionary) + throws Exception { + try (SegmentDirectory segmentDirectory = new SegmentLocalFSDirectory(INDEX_DIR, ReadMode.mmap); + SegmentDirectory.Reader reader = segmentDirectory.createReader()) { + ColumnMetadata metadata = segmentDirectory.getSegmentMetadata().getColumnMetadataFor(column); + assertEquals(metadata.hasDictionary(), expectDictionary); + assertEquals(metadata.getForwardIndexEncoding(), FieldConfig.EncodingType.RAW); + assertEquals(reader.hasIndexFor(column, StandardIndexes.dictionary()), expectDictionary); + if (expectDictionary) { + // Check the dictionary against the source data, not against metadata derived from the same rebuild. + TreeSet expectedValues = new TreeSet<>(); + for (GenericRow row : TEST_DATA) { + expectedValues.add(((Number) row.getValue(column)).intValue()); + } + try (Dictionary dictionary = DictionaryIndexType.read(reader, metadata)) { + assertEquals(dictionary.length(), expectedValues.size()); + int dictId = 0; + for (int expected : expectedValues) { + assertEquals(dictionary.getIntValue(dictId++), expected); + } + } + } + } + } + private void applyCodecRewrite(String column) throws Exception { + applyOperations(column, List.of(ForwardIndexHandler.Operation.REWRITE_FORWARD_INDEX)); + } + + private void applyOperations(String column, List expectedOperations) + throws Exception { try (SegmentDirectory segmentDirectory = new SegmentLocalFSDirectory(INDEX_DIR, ReadMode.mmap); SegmentDirectory.Writer writer = segmentDirectory.createWriter()) { _segmentDirectory = segmentDirectory; _writer = writer; ForwardIndexHandler handler = createForwardIndexHandler(); - assertEquals(handler.computeOperations(writer), - Map.of(column, List.of(ForwardIndexHandler.Operation.REWRITE_FORWARD_INDEX))); + assertEquals(handler.computeOperations(writer), Map.of(column, expectedOperations)); handler.updateIndices(writer); handler.postUpdateIndicesCleanup(writer); } diff --git a/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/ForwardIndexConfig.java b/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/ForwardIndexConfig.java index c23370d8767d..9545225b1a76 100644 --- a/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/ForwardIndexConfig.java +++ b/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/ForwardIndexConfig.java @@ -226,9 +226,11 @@ public CompressionCodec getCompressionCodec() { /// Returns the structurally normalized codec specification, or `null` for the legacy compression path. /// - /// Any non-null specification selects the V7 on-disk format. Before enabling it, upgrade every component that may - /// build or read segments. Before rolling those components back, remove the specification and rewrite affected V7 - /// forward indexes into a legacy format. + /// Any non-null specification selects the V7 on-disk format, which honors `targetDocsPerChunk` but not + /// `rawIndexWriterVersion`, `deriveNumDocsPerChunk` or `targetMaxChunkSize`. Before enabling it, upgrade every + /// component that may build or read segments. Before rolling those components back, remove the specification and + /// regenerate or re-push affected segments so the deep-store copy is in a legacy format; a reload alone only + /// rewrites the server-local copy and a later re-download would bring back an unreadable V7 index. @Nullable public String getCodecSpec() { return _codecSpec; From a0eaf72e350d24f510546571eaac8494300ec6d6 Mon Sep 17 00:00:00 2001 From: Xiang Fu Date: Sat, 5 Sep 2026 22:33:44 -0700 Subject: [PATCH 3/5] Resolve configured codec specs once per reload and add a V7 write benchmark Reload reconciliation built and discarded a CodecPipelineExecutor per column on each of the two computeOperations passes just to obtain a canonical spec string. Memoize the canonical form per handler, keyed by the configured spec and the stored type it is validated against. Trim the V7 creator test matrix: the compressionStatsEnabled flag is no longer a data-provider dimension (the "V7 reports no legacy compression stats" guarantee is pinned once), and the multi-chunk round trip uses its own small provider instead of running every pipeline at 10k docs. Add BenchmarkV7ForwardIndexWriter to pinot-perf so the per-chunk buffer behavior of the V7 write path is measurable against the legacy writer. --- .../perf/BenchmarkV7ForwardIndexWriter.java | 194 ++++++++++++++++++ .../index/loader/ForwardIndexHandler.java | 16 +- .../ForwardIndexCreatorFactoryTest.java | 30 +-- 3 files changed, 226 insertions(+), 14 deletions(-) create mode 100644 pinot-perf/src/main/java/org/apache/pinot/perf/BenchmarkV7ForwardIndexWriter.java diff --git a/pinot-perf/src/main/java/org/apache/pinot/perf/BenchmarkV7ForwardIndexWriter.java b/pinot-perf/src/main/java/org/apache/pinot/perf/BenchmarkV7ForwardIndexWriter.java new file mode 100644 index 000000000000..4c7122539342 --- /dev/null +++ b/pinot-perf/src/main/java/org/apache/pinot/perf/BenchmarkV7ForwardIndexWriter.java @@ -0,0 +1,194 @@ +/** + * 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.perf; + +import java.io.File; +import java.io.IOException; +import java.util.SplittableRandom; +import java.util.UUID; +import java.util.concurrent.TimeUnit; +import org.apache.commons.io.FileUtils; +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.local.io.writer.impl.FixedByteChunkWriter; +import org.apache.pinot.segment.spi.compression.ChunkCompressionType; +import org.apache.pinot.spi.data.FieldSpec.DataType; +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.BenchmarkMode; +import org.openjdk.jmh.annotations.Fork; +import org.openjdk.jmh.annotations.Level; +import org.openjdk.jmh.annotations.Measurement; +import org.openjdk.jmh.annotations.Mode; +import org.openjdk.jmh.annotations.OutputTimeUnit; +import org.openjdk.jmh.annotations.Param; +import org.openjdk.jmh.annotations.Scope; +import org.openjdk.jmh.annotations.Setup; +import org.openjdk.jmh.annotations.State; +import org.openjdk.jmh.annotations.TearDown; +import org.openjdk.jmh.annotations.Warmup; +import org.openjdk.jmh.runner.Runner; +import org.openjdk.jmh.runner.options.OptionsBuilder; + + +/// Measures the segment-creation (ingestion) write path of the V7 codec-pipeline raw forward index +/// against the legacy [FixedByteChunkForwardIndexWriter] baseline. +/// +/// Each invocation writes [#RECORDS] monotonically increasing values through the default ~1024-doc +/// chunking, so a single op spans roughly a thousand chunk encodes and exercises the steady-state +/// per-chunk cost rather than one-off setup. Pipelines cover a compression-only spec, a two-stage +/// transform+compression chain and a three-stage chain, for both INT and LONG. +/// +/// The point of comparison is per-chunk buffer churn: the legacy writer reuses one compression +/// buffer for the life of the writer, and the V7 writer reuses a single per-writer +/// `CodecPipelineExecutor.EncodeScratch` workspace instead of allocating and explicitly cleaning a +/// direct buffer per codec stage per chunk. Run with `-prof gc` to observe that: +/// `gc.alloc.rate.norm` counts the `DirectByteBuffer` wrappers and their cleaner registrations, so +/// a regression that reintroduces per-stage allocation shows up as allocated bytes per op scaling +/// with chunk count times stage count. +/// +/// Per-writer buffer lifecycle is not symmetric between the two arms: the legacy writer leaves its +/// header, chunk and compression buffers to the GC, while V7 cleans its buffers and scratch at +/// close. Only the slope of allocated bytes against chunk count within one arm is meaningful; the +/// absolute cross-arm delta carries a constant per-op offset and should not be read as churn. +/// +/// Build and run: +/// +/// ``` +/// ./mvnw install -DskipTests -pl pinot-perf -am +/// java -jar pinot-perf/target/benchmarks.jar BenchmarkV7ForwardIndexWriter -prof gc +/// ``` +/// +/// The shaded `benchmarks.jar` comes from the default `build-shaded-jar` profile; under +/// `-Ppinot-fastdev` that profile is off, so run [#main] off the module classpath instead. Trial +/// teardown prints the encoded bytes per row for the last file the trial wrote, tagged with the +/// writer that produced it, which gives the compression ratio alongside the throughput numbers. +@State(Scope.Benchmark) +@BenchmarkMode(Mode.AverageTime) +@OutputTimeUnit(TimeUnit.MILLISECONDS) +@Fork(1) +@Warmup(iterations = 2, time = 2) +@Measurement(iterations = 3, time = 3) +public class BenchmarkV7ForwardIndexWriter { + + /// Enough rows to span ~1000 chunks at the default chunk size, matching the per-million-row + /// allocation accounting raised in review. + private static final int RECORDS = 1_000_000; + + /// Pinot's default target docs per chunk; both writers round this up to 1024. + private static final int TARGET_DOCS_PER_CHUNK = 1000; + + /// Terminal compressor shared by every benchmarked pipeline, so the legacy baseline compresses + /// the same way the V7 pipelines finish. The legacy score therefore does not vary with + /// [#_codecSpec] and the repeated rows double as a noise estimate. + private static final ChunkCompressionType LEGACY_COMPRESSION = ChunkCompressionType.LZ4; + + /// The legacy arm ignores [#_codecSpec] entirely, so its output is labelled with the compressor + /// it actually used rather than the pipeline spec of the surrounding parameter combination. + private static final String LEGACY_LABEL = "legacy " + LEGACY_COMPRESSION + " (codecSpec ignored)"; + + @Param({"INT", "LONG"}) + DataType _dataType; + + @Param({"LZ4", "DELTA,LZ4", "DELTA,T64,LZ4"}) + String _codecSpec; + + private long[] _values; + private CodecPipelineExecutor _executor; + private File _targetDir; + private File _file; + private long _lastEncodedBytes; + private String _lastWriter; + + @Setup(Level.Trial) + public void setup() + throws IOException { + // Per-JVM directory: a concurrent run of this class (e.g. baseline vs change) must not delete + // the other run's in-flight file from its own trial teardown. + _targetDir = new File(FileUtils.getTempDirectory(), "BenchmarkV7ForwardIndexWriter-" + UUID.randomUUID()); + FileUtils.forceMkdir(_targetDir); + _file = new File(_targetDir, "forward-index"); + _executor = CodecPipelineExecutor.create(_codecSpec, _dataType); + // Slowly increasing values (epoch-second-like), the shape DELTA/T64 are meant for, and small + // enough that the INT and LONG runs encode the same logical sequence. + SplittableRandom random = new SplittableRandom(42); + _values = new long[RECORDS]; + long value = 1_600_000_000L; + for (int i = 0; i < RECORDS; i++) { + value += random.nextInt(64); + _values[i] = value; + } + } + + @TearDown(Level.Trial) + public void tearDown() { + System.out.printf("%n[%s %s] %.3f encoded bytes/row (%d raw)%n", _dataType, _lastWriter, + (double) _lastEncodedBytes / RECORDS, _dataType.size()); + FileUtils.deleteQuietly(_targetDir); + } + + /// Sizes the written index outside the measured region, then clears it so the next iteration + /// starts from an empty file. + @TearDown(Level.Iteration) + public void measureAndDeleteFile() { + if (_file.exists()) { + _lastEncodedBytes = _file.length(); + } + FileUtils.deleteQuietly(_file); + } + + @Benchmark + public int writeV7() + throws IOException { + _lastWriter = _codecSpec; + try (FixedByteChunkForwardIndexWriterV7 writer = new FixedByteChunkForwardIndexWriterV7(_file, _executor, RECORDS, + TARGET_DOCS_PER_CHUNK, _dataType.size())) { + writeAll(writer); + } + return RECORDS; + } + + @Benchmark + public int writeLegacy() + throws IOException { + _lastWriter = LEGACY_LABEL; + try (FixedByteChunkForwardIndexWriter writer = new FixedByteChunkForwardIndexWriter(_file, LEGACY_COMPRESSION, + RECORDS, TARGET_DOCS_PER_CHUNK, _dataType.size(), 4)) { + writeAll(writer); + } + return RECORDS; + } + + private void writeAll(FixedByteChunkWriter writer) { + if (_dataType == DataType.INT) { + for (int i = 0; i < RECORDS; i++) { + writer.putInt((int) _values[i]); + } + } else { + for (int i = 0; i < RECORDS; i++) { + writer.putLong(_values[i]); + } + } + } + + public static void main(String[] args) + throws Exception { + new Runner(new OptionsBuilder().include(BenchmarkV7ForwardIndexWriter.class.getSimpleName()).build()).run(); + } +} diff --git a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/ForwardIndexHandler.java b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/ForwardIndexHandler.java index 42b0109090e1..4ef043acb9f3 100644 --- a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/ForwardIndexHandler.java +++ b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/ForwardIndexHandler.java @@ -104,6 +104,13 @@ public class ForwardIndexHandler extends BaseIndexHandler { private static final List> DICTIONARY_BASED_INDEXES_TO_REWRITE = Arrays.asList(StandardIndexes.range(), StandardIndexes.fst(), StandardIndexes.inverted()); + /// Memoizes the canonical form of each configured codec spec for this handler's lifetime. + /// [#computeOperations] runs twice per handler (once from [#needUpdateIndices], once from [#updateIndices]) and + /// canonicalization is pure string work over an immutable spec, so resolving the same configured spec again only + /// repeats the parse and validation. Keyed by the raw spec together with the stored type because parsing is + /// validated against the stored type: the same spec may canonicalize for one type and be rejected for another. + private final Map _canonicalCodecSpecs = new HashMap<>(); + /// Re-enable operations are split by target encoding so the intent is explicit at the operation level: a /// `forwardIndex.disabled` column being re-enabled may want to come back as either dict-encoded or raw, /// depending on the new config. Both variants flow through the same regenerate-from-inverted-index path @@ -114,6 +121,10 @@ protected enum Operation { ENABLE_DICTIONARY, REWRITE_FORWARD_INDEX } + /// Key of [#_canonicalCodecSpecs]: the configured codec spec plus the stored type it is validated against. + private record CodecSpecKey(String _spec, DataType _storedType) { + } + @VisibleForTesting public ForwardIndexHandler(SegmentDirectory segmentDirectory, IndexLoadingConfig indexLoadingConfig) { this(segmentDirectory, indexLoadingConfig.getFieldIndexConfigByColName(), indexLoadingConfig.getTableConfig(), @@ -542,8 +553,9 @@ private boolean shouldRewriteRawForwardIndex(String column, SegmentDirectory.Rea String newCodecSpec = newConfig.getCodecSpec(); if (newCodecSpec != null) { // Compare canonical forms so equivalent spellings (case, aliases, default arguments) do not trigger a rewrite. - String canonicalNewSpec = CodecPipelineExecutor.create(newCodecSpec, - existingColMetadata.getDataType().getStoredType()).getCanonicalSpec(); + DataType storedType = existingColMetadata.getDataType().getStoredType(); + String canonicalNewSpec = _canonicalCodecSpecs.computeIfAbsent(new CodecSpecKey(newCodecSpec, storedType), + key -> CodecPipelineExecutor.create(key._spec(), key._storedType()).getCanonicalSpec()); return !canonicalNewSpec.equals(existingCodecSpec); } diff --git a/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/forward/ForwardIndexCreatorFactoryTest.java b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/forward/ForwardIndexCreatorFactoryTest.java index e60a9e81a8e6..27143342fab8 100644 --- a/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/forward/ForwardIndexCreatorFactoryTest.java +++ b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/forward/ForwardIndexCreatorFactoryTest.java @@ -124,17 +124,14 @@ public void testTableConfigConstructorPropagatesCompressionStatsFlag() @DataProvider(name = "v7CodecSpecs") public Object[][] v7CodecSpecs() { return new Object[][]{ - {"LZ4", DataType.INT, false}, {"LZ4", DataType.INT, true}, - {"DELTA,LZ4", DataType.INT, false}, {"DELTA,LZ4", DataType.INT, true}, - {"ZSTD(3)", DataType.LONG, false}, {"ZSTD(3)", DataType.LONG, true}, - {"DELTADELTA,GORILLA,ZSTD(3)", DataType.LONG, false}, - {"DELTADELTA,GORILLA,ZSTD(3)", DataType.LONG, true} + {"LZ4", DataType.INT}, {"DELTA,LZ4", DataType.INT}, + {"ZSTD(3)", DataType.LONG}, {"DELTADELTA,GORILLA,ZSTD(3)", DataType.LONG} }; } /// Both compression-only and transform pipelines use V7, including a partial final chunk. @Test(dataProvider = "v7CodecSpecs") - public void testCodecSpecRoundTripUsesV7Format(String codecSpec, DataType storedType, boolean compressionStatsEnabled) + public void testCodecSpecRoundTripUsesV7Format(String codecSpec, DataType storedType) throws Exception { File indexDir = Files.createTempDirectory("ForwardIndexCreatorFactoryTest").toFile(); try { @@ -143,7 +140,8 @@ public void testCodecSpecRoundTripUsesV7Format(String codecSpec, DataType stored .withTargetDocsPerChunk(2) .build(); TableConfig tableConfig = new TableConfigBuilder(TableType.OFFLINE).setTableName("testTable").build(); - tableConfig.getIndexingConfig().setCompressionStatsEnabled(compressionStatsEnabled); + // Legacy compression stats are opted in here on purpose: a codecSpec column never reports them. + tableConfig.getIndexingConfig().setCompressionStatsEnabled(true); long[] values = storedType == DataType.INT ? new long[]{11, 13, 21} : new long[]{Long.MIN_VALUE, (long) Integer.MAX_VALUE + 1, Long.MAX_VALUE}; @@ -151,14 +149,12 @@ public void testCodecSpecRoundTripUsesV7Format(String codecSpec, DataType stored newContext(indexDir, false, tableConfig, values.length, storedType), config)) { assertFalse(creator.isDictionaryEncoded()); assertNull(creator.getRawForwardIndexChunkCompressionType()); - assertEquals(creator.getRawForwardIndexUncompressedValueSizeInBytes(), -1L); for (long value : values) { if (storedType == DataType.INT) { creator.putInt((int) value); } else { creator.putLong(value); } - assertEquals(creator.getRawForwardIndexUncompressedValueSizeInBytes(), -1L); } creator.seal(); assertEquals(creator.getRawForwardIndexUncompressedValueSizeInBytes(), -1L); @@ -185,10 +181,21 @@ public void testCodecSpecRoundTripUsesV7Format(String codecSpec, DataType stored } } + /// One multi-stage pipeline per stored type, plus a single-stage spec so the executor's + /// multi-stage-only scratch slots and the single-stage path both run at chunk-boundary scale. + /// Every spec in `v7CodecSpecs` is already round-tripped, including a partial final chunk, by + /// [#testCodecSpecRoundTripUsesV7Format] above. + @DataProvider(name = "v7MultiChunkCodecSpecs") + public Object[][] v7MultiChunkCodecSpecs() { + return new Object[][]{ + {"LZ4", DataType.INT}, {"DELTA,LZ4", DataType.INT}, {"DELTADELTA,GORILLA,ZSTD(3)", DataType.LONG} + }; + } + /// Realistic-volume round trip: many chunks, a non-power-of-two target normalized to 1024 docs per /// chunk, a partial final chunk, and reads through both a sequential and a random-access context. - @Test(dataProvider = "v7CodecSpecs") - public void testCodecSpecMultiChunkRoundTrip(String codecSpec, DataType storedType, boolean compressionStatsEnabled) + @Test(dataProvider = "v7MultiChunkCodecSpecs") + public void testCodecSpecMultiChunkRoundTrip(String codecSpec, DataType storedType) throws Exception { File indexDir = Files.createTempDirectory("ForwardIndexCreatorFactoryTest").toFile(); try { @@ -203,7 +210,6 @@ public void testCodecSpecMultiChunkRoundTrip(String codecSpec, DataType storedTy .withTargetDocsPerChunk(1000) .build(); TableConfig tableConfig = new TableConfigBuilder(TableType.OFFLINE).setTableName("testTable").build(); - tableConfig.getIndexingConfig().setCompressionStatsEnabled(compressionStatsEnabled); try (ForwardIndexCreator creator = ForwardIndexCreatorFactory.createIndexCreator( newContext(indexDir, false, tableConfig, numDocs, storedType), config)) { for (long value : values) { From b9800398044b47a8fbf65698100aac2356f467f7 Mon Sep 17 00:00:00 2001 From: Xiang Fu Date: Fri, 25 Sep 2026 00:08:13 -0700 Subject: [PATCH 4/5] Make V7 writer close non-throwing on partial writes and cover payload corruption Closing the V7 writer before every declared document was written is the state of every creator when a segment build aborts. Throwing there masked the original failure in the driver's catch-and-close path and stopped sibling creators from being closed. Log a warning and leave the file header-less instead, make close idempotent, reject writes after close, and allocate all direct buffers after the file is open so a failed open leaks nothing. Drop the per-row docId bounds check in the V7 reader in favour of a once-per-chunk check, matching the legacy readers. Remove a dead null-check in the creator factory and rename the handler's record components to plain names. Tests: corrupt payload bytes fail inside the codec pipeline and do not poison the context's cached chunk; early close leaves an unreadable file and rejects further writes; a legacy compressionCodec change is applied in the same reload as a standalone-dictionary toggle; leaving V7 with an explicit legacy codec lands on that codec. --- .../FixedByteChunkForwardIndexWriterV7.java | 92 ++++++++++++------- .../forward/ForwardIndexCreatorFactory.java | 5 +- .../index/loader/ForwardIndexHandler.java | 4 +- .../FixedByteChunkSVForwardIndexReaderV7.java | 13 ++- .../ForwardIndexCreatorFactoryTest.java | 57 +++++++++++- .../index/loader/ForwardIndexHandlerTest.java | 40 ++++++++ 6 files changed, 170 insertions(+), 41 deletions(-) diff --git a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/io/writer/impl/FixedByteChunkForwardIndexWriterV7.java b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/io/writer/impl/FixedByteChunkForwardIndexWriterV7.java index f4facd003ebe..a35dbf6687c5 100644 --- a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/io/writer/impl/FixedByteChunkForwardIndexWriterV7.java +++ b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/io/writer/impl/FixedByteChunkForwardIndexWriterV7.java @@ -29,6 +29,8 @@ import org.apache.pinot.segment.local.io.codec.CodecPipelineExecutor; import org.apache.pinot.segment.spi.memory.CleanerUtil; import org.apache.pinot.spi.data.FieldSpec.DataType; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; /// Chunk-based raw (non-dictionary-encoded) forward index writer for single-value fixed-width @@ -59,9 +61,15 @@ /// are 8-byte longs to support files larger than 2 GB. The per-chunk size header allows readers /// to verify decoded output and to skip/read chunks without scanning adjacent offsets. /// +/// Closing the writer before every declared document was written releases its resources and leaves +/// the file without a header, so it can never be mistaken for a valid index; it does not throw. That +/// is the state of every creator when a segment build aborts, and a throwing `close()` would mask the +/// original failure and stop sibling creators from being closed. +/// /// This class is *not* thread-safe. @NotThreadSafe public class FixedByteChunkForwardIndexWriterV7 implements FixedByteChunkWriter { + private static final Logger LOGGER = LoggerFactory.getLogger(FixedByteChunkForwardIndexWriterV7.class); /// Frozen on-disk format version. The version must be paired with [#FORMAT_MAGIC] because legacy /// fixed-byte writers also accept arbitrary versions greater than or equal to 4. @@ -99,6 +107,7 @@ public class FixedByteChunkForwardIndexWriterV7 implements FixedByteChunkWriter // Hold both the RAF and its FileChannel: closing the channel closes the underlying FD, but // some JVM finalizers close the FD when the RAF becomes unreachable. Holding the RAF as a // field anchors it to the writer's lifetime and removes any reliance on finalizer ordering. + private final File _file; private final RandomAccessFile _raf; private final FileChannel _dataFile; private final CodecPipelineExecutor _executor; @@ -109,13 +118,14 @@ public class FixedByteChunkForwardIndexWriterV7 implements FixedByteChunkWriter private final int _maxFullChunkEncodedSize; private final ByteBuffer _header; private final ByteBuffer _chunkBuffer; - private final ByteBuffer _chunkHeaderBuffer = ByteBuffer.allocateDirect(CHUNK_HEADER_BYTES); + private final ByteBuffer _chunkHeaderBuffer; private final int _numChunks; private final int _totalDocs; private long _dataOffset; private int _docsWritten; private int _chunksWritten; + private boolean _closed; /// Creates a new writer. /// @@ -130,6 +140,7 @@ public FixedByteChunkForwardIndexWriterV7(File file, CodecPipelineExecutor execu if (totalDocs < 0) { throw new IllegalArgumentException("totalDocs must be non-negative, got: " + totalDocs); } + _file = file; _executor = executor; _numDocsPerChunk = validateAndNormalizeNumDocsPerChunk(executor, sizeOfEntry, numDocsPerChunk); _sizeOfEntry = sizeOfEntry; @@ -164,29 +175,24 @@ public FixedByteChunkForwardIndexWriterV7(File file, CodecPipelineExecutor execu int dataHeaderStart = (int) dataHeaderStartLong; int totalHeaderBytes = (int) totalHeaderBytesLong; - _header = ByteBuffer.allocateDirect(totalHeaderBytes); - _header.putInt(VERSION); - _header.putInt(FORMAT_MAGIC); - _header.putInt(_numChunks); - _header.putInt(_numDocsPerChunk); - _header.putInt(sizeOfEntry); - _header.putInt(totalDocs); - _header.putInt(specBytes.length); - _header.putInt(dataHeaderStart); - _header.put(specBytes); - // chunk offsets will be filled in during writeChunk() calls - _dataOffset = totalHeaderBytes; - // Open file first, then allocate the direct buffer under a try/catch so that an OOM during - // allocation closes the already-open file descriptor (the caller has no reference to a - // partially-constructed object and cannot invoke close() itself). + // Open the file first, then allocate every direct buffer under one try/catch, so that a failure + // at any point releases the descriptor and whatever was allocated so far. The caller never gets + // a reference to a partially-constructed writer and cannot invoke close() itself. RandomAccessFile raf = new RandomAccessFile(file, "rw"); - FileChannel channel = raf.getChannel(); + ByteBuffer header = null; + ByteBuffer chunkBuffer = null; + ByteBuffer chunkHeaderBuffer = null; try { raf.setLength(0L); - _chunkBuffer = ByteBuffer.allocateDirect((int) chunkSizeLong); + header = ByteBuffer.allocateDirect(totalHeaderBytes); + chunkBuffer = ByteBuffer.allocateDirect((int) chunkSizeLong); + chunkHeaderBuffer = ByteBuffer.allocateDirect(CHUNK_HEADER_BYTES); } catch (Throwable t) { + CleanerUtil.cleanQuietly(header); + CleanerUtil.cleanQuietly(chunkBuffer); + CleanerUtil.cleanQuietly(chunkHeaderBuffer); try { raf.close(); } catch (IOException closeEx) { @@ -195,7 +201,21 @@ public FixedByteChunkForwardIndexWriterV7(File file, CodecPipelineExecutor execu throw t; } _raf = raf; - _dataFile = channel; + _dataFile = raf.getChannel(); + _header = header; + _chunkBuffer = chunkBuffer; + _chunkHeaderBuffer = chunkHeaderBuffer; + + _header.putInt(VERSION); + _header.putInt(FORMAT_MAGIC); + _header.putInt(_numChunks); + _header.putInt(_numDocsPerChunk); + _header.putInt(sizeOfEntry); + _header.putInt(totalDocs); + _header.putInt(specBytes.length); + _header.putInt(dataHeaderStart); + _header.put(specBytes); + // chunk offsets will be filled in during writeChunk() calls } /// Writes a 4-byte integer value. @@ -239,6 +259,9 @@ public void putDouble(double value) { /// guard the writer keeps producing chunks past the declared length and only `close()` catches /// the mismatch, leaving a semantically-invalid partial file behind. private void checkRoomForOneMore() { + if (_closed) { + throw new IllegalStateException("V7 forward index writer for " + _file + " is closed"); + } if (_docsWritten >= _totalDocs) { throw new IllegalStateException( "Cannot write past declared totalDocs=" + _totalDocs + " (already wrote " + _docsWritten + ")"); @@ -285,23 +308,30 @@ private void writeChunk() { _chunkBuffer.clear(); } + /// Idempotent. Writes the header only when every declared document was written; otherwise logs a + /// warning and leaves the file header-less (see the class Javadoc), releasing resources either way. @Override public void close() throws IOException { + if (_closed) { + return; + } + _closed = true; try { - if (_chunkBuffer.position() > 0) { - writeChunk(); - } - if (_docsWritten != _totalDocs) { - throw new IllegalStateException( - "Expected " + _totalDocs + " docs but only " + _docsWritten + " were written"); - } - if (_chunksWritten != _numChunks) { - throw new IllegalStateException( - "Expected " + _numChunks + " chunks but wrote " + _chunksWritten); + if (_docsWritten == _totalDocs) { + if (_chunkBuffer.position() > 0) { + writeChunk(); + } + if (_chunksWritten != _numChunks) { + throw new IllegalStateException( + "Expected " + _numChunks + " chunks but wrote " + _chunksWritten); + } + _header.flip(); + writeFully(_header, 0); + } else { + LOGGER.warn("Closing V7 forward index writer for {} after {} of {} declared docs; the file is left without a" + + " header and cannot be loaded", _file, _docsWritten, _totalDocs); } - _header.flip(); - writeFully(_header, 0); } finally { // Close the RAF (which closes its FileChannel) so the underlying file descriptor is released // by an explicit call rather than relying on JVM finalizers. diff --git a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/forward/ForwardIndexCreatorFactory.java b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/forward/ForwardIndexCreatorFactory.java index 0fd5141f39a9..10f685f7bf0f 100644 --- a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/forward/ForwardIndexCreatorFactory.java +++ b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/forward/ForwardIndexCreatorFactory.java @@ -80,7 +80,6 @@ public static ForwardIndexCreator createIndexCreator(IndexCreationContext contex // Raw forward index DataType storedType = fieldSpec.getDataType().getStoredType(); ForwardIndexCreator creator = null; - ChunkCompressionType chunkCompressionType = null; // codecSpec always selects the self-describing V7 codec-pipeline format. The legacy raw // writers remain available only through compressionCodec/chunkCompressionType. @@ -103,9 +102,7 @@ public static ForwardIndexCreator createIndexCreator(IndexCreationContext contex } if (creator == null) { - if (chunkCompressionType == null) { - chunkCompressionType = indexConfig.getChunkCompressionType(); - } + ChunkCompressionType chunkCompressionType = indexConfig.getChunkCompressionType(); if (chunkCompressionType == null) { chunkCompressionType = ForwardIndexType.getDefaultCompressionType(fieldSpec.getFieldType()); } diff --git a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/ForwardIndexHandler.java b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/ForwardIndexHandler.java index 4ef043acb9f3..920fd98d3ae2 100644 --- a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/ForwardIndexHandler.java +++ b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/ForwardIndexHandler.java @@ -122,7 +122,7 @@ protected enum Operation { } /// Key of [#_canonicalCodecSpecs]: the configured codec spec plus the stored type it is validated against. - private record CodecSpecKey(String _spec, DataType _storedType) { + private record CodecSpecKey(String spec, DataType storedType) { } @VisibleForTesting @@ -555,7 +555,7 @@ private boolean shouldRewriteRawForwardIndex(String column, SegmentDirectory.Rea // Compare canonical forms so equivalent spellings (case, aliases, default arguments) do not trigger a rewrite. DataType storedType = existingColMetadata.getDataType().getStoredType(); String canonicalNewSpec = _canonicalCodecSpecs.computeIfAbsent(new CodecSpecKey(newCodecSpec, storedType), - key -> CodecPipelineExecutor.create(key._spec(), key._storedType()).getCanonicalSpec()); + key -> CodecPipelineExecutor.create(key.spec(), key.storedType()).getCanonicalSpec()); return !canonicalNewSpec.equals(existingCodecSpec); } diff --git a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/forward/FixedByteChunkSVForwardIndexReaderV7.java b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/forward/FixedByteChunkSVForwardIndexReaderV7.java index b997e18a58fc..f601a3179317 100644 --- a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/forward/FixedByteChunkSVForwardIndexReaderV7.java +++ b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/forward/FixedByteChunkSVForwardIndexReaderV7.java @@ -44,6 +44,9 @@ /// [ByteBuffer] returned by `getChunkBuffer` is the context's reusable scratch buffer — /// its position/limit are mutated on every chunk transition and it must not be retained across /// subsequent `getInt`/`getLong` calls. +/// +/// Byte-range prefetch hints (`isBufferByteRangeInfoSupported` and friends) are deliberately not +/// implemented yet for this format and fall back to the interface defaults. public final class FixedByteChunkSVForwardIndexReaderV7 implements ForwardIndexReader { @@ -370,9 +373,9 @@ public void close() // ------------------------------------------------------------------------- private ByteBuffer getChunkBuffer(int docId, Context context) { - if (docId < 0 || docId >= _totalDocs) { - throw new IndexOutOfBoundsException("docId " + docId + " is out of bounds [0, " + _totalDocs + ")"); - } + // No per-row bounds check, matching the legacy fixed-byte readers: an out-of-range docId maps to a + // chunk id that loadChunk rejects, and a row past totalDocs inside the last chunk is caught by the + // decoded buffer's limit on the absolute get. int chunkId = docId >>> _shift; if (context._chunkId == chunkId) { return context._chunkBuffer; @@ -386,6 +389,10 @@ private ByteBuffer loadChunk(int chunkId, Context context) { if (context._closed) { throw new IllegalStateException("V7 forward-index reader context is closed"); } + if (chunkId >= _numChunks) { + throw new IndexOutOfBoundsException( + "Chunk " + chunkId + " is out of bounds [0, " + _numChunks + "); totalDocs=" + _totalDocs); + } long chunkStart = getChunkOffset(chunkId); // Validate the chunk offset before using it as a buffer index. The constructor validates the // chunk-offset table, but per-frame sizes are only verified here, on first access: the frame diff --git a/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/forward/ForwardIndexCreatorFactoryTest.java b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/forward/ForwardIndexCreatorFactoryTest.java index 27143342fab8..3679e188682a 100644 --- a/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/forward/ForwardIndexCreatorFactoryTest.java +++ b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/forward/ForwardIndexCreatorFactoryTest.java @@ -275,10 +275,17 @@ public void testV7WriterAndReaderGuards() writer.putInt(3); expectThrows(IllegalStateException.class, () -> writer.putInt(4)); } + // Closing early (the state of every creator when a segment build aborts) must not throw, must leave + // a file that is not recognized as V7, and must reject further writes; a second close is a no-op. FixedByteChunkForwardIndexWriterV7 shortWriter = new FixedByteChunkForwardIndexWriterV7(indexFile, intExecutor, 3, 2, Integer.BYTES); shortWriter.putInt(1); - expectThrows(IllegalStateException.class, shortWriter::close); + shortWriter.close(); + shortWriter.close(); + expectThrows(IllegalStateException.class, () -> shortWriter.putInt(2)); + try (PinotDataBuffer buffer = PinotDataBuffer.mapReadOnlyBigEndianFile(indexFile)) { + assertFalse(FixedByteChunkSVForwardIndexReaderV7.hasCodecPipelineHeader(buffer)); + } try (FixedByteChunkForwardIndexWriterV7 writer = new FixedByteChunkForwardIndexWriterV7(indexFile, CodecPipelineExecutor.create("LZ4", DataType.LONG), 2, 2, Long.BYTES)) { @@ -450,6 +457,54 @@ public void testMalformedV7FileIsRejected(String mutation, Class reader.getInt(docsPerChunk, context)); + expectThrows(RuntimeException.class, () -> reader.getInt(docsPerChunk + 1, context)); + assertEquals(reader.getInt(0, context), 3); + assertEquals(reader.getInt(1, context), 10); + assertEquals(reader.getInt(2 * docsPerChunk, context), 2 * docsPerChunk * 7 + 3); + expectThrows(IndexOutOfBoundsException.class, () -> reader.getInt(numDocs, context)); + expectThrows(IndexOutOfBoundsException.class, () -> reader.getInt(-1, context)); + } + } + } finally { + FileUtils.deleteQuietly(indexDir); + } + } + /// Version 7 alone does not select the codec-pipeline reader: legacy fixed-byte writers accept /// arbitrary versions greater than or equal to 4 and lack the V7 format magic. @Test diff --git a/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/ForwardIndexHandlerTest.java b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/ForwardIndexHandlerTest.java index 3fa71c999f2c..d8951d493384 100644 --- a/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/ForwardIndexHandlerTest.java +++ b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/ForwardIndexHandlerTest.java @@ -2859,6 +2859,46 @@ public void testCodecSpecRemoveOnlyRollback() .build()); applyCodecRewrite(DIM_LZ4_INTEGER); assertRawForwardIndexState(DIM_LZ4_INTEGER, null, ChunkCompressionType.LZ4); + + // Leaving V7 with an explicit legacy codec lands on that codec rather than the field-type default. + _fieldConfigMap.put(DIM_LZ4_INTEGER, rawFieldConfigWithCodecSpec(DIM_LZ4_INTEGER, "T64,LZ4")); + applyCodecRewrite(DIM_LZ4_INTEGER); + assertRawForwardIndexState(DIM_LZ4_INTEGER, "T64,LZ4", null); + _fieldConfigMap.put(DIM_LZ4_INTEGER, new FieldConfig.Builder(DIM_LZ4_INTEGER) + .withEncodingType(FieldConfig.EncodingType.RAW) + .withCompressionCodec(CompressionCodec.ZSTANDARD) + .build()); + applyCodecRewrite(DIM_LZ4_INTEGER); + assertRawForwardIndexState(DIM_LZ4_INTEGER, null, ChunkCompressionType.ZSTANDARD); + } + + /// The relaxed reload gate also applies to legacy tables: a `compressionCodec` change on a RAW column + /// is now applied in the same reload as a standalone-dictionary toggle instead of being deferred. + @Test + public void testDictionaryToggleAndCompressionCodecTogether() + throws Exception { + String column = DIM_LZ4_INTEGER; + ObjectNode indexes = JsonUtils.newObjectNode(); + indexes.set("dictionary", JsonUtils.newObjectNode()); + _invertedIndexColumns.add(column); + _noDictionaryColumns.remove(column); + _fieldConfigMap.put(column, new FieldConfig(column, FieldConfig.EncodingType.RAW, null, + List.of(FieldConfig.IndexType.INVERTED), CompressionCodec.ZSTANDARD, null, indexes, null, null)); + applyOperations(column, List.of(ForwardIndexHandler.Operation.ENABLE_DICTIONARY, + ForwardIndexHandler.Operation.REWRITE_FORWARD_INDEX)); + assertRawForwardIndexState(column, null, ChunkCompressionType.ZSTANDARD); + assertStandaloneDictionaryState(column, true); + + _invertedIndexColumns.remove(column); + _noDictionaryColumns.add(column); + _fieldConfigMap.put(column, new FieldConfig.Builder(column) + .withEncodingType(FieldConfig.EncodingType.RAW) + .withCompressionCodec(CompressionCodec.SNAPPY) + .build()); + applyOperations(column, List.of(ForwardIndexHandler.Operation.DISABLE_DICTIONARY, + ForwardIndexHandler.Operation.REWRITE_FORWARD_INDEX)); + assertRawForwardIndexState(column, null, ChunkCompressionType.SNAPPY); + assertStandaloneDictionaryState(column, false); } private void assertNoCodecRewrite(String column) From f9d64fb057f5e641c36fbe31c4e6c94c157262aa Mon Sep 17 00:00:00 2001 From: Xiang Fu Date: Fri, 25 Sep 2026 18:06:36 -0700 Subject: [PATCH 5/5] Drop unrelated edits and consolidate the V7 round-trip tests Review feedback on #19307, all aimed at narrowing this PR: - Revert the comment-only edit in TextIndexHandler; it is unrelated to the V7 format. The comment there still names the old rewriteForwardIndexForCompressionChange; renaming it is left to whichever PR the reviewer prefers. - Revert RawForwardIndexWithDictionaryTest to its dictionary-focused fixture. The codec columns it grew (codecIntDim/codecLongDim), the codecFieldConfig helper and testCodecPipelineQueries move to the codec integration test in #19309, which already covers the same specs over INT and LONG through both query engines, across chunk boundaries and alongside a dict-encoded column. - Consolidate ForwardIndexCreatorFactoryTest's two codecSpec round trips behind a shared roundTripCodecSpecIndex helper that builds the config, writes the values, asserts the creator contract every codecSpec column must satisfy, checks V7 routing and reads every value back. Both tests keep their distinct assertions: the extreme LONG values and codec-spec header round trip in one, the power-of-two chunk normalization and the random-access context in the other. --- .../RawForwardIndexWithDictionaryTest.java | 39 +----- .../invertedindex/TextIndexHandler.java | 2 +- .../ForwardIndexCreatorFactoryTest.java | 121 ++++++++---------- 3 files changed, 57 insertions(+), 105 deletions(-) diff --git a/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/custom/RawForwardIndexWithDictionaryTest.java b/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/custom/RawForwardIndexWithDictionaryTest.java index fc39bae091f0..62e3672b5277 100644 --- a/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/custom/RawForwardIndexWithDictionaryTest.java +++ b/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/custom/RawForwardIndexWithDictionaryTest.java @@ -55,7 +55,6 @@ /// /// All three columns get identical values so query outputs must match exactly across filters, aggregations, /// GROUP BY, DISTINCT, IN, and REGEXP_LIKE predicates. -/// Additional INT/LONG columns exercise V7 codec pipelines through the same shared cluster. @Test(suiteName = "CustomClusterIntegrationTest") public class RawForwardIndexWithDictionaryTest extends CustomDataQueryClusterIntegrationTest { private static final String TABLE_NAME = "RawForwardIndexWithDictionaryTest"; @@ -66,9 +65,6 @@ public class RawForwardIndexWithDictionaryTest extends CustomDataQueryClusterInt private static final String RAW_DICT_INT_DIMENSION = "rawDictIntDim"; private static final String RAW_DICT_INV_INT_DIMENSION = "rawDictInvIntDim"; private static final String RAW_DICT_RANGE_INT_DIMENSION = "rawDictRangeIntDim"; - private static final String CODEC_INT_DIMENSION = "codecIntDim"; - private static final String CODEC_LONG_DIMENSION = "codecLongDim"; - private static final long CODEC_LONG_BASE = (long) Integer.MAX_VALUE + 1; private static final String METRIC_COLUMN = "metric"; private static final int ROW_COUNT = 1000; private static final int UNIQUE_DIMENSION_VALUES = 20; @@ -112,8 +108,6 @@ public Schema createSchema() { .addSingleValueDimension(RAW_DICT_INT_DIMENSION, FieldSpec.DataType.INT) .addSingleValueDimension(RAW_DICT_INV_INT_DIMENSION, FieldSpec.DataType.INT) .addSingleValueDimension(RAW_DICT_RANGE_INT_DIMENSION, FieldSpec.DataType.INT) - .addSingleValueDimension(CODEC_INT_DIMENSION, FieldSpec.DataType.INT) - .addSingleValueDimension(CODEC_LONG_DIMENSION, FieldSpec.DataType.LONG) .addMetric(METRIC_COLUMN, FieldSpec.DataType.LONG) .addDateTime(TIMESTAMP_FIELD_NAME, FieldSpec.DataType.LONG, "1:MILLISECONDS:EPOCH", "1:MILLISECONDS") .build(); @@ -130,8 +124,6 @@ public List createAvroFiles() .requiredInt(RAW_DICT_INT_DIMENSION) .requiredInt(RAW_DICT_INV_INT_DIMENSION) .requiredInt(RAW_DICT_RANGE_INT_DIMENSION) - .requiredInt(CODEC_INT_DIMENSION) - .requiredLong(CODEC_LONG_DIMENSION) .requiredLong(METRIC_COLUMN) .requiredLong(TIMESTAMP_FIELD_NAME) .endRecord(); @@ -153,8 +145,6 @@ public List createAvroFiles() record.put(RAW_DICT_INT_DIMENSION, intValue); record.put(RAW_DICT_INV_INT_DIMENSION, intValue); record.put(RAW_DICT_RANGE_INT_DIMENSION, intValue); - record.put(CODEC_INT_DIMENSION, intValue); - record.put(CODEC_LONG_DIMENSION, CODEC_LONG_BASE + i); record.put(METRIC_COLUMN, random.nextInt(10_000)); record.put(TIMESTAMP_FIELD_NAME, currentTimeMillis + i); fileWriter.append(record); @@ -188,34 +178,7 @@ protected List getFieldConfigs() { FieldConfig rawDictRangeInt = new FieldConfig(RAW_DICT_RANGE_INT_DIMENSION, FieldConfig.EncodingType.RAW, null, null, null, null, dictionaryIndex.deepCopy(), null, null); - return List.of(rawDictString, rawDictInvString, rawDictInt, rawDictInvInt, rawDictRangeInt, - codecFieldConfig(CODEC_INT_DIMENSION, "DELTA,T64,LZ4"), - codecFieldConfig(CODEC_LONG_DIMENSION, "ZSTD(3)")); - } - - private static FieldConfig codecFieldConfig(String column, String codecSpec) { - ObjectNode forward = JsonUtils.newObjectNode().put("codecSpec", codecSpec).put("targetDocsPerChunk", 128); - ObjectNode indexes = JsonUtils.newObjectNode(); - indexes.set("forward", forward); - return new FieldConfig.Builder(column).withEncodingType(FieldConfig.EncodingType.RAW).withIndexes(indexes).build(); - } - - /// Exercises persisted INT/LONG pipelines through ingestion and both query engines, across chunk boundaries. - @Test(dataProvider = "useBothQueryEngines") - public void testCodecPipelineQueries(boolean useMultiStageQueryEngine) - throws Exception { - setUseMultiStageQueryEngine(useMultiStageQueryEngine); - JsonNode response = postQuery("SELECT " + CODEC_INT_DIMENSION + ", " + CODEC_LONG_DIMENSION + " FROM " - + getTableName() + " WHERE " + CODEC_INT_DIMENSION + " = 7 ORDER BY " + CODEC_LONG_DIMENSION + " LIMIT 1000"); - assertTrue(response.path("exceptions").isEmpty(), response.toString()); - JsonNode rows = response.path("resultTable").path("rows"); - assertEquals(rows.size(), ROW_COUNT / UNIQUE_DIMENSION_VALUES); - for (int i = 0; i < rows.size(); i++) { - assertEquals(rows.get(i).get(0).asInt(), 7); - assertEquals(rows.get(i).get(1).asLong(), CODEC_LONG_BASE + 7 + (long) i * UNIQUE_DIMENSION_VALUES); - } - assertEquals(scalarLong("SELECT SUM(" + CODEC_INT_DIMENSION + ") FROM " + getTableName()), - (long) ROW_COUNT * (UNIQUE_DIMENSION_VALUES - 1) / 2); + return List.of(rawDictString, rawDictInvString, rawDictInt, rawDictInvInt, rawDictRangeInt); } @Override diff --git a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/invertedindex/TextIndexHandler.java b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/invertedindex/TextIndexHandler.java index d94571b667bd..4273dfbe4249 100644 --- a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/invertedindex/TextIndexHandler.java +++ b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/invertedindex/TextIndexHandler.java @@ -373,7 +373,7 @@ private void convertTextIndexToV3Format(SegmentDirectory.Writer segmentWriter, S return; } - // Write the combined file to V3 format (similar to ForwardIndexHandler.rewriteForwardIndex) + // Write the combined file to V3 format (similar to rewriteForwardIndexForCompressionChange) LoaderUtils.writeIndexToV3Format(segmentWriter, columnName, combinedTextIndexFile, StandardIndexes.text()); LOGGER.info("Successfully converted text index to V3 combined format for column: {}", columnName); diff --git a/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/forward/ForwardIndexCreatorFactoryTest.java b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/forward/ForwardIndexCreatorFactoryTest.java index 3679e188682a..85f3d963f627 100644 --- a/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/forward/ForwardIndexCreatorFactoryTest.java +++ b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/forward/ForwardIndexCreatorFactoryTest.java @@ -135,46 +135,14 @@ public void testCodecSpecRoundTripUsesV7Format(String codecSpec, DataType stored throws Exception { File indexDir = Files.createTempDirectory("ForwardIndexCreatorFactoryTest").toFile(); try { - ForwardIndexConfig config = new ForwardIndexConfig.Builder(FieldConfig.EncodingType.RAW) - .withCodecSpec(codecSpec) - .withTargetDocsPerChunk(2) - .build(); - TableConfig tableConfig = new TableConfigBuilder(TableType.OFFLINE).setTableName("testTable").build(); - // Legacy compression stats are opted in here on purpose: a codecSpec column never reports them. - tableConfig.getIndexingConfig().setCompressionStatsEnabled(true); long[] values = storedType == DataType.INT ? new long[]{11, 13, 21} : new long[]{Long.MIN_VALUE, (long) Integer.MAX_VALUE + 1, Long.MAX_VALUE}; - try (ForwardIndexCreator creator = ForwardIndexCreatorFactory.createIndexCreator( - newContext(indexDir, false, tableConfig, values.length, storedType), config)) { - assertFalse(creator.isDictionaryEncoded()); - assertNull(creator.getRawForwardIndexChunkCompressionType()); - for (long value : values) { - if (storedType == DataType.INT) { - creator.putInt((int) value); - } else { - creator.putLong(value); - } - } - creator.seal(); - assertEquals(creator.getRawForwardIndexUncompressedValueSizeInBytes(), -1L); - } - File indexFile = new File(indexDir, COLUMN_NAME + V1Constants.Indexes.RAW_SV_FORWARD_INDEX_FILE_EXTENSION); - assertTrue(indexFile.exists()); - try (PinotDataBuffer buffer = PinotDataBuffer.mapReadOnlyBigEndianFile(indexFile); - ForwardIndexReader reader = - ForwardIndexReaderFactory.getInstance().createRawIndexReader(buffer, storedType, true)) { - assertTrue(reader instanceof FixedByteChunkSVForwardIndexReaderV7, - "codecSpec was routed to " + reader.getClass().getSimpleName()); + // Legacy compression stats are opted in here on purpose: a codecSpec column never reports them. + File indexFile = roundTripCodecSpecIndex(indexDir, codecSpec, storedType, 2, values, true, 0); + try (PinotDataBuffer buffer = PinotDataBuffer.mapReadOnlyBigEndianFile(indexFile)) { assertEquals(buffer.getInt(0), FixedByteChunkSVForwardIndexReaderV7.VERSION); assertEquals(FixedByteChunkSVForwardIndexReaderV7.readCodecSpec(buffer), codecSpec); - FixedByteChunkSVForwardIndexReaderV7 v7Reader = (FixedByteChunkSVForwardIndexReaderV7) reader; - try (FixedByteChunkSVForwardIndexReaderV7.Context context = v7Reader.createContext()) { - for (int i = 0; i < values.length; i++) { - assertEquals(storedType == DataType.INT ? v7Reader.getInt(i, context) : v7Reader.getLong(i, context), - values[i]); - } - } } } finally { FileUtils.deleteQuietly(indexDir); @@ -205,47 +173,68 @@ public void testCodecSpecMultiChunkRoundTrip(String codecSpec, DataType storedTy for (int i = 0; i < numDocs; i++) { values[i] = storedType == DataType.INT ? random.nextInt() : random.nextLong(); } - ForwardIndexConfig config = new ForwardIndexConfig.Builder(FieldConfig.EncodingType.RAW) - .withCodecSpec(codecSpec) - .withTargetDocsPerChunk(1000) - .build(); - TableConfig tableConfig = new TableConfigBuilder(TableType.OFFLINE).setTableName("testTable").build(); - try (ForwardIndexCreator creator = ForwardIndexCreatorFactory.createIndexCreator( - newContext(indexDir, false, tableConfig, numDocs, storedType), config)) { - for (long value : values) { - if (storedType == DataType.INT) { - creator.putInt((int) value); - } else { - creator.putLong(value); - } - } - creator.seal(); - } - File indexFile = new File(indexDir, COLUMN_NAME + V1Constants.Indexes.RAW_SV_FORWARD_INDEX_FILE_EXTENSION); - try (PinotDataBuffer buffer = PinotDataBuffer.mapReadOnlyBigEndianFile(indexFile); - ForwardIndexReader reader = - ForwardIndexReaderFactory.getInstance().createRawIndexReader(buffer, storedType, true)) { - FixedByteChunkSVForwardIndexReaderV7 v7Reader = (FixedByteChunkSVForwardIndexReaderV7) reader; + File indexFile = roundTripCodecSpecIndex(indexDir, codecSpec, storedType, 1000, values, false, 2000); + try (PinotDataBuffer buffer = PinotDataBuffer.mapReadOnlyBigEndianFile(indexFile)) { // Header ints: version, magic, numChunks, numDocsPerChunk, sizeOfEntry, totalDocs assertEquals(buffer.getInt(3 * Integer.BYTES), 1024, "targetDocsPerChunk=1000 should normalize to 1024"); assertEquals(buffer.getInt(2 * Integer.BYTES), (numDocs + 1023) / 1024); assertEquals(buffer.getInt(5 * Integer.BYTES), numDocs); - try (FixedByteChunkSVForwardIndexReaderV7.Context sequential = v7Reader.createContext(); - FixedByteChunkSVForwardIndexReaderV7.Context randomAccess = v7Reader.createContext()) { - for (int docId = 0; docId < numDocs; docId++) { - assertEquals(readValue(v7Reader, storedType, docId, sequential), values[docId], "docId " + docId); - } - for (int i = 0; i < 2000; i++) { - int docId = random.nextInt(numDocs); - assertEquals(readValue(v7Reader, storedType, docId, randomAccess), values[docId], "docId " + docId); - } - } } } finally { FileUtils.deleteQuietly(indexDir); } } + /// Writes `values` through a `codecSpec` forward-index creator and reads every one of them back, then + /// returns the index file so the caller can assert on its header. Covers the parts both codecSpec round + /// trips share: the creator contract that holds for any codecSpec column (not dictionary encoded, no + /// legacy chunk compression type, and no legacy uncompressed value size even when the table opts into + /// compression stats), that the pipeline routed the file to the V7 reader, and a sequential read of every + /// value. `randomReads` further reads land on random doc ids through a second, random-access context. + private static File roundTripCodecSpecIndex(File indexDir, String codecSpec, DataType storedType, + int targetDocsPerChunk, long[] values, boolean compressionStatsEnabled, int randomReads) + throws Exception { + ForwardIndexConfig config = new ForwardIndexConfig.Builder(FieldConfig.EncodingType.RAW).withCodecSpec(codecSpec) + .withTargetDocsPerChunk(targetDocsPerChunk).build(); + TableConfig tableConfig = new TableConfigBuilder(TableType.OFFLINE).setTableName("testTable").build(); + tableConfig.getIndexingConfig().setCompressionStatsEnabled(compressionStatsEnabled); + try (ForwardIndexCreator creator = ForwardIndexCreatorFactory.createIndexCreator( + newContext(indexDir, false, tableConfig, values.length, storedType), config)) { + assertFalse(creator.isDictionaryEncoded()); + assertNull(creator.getRawForwardIndexChunkCompressionType()); + for (long value : values) { + if (storedType == DataType.INT) { + creator.putInt((int) value); + } else { + creator.putLong(value); + } + } + creator.seal(); + assertEquals(creator.getRawForwardIndexUncompressedValueSizeInBytes(), -1L); + } + File indexFile = new File(indexDir, COLUMN_NAME + V1Constants.Indexes.RAW_SV_FORWARD_INDEX_FILE_EXTENSION); + assertTrue(indexFile.exists()); + try (PinotDataBuffer buffer = PinotDataBuffer.mapReadOnlyBigEndianFile(indexFile); + ForwardIndexReader reader = + ForwardIndexReaderFactory.getInstance().createRawIndexReader(buffer, storedType, true)) { + assertTrue(reader instanceof FixedByteChunkSVForwardIndexReaderV7, + "codecSpec was routed to " + reader.getClass().getSimpleName()); + FixedByteChunkSVForwardIndexReaderV7 v7Reader = (FixedByteChunkSVForwardIndexReaderV7) reader; + try (FixedByteChunkSVForwardIndexReaderV7.Context sequential = v7Reader.createContext(); + FixedByteChunkSVForwardIndexReaderV7.Context randomAccess = v7Reader.createContext()) { + for (int docId = 0; docId < values.length; docId++) { + assertEquals(readValue(v7Reader, storedType, docId, sequential), values[docId], "docId " + docId); + } + Random random = new Random(7); + for (int i = 0; i < randomReads; i++) { + int docId = random.nextInt(values.length); + assertEquals(readValue(v7Reader, storedType, docId, randomAccess), values[docId], "docId " + docId); + } + } + } + return indexFile; + } + private static long readValue(FixedByteChunkSVForwardIndexReaderV7 reader, DataType storedType, int docId, FixedByteChunkSVForwardIndexReaderV7.Context context) { return storedType == DataType.INT ? reader.getInt(docId, context) : reader.getLong(docId, context);