Jackie-Jiang commented on code in PR #19587: URL: https://github.com/apache/pinot/pull/19587#discussion_r4201781187
########## pinot-common/src/main/java/org/apache/pinot/common/utils/RoaringBitmapUnion.java: ########## @@ -0,0 +1,249 @@ +/** + * 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.common.utils; + +import java.io.IOException; +import java.nio.ByteBuffer; +import java.util.Objects; +import org.roaringbitmap.ContainerPointer; +import org.roaringbitmap.RoaringBitmap; + + +/// Interim stand-in for `org.roaringbitmap.RoaringBitmapUnion`, which is proposed to RoaringBitmap for +/// apache/pinot#19587 and not released yet. It has the same name and the same methods, so that once a RoaringBitmap +/// release ships the class the swap is mechanical: +/// +/// - change the import at the call sites to `org.roaringbitmap.RoaringBitmapUnion`, +/// - make [RoaringBitmapUtils#deserializeToUnion] call `RoaringBitmapUnion.takeOwnership(deserialize(bytes))`, +/// - delete this class, [MutableRoaringBitmapUnion] and their two tests (the library tests its own classes). +/// +/// `RoaringBitmapUnionTest` fails as soon as the library class is on the classpath, as a reminder. This class is +/// internal to Pinot and goes away with the swap; code outside this repository should not depend on it. +/// +/// An incremental union accumulator: it folds bitmaps that arrive over time into one bitmap with RoaringBitmap's lazy +/// union, which skips cardinality maintenance and keeps overlapping containers in a form that makes further unions +/// cheap, and it repairs the result only when it is read. Callers only ever observe valid bitmaps: +/// +/// - [#add(RoaringBitmap)] never modifies or retains its argument. +/// - [#get()] returns the accumulated bitmap without copying. The union never modifies that instance again: its next +/// mutating call first copies it, so the returned bitmap stays valid. Callers must treat it as read-only while they +/// still intend to use the union. +/// - [#take()] transfers ownership of the accumulated bitmap and leaves the union empty. +/// +/// Callers should code against the library contract, which is stricter than this class in two places: a bitmap +/// passed to [#takeOwnership(RoaringBitmap)] is relinquished for good, and it must be a plain [RoaringBitmap], not an +/// instance of a subclass. +/// +/// Differences from the library class: +/// +/// - The lazy primitives of [RoaringBitmap] are `protected`, so the accumulated state is a private subclass that +/// reaches them through inheritance. The bitmaps handed out by [#get()] and [#take()] are instances of that +/// subclass; it adds no state and overrides nothing. +/// - [#takeOwnership(RoaringBitmap)] adopts without copying only a bitmap that a union handed out; any other bitmap +/// is copied. Pinot never needs more: bitmaps are deserialized straight into a union with +/// [RoaringBitmapUtils#deserializeToUnion], and copies are made by adding to an empty union. +/// - [#add(int)] repairs pending lazy state before inserting, because inserting into a lazy container needs the +/// library's internals, and it does not re-encode a run container that received the value. A given accumulator +/// receives either bitmaps or single values in Pinot, never both. +/// - The released lazy union only pays off once containers are dense: while they are small arrays it allocates a new +/// container per union where the eager union merges in place, and it inserts container keys that are new to the +/// accumulator one at a time, which is quadratic when an input brings many of them. So an input is unioned eagerly +/// while the accumulator holds few values per container (hashed values spread over many containers stay there for +/// good) or when the input would insert many new keys, and lazily otherwise. The library class does not need +/// this: its lazy union merges small arrays in place and new keys in one pass. +/// +/// Instances are not thread-safe. +public final class RoaringBitmapUnion { + // The lazy union starts to pay off when containers hold more values than the library's lazy array bound: past it + // they are kept as bitmaps whose unions skip cardinality maintenance, below it they are arrays that the lazy union + // copies on every union + private static final int MIN_VALUES_PER_CONTAINER_FOR_LAZY_UNION = 1024; + // An input that would insert more new keys than this between existing ones is unioned eagerly. Each insert shifts + // the accumulator's key and container arrays, and the eager union's single merge pass costs about as much as a + // few of those shifts. + private static final int MAX_NEW_KEYS_FOR_LAZY_UNION = 4; + + // The accumulated bitmap. Never null. + private LazyBitmap _bitmap; + // Whether the bitmap holds lazy state that must be repaired before it is read + private boolean _dirty; + // Whether an alias returned by get() is outstanding and must not be mutated + private boolean _published; + // Number of values added so far, counting duplicates: an upper bound of the cardinality that is known without + // repairing, used to estimate how dense the containers are + private long _numValuesAdded; + // A cardinality observed on repaired state. Unions never remove values, so this remains a lower bound and proves + // density without repairing every lazy union. New container keys can invalidate that density proof. + private long _lastKnownCardinality; + + /// Creates an empty union. + public RoaringBitmapUnion() { + _bitmap = new LazyBitmap(); + } + + private RoaringBitmapUnion(LazyBitmap adopted) { + _bitmap = adopted; + // Adopted state is normalized on the first read, like the library class does + _dirty = true; + _numValuesAdded = adopted.getLongCardinality(); + _lastKnownCardinality = _numValuesAdded; + } + + /// Creates a union whose initial state is the given bitmap. The caller relinquishes the instance: it must not be + /// used again except through the union. + public static RoaringBitmapUnion takeOwnership(RoaringBitmap bitmap) { + Objects.requireNonNull(bitmap, "bitmap"); + if (bitmap instanceof LazyBitmap) { + return new RoaringBitmapUnion((LazyBitmap) bitmap); + } + LazyBitmap copy = new LazyBitmap(); + copy.lazyOr(bitmap); + return new RoaringBitmapUnion(copy); + } + + /// Deserializes a bitmap straight into a union that owns it. Not part of the library class: callers go through + /// [RoaringBitmapUtils#deserializeToUnion]. + static RoaringBitmapUnion deserialize(ByteBuffer byteBuffer) { + LazyBitmap bitmap = new LazyBitmap(); + try { + bitmap.deserialize(byteBuffer); + } catch (IOException e) { + throw new RuntimeException("Caught exception while deserializing RoaringBitmap", e); + } + return new RoaringBitmapUnion(bitmap); + } + + /// Unions the input into the accumulated state. The input is neither modified nor retained, so it may be reused or + /// mutated afterwards. Adding this union's own [#get()] result, or an empty bitmap, is a no-op. + public void add(RoaringBitmap input) { + Objects.requireNonNull(input, "input"); + if (input == _bitmap || input.isEmpty()) { + return; + } + beforeMutation(); + boolean dense = isDenseForLazyUnion(); + _numValuesAdded += input.getLongCardinality(); + if (dense && !insertsManyNewKeys(input)) { + _dirty = true; + _bitmap.lazyOr(input); + } else { + repairIfDirty(); + _bitmap.or(input); + } + } + + /// Unions a single value, treated as unsigned, into the accumulated state. + public void add(int value) { + beforeMutation(); + repairIfDirty(); + _bitmap.add(value); + _numValuesAdded++; + } + + /// Returns the accumulated bitmap with all pending lazy state repaired. The returned instance is the union's + /// current state, not a copy; the union copies it before its next mutation, so the returned bitmap stays valid. + /// Repeated calls without an intervening mutation return the same instance. + public RoaringBitmap get() { + repairIfDirty(); + _published = true; + return _bitmap; + } + + /// Returns the accumulated bitmap, repaired as by [#get()], and transfers its ownership to the caller. The union is + /// empty afterwards and can be reused. + public RoaringBitmap take() { + repairIfDirty(); + RoaringBitmap result = _bitmap; + _bitmap = new LazyBitmap(); + _dirty = false; + _published = false; + _numValuesAdded = 0; + _lastKnownCardinality = 0; + return result; + } + + /// Returns whether the lazy union would insert more than a few of the input's keys between keys the accumulator + /// already has. Keys past the accumulator's last key do not count: they are appended in one step. + private boolean insertsManyNewKeys(RoaringBitmap input) { + if (input.getContainerCount() <= MAX_NEW_KEYS_FOR_LAZY_UNION) { + return false; + } + ContainerPointer own = _bitmap.getContainerPointer(); + ContainerPointer other = input.getContainerPointer(); + int numNewKeys = 0; + while (other.getContainer() != null) { + char key = other.key(); + while (own.getContainer() != null && own.key() < key) { + own.advance(); + } + if (own.getContainer() == null) { + return false; + } + if (own.key() != key && ++numNewKeys > MAX_NEW_KEYS_FOR_LAZY_UNION) { + return true; + } + other.advance(); + } + return false; + } + + private boolean isDenseForLazyUnion() { + long minCardinality = (long) MIN_VALUES_PER_CONTAINER_FOR_LAZY_UNION * _bitmap.getContainerCount(); + if (_lastKnownCardinality >= minCardinality) { + return true; Review Comment: Non-blocking performance suggestion: average density can still send sparse unions down the expensive path. One dense container can keep the average above 1,024 while subsequent inputs touch only sparse containers. With a seed containing `[0, 65536)`, followed by 500 disjoint inputs each adding one value in container keys 1–50 (`(key << 16) | (inputIndex * 2)`), the exact PR implementation allocated 13.6 MB versus 226 KB for eager union; the mutable variant allocated 15.0 MB versus 262 KB. Median fold times were approximately 27% and 23% slower respectively across five warmed measurement rounds. This shape can occur in disjoint posting lists for IN/range filters or serialized bitmap aggregation. The focused probe used Java 25 and RoaringBitmap 1.6.23. All four modes passed exact bitmap equality, `validate()`, and seed/input immutability checks. These are isolated fold measurements, not whole-query timings. Please consider selecting laziness using the containers actually being merged and adding this skewed-density regression case. The same condition exists in `MutableRoaringBitmapUnion` at lines 164–167. ########## pinot-common/src/main/java/org/apache/pinot/common/utils/MutableRoaringBitmapUnion.java: ########## @@ -0,0 +1,207 @@ +/** + * 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.common.utils; + +import java.util.Objects; +import org.roaringbitmap.buffer.ImmutableRoaringBitmap; +import org.roaringbitmap.buffer.MappeableContainerPointer; +import org.roaringbitmap.buffer.MutableRoaringBitmap; + + +/// Interim stand-in for `org.roaringbitmap.buffer.MutableRoaringBitmapUnion`, the buffer counterpart of +/// [RoaringBitmapUnion]: same name and methods as the class proposed to RoaringBitmap, so that the swap is an import +/// change plus deleting this class. It is internal to Pinot and goes away with the swap. See [RoaringBitmapUnion] for +/// the contract and the swap steps. +/// +/// Inputs may be read-only bitmaps over memory-mapped buffers; they are only read. +/// +/// Differences from the library class: +/// +/// - The accumulated state is a private subclass of [MutableRoaringBitmap] that reaches the protected lazy primitives +/// through inheritance; [#get()] and [#take()] hand out instances of it. +/// - [#takeOwnership(MutableRoaringBitmap)] adopts without copying only a bitmap that a union handed out; any other +/// bitmap is copied. Callers should still treat the argument as relinquished, and should not pass subclasses. +/// - [#add(int)] repairs pending lazy state before inserting and does not re-encode a run container that received +/// the value. +/// - An input is unioned eagerly while the accumulator holds few values per container, or when it would insert many +/// container keys that are new to the accumulator, because the released lazy union only pays off for dense +/// containers and inserts new keys one at a time. +/// +/// Instances are not thread-safe. +public final class MutableRoaringBitmapUnion { + // See RoaringBitmapUnion + private static final int MIN_VALUES_PER_CONTAINER_FOR_LAZY_UNION = 1024; + private static final int MAX_NEW_KEYS_FOR_LAZY_UNION = 4; + + // The accumulated bitmap. Never null. + private LazyBitmap _bitmap; + // Whether the bitmap holds lazy state that must be repaired before it is read + private boolean _dirty; + // Whether an alias returned by get() is outstanding and must not be mutated + private boolean _published; + // Number of values added so far, counting duplicates: an upper bound of the cardinality that is known without + // repairing, used to estimate how dense the containers are + private long _numValuesAdded; + // A cardinality observed on repaired state. Unions never remove values, so this remains a lower bound and proves + // density without repairing every lazy union. New container keys can invalidate that density proof. + private long _lastKnownCardinality; + + /// Creates an empty union. + public MutableRoaringBitmapUnion() { + _bitmap = new LazyBitmap(); + } + + private MutableRoaringBitmapUnion(LazyBitmap adopted) { + _bitmap = adopted; + // Adopted state is normalized on the first read, like the library class does + _dirty = true; + _numValuesAdded = adopted.getLongCardinality(); + _lastKnownCardinality = _numValuesAdded; + } + + /// Creates a union whose initial state is the given bitmap. The caller relinquishes the instance: it must not be + /// used again except through the union. + public static MutableRoaringBitmapUnion takeOwnership(MutableRoaringBitmap bitmap) { Review Comment: Non-blocking cleanup: neither `takeOwnership` factory has a production caller, and `MutableRoaringBitmapUnion.add(int)` is also unused outside tests. These add implementation and maintenance solely for prospective upstream API parity. Consider keeping only the subset current callers require; those callers can still migrate to the upstream class later. ########## pinot-segment-local/src/main/java/org/apache/pinot/segment/local/startree/v2/builder/OnHeapSingleTreeBuilder.java: ########## @@ -53,6 +55,29 @@ Record getStarTreeRecord(int docId) { return _records.get(docId); } + @Override + @SuppressWarnings("unchecked") + boolean[] preSerializeMetrics() { + // Records stay on heap until the forward indexes are written, so serialize the variable-length metrics now and + // keep the bytes in place of the aggregated values (the bytes are smaller than the objects they replace). This + // completes the aggregators' maximum serialized size before it is consumed. + boolean[] preSerializedMetrics = new boolean[_numMetrics]; + for (int i = 0; i < _numMetrics; i++) { + ValueAggregator valueAggregator = _valueAggregators[i]; + if (valueAggregator.getAggregatedValueType() != DataType.BYTES) { + continue; + } + for (Record record : _records) { + Object value = record._metrics[i]; + if (value != null) { + record._metrics[i] = valueAggregator.serializeAggregatedValue(value); Review Comment: Non-blocking performance suggestion: restrict this prepass to aggregators that need serialization to determine their maximum size. It currently includes every BYTES aggregator, even fixed-size aggregators whose maximum size is already known. For `SUMPRECISION` over zero-valued INT/LONG data, records can share `BigDecimal.ZERO`. With the supported `precision=1000` setting, this pass replaces those shared references with a fresh 418-byte padded array per record: at least 418 MB of retained payload per million star-tree records, even without a bitmap metric. Previously those arrays were serialized and immediately copied into the forward-index writer, so they could be collected. The comment that the bytes are smaller than the objects they replace does not hold for this case. At minimum, skip fixed-size aggregators; preferably limit the pass to aggregators that actually need it. ########## pinot-core/src/test/java/org/apache/pinot/core/query/aggregation/function/DistinctCountBitmapLazyUnionTest.java: ########## @@ -0,0 +1,329 @@ +/** + * 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.core.query.aggregation.function; + +import java.nio.ByteBuffer; +import java.util.Map; +import java.util.Random; +import javax.annotation.Nullable; +import org.apache.pinot.common.CustomObject; +import org.apache.pinot.common.request.context.ExpressionContext; +import org.apache.pinot.common.utils.RoaringBitmapUtils; +import org.apache.pinot.core.common.BlockValSet; +import org.apache.pinot.core.query.aggregation.AggregationResultHolder; +import org.apache.pinot.core.query.aggregation.function.AggregationFunction.SerializedIntermediateResult; +import org.apache.pinot.core.query.aggregation.groupby.GroupByResultHolder; +import org.apache.pinot.spi.data.FieldSpec.DataType; +import org.mockito.Mockito; +import org.roaringbitmap.RoaringBitmap; +import org.testng.annotations.Test; + +import static org.testng.Assert.assertEquals; +import static org.testng.Assert.assertFalse; +import static org.testng.Assert.assertSame; +import static org.testng.Assert.assertTrue; + + +/// Covers the serialized-bitmap (`BYTES`) aggregation paths of [DistinctCountBitmapAggregationFunction], which union +/// input bitmaps lazily and repair the accumulator at extraction. The input cardinalities are chosen to cross the +/// container thresholds of the lazy union (array containers promote to bitmap containers past 1024 combined +/// cardinality; repair converts back to array containers at up to 4096), so both promoted and non-promoted +/// accumulator states are verified against an eagerly unioned reference. Intermediate-result merges must remain +/// valid for cardinality reads and serialization immediately after every merge. +public class DistinctCountBitmapLazyUnionTest { + private static final ExpressionContext EXPRESSION = ExpressionContext.forIdentifier("bitmapCol"); + private static final Random RANDOM = new Random(42); + + private static byte[][] serializedBitmaps(int numBitmaps, int valuesPerBitmap, int maxValue) { + byte[][] serialized = new byte[numBitmaps][]; + for (int i = 0; i < numBitmaps; i++) { + RoaringBitmap bitmap = new RoaringBitmap(); + for (int j = 0; j < valuesPerBitmap; j++) { + bitmap.add(RANDOM.nextInt(maxValue)); + } + serialized[i] = RoaringBitmapUtils.serialize(bitmap); + } + return serialized; + } + + private static RoaringBitmap eagerUnion(byte[][] serialized, int from, int to) { + RoaringBitmap expected = new RoaringBitmap(); + for (int i = from; i < to; i++) { + expected.or(RoaringBitmapUtils.deserialize(serialized[i])); + } + return expected; + } + + private static Map<ExpressionContext, BlockValSet> mockBlockValSetMap(byte[][] serialized) { + return mockBlockValSetMap(serialized, null); + } + + private static Map<ExpressionContext, BlockValSet> mockBlockValSetMap(byte[][] serialized, + @Nullable RoaringBitmap nullBitmap) { + BlockValSet blockValSet = Mockito.mock(BlockValSet.class); + Mockito.when(blockValSet.getValueType()).thenReturn(DataType.BYTES); + Mockito.when(blockValSet.isSingleValue()).thenReturn(true); + Mockito.when(blockValSet.getBytesValuesSV()).thenReturn(serialized); + Mockito.when(blockValSet.getNullBitmap()).thenReturn(nullBitmap); + return Map.of(EXPRESSION, blockValSet); + } + + @Test + public void testAggregateSkipsNullRowsAcrossRanges() { + // With null handling enabled the block is consumed as several non-null ranges; the accumulator is re-read from + // the holder at the start of each range and written back at its end, and null rows never reach the union + byte[][] serialized = serializedBitmaps(120, 50, 100_000); + RoaringBitmap nullBitmap = RoaringBitmap.bitmapOf(0, 1, 17, 40, 41, 42, 99, 119); + DistinctCountBitmapAggregationFunction function = + new DistinctCountBitmapAggregationFunction(EXPRESSION, true); + AggregationResultHolder holder = function.createAggregationResultHolder(); + int blockSize = 60; + for (int from = 0; from < serialized.length; from += blockSize) { + byte[][] block = new byte[blockSize][]; + System.arraycopy(serialized, from, block, 0, blockSize); + RoaringBitmap blockNulls = new RoaringBitmap(); + for (int i = 0; i < blockSize; i++) { + if (nullBitmap.contains(from + i)) { + blockNulls.add(i); + } + } + function.aggregate(blockSize, holder, mockBlockValSetMap(block, blockNulls)); + } + + RoaringBitmap expected = new RoaringBitmap(); + for (int i = 0; i < serialized.length; i++) { + if (!nullBitmap.contains(i)) { + expected.or(RoaringBitmapUtils.deserialize(serialized[i])); + } + } + RoaringBitmap result = function.extractAggregationResult(holder); + assertEquals(result, expected); + assertEquals(result.getCardinality(), expected.getCardinality()); + assertFalse(result.isEmpty()); + } + + @Test + public void testAggregateAcrossBlocks() { + // Enough overlap-heavy inputs to promote accumulator containers to (lazy) bitmap containers, split into + // multiple aggregate() calls to verify the accumulator stays valid across blocks until extraction. An early + // extraction after the first block guards the published-bitmap safety net: no operator extracts and then keeps + // aggregating today, but if one did, the published bitmap must stay immutable and later extraction must still + // be complete. + byte[][] serialized = serializedBitmaps(200, 100, 200_000); + DistinctCountBitmapAggregationFunction function = + new DistinctCountBitmapAggregationFunction(EXPRESSION, false); + AggregationResultHolder holder = function.createAggregationResultHolder(); + int blockSize = 50; + RoaringBitmap published = null; + RoaringBitmap publishedCopy = null; + for (int from = 0; from < serialized.length; from += blockSize) { + byte[][] block = new byte[blockSize][]; + System.arraycopy(serialized, from, block, 0, blockSize); + function.aggregate(blockSize, holder, mockBlockValSetMap(block)); + if (published != null) { + assertEquals(published, publishedCopy); + assertEquals(published.getCardinality(), publishedCopy.getCardinality()); + } + if (from == 0) { + published = function.extractAggregationResult(holder); + assertEquals(published, eagerUnion(serialized, 0, blockSize)); + // Repeated extraction returns the same published instance + assertSame(function.extractAggregationResult(holder), published); + publishedCopy = published.clone(); + } + } + + RoaringBitmap result = function.extractAggregationResult(holder); + RoaringBitmap expected = eagerUnion(serialized, 0, serialized.length); + assertEquals(result, expected); + assertEquals(result.getCardinality(), expected.getCardinality()); + // The repaired accumulator must serialize into a form that round-trips + assertEquals(RoaringBitmapUtils.deserialize(RoaringBitmapUtils.serialize(result)), expected); + } + + @Test + public void testAggregateGroupBySV() { + int numGroups = 4; + byte[][] serialized = serializedBitmaps(400, 50, 100_000); + int[] groupKeys = new int[serialized.length]; + for (int i = 0; i < groupKeys.length; i++) { + groupKeys[i] = i % numGroups; + } + + DistinctCountBitmapAggregationFunction function = + new DistinctCountBitmapAggregationFunction(EXPRESSION, false); + GroupByResultHolder holder = function.createGroupByResultHolder(numGroups, numGroups); + function.aggregateGroupBySV(serialized.length, groupKeys, holder, mockBlockValSetMap(serialized)); + + for (int groupKey = 0; groupKey < numGroups; groupKey++) { + RoaringBitmap expected = new RoaringBitmap(); + for (int i = groupKey; i < serialized.length; i += numGroups) { + expected.or(RoaringBitmapUtils.deserialize(serialized[i])); + } + RoaringBitmap result = function.extractGroupByResult(holder, groupKey); + assertEquals(result, expected, "group " + groupKey); + assertEquals(result.getCardinality(), expected.getCardinality(), "group " + groupKey); + } + } + + @Test + public void testAggregateGroupByMV() { + // Every row belongs to multiple groups, so the same deserialized input bitmap is unioned into one group's + // accumulator and cloned as another group's initial accumulator — verifies no aliasing between accumulators + int numGroups = 3; + byte[][] serialized = serializedBitmaps(150, 80, 150_000); + int[][] groupKeys = new int[serialized.length][]; + for (int i = 0; i < groupKeys.length; i++) { + groupKeys[i] = new int[]{i % numGroups, (i + 1) % numGroups}; + } + + DistinctCountBitmapAggregationFunction function = + new DistinctCountBitmapAggregationFunction(EXPRESSION, false); + GroupByResultHolder holder = function.createGroupByResultHolder(numGroups, numGroups); + function.aggregateGroupByMV(serialized.length, groupKeys, holder, mockBlockValSetMap(serialized)); + + for (int groupKey = 0; groupKey < numGroups; groupKey++) { + RoaringBitmap expected = new RoaringBitmap(); + for (int i = 0; i < serialized.length; i++) { + if (i % numGroups == groupKey || (i + 1) % numGroups == groupKey) { + expected.or(RoaringBitmapUtils.deserialize(serialized[i])); + } + } + RoaringBitmap result = function.extractGroupByResult(holder, groupKey); + assertEquals(result, expected, "group " + groupKey); + assertEquals(result.getCardinality(), expected.getCardinality(), "group " + groupKey); + } + } + + @Test + public void testSmallBitmapsStayCorrectBelowPromotionThreshold() { + // All containers stay small array containers: the lazy path must degrade to plain array unions + byte[][] serialized = serializedBitmaps(50, 5, 1_000); + DistinctCountBitmapAggregationFunction function = + new DistinctCountBitmapAggregationFunction(EXPRESSION, false); + AggregationResultHolder holder = function.createAggregationResultHolder(); + function.aggregate(serialized.length, holder, mockBlockValSetMap(serialized)); + + RoaringBitmap expected = eagerUnion(serialized, 0, serialized.length); + assertEquals(function.extractAggregationResult(holder), expected); + } + + @Test + public void testMergeSparseBitmapsAcrossUnsignedRange() { Review Comment: Non-blocking scope cleanup: the new merge-only tests construct ordinary `RoaringBitmap` inputs and exercise `merge()`, which remains unchanged in the final diff. The additions to `IndexedTableTest` and `AggregateOperatorTest` do likewise. Consider removing these residual tests from this PR, or feeding results produced by the changed BYTES aggregation/extraction path so they verify the new accumulator boundary. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
