gianm commented on code in PR #19561: URL: https://github.com/apache/druid/pull/19561#discussion_r3364404827
########## processing/src/test/java/org/apache/druid/query/aggregation/simd/SimdSumVectorAggregatorTest.java: ########## @@ -0,0 +1,328 @@ +/* + * 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.druid.query.aggregation.simd; + +import org.apache.druid.java.util.common.StringUtils; +import org.apache.druid.query.aggregation.DoubleSumVectorAggregator; +import org.apache.druid.query.aggregation.FloatSumVectorAggregator; +import org.apache.druid.query.aggregation.LongSumVectorAggregator; +import org.apache.druid.segment.vector.VectorValueSelector; +import org.apache.druid.testing.InitializedNullHandlingTest; +import org.junit.Assert; +import org.junit.Test; + +import javax.annotation.Nullable; +import java.nio.ByteBuffer; +import java.util.Random; +import java.util.function.IntPredicate; + +/** + * For each (sum type, vector size, null pattern) combination, drives the SIMD and scalar sum vector aggregators + * directly and asserts equivalent buffer state. Two paths covered: + * - the ungrouped no-null path (SIMD subclass's overridden {@code aggregate(buf, pos, start, end)} vs the scalar + * parent's implementation), and + * - the null-aware path on the SIMD class (the new {@code aggregate(buf, pos, start, end, nullVector)} declared by + * {@link org.apache.druid.query.aggregation.NullAwareVectorAggregator}), compared against a manually-computed + * reference sum. + */ +public class SimdSumVectorAggregatorTest extends InitializedNullHandlingTest +{ + private static final int[] VECTOR_SIZES = {1, 8, 17, 64, 1023}; + + @Test + public void testLongSum() + { + for (int size : VECTOR_SIZES) { + for (NullPattern pattern : NullPattern.values()) { + runLong(size, pattern); + } + } + } + + @Test + public void testDoubleSum() + { + for (int size : VECTOR_SIZES) { + for (NullPattern pattern : NullPattern.values()) { + runDouble(size, pattern); + } + } + } + + @Test + public void testFloatSum() + { + for (int size : VECTOR_SIZES) { + for (NullPattern pattern : NullPattern.values()) { + runFloat(size, pattern); + } + } + } + + private static void runLong(int size, NullPattern pattern) + { + final long[] values = randomLongs(size, 0); + final boolean[] nulls = pattern.toMask(size); + final FakeVectorValueSelector selector = new FakeVectorValueSelector(size, values, null, null, nulls); + final String msg = StringUtils.format("type[long] size[%s] nulls[%s]", size, pattern); + + final LongSumVectorAggregator scalar = new LongSumVectorAggregator(selector); + final SimdLongSumVectorAggregator simd = new SimdLongSumVectorAggregator(selector); + + if (nulls == null) { + final ByteBuffer scalarBuf = ByteBuffer.allocate(Long.BYTES); + final ByteBuffer simdBuf = ByteBuffer.allocate(Long.BYTES); + scalar.init(scalarBuf, 0); + simd.init(simdBuf, 0); + scalar.aggregate(scalarBuf, 0, 0, size); + simd.aggregate(simdBuf, 0, 0, size); + Assert.assertEquals(msg, scalarBuf.getLong(0), simdBuf.getLong(0)); + } else { + long expected = 0; + boolean anyNonNull = false; + for (int i = 0; i < size; i++) { + if (!nulls[i]) { + expected += values[i]; + anyNonNull = true; + } + } + final ByteBuffer simdBuf = ByteBuffer.allocate(Long.BYTES); + simd.init(simdBuf, 0); + final boolean reported = simd.aggregate(simdBuf, 0, 0, size, nulls); + Assert.assertEquals(msg + " (anyNonNull)", anyNonNull, reported); + if (reported) { + Assert.assertEquals(msg, expected, simdBuf.getLong(0)); + } + } + } + + private static void runDouble(int size, NullPattern pattern) + { + final double[] values = randomDoubles(size, 1); + final boolean[] nulls = pattern.toMask(size); + final FakeVectorValueSelector selector = new FakeVectorValueSelector(size, null, values, null, nulls); + final String msg = StringUtils.format("type[double] size[%s] nulls[%s]", size, pattern); + + final DoubleSumVectorAggregator scalar = new DoubleSumVectorAggregator(selector); + final SimdDoubleSumVectorAggregator simd = new SimdDoubleSumVectorAggregator(selector); + + if (nulls == null) { + final ByteBuffer scalarBuf = ByteBuffer.allocate(Double.BYTES); + final ByteBuffer simdBuf = ByteBuffer.allocate(Double.BYTES); + scalar.init(scalarBuf, 0); + simd.init(simdBuf, 0); + scalar.aggregate(scalarBuf, 0, 0, size); + simd.aggregate(simdBuf, 0, 0, size); + Assert.assertEquals( + msg, + scalarBuf.getDouble(0), + simdBuf.getDouble(0), + Math.max(Math.abs(scalarBuf.getDouble(0)) * 1e-12, 1e-12) + ); + } else { + double expected = 0; + boolean anyNonNull = false; + for (int i = 0; i < size; i++) { + if (!nulls[i]) { + expected += values[i]; + anyNonNull = true; + } + } + final ByteBuffer simdBuf = ByteBuffer.allocate(Double.BYTES); + simd.init(simdBuf, 0); + final boolean reported = simd.aggregate(simdBuf, 0, 0, size, nulls); + Assert.assertEquals(msg + " (anyNonNull)", anyNonNull, reported); + if (reported) { + Assert.assertEquals(msg, expected, simdBuf.getDouble(0), Math.max(Math.abs(expected) * 1e-12, 1e-12)); + } + } + } + + private static void runFloat(int size, NullPattern pattern) + { + final float[] values = randomFloats(size, 2); + final boolean[] nulls = pattern.toMask(size); + final FakeVectorValueSelector selector = new FakeVectorValueSelector(size, null, null, values, nulls); + final String msg = StringUtils.format("type[float] size[%s] nulls[%s]", size, pattern); + + final FloatSumVectorAggregator scalar = new FloatSumVectorAggregator(selector); + final SimdFloatSumVectorAggregator simd = new SimdFloatSumVectorAggregator(selector); + + if (nulls == null) { + final ByteBuffer scalarBuf = ByteBuffer.allocate(Float.BYTES); + final ByteBuffer simdBuf = ByteBuffer.allocate(Float.BYTES); + scalar.init(scalarBuf, 0); + simd.init(simdBuf, 0); + scalar.aggregate(scalarBuf, 0, 0, size); Review Comment: all the tests seem to use `buf, 0, 0, size` as parameters. Please improve coverage by varying the position, startRow, and endRow. It should be ok to do one call with `buf, 0, 0, size` and another with `buf, 1, 1, size + 1` (and put some junk in the first row of the `selector` that the startRow of `1` is meant to ignore). ########## benchmarks/src/test/java/org/apache/druid/benchmark/query/SqlExpressionBenchmark.java: ########## @@ -50,7 +50,7 @@ public class SqlExpressionBenchmark extends SqlBaseQueryBenchmark // non-expression reference queries // =========================== // 0: non-expression timeseries reference, 1 columns - "SELECT SUM(long1) FROM expressions", + "SELECT SUM(long5) FROM expressions", Review Comment: why this change? ########## processing/src/test/java/org/apache/druid/query/aggregation/simd/SimdSumVectorAggregatorTest.java: ########## @@ -0,0 +1,328 @@ +/* + * 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.druid.query.aggregation.simd; + +import org.apache.druid.java.util.common.StringUtils; +import org.apache.druid.query.aggregation.DoubleSumVectorAggregator; +import org.apache.druid.query.aggregation.FloatSumVectorAggregator; +import org.apache.druid.query.aggregation.LongSumVectorAggregator; +import org.apache.druid.segment.vector.VectorValueSelector; +import org.apache.druid.testing.InitializedNullHandlingTest; +import org.junit.Assert; +import org.junit.Test; + +import javax.annotation.Nullable; +import java.nio.ByteBuffer; +import java.util.Random; +import java.util.function.IntPredicate; + +/** + * For each (sum type, vector size, null pattern) combination, drives the SIMD and scalar sum vector aggregators + * directly and asserts equivalent buffer state. Two paths covered: + * - the ungrouped no-null path (SIMD subclass's overridden {@code aggregate(buf, pos, start, end)} vs the scalar + * parent's implementation), and + * - the null-aware path on the SIMD class (the new {@code aggregate(buf, pos, start, end, nullVector)} declared by + * {@link org.apache.druid.query.aggregation.NullAwareVectorAggregator}), compared against a manually-computed + * reference sum. + */ +public class SimdSumVectorAggregatorTest extends InitializedNullHandlingTest Review Comment: For test coverage: add `useVectorApi` as a parameter in some of the parameterized tests? Maybe the timeseries tests and groupBy tests. -- 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]
