From 5ca1d97bced079055f26acb40c80eb5c511930cf Mon Sep 17 00:00:00 2001 From: Xiang Fu Date: Tue, 11 Jun 2024 08:50:53 -0700 Subject: [PATCH] Fix [Type]ArrayList elements() method usage (#13354) --- .../pinot/common/utils/ArrayListUtils.java | 139 +++++++++++++++++ .../apache/pinot/common/utils/DataSchema.java | 10 +- .../common/utils/ArrayListUtilsTest.java | 146 ++++++++++++++++++ .../results/AggregationResultsBlock.java | 11 +- .../blocks/results/GroupByResultsBlock.java | 11 +- .../HistogramAggregationFunction.java | 7 +- .../runtime/operator/utils/TypeUtils.java | 11 +- 7 files changed, 309 insertions(+), 26 deletions(-) create mode 100644 pinot-common/src/main/java/org/apache/pinot/common/utils/ArrayListUtils.java create mode 100644 pinot-common/src/test/java/org/apache/pinot/common/utils/ArrayListUtilsTest.java diff --git a/pinot-common/src/main/java/org/apache/pinot/common/utils/ArrayListUtils.java b/pinot-common/src/main/java/org/apache/pinot/common/utils/ArrayListUtils.java new file mode 100644 index 000000000000..f74f32655593 --- /dev/null +++ b/pinot-common/src/main/java/org/apache/pinot/common/utils/ArrayListUtils.java @@ -0,0 +1,139 @@ +/** + * 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 it.unimi.dsi.fastutil.doubles.DoubleArrayList; +import it.unimi.dsi.fastutil.floats.FloatArrayList; +import it.unimi.dsi.fastutil.ints.IntArrayList; +import it.unimi.dsi.fastutil.longs.LongArrayList; +import it.unimi.dsi.fastutil.objects.ObjectArrayList; + + +/** + * Utility class for {@link IntArrayList}, {@link LongArrayList}, {@link FloatArrayList}, {@link DoubleArrayList}, + * {@link ObjectArrayList}. + */ +public class ArrayListUtils { + private ArrayListUtils() { + } + + /** + * Best effort extract the given {@link IntArrayList} to an int array without copying the elements. + * The {@link IntArrayList#elements()} returned int array may be longer than the actual size of the + * {@link IntArrayList}, and the actual size of the {@link IntArrayList} can be retrieved using + * {@link IntArrayList#size()}. + * This method checks the length of the returned int array and returns the same if it is equal to the size of the + * {@link IntArrayList}, otherwise, it copies the elements to a new int array and returns it. + * + *

Use this method only if you are sure that the returned int array will not be modified. + *

Otherwise, use {@link IntArrayList#toIntArray()}. + * + * @param intArrayList Input {@link IntArrayList} + * @return Best effort extracted int array without copying the elements + */ + public static int[] toIntArray(IntArrayList intArrayList) { + int[] intArrayListElements = intArrayList.elements(); + return intArrayListElements.length == intArrayList.size() ? intArrayListElements : intArrayList.toIntArray(); + } + + /** + * Best effort extract the given {@link LongArrayList} to a long array without copying the elements. + * The {@link LongArrayList#elements()} returned long array may be longer than the actual size of the + * {@link LongArrayList}, and the actual size of the {@link LongArrayList} can be retrieved using + * {@link LongArrayList#size()}. + * This method checks the length of the returned long array and returns the same if it is equal to the size of the + * {@link LongArrayList}, otherwise, it copies the elements to a new long array and returns it. + * + *

Use this method only if you are sure that the returned long array will not be modified. + *

Otherwise, use {@link LongArrayList#toLongArray()}. + * + * @param longArrayList Input {@link LongArrayList} + * @return Best effort extracted long array without copying the elements + */ + public static long[] toLongArray(LongArrayList longArrayList) { + long[] longArrayListElements = longArrayList.elements(); + return longArrayListElements.length == longArrayList.size() ? longArrayListElements : longArrayList.toLongArray(); + } + + /** + * Best effort extract the given {@link FloatArrayList} to a float array without copying the elements. + * The {@link FloatArrayList#elements()} returned float array may be longer than the actual size of the + * {@link FloatArrayList}, and the actual size of the {@link FloatArrayList} can be retrieved using + * {@link FloatArrayList#size()}. + * This method checks the length of the returned float array and returns the same if it is equal to the size of the + * {@link FloatArrayList}, otherwise, it copies the elements to a new float array and returns it. + * + *

Use this method only if you are sure that the returned float array will not be modified. + *

Otherwise, use {@link FloatArrayList#toFloatArray()}. + * + * @param floatArrayList Input {@link FloatArrayList} + * @return Best effort extracted float array without copying the elements + */ + public static float[] toFloatArray(FloatArrayList floatArrayList) { + float[] floatArrayListElements = floatArrayList.elements(); + return floatArrayListElements.length == floatArrayList.size() ? floatArrayListElements + : floatArrayList.toFloatArray(); + } + + /** + * Best effort extract the given {@link DoubleArrayList} to a double array without copying the elements. + * The {@link DoubleArrayList#elements()} returned double array may be longer than the actual size of the + * {@link DoubleArrayList}, and the actual size of the {@link DoubleArrayList} can be retrieved using + * {@link DoubleArrayList#size()}. + * This method checks the length of the returned double array and returns the same if it is equal to the size of the + * {@link DoubleArrayList}, otherwise, it copies the elements to a new double array and returns it. + * + *

Use this method only if you are sure that the returned double array will not be modified. + *

Otherwise, use {@link DoubleArrayList#toDoubleArray()}. + * + * @param doubleArrayList Input {@link DoubleArrayList} + * @return Best effort extracted double array without copying the elements + */ + public static double[] toDoubleArray(DoubleArrayList doubleArrayList) { + double[] doubleArrayListElements = doubleArrayList.elements(); + return doubleArrayListElements.length == doubleArrayList.size() ? doubleArrayListElements + : doubleArrayList.toDoubleArray(); + } + + /** + * Convert the given {@link ObjectArrayList} to a string array. + * The method {@link ObjectArrayList#elements()} could return either Object[] or String[]. The casting to String[] + * is not guaranteed to work, and it may throw {@link ClassCastException} if the internal object is not a String + * array. + *

+ * This method first get `elements` as Object, then check if it's instance of String[]. + * Only return the reference when the internal object is a String array and the length equals to ObjectArrayList + * size. + * For all the other scenarios, just copy the elements to a new string array and returns it. + *

+ * + * @param stringArrayList Input {@link ObjectArrayList} + * @return Copied string array + */ + public static String[] toStringArray(ObjectArrayList stringArrayList) { + Object elements = stringArrayList.elements(); + if (elements instanceof String[]) { + String[] stringArrayListElements = (String[]) elements; + if (stringArrayListElements.length == stringArrayList.size()) { + return stringArrayListElements; + } + } + return stringArrayList.toArray(new String[0]); + } +} diff --git a/pinot-common/src/main/java/org/apache/pinot/common/utils/DataSchema.java b/pinot-common/src/main/java/org/apache/pinot/common/utils/DataSchema.java index 9bd8dacef109..f6c1bc398ccd 100644 --- a/pinot-common/src/main/java/org/apache/pinot/common/utils/DataSchema.java +++ b/pinot-common/src/main/java/org/apache/pinot/common/utils/DataSchema.java @@ -523,7 +523,7 @@ private static int[] toIntArray(Object value) { return (int[]) value; } else if (value instanceof IntArrayList) { // For ArrayAggregationFunction - return ((IntArrayList) value).elements(); + return ArrayListUtils.toIntArray((IntArrayList) value); } throw new IllegalStateException(String.format("Cannot convert: '%s' to int[]", value)); } @@ -533,7 +533,7 @@ private static float[] toFloatArray(Object value) { return (float[]) value; } else if (value instanceof FloatArrayList) { // For ArrayAggregationFunction - return ((FloatArrayList) value).elements(); + return ArrayListUtils.toFloatArray((FloatArrayList) value); } throw new IllegalStateException(String.format("Cannot convert: '%s' to float[]", value)); } @@ -543,7 +543,7 @@ private static double[] toDoubleArray(Object value) { return (double[]) value; } else if (value instanceof DoubleArrayList) { // For HistogramAggregationFunction and ArrayAggregationFunction - return ((DoubleArrayList) value).elements(); + return ArrayListUtils.toDoubleArray((DoubleArrayList) value); } else if (value instanceof int[]) { int[] intValues = (int[]) value; int length = intValues.length; @@ -576,7 +576,7 @@ private static long[] toLongArray(Object value) { return (long[]) value; } else if (value instanceof LongArrayList) { // For FunnelCountAggregationFunction and ArrayAggregationFunction - return ((LongArrayList) value).elements(); + return ArrayListUtils.toLongArray((LongArrayList) value); } else { int[] intValues = (int[]) value; int length = intValues.length; @@ -593,7 +593,7 @@ private static String[] toStringArray(Object value) { return (String[]) value; } else if (value instanceof ObjectArrayList) { // For ArrayAggregationFunction - return ((ObjectArrayList) value).toArray(new String[0]); + return ArrayListUtils.toStringArray((ObjectArrayList) value); } throw new IllegalStateException(String.format("Cannot convert: '%s' to String[]", value)); } diff --git a/pinot-common/src/test/java/org/apache/pinot/common/utils/ArrayListUtilsTest.java b/pinot-common/src/test/java/org/apache/pinot/common/utils/ArrayListUtilsTest.java new file mode 100644 index 000000000000..815114e899fa --- /dev/null +++ b/pinot-common/src/test/java/org/apache/pinot/common/utils/ArrayListUtilsTest.java @@ -0,0 +1,146 @@ +/** + * 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 it.unimi.dsi.fastutil.doubles.DoubleArrayList; +import it.unimi.dsi.fastutil.floats.FloatArrayList; +import it.unimi.dsi.fastutil.ints.IntArrayList; +import it.unimi.dsi.fastutil.longs.LongArrayList; +import it.unimi.dsi.fastutil.objects.ObjectArrayList; +import org.testng.annotations.Test; + +import static org.testng.Assert.assertEquals; + + +public class ArrayListUtilsTest { + @Test + public void testToIntArray() { + // Test empty list + IntArrayList intArrayList = new IntArrayList(); + int[] intArray = ArrayListUtils.toIntArray(intArrayList); + assertEquals(intArray.length, 0); + + // Test list with one element + intArrayList.add(1); + intArray = ArrayListUtils.toIntArray(intArrayList); + assertEquals(intArray.length, 1); + assertEquals(intArray[0], 1); + + // Test list with multiple elements + intArrayList.add(2); + intArrayList.add(3); + intArray = ArrayListUtils.toIntArray(intArrayList); + assertEquals(intArray.length, 3); + assertEquals(intArray[0], 1); + assertEquals(intArray[1], 2); + assertEquals(intArray[2], 3); + } + + @Test + public void testToLongArray() { + // Test empty list + LongArrayList longArrayList = new LongArrayList(); + long[] longArray = ArrayListUtils.toLongArray(longArrayList); + assertEquals(longArray.length, 0); + + // Test list with one element + longArrayList.add(1L); + longArray = ArrayListUtils.toLongArray(longArrayList); + assertEquals(longArray.length, 1); + assertEquals(longArray[0], 1L); + + // Test list with multiple elements + longArrayList.add(2L); + longArrayList.add(3L); + longArray = ArrayListUtils.toLongArray(longArrayList); + assertEquals(longArray.length, 3); + assertEquals(longArray[0], 1L); + assertEquals(longArray[1], 2L); + assertEquals(longArray[2], 3L); + } + + @Test + public void testToFloatArray() { + // Test empty list + FloatArrayList floatArrayList = new FloatArrayList(); + float[] floatArray = ArrayListUtils.toFloatArray(floatArrayList); + assertEquals(floatArray.length, 0); + + // Test list with one element + floatArrayList.add(1.0f); + floatArray = ArrayListUtils.toFloatArray(floatArrayList); + assertEquals(floatArray.length, 1); + assertEquals(floatArray[0], 1.0f); + + // Test list with multiple elements + floatArrayList.add(2.0f); + floatArrayList.add(3.0f); + floatArray = ArrayListUtils.toFloatArray(floatArrayList); + assertEquals(floatArray.length, 3); + assertEquals(floatArray[0], 1.0f); + assertEquals(floatArray[1], 2.0f); + assertEquals(floatArray[2], 3.0f); + } + + @Test + public void testToDoubleArray() { + // Test empty list + DoubleArrayList doubleArrayList = new DoubleArrayList(); + double[] doubleArray = ArrayListUtils.toDoubleArray(doubleArrayList); + assertEquals(doubleArray.length, 0); + + // Test list with one element + doubleArrayList.add(1.0); + doubleArray = ArrayListUtils.toDoubleArray(doubleArrayList); + assertEquals(doubleArray.length, 1); + assertEquals(doubleArray[0], 1.0); + + // Test list with multiple elements + doubleArrayList.add(2.0); + doubleArrayList.add(3.0); + doubleArray = ArrayListUtils.toDoubleArray(doubleArrayList); + assertEquals(doubleArray.length, 3); + assertEquals(doubleArray[0], 1.0); + assertEquals(doubleArray[1], 2.0); + assertEquals(doubleArray[2], 3.0); + } + + @Test + public void testToStringArray() { + // Test empty list + ObjectArrayList stringArrayList = new ObjectArrayList(); + String[] stringArray = ArrayListUtils.toStringArray(stringArrayList); + assertEquals(stringArray.length, 0); + + // Test list with one element + stringArrayList.add("1"); + stringArray = ArrayListUtils.toStringArray(stringArrayList); + assertEquals(stringArray.length, 1); + assertEquals(stringArray[0], "1"); + + // Test list with multiple elements + stringArrayList.add("2"); + stringArrayList.add("3"); + stringArray = ArrayListUtils.toStringArray(stringArrayList); + assertEquals(stringArray.length, 3); + assertEquals(stringArray[0], "1"); + assertEquals(stringArray[1], "2"); + assertEquals(stringArray[2], "3"); + } +} diff --git a/pinot-core/src/main/java/org/apache/pinot/core/operator/blocks/results/AggregationResultsBlock.java b/pinot-core/src/main/java/org/apache/pinot/core/operator/blocks/results/AggregationResultsBlock.java index 8c3e025af305..4e0a1b306311 100644 --- a/pinot-core/src/main/java/org/apache/pinot/core/operator/blocks/results/AggregationResultsBlock.java +++ b/pinot-core/src/main/java/org/apache/pinot/core/operator/blocks/results/AggregationResultsBlock.java @@ -30,6 +30,7 @@ import org.apache.commons.lang3.tuple.Pair; import org.apache.pinot.common.datatable.DataTable; import org.apache.pinot.common.request.context.FilterContext; +import org.apache.pinot.common.utils.ArrayListUtils; import org.apache.pinot.common.utils.DataSchema; import org.apache.pinot.common.utils.DataSchema.ColumnDataType; import org.apache.pinot.core.common.datatable.DataTableBuilder; @@ -195,19 +196,19 @@ private void setFinalResult(DataTableBuilder dataTableBuilder, ColumnDataType[] dataTableBuilder.setColumn(index, (ByteArray) result); break; case INT_ARRAY: - dataTableBuilder.setColumn(index, ((IntArrayList) result).elements()); + dataTableBuilder.setColumn(index, ArrayListUtils.toIntArray((IntArrayList) result)); break; case LONG_ARRAY: - dataTableBuilder.setColumn(index, ((LongArrayList) result).elements()); + dataTableBuilder.setColumn(index, ArrayListUtils.toLongArray((LongArrayList) result)); break; case FLOAT_ARRAY: - dataTableBuilder.setColumn(index, ((FloatArrayList) result).elements()); + dataTableBuilder.setColumn(index, ArrayListUtils.toFloatArray((FloatArrayList) result)); break; case DOUBLE_ARRAY: - dataTableBuilder.setColumn(index, ((DoubleArrayList) result).elements()); + dataTableBuilder.setColumn(index, ArrayListUtils.toDoubleArray((DoubleArrayList) result)); break; case STRING_ARRAY: - dataTableBuilder.setColumn(index, ((ObjectArrayList) result).elements()); + dataTableBuilder.setColumn(index, ArrayListUtils.toStringArray((ObjectArrayList) result)); break; default: throw new IllegalStateException("Illegal column data type in final result: " + columnDataType); diff --git a/pinot-core/src/main/java/org/apache/pinot/core/operator/blocks/results/GroupByResultsBlock.java b/pinot-core/src/main/java/org/apache/pinot/core/operator/blocks/results/GroupByResultsBlock.java index 81f46e6277c1..dfc5faa28930 100644 --- a/pinot-core/src/main/java/org/apache/pinot/core/operator/blocks/results/GroupByResultsBlock.java +++ b/pinot-core/src/main/java/org/apache/pinot/core/operator/blocks/results/GroupByResultsBlock.java @@ -33,6 +33,7 @@ import java.util.Map; import org.apache.pinot.common.datatable.DataTable; import org.apache.pinot.common.datatable.DataTable.MetadataKey; +import org.apache.pinot.common.utils.ArrayListUtils; import org.apache.pinot.common.utils.DataSchema; import org.apache.pinot.common.utils.DataSchema.ColumnDataType; import org.apache.pinot.core.common.datatable.DataTableBuilder; @@ -251,28 +252,28 @@ private void setDataTableColumn(ColumnDataType storedColumnDataType, DataTableBu break; case INT_ARRAY: if (value instanceof IntArrayList) { - dataTableBuilder.setColumn(columnIndex, ((IntArrayList) value).elements()); + dataTableBuilder.setColumn(columnIndex, ArrayListUtils.toIntArray((IntArrayList) value)); } else { dataTableBuilder.setColumn(columnIndex, (int[]) value); } break; case LONG_ARRAY: if (value instanceof LongArrayList) { - dataTableBuilder.setColumn(columnIndex, ((LongArrayList) value).elements()); + dataTableBuilder.setColumn(columnIndex, ArrayListUtils.toLongArray((LongArrayList) value)); } else { dataTableBuilder.setColumn(columnIndex, (long[]) value); } break; case FLOAT_ARRAY: if (value instanceof FloatArrayList) { - dataTableBuilder.setColumn(columnIndex, ((FloatArrayList) value).elements()); + dataTableBuilder.setColumn(columnIndex, ArrayListUtils.toFloatArray((FloatArrayList) value)); } else { dataTableBuilder.setColumn(columnIndex, (float[]) value); } break; case DOUBLE_ARRAY: if (value instanceof DoubleArrayList) { - dataTableBuilder.setColumn(columnIndex, ((DoubleArrayList) value).elements()); + dataTableBuilder.setColumn(columnIndex, ArrayListUtils.toDoubleArray((DoubleArrayList) value)); } else { dataTableBuilder.setColumn(columnIndex, (double[]) value); } @@ -280,7 +281,7 @@ private void setDataTableColumn(ColumnDataType storedColumnDataType, DataTableBu case STRING_ARRAY: if (value instanceof ObjectArrayList) { //noinspection unchecked - dataTableBuilder.setColumn(columnIndex, ((ObjectArrayList) value).elements()); + dataTableBuilder.setColumn(columnIndex, ArrayListUtils.toStringArray((ObjectArrayList) value)); } else { dataTableBuilder.setColumn(columnIndex, (String[]) value); } diff --git a/pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/HistogramAggregationFunction.java b/pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/HistogramAggregationFunction.java index 77f5363fb8b2..f72e42903cad 100644 --- a/pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/HistogramAggregationFunction.java +++ b/pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/HistogramAggregationFunction.java @@ -225,12 +225,7 @@ public ColumnDataType getFinalResultColumnType() { @Override public DoubleArrayList extractFinalResult(DoubleArrayList doubleArrayList) { - int count = doubleArrayList.size(); - if (count < 1L) { - throw new IllegalStateException("histogram result shouldn't be empty!"); - } else { - return new DoubleArrayList(doubleArrayList.elements()); - } + return doubleArrayList; } @Override diff --git a/pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/utils/TypeUtils.java b/pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/utils/TypeUtils.java index 933c3be95f05..80841b85549c 100644 --- a/pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/utils/TypeUtils.java +++ b/pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/utils/TypeUtils.java @@ -23,6 +23,7 @@ import it.unimi.dsi.fastutil.ints.IntArrayList; import it.unimi.dsi.fastutil.longs.LongArrayList; import it.unimi.dsi.fastutil.objects.ObjectArrayList; +import org.apache.pinot.common.utils.ArrayListUtils; import org.apache.pinot.common.utils.DataSchema.ColumnDataType; @@ -51,21 +52,21 @@ public static Object convert(Object value, ColumnDataType storedType) { case INT_ARRAY: if (value instanceof IntArrayList) { // For ArrayAggregationFunction - return ((IntArrayList) value).elements(); + return ArrayListUtils.toIntArray((IntArrayList) value); } else { return value; } case LONG_ARRAY: if (value instanceof LongArrayList) { // For FunnelCountAggregationFunction and ArrayAggregationFunction - return ((LongArrayList) value).elements(); + return ArrayListUtils.toLongArray((LongArrayList) value); } else { return value; } case FLOAT_ARRAY: if (value instanceof FloatArrayList) { // For ArrayAggregationFunction - return ((FloatArrayList) value).elements(); + return ArrayListUtils.toFloatArray((FloatArrayList) value); } else if (value instanceof double[]) { // This is due to for parsing array literal value like [0.1, 0.2, 0.3]. // The parsed value is stored as double[] in java, however the calcite type is FLOAT_ARRAY. @@ -80,14 +81,14 @@ public static Object convert(Object value, ColumnDataType storedType) { case DOUBLE_ARRAY: if (value instanceof DoubleArrayList) { // For HistogramAggregationFunction and ArrayAggregationFunction - return ((DoubleArrayList) value).elements(); + return ArrayListUtils.toDoubleArray((DoubleArrayList) value); } else { return value; } case STRING_ARRAY: if (value instanceof ObjectArrayList) { // For ArrayAggregationFunction - return ((ObjectArrayList) value).toArray(new String[0]); + return ArrayListUtils.toStringArray((ObjectArrayList) value); } else { return value; }