From a43da48fc71b3df0ccbaeea20d2df92c206bbab9 Mon Sep 17 00:00:00 2001 From: Xiang Fu Date: Sat, 5 Sep 2026 20:09:48 -0700 Subject: [PATCH 1/2] DATA-3221 (8+9): shared column shape and primitive min/max in ColumnMetadataImpl A server retains one ColumnMetadataImpl per (segment, column) for the segment lifetime, so a wide external table of 1000 Parquet-backed nullable INT columns pays for it 1000 times. Two changes take the retained graph from ~104 to ~64 bytes per column, measured by allocating 500k instances with a shared FieldSpec and diffing the used heap after a forced GC: - Shared column shape. totalDocs, totalNumberOfEntries, maxNumberOfMultiValues and bitsPerElement are not column-distinguishing: build() pins the two MV ints to totalDocs and 0 for every SV column, and bitsPerElement is UNAVAILABLE for every raw column. They move into an immutable SharedShape behind one ref, interned through a weak interner keyed by its own equals/hashCode exactly like the existing FIELD_SPEC_INTERNER, so the SV columns of a segment share at most ~34 instances however wide it is (one for a raw external table) and segments with the same row count share across the JVM. - Primitive min/max. The min/max of a fixed-width stored type is stored as raw bits and boxed only inside getMinValue()/getMaxValue(), which drops ~30 bytes and two surviving objects per numeric column. FLOAT/DOUBLE go through floatToIntBits/doubleToLongBits so NaN and -0.0 keep comparing exactly as Float.equals does. STRING, BYTES, BIG_DECIMAL and COMPLEX min/max stay object refs, as does a value whose class is not the box class of its stored type, so no Builder caller can lose a value. The two longs the values need would have grown the object, so they are a union: a fixed-width column derives its three element lengths from storedType.size() and its multi-value count, and a var-width column packs those lengths into the same two words instead of its (object) min/max. Net layout, verified by measurement: 72 -> 64 bytes, and no column type regresses. No on-disk format, public signature or REST payload changes: every getter keeps its signature and value, the segment-metadata JSON is bean-serialized from the same getter set, and _flags widening from byte to short is private. Co-Authored-By: Claude Opus 5 --- .../datasource/ImmutableDataSourceTest.java | 6 +- .../index/metadata/ColumnMetadataImpl.java | 280 +++++++++++++----- .../metadata/ColumnMetadataImplTest.java | 248 ++++++++++++++++ 3 files changed, 465 insertions(+), 69 deletions(-) diff --git a/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/datasource/ImmutableDataSourceTest.java b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/datasource/ImmutableDataSourceTest.java index 0c58e20b9a0c..1339be2f3282 100644 --- a/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/datasource/ImmutableDataSourceTest.java +++ b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/datasource/ImmutableDataSourceTest.java @@ -75,9 +75,11 @@ public void testSingleValueColumn() { assertEquals(metadata.getNumValues(), NUM_DOCS); assertEquals(metadata.getNumValues(), columnMetadata.getTotalNumberOfEntries()); assertEquals(metadata.getCardinality(), 37); - assertSame(metadata.getMinValue(), columnMetadata.getMinValue()); + // The min/max of a fixed-width column is held as raw bits and boxed on read, so the data source view delegates + // an equal value rather than the same instance. + assertEquals(metadata.getMinValue(), columnMetadata.getMinValue()); assertEquals(metadata.getMinValue(), -5); - assertSame(metadata.getMaxValue(), columnMetadata.getMaxValue()); + assertEquals(metadata.getMaxValue(), columnMetadata.getMaxValue()); assertEquals(metadata.getMaxValue(), 123456); assertSame(metadata.getPartitionFunction(), partitionFunction); assertSame(metadata.getPartitions(), partitions); diff --git a/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/metadata/ColumnMetadataImpl.java b/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/metadata/ColumnMetadataImpl.java index 6fc61b5233e6..11c28cab7fd0 100644 --- a/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/metadata/ColumnMetadataImpl.java +++ b/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/metadata/ColumnMetadataImpl.java @@ -72,24 +72,43 @@ /// as immutable: a setter call on one would bleed into every other segment and table that shares it, and would /// corrupt the interner's hash bucket (nothing ever mutated one; copy via a JSON round-trip before mutating). /// -/// The object layout is kept at 72 bytes for an ordinary column for the same reason: the six booleans and the -/// forward-index encoding are packed into one [#_flags] byte, and the refs only a partitioned column or an OPEN_STRUCT -/// parent/child carries (partition function and partitions, parent column, sparse keys) live in a lazily allocated -/// [Extras] holder that stays `null` for every other column. The compression stats stay a direct ref because the -/// segment creator writes them for every raw column. None of this is visible through the public getters, so the +/// The object layout is kept at 64 bytes for an ordinary column for the same reason: the six booleans, the +/// forward-index encoding and the two min/max representation bits are packed into one [#_flags] short; the refs only +/// a partitioned column or an OPEN_STRUCT parent/child carries (partition function and partitions, parent column, +/// sparse keys) live in a lazily allocated [Extras] holder that stays `null` for every other column; the four ints +/// that are not column-distinguishing live in a shared [SharedShape]; and the three element-length ints share their +/// two words with the numeric min/max (see [#_minWord]). The compression stats stay a direct ref because the segment +/// creator writes them for every raw column. None of this is visible through the public getters, so the /// `/tables/{table}/segments/{segment}/metadata` payload (bean-serialized from the getters) is unchanged. +/// +/// | bytes | field(s) | +/// |------:|----------| +/// | 12 | object header | +/// | 4 | `_cardinality` | +/// | 16 | `_minWord`, `_maxWord` | +/// | 2+2 | `_flags` plus alignment padding | +/// | 28 | `_fieldSpec`, `_shape`, `_minValue`, `_maxValue`, `_extras`, `_compressionMetadata`, `_indexTypeSizes` | +/// | 64 | total (was 72: eight ints, six refs and a flags byte) | +/// +/// On top of the eight bytes this saves directly, a fixed-width column no longer retains a box per min/max value +/// (~30 bytes and two objects per column for a nullable INT column), and the [SharedShape] is amortized over every +/// column of the segment that has the same shape. @SuppressWarnings({"rawtypes", "unchecked"}) public class ColumnMetadataImpl implements ColumnMetadata { private static final long SIZE_MASK = 0xffffffffffffL; // Bits of _flags - private static final byte HAS_DICTIONARY = 1; - private static final byte DICTIONARY_ENCODED_FORWARD_INDEX = 1 << 1; - private static final byte SORTED = 1 << 2; - private static final byte NON_NULL = 1 << 3; - private static final byte MIN_MAX_VALUE_INVALID = 1 << 4; - private static final byte ASCII = 1 << 5; - private static final byte AUTO_GENERATED = 1 << 6; + private static final short HAS_DICTIONARY = 1; + private static final short DICTIONARY_ENCODED_FORWARD_INDEX = 1 << 1; + private static final short SORTED = 1 << 2; + private static final short NON_NULL = 1 << 3; + private static final short MIN_MAX_VALUE_INVALID = 1 << 4; + private static final short ASCII = 1 << 5; + private static final short AUTO_GENERATED = 1 << 6; + /// Set when the min (max) value is held as raw bits in [#_minWord] ([#_maxWord]) rather than as an object in + /// [#_minValue] ([#_maxValue]); an absent value sets neither. + private static final short MIN_VALUE_IN_WORD = 1 << 7; + private static final short MAX_VALUE_IN_WORD = 1 << 8; /// Canonical instances of the [FieldSpec]s parsed from `metadata.properties`, keyed by [FieldSpec#equals] / /// [FieldSpec#hashCode] (name, data type, single-value, default null value, max length, date-time format and @@ -98,22 +117,35 @@ public class ColumnMetadataImpl implements ColumnMetadata { /// any of them and is released once the last one is unloaded. Thread-safe. private static final Interner FIELD_SPEC_INTERNER = Interners.newWeakInterner(); + /// Canonical instances of the [SharedShape]s, held weakly exactly like [#FIELD_SPEC_INTERNER]: the canonical + /// instance is one of the instances the loaded segments retain, so it lives as long as any of them. + private static final Interner SHAPE_INTERNER = Interners.newWeakInterner(); + private final FieldSpec _fieldSpec; - private final int _totalDocs; + /// The ints that are not column-distinguishing, shared with every other column that has the same shape. + private final SharedShape _shape; private final int _cardinality; + /// Two words with a use that depends on whether the stored type is fixed width, which is exactly the condition + /// under which the other use is dead: + /// - fixed-width stored type (INT, LONG, FLOAT, DOUBLE, and BOOLEAN/TIMESTAMP through their stored type): the raw + /// bits of the min and max value, boxed on demand by [#getMinValue()] / [#getMaxValue()], with presence carried + /// by [#MIN_VALUE_IN_WORD] / [#MAX_VALUE_IN_WORD]. The element lengths are dead here because [Builder#build()] + /// pins them to `storedType.size()`. + /// - otherwise: `_minWord` packs `lengthOfShortestElement` (high half) and `lengthOfLongestElement` (low half), + /// `_maxWord` holds `maxRowLengthInBytes`. The value words are dead here because a STRING, BYTES, BIG_DECIMAL or + /// COMPLEX min/max is an object, kept in [#_minValue] / [#_maxValue]. + /// + /// A fixed-width column whose builder was handed a min/max that is not the box class of its stored type falls back + /// to [#_minValue] / [#_maxValue] as well, so no caller can lose a value by handing over an unexpected type. + private final long _minWord; + private final long _maxWord; @Nullable private final Comparable _minValue; @Nullable private final Comparable _maxValue; - private final int _lengthOfShortestElement; - private final int _lengthOfLongestElement; - private final int _totalNumberOfEntries; - private final int _maxNumberOfMultiValues; - private final int _maxRowLengthInBytes; - private final int _bitsPerElement; - /// hasDictionary, forward-index encoding, sorted, nonNull, minMaxValueInvalid, ascii and autoGenerated, see the - /// bit constants above. - private final byte _flags; + /// hasDictionary, forward-index encoding, sorted, nonNull, minMaxValueInvalid, ascii, autoGenerated and the two + /// min/max representation bits, see the bit constants above. + private final short _flags; @Nullable private final Extras _extras; @Nullable @@ -132,30 +164,29 @@ public class ColumnMetadataImpl implements ColumnMetadata { @Nullable private long[] _indexTypeSizes; - private ColumnMetadataImpl(FieldSpec fieldSpec, int totalDocs, int cardinality, @Nullable Comparable minValue, - @Nullable Comparable maxValue, int lengthOfShortestElement, int lengthOfLongestElement, - int totalNumberOfEntries, int maxNumberOfMultiValues, int maxRowLengthInBytes, int bitsPerElement, byte flags, - @Nullable Extras extras, @Nullable CompressionMetadata compressionMetadata) { + private ColumnMetadataImpl(FieldSpec fieldSpec, SharedShape shape, int cardinality, long minWord, long maxWord, + @Nullable Comparable minValue, @Nullable Comparable maxValue, short flags, @Nullable Extras extras, + @Nullable CompressionMetadata compressionMetadata) { _fieldSpec = fieldSpec; - _totalDocs = totalDocs; + _shape = shape; _cardinality = cardinality; + _minWord = minWord; + _maxWord = maxWord; _minValue = minValue; _maxValue = maxValue; - _lengthOfShortestElement = lengthOfShortestElement; - _lengthOfLongestElement = lengthOfLongestElement; - _totalNumberOfEntries = totalNumberOfEntries; - _maxNumberOfMultiValues = maxNumberOfMultiValues; - _maxRowLengthInBytes = maxRowLengthInBytes; - _bitsPerElement = bitsPerElement; _flags = flags; _extras = extras; _compressionMetadata = compressionMetadata; } - private boolean hasFlag(byte flag) { + private boolean hasFlag(short flag) { return (_flags & flag) != 0; } + private boolean isFixedWidth() { + return _fieldSpec.getDataType().getStoredType().isFixedWidth(); + } + @Override public FieldSpec getFieldSpec() { return _fieldSpec; @@ -163,7 +194,7 @@ public FieldSpec getFieldSpec() { @Override public int getTotalDocs() { - return _totalDocs; + return _shape._totalDocs; } @Override @@ -191,16 +222,37 @@ public boolean isNonNull() { return hasFlag(NON_NULL); } + /// Returns the value equal to the one the builder was handed. A fixed-width min/max is boxed on every call rather + /// than retained: a server keeps one instance of this class per (segment, column) for the segment lifetime, while + /// the callers (segment pruners, aggregation rewrites, range-index construction) read it a handful of times per + /// query and let the box die in the young generation. @Nullable @Override public Comparable getMinValue() { - return _minValue; + return hasFlag(MIN_VALUE_IN_WORD) ? boxValueWord(_minWord) : _minValue; } @Nullable @Override public Comparable getMaxValue() { - return _maxValue; + return hasFlag(MAX_VALUE_IN_WORD) ? boxValueWord(_maxWord) : _maxValue; + } + + /// Boxes a value word written by [Builder#toValueWord]; only reached for a fixed-width stored type. + private Comparable boxValueWord(long word) { + DataType storedType = _fieldSpec.getDataType().getStoredType(); + switch (storedType) { + case INT: + return (int) word; + case LONG: + return word; + case FLOAT: + return Float.intBitsToFloat((int) word); + case DOUBLE: + return Double.longBitsToDouble(word); + default: + throw new IllegalStateException("Unsupported stored type for a packed min/max value: " + storedType); + } } @Override @@ -210,12 +262,12 @@ public boolean isMinMaxValueInvalid() { @Override public int getLengthOfShortestElement() { - return _lengthOfShortestElement; + return isFixedWidth() ? _fieldSpec.getDataType().getStoredType().size() : (int) (_minWord >> 32); } @Override public int getLengthOfLongestElement() { - return _lengthOfLongestElement; + return isFixedWidth() ? _fieldSpec.getDataType().getStoredType().size() : (int) _minWord; } @Override @@ -225,22 +277,29 @@ public boolean isAscii() { @Override public int getBitsPerElement() { - return _bitsPerElement; + return _shape._bitsPerElement; } @Override public int getTotalNumberOfEntries() { - return _totalNumberOfEntries; + return _shape._totalNumberOfEntries; } @Override public int getMaxNumberOfMultiValues() { - return _maxNumberOfMultiValues; + return _shape._maxNumberOfMultiValues; } + /// [Builder#build()] pins this to `lengthOfLongestElement` for an SV column and to + /// `maxNumberOfMultiValues * storedType.size()` for a fixed-width MV column, so only a var-width MV column needs + /// the value stored (in [#_maxWord]). @Override public int getMaxRowLengthInBytes() { - return _maxRowLengthInBytes; + if (isFixedWidth()) { + int size = _fieldSpec.getDataType().getStoredType().size(); + return _fieldSpec.isSingleValueField() ? size : _shape._maxNumberOfMultiValues * size; + } + return (int) _maxWord; } @Nullable @@ -353,16 +412,14 @@ public boolean equals(Object o) { if (o == null || getClass() != o.getClass()) { return false; } + // The representation two equal columns pick is a function of their field spec and their values, so comparing the + // raw words and flags is equivalent to comparing the boxed min/max and the unpacked lengths. ColumnMetadataImpl that = (ColumnMetadataImpl) o; - return _totalDocs == that._totalDocs - && _cardinality == that._cardinality + return _cardinality == that._cardinality && _flags == that._flags - && _lengthOfShortestElement == that._lengthOfShortestElement - && _lengthOfLongestElement == that._lengthOfLongestElement - && _totalNumberOfEntries == that._totalNumberOfEntries - && _maxNumberOfMultiValues == that._maxNumberOfMultiValues - && _maxRowLengthInBytes == that._maxRowLengthInBytes - && _bitsPerElement == that._bitsPerElement + && _minWord == that._minWord + && _maxWord == that._maxWord + && Objects.equals(_shape, that._shape) && Objects.equals(_fieldSpec, that._fieldSpec) && Objects.equals(_minValue, that._minValue) && Objects.equals(_maxValue, that._maxValue) @@ -373,9 +430,8 @@ public boolean equals(Object o) { @Override public int hashCode() { - return Objects.hash(_fieldSpec, _totalDocs, _cardinality, _flags, _minValue, _maxValue, _lengthOfShortestElement, - _lengthOfLongestElement, _totalNumberOfEntries, _maxNumberOfMultiValues, _maxRowLengthInBytes, _bitsPerElement, - _extras, _compressionMetadata, Arrays.hashCode(_indexTypeSizes)); + return Objects.hash(_fieldSpec, _shape, _cardinality, _flags, _minWord, _maxWord, _minValue, _maxValue, _extras, + _compressionMetadata, Arrays.hashCode(_indexTypeSizes)); } // Keeps the pre-packing field names and order, which tests and log consumers match on @@ -383,21 +439,21 @@ public int hashCode() { public String toString() { return "ColumnMetadataImpl{" + "_fieldSpec=" + _fieldSpec - + ", _totalDocs=" + _totalDocs + + ", _totalDocs=" + getTotalDocs() + ", _cardinality=" + _cardinality + ", _hasDictionary=" + hasDictionary() + ", _forwardIndexEncoding=" + getForwardIndexEncoding() + ", _sorted=" + isSorted() + ", _nonNull=" + isNonNull() - + ", _minValue=" + _minValue - + ", _maxValue=" + _maxValue + + ", _minValue=" + getMinValue() + + ", _maxValue=" + getMaxValue() + ", _minMaxValueInvalid=" + isMinMaxValueInvalid() - + ", _lengthOfShortestElement=" + _lengthOfShortestElement - + ", _lengthOfLongestElement=" + _lengthOfLongestElement + + ", _lengthOfShortestElement=" + getLengthOfShortestElement() + + ", _lengthOfLongestElement=" + getLengthOfLongestElement() + ", _isAscii=" + isAscii() - + ", _totalNumberOfEntries=" + _totalNumberOfEntries - + ", _maxNumberOfMultiValues=" + _maxNumberOfMultiValues - + ", _maxRowLengthInBytes=" + _maxRowLengthInBytes - + ", _bitsPerElement=" + _bitsPerElement + + ", _totalNumberOfEntries=" + getTotalNumberOfEntries() + + ", _maxNumberOfMultiValues=" + getMaxNumberOfMultiValues() + + ", _maxRowLengthInBytes=" + getMaxRowLengthInBytes() + + ", _bitsPerElement=" + getBitsPerElement() + ", _partitionFunction=" + getPartitionFunction() + ", _partitions=" + getPartitions() + ", _autoGenerated=" + isAutoGenerated() @@ -667,6 +723,51 @@ public static Builder builder() { return new Builder(); } + /// The ints of a column that are not column-distinguishing, interned through [#SHAPE_INTERNER] so the columns of a + /// segment that have the same shape hold one instance instead of four ints each. + /// + /// `totalDocs` is identical for every column of a segment; `totalNumberOfEntries` and `maxNumberOfMultiValues` are + /// pinned by [Builder#build()] to `totalDocs` and `0` for every SV column; and `bitsPerElement` is `UNAVAILABLE` + /// for every raw column and takes one of at most 33 values (`1..32`) for a dictionary-encoded one. So the SV + /// columns of a segment share at most ~34 instances however wide the segment is, and a wide external table of raw + /// columns shares exactly one. The remaining four ints are not held here: `cardinality` is distinguishing, and the + /// three element lengths are derived or packed into [#_minWord] / [#_maxWord]. + /// + /// Immutable and thread-safe. + private static final class SharedShape { + private final int _totalDocs; + private final int _totalNumberOfEntries; + private final int _maxNumberOfMultiValues; + private final int _bitsPerElement; + + private SharedShape(int totalDocs, int totalNumberOfEntries, int maxNumberOfMultiValues, int bitsPerElement) { + _totalDocs = totalDocs; + _totalNumberOfEntries = totalNumberOfEntries; + _maxNumberOfMultiValues = maxNumberOfMultiValues; + _bitsPerElement = bitsPerElement; + } + + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + SharedShape that = (SharedShape) o; + return _totalDocs == that._totalDocs + && _totalNumberOfEntries == that._totalNumberOfEntries + && _maxNumberOfMultiValues == that._maxNumberOfMultiValues + && _bitsPerElement == that._bitsPerElement; + } + + @Override + public int hashCode() { + return Objects.hash(_totalDocs, _totalNumberOfEntries, _maxNumberOfMultiValues, _bitsPerElement); + } + } + /// The refs that only a partitioned column (partition function and partitions) or an OPEN_STRUCT parent/child /// (sparse keys / parent column) carries. Ordinary columns hold no holder at all, so they never pay for the four /// slots; a column that has any of them pays one extra object. @@ -968,7 +1069,7 @@ public ColumnMetadataImpl build() { _bitsPerElement = UNAVAILABLE; } - byte flags = 0; + short flags = 0; if (_hasDictionary) { flags |= HAS_DICTIONARY; } @@ -990,12 +1091,57 @@ public ColumnMetadataImpl build() { if (_autoGenerated) { flags |= AUTO_GENERATED; } - return new ColumnMetadataImpl(_fieldSpec, _totalDocs, _cardinality, _minValue, _maxValue, - _lengthOfShortestElement, _lengthOfLongestElement, _totalNumberOfEntries, _maxNumberOfMultiValues, - _maxRowLengthInBytes, _bitsPerElement, flags, + + // Fill the two words with whichever of the two uses this column has (see ColumnMetadataImpl#_minWord). + long minWord = 0; + long maxWord = 0; + Comparable minValue = _minValue; + Comparable maxValue = _maxValue; + if (storedType.isFixedWidth()) { + Long minBits = toValueWord(storedType, minValue); + if (minBits != null) { + minWord = minBits; + minValue = null; + flags |= MIN_VALUE_IN_WORD; + } + Long maxBits = toValueWord(storedType, maxValue); + if (maxBits != null) { + maxWord = maxBits; + maxValue = null; + flags |= MAX_VALUE_IN_WORD; + } + } else { + minWord = ((long) _lengthOfShortestElement << 32) | (_lengthOfLongestElement & 0xffffffffL); + maxWord = _maxRowLengthInBytes & 0xffffffffL; + } + + SharedShape shape = SHAPE_INTERNER.intern( + new SharedShape(_totalDocs, _totalNumberOfEntries, _maxNumberOfMultiValues, _bitsPerElement)); + return new ColumnMetadataImpl(_fieldSpec, shape, _cardinality, minWord, maxWord, minValue, maxValue, flags, Extras.create(_partitionFunction, _partitions, _parentColumn, _sparseKeys), CompressionMetadata.create(_uncompressedValueSizeInBytes, _forwardIndexChunkCompressionType, _dictionaryUncompressedValueSizeInBytes)); } + + /// Returns the raw bits of a min/max value of a fixed-width stored type, or `null` when there is no value or the + /// value is not the box class of the stored type (in which case it stays an object ref, so an unexpected type + /// from a [Builder] caller is preserved rather than dropped or mistranslated). FLOAT and DOUBLE go through + /// [Float#floatToIntBits] / [Double#doubleToLongBits] rather than the raw variants, so a NaN keeps comparing + /// equal to a NaN exactly as [Float#equals] does today. + @Nullable + private static Long toValueWord(DataType storedType, @Nullable Comparable value) { + switch (storedType) { + case INT: + return value instanceof Integer ? (long) (Integer) value : null; + case LONG: + return value instanceof Long ? (Long) value : null; + case FLOAT: + return value instanceof Float ? (long) Float.floatToIntBits((Float) value) : null; + case DOUBLE: + return value instanceof Double ? Double.doubleToLongBits((Double) value) : null; + default: + return null; + } + } } } diff --git a/pinot-segment-spi/src/test/java/org/apache/pinot/segment/spi/index/metadata/ColumnMetadataImplTest.java b/pinot-segment-spi/src/test/java/org/apache/pinot/segment/spi/index/metadata/ColumnMetadataImplTest.java index 437918a67880..c776b597bb1c 100644 --- a/pinot-segment-spi/src/test/java/org/apache/pinot/segment/spi/index/metadata/ColumnMetadataImplTest.java +++ b/pinot-segment-spi/src/test/java/org/apache/pinot/segment/spi/index/metadata/ColumnMetadataImplTest.java @@ -20,7 +20,9 @@ import com.fasterxml.jackson.databind.JsonNode; import java.lang.ref.WeakReference; +import java.lang.reflect.Field; import java.math.BigDecimal; +import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.Set; @@ -54,6 +56,7 @@ import static org.testng.Assert.assertEquals; import static org.testng.Assert.assertFalse; import static org.testng.Assert.assertNotEquals; +import static org.testng.Assert.assertNotNull; import static org.testng.Assert.assertNotSame; import static org.testng.Assert.assertNull; import static org.testng.Assert.assertSame; @@ -348,6 +351,251 @@ public void flagsRoundTripIndependently() { none.toString()); } + /// The min/max value of a fixed-width stored type is held as raw bits and boxed on read, so every stored type must + /// come back as the very value [ColumnMetadataImpl#fromPropertiesConfiguration] parsed: same class, same value. + @Test + public void minMaxValuesRoundTripForEveryDataType() { + Map> expected = new LinkedHashMap<>(); + expected.put(DataType.INT, List.of("-5", "7", -5, 7)); + expected.put(DataType.LONG, List.of("-5000000000", "7000000000", -5000000000L, 7000000000L)); + expected.put(DataType.FLOAT, List.of("-1.5", "2.5", -1.5f, 2.5f)); + expected.put(DataType.DOUBLE, List.of("-1.5", "2.5", -1.5d, 2.5d)); + // BOOLEAN is stored as INT and TIMESTAMP as LONG, so their min/max are the stored type's box. + expected.put(DataType.BOOLEAN, List.of("0", "1", 0, 1)); + expected.put(DataType.TIMESTAMP, List.of("1000", "2000", 1000L, 2000L)); + expected.put(DataType.STRING, List.of("aa", "zz", "aa", "zz")); + expected.put(DataType.JSON, List.of("{}", "{}", "{}", "{}")); + expected.put(DataType.BIG_DECIMAL, List.of("-1.50", "2.5", new BigDecimal("-1.50"), new BigDecimal("2.5"))); + + expected.forEach((dataType, values) -> { + ColumnMetadataImpl metadata = withMinMax(dataType, (String) values.get(0), (String) values.get(1)); + assertEquals(metadata.getMinValue(), values.get(2), dataType.name()); + assertEquals(metadata.getMaxValue(), values.get(3), dataType.name()); + assertEquals(metadata.getMinValue().getClass(), values.get(2).getClass(), dataType.name()); + assertEquals(metadata.getMaxValue().getClass(), values.get(3).getClass(), dataType.name()); + assertFalse(metadata.isMinMaxValueInvalid(), dataType.name()); + // The REST payload is serialized from the getters, so it carries exactly the node the value serializes to. + JsonNode json = JsonUtils.objectToJsonNode(metadata); + assertEquals(json.get("minValue"), JsonUtils.objectToJsonNode(values.get(2)), dataType.name()); + assertEquals(json.get("maxValue"), JsonUtils.objectToJsonNode(values.get(3)), dataType.name()); + }); + + // BYTES parses to a ByteArray, which is var-width and therefore always kept as an object. + ColumnMetadataImpl bytes = withMinMax(DataType.BYTES, "0a0b", "ff"); + assertEquals(bytes.getMinValue(), BytesUtils.toByteArray("0a0b")); + assertEquals(bytes.getMaxValue(), BytesUtils.toByteArray("ff")); + } + + /// A column with no min/max keeps reporting `null` for both, and the min-max-invalid flag is independent of them. + @Test + public void minMaxValuesAbsentOrInvalid() { + for (DataType dataType : List.of(DataType.INT, DataType.LONG, DataType.FLOAT, DataType.DOUBLE, DataType.BOOLEAN, + DataType.TIMESTAMP, DataType.STRING, DataType.BYTES, DataType.BIG_DECIMAL)) { + ColumnMetadataImpl absent = withMinMax(dataType, null, null); + assertNull(absent.getMinValue(), dataType.name()); + assertNull(absent.getMaxValue(), dataType.name()); + assertFalse(absent.isMinMaxValueInvalid(), dataType.name()); + + PropertiesConfiguration invalidConfig = minMaxConfig(dataType, null, null); + invalidConfig.setProperty(Column.getKeyFor("col", Column.MIN_MAX_VALUE_INVALID), true); + ColumnMetadataImpl invalid = ColumnMetadataImpl.fromPropertiesConfiguration(invalidConfig, 10, "col"); + assertNull(invalid.getMinValue(), dataType.name()); + assertNull(invalid.getMaxValue(), dataType.name()); + assertTrue(invalid.isMinMaxValueInvalid(), dataType.name()); + assertNotEquals(invalid, absent, dataType.name()); + + // Only one of the two present: the other stays null rather than reading back the packed zero. + String value = dataType == DataType.BYTES ? "0a" : "1"; + ColumnMetadataImpl minOnly = withMinMax(dataType, value, null); + assertNotNull(minOnly.getMinValue(), dataType.name()); + assertNull(minOnly.getMaxValue(), dataType.name()); + ColumnMetadataImpl maxOnly = withMinMax(dataType, null, value); + assertNull(maxOnly.getMinValue(), dataType.name()); + assertNotNull(maxOnly.getMaxValue(), dataType.name()); + assertNotEquals(maxOnly, minOnly, dataType.name()); + } + + // A COMPLEX column has no min/max at all and is flagged invalid. + PropertiesConfiguration complexConfig = complexConfig("metrics", "cpu"); + complexConfig.setProperty(Column.getKeyFor("metrics", Column.CARDINALITY), 1); + ColumnMetadataImpl complex = ColumnMetadataImpl.fromPropertiesConfiguration(complexConfig, 10, "metrics"); + assertNull(complex.getMinValue()); + assertNull(complex.getMaxValue()); + assertTrue(complex.isMinMaxValueInvalid()); + } + + /// A packed min/max takes part in equality, hashCode and toString exactly as the boxed value did, including the + /// FLOAT/DOUBLE corner cases where bit equality and [Float#equals] must agree. + @Test + public void packedMinMaxValuesParticipateInValueObjectMethods() { + ColumnMetadataImpl first = withMinMax(DataType.INT, "-5", "7"); + assertEquals(first, withMinMax(DataType.INT, "-5", "7")); + assertEquals(first.hashCode(), withMinMax(DataType.INT, "-5", "7").hashCode()); + assertNotEquals(first, withMinMax(DataType.INT, "-5", "8")); + assertNotEquals(first, withMinMax(DataType.INT, "-5", null)); + assertTrue(first.toString().contains("_minValue=-5, _maxValue=7"), first.toString()); + + // Zero is the value the words hold when a value is absent, so it must stay distinguishable from absence. + ColumnMetadataImpl zero = withMinMax(DataType.INT, "0", "0"); + assertEquals(zero.getMinValue(), 0); + assertNotEquals(zero, withMinMax(DataType.INT, null, null)); + + // -0.0 is not equal to 0.0 for Float/Double, and NaN is equal to itself: bit equality agrees with both. + assertNotEquals(withMinMax(DataType.FLOAT, "-0.0", "1").getMinValue(), + withMinMax(DataType.FLOAT, "0.0", "1").getMinValue()); + assertNotEquals(withMinMax(DataType.DOUBLE, "-0.0", "1"), withMinMax(DataType.DOUBLE, "0.0", "1")); + ColumnMetadataImpl nan = withMinMax(DataType.DOUBLE, "NaN", "NaN"); + assertEquals(nan.getMinValue(), Double.NaN); + assertEquals(nan, withMinMax(DataType.DOUBLE, "NaN", "NaN")); + assertEquals(nan.hashCode(), withMinMax(DataType.DOUBLE, "NaN", "NaN").hashCode()); + } + + /// [ColumnMetadataImpl.Builder] is public, so a caller may hand a fixed-width column a min/max that is not the box + /// class of its stored type. Such a value cannot be packed and must survive as the object it is. + @Test + public void minMaxOfUnexpectedTypeIsKeptVerbatim() { + ColumnMetadataImpl metadata = unexpectedMinMax(); + assertEquals(metadata.getMinValue(), "not-an-int"); + assertEquals(metadata.getMaxValue(), 3L); + assertEquals(metadata, unexpectedMinMax()); + assertEquals(metadata.hashCode(), unexpectedMinMax().hashCode()); + // The lengths of a fixed-width column are derived, so the fallback cannot disturb them. + assertEquals(metadata.getLengthOfLongestElement(), Integer.BYTES); + } + + private static ColumnMetadataImpl unexpectedMinMax() { + return ColumnMetadataImpl.builder() + .setFieldSpec(new DimensionFieldSpec("col", DataType.INT, true)) + .setTotalDocs(10) + .setMinValue("not-an-int") + .setMaxValue(3L) + .build(); + } + + /// The element lengths share their two words with the numeric min/max, so a var-width column must round-trip all + /// three of them while a fixed-width column derives them from its stored type and its multi-value count. + @Test + public void elementLengthsRoundTrip() { + ColumnMetadataImpl varWidthMv = ColumnMetadataImpl.builder() + .setFieldSpec(new DimensionFieldSpec("col", DataType.STRING, false)) + .setTotalDocs(10) + .setLengthOfShortestElement(2) + .setLengthOfLongestElement(7) + .setMaxNumberOfMultiValues(3) + .setMaxRowLengthInBytes(15) + .setTotalNumberOfEntries(30) + .setMinValue("aa") + .setMaxValue("zz") + .build(); + assertEquals(varWidthMv.getLengthOfShortestElement(), 2); + assertEquals(varWidthMv.getLengthOfLongestElement(), 7); + assertEquals(varWidthMv.getMaxRowLengthInBytes(), 15); + assertEquals(varWidthMv.getMaxNumberOfMultiValues(), 3); + assertEquals(varWidthMv.getTotalNumberOfEntries(), 30); + assertEquals(varWidthMv.getMinValue(), "aa"); + assertEquals(varWidthMv.getMaxValue(), "zz"); + assertFalse(varWidthMv.isFixedLength()); + + // A var-width SV column: the max row length is the longest element. + ColumnMetadataImpl varWidthSv = ColumnMetadataImpl.builder() + .setFieldSpec(new DimensionFieldSpec("col", DataType.STRING, true)) + .setTotalDocs(10).setLengthOfShortestElement(4).setLengthOfLongestElement(4).build(); + assertEquals(varWidthSv.getMaxRowLengthInBytes(), 4); + assertEquals(varWidthSv.getTotalNumberOfEntries(), 10); + assertEquals(varWidthSv.getMaxNumberOfMultiValues(), 0); + assertTrue(varWidthSv.isFixedLength()); + + // Pre-1.6.0 raw var-width columns write no lengths at all: the UNAVAILABLE sentinel must survive the packing. + ColumnMetadataImpl unavailable = ColumnMetadataImpl.fromPropertiesConfiguration(baseConfig("col"), 10, "col"); + assertEquals(unavailable.getLengthOfShortestElement(), ColumnMetadata.UNAVAILABLE); + assertEquals(unavailable.getLengthOfLongestElement(), ColumnMetadata.UNAVAILABLE); + assertEquals(unavailable.getMaxRowLengthInBytes(), ColumnMetadata.UNAVAILABLE); + + // Fixed-width columns derive all three from the stored type, min/max being packed in the same words. + ColumnMetadataImpl fixedSv = ColumnMetadataImpl.builder() + .setFieldSpec(new DimensionFieldSpec("col", DataType.TIMESTAMP, true)) + .setTotalDocs(10).setMinValue(1L).setMaxValue(2L).build(); + assertEquals(fixedSv.getLengthOfShortestElement(), Long.BYTES); + assertEquals(fixedSv.getLengthOfLongestElement(), Long.BYTES); + assertEquals(fixedSv.getMaxRowLengthInBytes(), Long.BYTES); + assertEquals(fixedSv.getMinValue(), 1L); + + ColumnMetadataImpl fixedMv = ColumnMetadataImpl.builder() + .setFieldSpec(new DimensionFieldSpec("col", DataType.INT, false)) + .setTotalDocs(10).setMaxNumberOfMultiValues(3).setTotalNumberOfEntries(25).setMinValue(1).setMaxValue(2) + .build(); + assertEquals(fixedMv.getLengthOfLongestElement(), Integer.BYTES); + assertEquals(fixedMv.getMaxRowLengthInBytes(), 3 * Integer.BYTES); + assertEquals(fixedMv.getTotalNumberOfEntries(), 25); + assertEquals(fixedMv.getMinValue(), 1); + } + + /// The ints that are not column-distinguishing are interned, so the columns of one segment hold one instance + /// instead of four ints each. This is the whole point of the indirection, so pin it on the private field. + @Test + public void columnsOfASegmentShareTheirShape() + throws Exception { + ColumnMetadataImpl intColumn = rawColumn(DataType.INT, 1000); + ColumnMetadataImpl stringColumn = ColumnMetadataImpl.builder() + .setFieldSpec(new DimensionFieldSpec("other", DataType.STRING, true)) + .setTotalDocs(1000).setCardinality(900).setLengthOfShortestElement(1).setLengthOfLongestElement(17).build(); + assertSame(shapeOf(stringColumn), shapeOf(intColumn), + "columns of one segment must share their shape whatever their type, cardinality or element lengths"); + + assertNotSame(shapeOf(rawColumn(DataType.INT, 1001)), shapeOf(intColumn), "total docs"); + ColumnMetadataImpl dictionary = ColumnMetadataImpl.builder() + .setFieldSpec(new DimensionFieldSpec("col", DataType.INT, true)) + .setTotalDocs(1000).setHasDictionary(true).setBitsPerElement(8).build(); + assertNotSame(shapeOf(dictionary), shapeOf(intColumn), "bits per element"); + assertEquals(dictionary.getBitsPerElement(), 8); + assertEquals(intColumn.getBitsPerElement(), ColumnMetadata.UNAVAILABLE); + ColumnMetadataImpl multiValue = ColumnMetadataImpl.builder() + .setFieldSpec(new DimensionFieldSpec("col", DataType.INT, false)) + .setTotalDocs(1000).setMaxNumberOfMultiValues(3).setTotalNumberOfEntries(2000).build(); + assertNotSame(shapeOf(multiValue), shapeOf(intColumn), "multi-value counts"); + } + + /// The shapes are held weakly, so a segment's shape is released with the last column that holds it; an interner + /// that pinned them would leak one per distinct segment shape ever loaded. + @Test + public void unreferencedShapeIsReleased() + throws Exception { + WeakReference shape = new WeakReference<>(shapeOf(rawColumn(DataType.INT, 987654321))); + for (int i = 0; i < 100 && shape.get() != null; i++) { + System.gc(); + Thread.sleep(10); + } + assertNull(shape.get(), "the interner must not keep an unloaded segment's shape alive"); + } + + private static ColumnMetadataImpl rawColumn(DataType dataType, int totalDocs) { + return ColumnMetadataImpl.builder() + .setFieldSpec(new DimensionFieldSpec("col", dataType, true)) + .setTotalDocs(totalDocs).setCardinality(5).build(); + } + + private static Object shapeOf(ColumnMetadataImpl metadata) + throws Exception { + Field field = ColumnMetadataImpl.class.getDeclaredField("_shape"); + field.setAccessible(true); + return field.get(metadata); + } + + private static ColumnMetadataImpl withMinMax(DataType dataType, @Nullable String min, @Nullable String max) { + return ColumnMetadataImpl.fromPropertiesConfiguration(minMaxConfig(dataType, min, max), 10, "col"); + } + + private static PropertiesConfiguration minMaxConfig(DataType dataType, @Nullable String min, @Nullable String max) { + PropertiesConfiguration config = configFor(FieldType.DIMENSION, dataType, null); + if (min != null) { + config.setProperty(Column.getKeyFor("col", Column.MIN_VALUE), min); + } + if (max != null) { + config.setProperty(Column.getKeyFor("col", Column.MAX_VALUE), max); + } + return config; + } + private static Set flags(ColumnMetadataImpl metadata) { Set flags = new TreeSet<>(); if (metadata.hasDictionary()) { From 42fc38a698b83ca6a4c699085c656a2f5edc4ff7 Mon Sep 17 00:00:00 2001 From: Xiang Fu Date: Sat, 5 Sep 2026 22:03:17 -0700 Subject: [PATCH 2/2] DATA-3221 (8+9 follow-up): drop the shared shape, pack bitsPerElement instead Review of 8eab380 found the SharedShape indirection to be a net regression for every column whose shape tuple is not shared with another, and to run its interner on the query path. Both are fixed by dropping it; the primitive min/max union, which is where the win actually is, stays. Measured with 500k instances built from one shared FieldSpec, used heap diffed after a forced GC (SerialGC, two allocation counts so the slope cancels the constant offset), with the instance size read exactly off the field offsets: variant instance raw INT SV MV INT SV STRING (one shape) (distinct) (dict) parent of 8eab380 72 B 104 101 72 8eab380 (SharedShape) 64 B 64 140 64 no shape, four ints inline 80 B 83 83 80 this commit 72 B 72 72 72 An unshared shape costs a 32-byte object plus its ~44-byte weak interner entry, so a multi-value column paid ~36 bytes more than before 8eab380. Simply putting the four ints back inline pushes the object from 72 to 80 bytes, which instead regresses every var-width column by 8. Packing bitsPerElement into the flags word - which widens from short to int and takes the field with it - frees the int that keeps the object at 72, so no column type regresses against the parent commit, while a numeric column still sheds its two min/max boxes (104 -> 72). Against 8eab380 this costs 8 bytes per column on the wide raw-INT external table it targeted (64 -> 72); that is the price of the multi-value case not costing 76. bitsPerElement is written by the segment creator as getNumBitsPerValue(...) and never exceeds Integer.SIZE, but it is read verbatim from metadata.properties and the Builder is public, so a value outside the 23-bit encodable range falls back to the Extras holder (whose int fits in the padding its four refs leave) rather than being truncated. Builder#build() is not a segment-load-only path: IndexSegment#getDataSource( String, Schema) rebuilds a virtual column's metadata on every call, so the interner ran per query per segment. It is gone, and the constraint is now documented on Builder. A FLOAT/DOUBLE NaN min/max comes back canonicalized, because floatToIntBits collapses the payload and signalling bit. That is kept rather than switched to the raw variants, so equals() on the words keeps agreeing with Float.equals, and it is now documented on getMinValue() and pinned by a test. No on-disk format, public signature or REST payload changes. Co-Authored-By: Claude Opus 5 --- .../index/metadata/ColumnMetadataImpl.java | 200 +++++++++--------- .../metadata/ColumnMetadataImplTest.java | 133 ++++++++---- 2 files changed, 191 insertions(+), 142 deletions(-) diff --git a/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/metadata/ColumnMetadataImpl.java b/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/metadata/ColumnMetadataImpl.java index 11c28cab7fd0..6bb3e423b04a 100644 --- a/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/metadata/ColumnMetadataImpl.java +++ b/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/metadata/ColumnMetadataImpl.java @@ -72,43 +72,53 @@ /// as immutable: a setter call on one would bleed into every other segment and table that shares it, and would /// corrupt the interner's hash bucket (nothing ever mutated one; copy via a JSON round-trip before mutating). /// -/// The object layout is kept at 64 bytes for an ordinary column for the same reason: the six booleans, the -/// forward-index encoding and the two min/max representation bits are packed into one [#_flags] short; the refs only -/// a partitioned column or an OPEN_STRUCT parent/child carries (partition function and partitions, parent column, -/// sparse keys) live in a lazily allocated [Extras] holder that stays `null` for every other column; the four ints -/// that are not column-distinguishing live in a shared [SharedShape]; and the three element-length ints share their -/// two words with the numeric min/max (see [#_minWord]). The compression stats stay a direct ref because the segment -/// creator writes them for every raw column. None of this is visible through the public getters, so the -/// `/tables/{table}/segments/{segment}/metadata` payload (bean-serialized from the getters) is unchanged. +/// The object layout is kept at 72 bytes for every column for the same reason: the six booleans, the forward-index +/// encoding, the two min/max representation bits and `bitsPerElement` are packed into one [#_flags] int; the refs +/// only a partitioned column or an OPEN_STRUCT parent/child carries (partition function and partitions, parent +/// column, sparse keys) live in a lazily allocated [Extras] holder that stays `null` for every other column; and the +/// three element-length ints share their two words with the numeric min/max (see [#_minWord]). The compression stats +/// stay a direct ref because the segment creator writes them for every raw column. None of this is visible through +/// the public getters, so the `/tables/{table}/segments/{segment}/metadata` payload (bean-serialized from the +/// getters) is unchanged. /// /// | bytes | field(s) | /// |------:|----------| /// | 12 | object header | -/// | 4 | `_cardinality` | /// | 16 | `_minWord`, `_maxWord` | -/// | 2+2 | `_flags` plus alignment padding | -/// | 28 | `_fieldSpec`, `_shape`, `_minValue`, `_maxValue`, `_extras`, `_compressionMetadata`, `_indexTypeSizes` | -/// | 64 | total (was 72: eight ints, six refs and a flags byte) | +/// | 16 | `_totalDocs`, `_cardinality`, `_totalNumberOfEntries`, `_maxNumberOfMultiValues` | +/// | 4 | `_flags` | +/// | 24 | `_fieldSpec`, `_minValue`, `_maxValue`, `_extras`, `_compressionMetadata`, `_indexTypeSizes` | +/// | 72 | total | /// -/// On top of the eight bytes this saves directly, a fixed-width column no longer retains a box per min/max value -/// (~30 bytes and two objects per column for a nullable INT column), and the [SharedShape] is amortized over every -/// column of the segment that has the same shape. +/// The saving over the eight ints, six refs and flags byte this replaced is not in the object itself, which is the +/// same 72 bytes, but in what it no longer retains: a fixed-width column holds no box per min/max value, which is +/// ~32 bytes and two surviving objects per numeric column. @SuppressWarnings({"rawtypes", "unchecked"}) public class ColumnMetadataImpl implements ColumnMetadata { private static final long SIZE_MASK = 0xffffffffffffL; // Bits of _flags - private static final short HAS_DICTIONARY = 1; - private static final short DICTIONARY_ENCODED_FORWARD_INDEX = 1 << 1; - private static final short SORTED = 1 << 2; - private static final short NON_NULL = 1 << 3; - private static final short MIN_MAX_VALUE_INVALID = 1 << 4; - private static final short ASCII = 1 << 5; - private static final short AUTO_GENERATED = 1 << 6; + private static final int HAS_DICTIONARY = 1; + private static final int DICTIONARY_ENCODED_FORWARD_INDEX = 1 << 1; + private static final int SORTED = 1 << 2; + private static final int NON_NULL = 1 << 3; + private static final int MIN_MAX_VALUE_INVALID = 1 << 4; + private static final int ASCII = 1 << 5; + private static final int AUTO_GENERATED = 1 << 6; /// Set when the min (max) value is held as raw bits in [#_minWord] ([#_maxWord]) rather than as an object in /// [#_minValue] ([#_maxValue]); an absent value sets neither. - private static final short MIN_VALUE_IN_WORD = 1 << 7; - private static final short MAX_VALUE_IN_WORD = 1 << 8; + private static final int MIN_VALUE_IN_WORD = 1 << 7; + private static final int MAX_VALUE_IN_WORD = 1 << 8; + + // The remaining 23 bits of _flags hold bitsPerElement + 1, so that the UNAVAILABLE sentinel encodes as 0. The + // segment creator writes getNumBitsPerValue(cardinality - 1), which never exceeds Integer.SIZE, but the value is + // read verbatim from metadata.properties and the Builder is public, so a value outside the encodable range falls + // back to Extras rather than being truncated. + private static final int BITS_PER_ELEMENT_SHIFT = 9; + private static final int BITS_PER_ELEMENT_MASK = (1 << 23) - 1; + /// Encoded value meaning the real one did not fit and is held in [Extras#_bitsPerElement]. + private static final int BITS_PER_ELEMENT_IN_EXTRAS = BITS_PER_ELEMENT_MASK; + private static final int MAX_ENCODABLE_BITS_PER_ELEMENT = BITS_PER_ELEMENT_MASK - 2; /// Canonical instances of the [FieldSpec]s parsed from `metadata.properties`, keyed by [FieldSpec#equals] / /// [FieldSpec#hashCode] (name, data type, single-value, default null value, max length, date-time format and @@ -117,14 +127,11 @@ public class ColumnMetadataImpl implements ColumnMetadata { /// any of them and is released once the last one is unloaded. Thread-safe. private static final Interner FIELD_SPEC_INTERNER = Interners.newWeakInterner(); - /// Canonical instances of the [SharedShape]s, held weakly exactly like [#FIELD_SPEC_INTERNER]: the canonical - /// instance is one of the instances the loaded segments retain, so it lives as long as any of them. - private static final Interner SHAPE_INTERNER = Interners.newWeakInterner(); - private final FieldSpec _fieldSpec; - /// The ints that are not column-distinguishing, shared with every other column that has the same shape. - private final SharedShape _shape; + private final int _totalDocs; private final int _cardinality; + private final int _totalNumberOfEntries; + private final int _maxNumberOfMultiValues; /// Two words with a use that depends on whether the stored type is fixed width, which is exactly the condition /// under which the other use is dead: /// - fixed-width stored type (INT, LONG, FLOAT, DOUBLE, and BOOLEAN/TIMESTAMP through their stored type): the raw @@ -143,9 +150,9 @@ public class ColumnMetadataImpl implements ColumnMetadata { private final Comparable _minValue; @Nullable private final Comparable _maxValue; - /// hasDictionary, forward-index encoding, sorted, nonNull, minMaxValueInvalid, ascii, autoGenerated and the two - /// min/max representation bits, see the bit constants above. - private final short _flags; + /// hasDictionary, forward-index encoding, sorted, nonNull, minMaxValueInvalid, ascii, autoGenerated, the two + /// min/max representation bits and the encoded `bitsPerElement`, see the bit constants above. + private final int _flags; @Nullable private final Extras _extras; @Nullable @@ -164,12 +171,15 @@ public class ColumnMetadataImpl implements ColumnMetadata { @Nullable private long[] _indexTypeSizes; - private ColumnMetadataImpl(FieldSpec fieldSpec, SharedShape shape, int cardinality, long minWord, long maxWord, - @Nullable Comparable minValue, @Nullable Comparable maxValue, short flags, @Nullable Extras extras, + private ColumnMetadataImpl(FieldSpec fieldSpec, int totalDocs, int cardinality, int totalNumberOfEntries, + int maxNumberOfMultiValues, long minWord, long maxWord, @Nullable Comparable minValue, + @Nullable Comparable maxValue, int flags, @Nullable Extras extras, @Nullable CompressionMetadata compressionMetadata) { _fieldSpec = fieldSpec; - _shape = shape; + _totalDocs = totalDocs; _cardinality = cardinality; + _totalNumberOfEntries = totalNumberOfEntries; + _maxNumberOfMultiValues = maxNumberOfMultiValues; _minWord = minWord; _maxWord = maxWord; _minValue = minValue; @@ -179,7 +189,7 @@ private ColumnMetadataImpl(FieldSpec fieldSpec, SharedShape shape, int cardinali _compressionMetadata = compressionMetadata; } - private boolean hasFlag(short flag) { + private boolean hasFlag(int flag) { return (_flags & flag) != 0; } @@ -194,7 +204,7 @@ public FieldSpec getFieldSpec() { @Override public int getTotalDocs() { - return _shape._totalDocs; + return _totalDocs; } @Override @@ -226,12 +236,20 @@ public boolean isNonNull() { /// than retained: a server keeps one instance of this class per (segment, column) for the segment lifetime, while /// the callers (segment pruners, aggregation rewrites, range-index construction) read it a handful of times per /// query and let the box die in the young generation. + /// + /// The value returned is [Object#equals]-equal to the one handed in, and bit-identical to it for every value but + /// a FLOAT/DOUBLE `NaN`: [Float#floatToIntBits] collapses the NaN payload and signalling bit onto the canonical + /// quiet NaN, so a `NaN` comes back canonicalized. That is deliberate rather than incidental - it keeps + /// [#equals(Object)] on the raw words agreeing with [Float#equals] / [Double#equals], which likewise treat all + /// NaNs as one value - and it is unobservable to the callers above, which compare and range-check the value. + /// `-0.0` is not affected: it keeps its own bits and stays distinct from `0.0`, exactly as [Float#equals] has it. @Nullable @Override public Comparable getMinValue() { return hasFlag(MIN_VALUE_IN_WORD) ? boxValueWord(_minWord) : _minValue; } + /// Boxed on read like [#getMinValue()], with the same guarantees. @Nullable @Override public Comparable getMaxValue() { @@ -277,17 +295,19 @@ public boolean isAscii() { @Override public int getBitsPerElement() { - return _shape._bitsPerElement; + int encoded = (_flags >>> BITS_PER_ELEMENT_SHIFT) & BITS_PER_ELEMENT_MASK; + // The fallback is only encoded when the value was handed to Extras, which is therefore non-null here. + return encoded != BITS_PER_ELEMENT_IN_EXTRAS ? encoded - 1 : _extras._bitsPerElement; } @Override public int getTotalNumberOfEntries() { - return _shape._totalNumberOfEntries; + return _totalNumberOfEntries; } @Override public int getMaxNumberOfMultiValues() { - return _shape._maxNumberOfMultiValues; + return _maxNumberOfMultiValues; } /// [Builder#build()] pins this to `lengthOfLongestElement` for an SV column and to @@ -297,7 +317,7 @@ public int getMaxNumberOfMultiValues() { public int getMaxRowLengthInBytes() { if (isFixedWidth()) { int size = _fieldSpec.getDataType().getStoredType().size(); - return _fieldSpec.isSingleValueField() ? size : _shape._maxNumberOfMultiValues * size; + return _fieldSpec.isSingleValueField() ? size : _maxNumberOfMultiValues * size; } return (int) _maxWord; } @@ -415,11 +435,13 @@ public boolean equals(Object o) { // The representation two equal columns pick is a function of their field spec and their values, so comparing the // raw words and flags is equivalent to comparing the boxed min/max and the unpacked lengths. ColumnMetadataImpl that = (ColumnMetadataImpl) o; - return _cardinality == that._cardinality + return _totalDocs == that._totalDocs + && _cardinality == that._cardinality + && _totalNumberOfEntries == that._totalNumberOfEntries + && _maxNumberOfMultiValues == that._maxNumberOfMultiValues && _flags == that._flags && _minWord == that._minWord && _maxWord == that._maxWord - && Objects.equals(_shape, that._shape) && Objects.equals(_fieldSpec, that._fieldSpec) && Objects.equals(_minValue, that._minValue) && Objects.equals(_maxValue, that._maxValue) @@ -430,8 +452,8 @@ public boolean equals(Object o) { @Override public int hashCode() { - return Objects.hash(_fieldSpec, _shape, _cardinality, _flags, _minWord, _maxWord, _minValue, _maxValue, _extras, - _compressionMetadata, Arrays.hashCode(_indexTypeSizes)); + return Objects.hash(_fieldSpec, _totalDocs, _cardinality, _totalNumberOfEntries, _maxNumberOfMultiValues, _flags, + _minWord, _maxWord, _minValue, _maxValue, _extras, _compressionMetadata, Arrays.hashCode(_indexTypeSizes)); } // Keeps the pre-packing field names and order, which tests and log consumers match on @@ -723,54 +745,10 @@ public static Builder builder() { return new Builder(); } - /// The ints of a column that are not column-distinguishing, interned through [#SHAPE_INTERNER] so the columns of a - /// segment that have the same shape hold one instance instead of four ints each. - /// - /// `totalDocs` is identical for every column of a segment; `totalNumberOfEntries` and `maxNumberOfMultiValues` are - /// pinned by [Builder#build()] to `totalDocs` and `0` for every SV column; and `bitsPerElement` is `UNAVAILABLE` - /// for every raw column and takes one of at most 33 values (`1..32`) for a dictionary-encoded one. So the SV - /// columns of a segment share at most ~34 instances however wide the segment is, and a wide external table of raw - /// columns shares exactly one. The remaining four ints are not held here: `cardinality` is distinguishing, and the - /// three element lengths are derived or packed into [#_minWord] / [#_maxWord]. - /// - /// Immutable and thread-safe. - private static final class SharedShape { - private final int _totalDocs; - private final int _totalNumberOfEntries; - private final int _maxNumberOfMultiValues; - private final int _bitsPerElement; - - private SharedShape(int totalDocs, int totalNumberOfEntries, int maxNumberOfMultiValues, int bitsPerElement) { - _totalDocs = totalDocs; - _totalNumberOfEntries = totalNumberOfEntries; - _maxNumberOfMultiValues = maxNumberOfMultiValues; - _bitsPerElement = bitsPerElement; - } - - @Override - public boolean equals(Object o) { - if (this == o) { - return true; - } - if (o == null || getClass() != o.getClass()) { - return false; - } - SharedShape that = (SharedShape) o; - return _totalDocs == that._totalDocs - && _totalNumberOfEntries == that._totalNumberOfEntries - && _maxNumberOfMultiValues == that._maxNumberOfMultiValues - && _bitsPerElement == that._bitsPerElement; - } - - @Override - public int hashCode() { - return Objects.hash(_totalDocs, _totalNumberOfEntries, _maxNumberOfMultiValues, _bitsPerElement); - } - } - /// The refs that only a partitioned column (partition function and partitions) or an OPEN_STRUCT parent/child - /// (sparse keys / parent column) carries. Ordinary columns hold no holder at all, so they never pay for the four - /// slots; a column that has any of them pays one extra object. + /// (sparse keys / parent column) carries, plus a `bitsPerElement` too large to encode in [#_flags]. Ordinary + /// columns hold no holder at all, so they never pay for the slots; a column that has any of them pays one extra + /// object. The int fits in the padding the four refs leave, so it costs nothing. private static final class Extras { @Nullable private final PartitionFunction _partitionFunction; @@ -780,20 +758,25 @@ private static final class Extras { private final String _parentColumn; @Nullable private final List _sparseKeys; + /// Only read when [#_flags] encodes [#BITS_PER_ELEMENT_IN_EXTRAS]; [ColumnMetadata#UNAVAILABLE] (which is always + /// encodable, so it never reaches here) stands for "not held". + private final int _bitsPerElement; private Extras(@Nullable PartitionFunction partitionFunction, @Nullable Set partitions, - @Nullable String parentColumn, @Nullable List sparseKeys) { + @Nullable String parentColumn, @Nullable List sparseKeys, int bitsPerElement) { _partitionFunction = partitionFunction; _partitions = partitions; _parentColumn = parentColumn; _sparseKeys = sparseKeys; + _bitsPerElement = bitsPerElement; } @Nullable private static Extras create(@Nullable PartitionFunction partitionFunction, @Nullable Set partitions, - @Nullable String parentColumn, @Nullable List sparseKeys) { - return partitionFunction == null && partitions == null && parentColumn == null && sparseKeys == null ? null - : new Extras(partitionFunction, partitions, parentColumn, sparseKeys); + @Nullable String parentColumn, @Nullable List sparseKeys, int bitsPerElement) { + return partitionFunction == null && partitions == null && parentColumn == null && sparseKeys == null + && bitsPerElement == UNAVAILABLE ? null + : new Extras(partitionFunction, partitions, parentColumn, sparseKeys, bitsPerElement); } @Override @@ -805,7 +788,8 @@ public boolean equals(Object o) { return false; } Extras that = (Extras) o; - return Objects.equals(_partitionFunction, that._partitionFunction) + return _bitsPerElement == that._bitsPerElement + && Objects.equals(_partitionFunction, that._partitionFunction) && Objects.equals(_partitions, that._partitions) && Objects.equals(_parentColumn, that._parentColumn) && Objects.equals(_sparseKeys, that._sparseKeys); @@ -813,7 +797,7 @@ public boolean equals(Object o) { @Override public int hashCode() { - return Objects.hash(_partitionFunction, _partitions, _parentColumn, _sparseKeys); + return Objects.hash(_partitionFunction, _partitions, _parentColumn, _sparseKeys, _bitsPerElement); } } @@ -871,6 +855,11 @@ private static CompressionMetadata create(long uncompressedValueSizeInBytes, } } + /// Not a segment-load-only path: a column that a segment predates is served by a virtual column, and + /// `IndexSegment#getDataSource(String, Schema)` rebuilds its metadata through this builder on every call, so + /// [#build()] runs per query per segment. It must therefore stay allocation-cheap and lock-free - in particular, + /// it must not intern anything: a shared side table would turn a query into a global map lookup and, held weakly, + /// into reference-queue churn. public static class Builder { private FieldSpec _fieldSpec; private int _totalDocs; @@ -1069,7 +1058,7 @@ public ColumnMetadataImpl build() { _bitsPerElement = UNAVAILABLE; } - short flags = 0; + int flags = 0; if (_hasDictionary) { flags |= HAS_DICTIONARY; } @@ -1091,6 +1080,9 @@ public ColumnMetadataImpl build() { if (_autoGenerated) { flags |= AUTO_GENERATED; } + boolean encodableBitsPerElement = + _bitsPerElement >= UNAVAILABLE && _bitsPerElement <= MAX_ENCODABLE_BITS_PER_ELEMENT; + flags |= (encodableBitsPerElement ? _bitsPerElement + 1 : BITS_PER_ELEMENT_IN_EXTRAS) << BITS_PER_ELEMENT_SHIFT; // Fill the two words with whichever of the two uses this column has (see ColumnMetadataImpl#_minWord). long minWord = 0; @@ -1115,10 +1107,10 @@ public ColumnMetadataImpl build() { maxWord = _maxRowLengthInBytes & 0xffffffffL; } - SharedShape shape = SHAPE_INTERNER.intern( - new SharedShape(_totalDocs, _totalNumberOfEntries, _maxNumberOfMultiValues, _bitsPerElement)); - return new ColumnMetadataImpl(_fieldSpec, shape, _cardinality, minWord, maxWord, minValue, maxValue, flags, - Extras.create(_partitionFunction, _partitions, _parentColumn, _sparseKeys), + return new ColumnMetadataImpl(_fieldSpec, _totalDocs, _cardinality, _totalNumberOfEntries, + _maxNumberOfMultiValues, minWord, maxWord, minValue, maxValue, flags, + Extras.create(_partitionFunction, _partitions, _parentColumn, _sparseKeys, + encodableBitsPerElement ? UNAVAILABLE : _bitsPerElement), CompressionMetadata.create(_uncompressedValueSizeInBytes, _forwardIndexChunkCompressionType, _dictionaryUncompressedValueSizeInBytes)); } diff --git a/pinot-segment-spi/src/test/java/org/apache/pinot/segment/spi/index/metadata/ColumnMetadataImplTest.java b/pinot-segment-spi/src/test/java/org/apache/pinot/segment/spi/index/metadata/ColumnMetadataImplTest.java index c776b597bb1c..54670ed031cc 100644 --- a/pinot-segment-spi/src/test/java/org/apache/pinot/segment/spi/index/metadata/ColumnMetadataImplTest.java +++ b/pinot-segment-spi/src/test/java/org/apache/pinot/segment/spi/index/metadata/ColumnMetadataImplTest.java @@ -20,7 +20,6 @@ import com.fasterxml.jackson.databind.JsonNode; import java.lang.ref.WeakReference; -import java.lang.reflect.Field; import java.math.BigDecimal; import java.util.LinkedHashMap; import java.util.List; @@ -530,55 +529,113 @@ public void elementLengthsRoundTrip() { assertEquals(fixedMv.getMinValue(), 1); } - /// The ints that are not column-distinguishing are interned, so the columns of one segment hold one instance - /// instead of four ints each. This is the whole point of the indirection, so pin it on the private field. + /// `bitsPerElement` is packed into the flags word rather than held in its own int, so every value a segment or a + /// [ColumnMetadataImpl.Builder] caller can produce must come back verbatim - including the ones too large to + /// encode, which fall back to the [ColumnMetadataImpl] extras holder. @Test - public void columnsOfASegmentShareTheirShape() - throws Exception { - ColumnMetadataImpl intColumn = rawColumn(DataType.INT, 1000); - ColumnMetadataImpl stringColumn = ColumnMetadataImpl.builder() - .setFieldSpec(new DimensionFieldSpec("other", DataType.STRING, true)) - .setTotalDocs(1000).setCardinality(900).setLengthOfShortestElement(1).setLengthOfLongestElement(17).build(); - assertSame(shapeOf(stringColumn), shapeOf(intColumn), - "columns of one segment must share their shape whatever their type, cardinality or element lengths"); - - assertNotSame(shapeOf(rawColumn(DataType.INT, 1001)), shapeOf(intColumn), "total docs"); - ColumnMetadataImpl dictionary = ColumnMetadataImpl.builder() + public void bitsPerElementRoundTrips() { + // -1 is the UNAVAILABLE sentinel of a raw column, 1..32 is what the segment creator writes, and the rest are + // values only a hand-written or corrupt metadata.properties can carry. + for (int bitsPerElement : new int[]{ + ColumnMetadata.UNAVAILABLE, 0, 1, 8, 32, 8388604, 8388605, 8388606, Integer.MAX_VALUE, -2, Integer.MIN_VALUE + }) { + ColumnMetadataImpl metadata = withBitsPerElement(bitsPerElement); + String message = "bitsPerElement " + bitsPerElement; + assertEquals(metadata.getBitsPerElement(), bitsPerElement, message); + assertEquals(metadata, withBitsPerElement(bitsPerElement), message); + assertEquals(metadata.hashCode(), withBitsPerElement(bitsPerElement).hashCode(), message); + assertNotEquals(metadata, withBitsPerElement(bitsPerElement - 1), message); + assertEquals(JsonUtils.objectToJsonNode(metadata).get("bitsPerElement").asInt(), bitsPerElement, message); + assertTrue(metadata.toString().contains("_bitsPerElement=" + bitsPerElement), metadata.toString()); + // The packing shares its word with the flags, so neither may bleed into the other. + assertTrue(metadata.hasDictionary(), message); + assertTrue(metadata.isSorted(), message); + assertTrue(metadata.isNonNull(), message); + assertTrue(metadata.isAutoGenerated(), message); + assertTrue(metadata.isAscii(), message); + assertTrue(metadata.isMinMaxValueInvalid(), message); + assertEquals(metadata.getForwardIndexEncoding(), EncodingType.DICTIONARY, message); + } + + // An unencodable value shares the extras holder with the rare refs, so the two must not displace each other. + ColumnMetadataImpl withExtras = ColumnMetadataImpl.builder() + .setFieldSpec(new DimensionFieldSpec("col", DataType.INT, true)) + .setTotalDocs(10).setHasDictionary(true).setBitsPerElement(Integer.MAX_VALUE).setParentColumn("parent") + .build(); + assertEquals(withExtras.getBitsPerElement(), Integer.MAX_VALUE); + assertEquals(withExtras.getParentColumn(), "parent"); + assertNotEquals(withExtras, ColumnMetadataImpl.builder() .setFieldSpec(new DimensionFieldSpec("col", DataType.INT, true)) - .setTotalDocs(1000).setHasDictionary(true).setBitsPerElement(8).build(); - assertNotSame(shapeOf(dictionary), shapeOf(intColumn), "bits per element"); - assertEquals(dictionary.getBitsPerElement(), 8); - assertEquals(intColumn.getBitsPerElement(), ColumnMetadata.UNAVAILABLE); + .setTotalDocs(10).setHasDictionary(true).setBitsPerElement(Integer.MAX_VALUE - 1).setParentColumn("parent") + .build()); + } + + /// The four ints that describe the shape of a column are read back from the instance itself and each of them + /// distinguishes two otherwise identical columns. + @Test + public void columnShapeFieldsRoundTrip() { ColumnMetadataImpl multiValue = ColumnMetadataImpl.builder() .setFieldSpec(new DimensionFieldSpec("col", DataType.INT, false)) - .setTotalDocs(1000).setMaxNumberOfMultiValues(3).setTotalNumberOfEntries(2000).build(); - assertNotSame(shapeOf(multiValue), shapeOf(intColumn), "multi-value counts"); + .setTotalDocs(1000).setCardinality(5).setMaxNumberOfMultiValues(3).setTotalNumberOfEntries(2000).build(); + assertEquals(multiValue.getTotalDocs(), 1000); + assertEquals(multiValue.getCardinality(), 5); + assertEquals(multiValue.getMaxNumberOfMultiValues(), 3); + assertEquals(multiValue.getTotalNumberOfEntries(), 2000); + + assertNotEquals(multiValue, mvColumn(1001, 5, 3, 2000), "total docs"); + assertNotEquals(multiValue, mvColumn(1000, 6, 3, 2000), "cardinality"); + assertNotEquals(multiValue, mvColumn(1000, 5, 4, 2000), "max number of multi values"); + assertNotEquals(multiValue, mvColumn(1000, 5, 3, 2001), "total number of entries"); + assertEquals(multiValue, mvColumn(1000, 5, 3, 2000)); + assertEquals(multiValue.hashCode(), mvColumn(1000, 5, 3, 2000).hashCode()); } - /// The shapes are held weakly, so a segment's shape is released with the last column that holds it; an interner - /// that pinned them would leak one per distinct segment shape ever loaded. + /// A FLOAT/DOUBLE min/max is held as `floatToIntBits` / `doubleToLongBits`, which collapses every NaN onto the + /// canonical quiet NaN. Documented on [ColumnMetadataImpl#getMinValue()]: the value stays `equals` to the one the + /// builder was handed, which is what `equals`, the pruners and the REST payload compare on, but a NaN payload is + /// not preserved bit for bit. @Test - public void unreferencedShapeIsReleased() - throws Exception { - WeakReference shape = new WeakReference<>(shapeOf(rawColumn(DataType.INT, 987654321))); - for (int i = 0; i < 100 && shape.get() != null; i++) { - System.gc(); - Thread.sleep(10); - } - assertNull(shape.get(), "the interner must not keep an unloaded segment's shape alive"); + public void nanMinMaxIsCanonicalizedButStaysEqual() { + float signallingNan = Float.intBitsToFloat(0x7f800001); + ColumnMetadataImpl floatColumn = ColumnMetadataImpl.builder() + .setFieldSpec(new DimensionFieldSpec("col", DataType.FLOAT, true)) + .setTotalDocs(10).setMinValue(signallingNan).setMaxValue(Float.NaN).build(); + assertEquals(floatColumn.getMinValue(), Float.NaN); + assertEquals(Float.floatToRawIntBits((Float) floatColumn.getMinValue()), 0x7fc00000); + assertNotEquals(Float.floatToRawIntBits(signallingNan), 0x7fc00000); + + double payloadNan = Double.longBitsToDouble(0x7ff8000000000123L); + ColumnMetadataImpl doubleColumn = ColumnMetadataImpl.builder() + .setFieldSpec(new DimensionFieldSpec("col", DataType.DOUBLE, true)) + .setTotalDocs(10).setMinValue(payloadNan).setMaxValue(Double.NaN).build(); + assertEquals(doubleColumn.getMinValue(), Double.NaN); + assertEquals(Double.doubleToRawLongBits((Double) doubleColumn.getMinValue()), 0x7ff8000000000000L); + // Every NaN is one value to Double.equals, so the two columns stay equal, as they were before the packing. + assertEquals(doubleColumn, ColumnMetadataImpl.builder() + .setFieldSpec(new DimensionFieldSpec("col", DataType.DOUBLE, true)) + .setTotalDocs(10).setMinValue(Double.NaN).setMaxValue(Double.NaN).build()); } - private static ColumnMetadataImpl rawColumn(DataType dataType, int totalDocs) { + private static ColumnMetadataImpl withBitsPerElement(int bitsPerElement) { return ColumnMetadataImpl.builder() - .setFieldSpec(new DimensionFieldSpec("col", dataType, true)) - .setTotalDocs(totalDocs).setCardinality(5).build(); + .setFieldSpec(new DimensionFieldSpec("col", DataType.INT, true)) + .setTotalDocs(10) + .setHasDictionary(true) + .setBitsPerElement(bitsPerElement) + .setSorted(true) + .setNonNull(true) + .setAutoGenerated(true) + .setAscii(true) + .setMinMaxValueInvalid(true) + .build(); } - private static Object shapeOf(ColumnMetadataImpl metadata) - throws Exception { - Field field = ColumnMetadataImpl.class.getDeclaredField("_shape"); - field.setAccessible(true); - return field.get(metadata); + private static ColumnMetadataImpl mvColumn(int totalDocs, int cardinality, int maxNumberOfMultiValues, + int totalNumberOfEntries) { + return ColumnMetadataImpl.builder() + .setFieldSpec(new DimensionFieldSpec("col", DataType.INT, false)) + .setTotalDocs(totalDocs).setCardinality(cardinality).setMaxNumberOfMultiValues(maxNumberOfMultiValues) + .setTotalNumberOfEntries(totalNumberOfEntries).build(); } private static ColumnMetadataImpl withMinMax(DataType dataType, @Nullable String min, @Nullable String max) {