yashmayya commented on code in PR #16678: URL: https://github.com/apache/pinot/pull/16678#discussion_r2594192675
########## pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/AnyValueAggregationFunction.java: ########## @@ -0,0 +1,377 @@ +/** + * 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.math.BigDecimal; +import java.nio.ByteBuffer; +import java.nio.charset.StandardCharsets; +import java.util.List; +import java.util.Map; +import org.apache.pinot.common.CustomObject; +import org.apache.pinot.common.request.context.ExpressionContext; +import org.apache.pinot.common.utils.DataSchema.ColumnDataType; +import org.apache.pinot.core.common.BlockValSet; +import org.apache.pinot.core.query.aggregation.AggregationResultHolder; +import org.apache.pinot.core.query.aggregation.ObjectAggregationResultHolder; +import org.apache.pinot.core.query.aggregation.groupby.GroupByResultHolder; +import org.apache.pinot.core.query.aggregation.groupby.ObjectGroupByResultHolder; +import org.apache.pinot.segment.spi.AggregationFunctionType; +import org.apache.pinot.segment.spi.index.reader.Dictionary; +import org.apache.pinot.spi.data.FieldSpec; + + +/** + * AnyValue aggregation function returns any arbitrary NON-NULL value from the column for each group. + * <p> + * This is useful for GROUP BY queries where you want to include a column in SELECT that has a 1:1 mapping with the + * GROUP BY columns, avoiding the need to add it to GROUP BY clause. The implementation is null-aware and will scan + * only until it finds the first non-null value in the current batch for each group/key. This makes it O(n) over the + * input once per group until the first value is set, with early-exit fast paths when there are no nulls. + * </p> + * <p><strong>Example:</strong></p> + * <pre>{@code + * SELECT CustomerID, + * ANY_VALUE(CustomerName), + * SUM(OrderValue) + * FROM Orders + * GROUP BY CustomerID + * }</pre> + */ +public class AnyValueAggregationFunction extends NullableSingleInputAggregationFunction<Object, Comparable<?>> { + // Result type is determined at runtime based on input expression type + private ColumnDataType _resultType; + + public AnyValueAggregationFunction(List<ExpressionContext> arguments, boolean nullHandlingEnabled) { + super(verifySingleArgument(arguments, "ANY_VALUE"), nullHandlingEnabled); + _resultType = null; + } + + @Override + public AggregationFunctionType getType() { + return AggregationFunctionType.ANYVALUE; + } + + @Override + public AggregationResultHolder createAggregationResultHolder() { + return new ObjectAggregationResultHolder(); + } + + @Override + public GroupByResultHolder createGroupByResultHolder(int initialCapacity, int maxCapacity) { + return new ObjectGroupByResultHolder(initialCapacity, maxCapacity); + } + + @Override + public ColumnDataType getIntermediateResultColumnType() { + // Default to STRING if result type is not yet determined + // TODO: See if UNKNOWN can be used instead + return _resultType != null ? _resultType : ColumnDataType.STRING; + } + + @Override + public ColumnDataType getFinalResultColumnType() { + return _resultType != null ? _resultType : ColumnDataType.STRING; + } Review Comment: I'm not sure it's a good idea to have these types change based on whether the operator has processed any blocks yet or not. This could lead to unexpected issues either now or in the future. I'm wondering if it'd be better to implement one function per type and rewrite `ANY_VALUE` to the correct type-specific function at compile time in the broker (see https://github.com/apache/pinot/pull/16980 for instance) instead. cc - @Jackie-Jiang ########## pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/OfflineClusterIntegrationTest.java: ########## @@ -4268,4 +4268,177 @@ private void runQueryAndAssert(String query, String newAddedColumn, FieldSpec fi } Assert.assertTrue(columnPresent, "Column " + newAddedColumn + " not present in result set"); } + + @Test(dataProvider = "useBothQueryEngines") + public void testAdaptiveRegexpLike(boolean useMultiStageQueryEngine) Review Comment: Unrelated tests? ########## pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/AggregationFunctionType.java: ########## @@ -61,6 +61,7 @@ public enum AggregationFunctionType { SUMPRECISION("sumPrecision", ReturnTypes.explicit(SqlTypeName.DECIMAL), OperandTypes.ANY, SqlTypeName.OTHER), AVG("avg", SqlTypeName.OTHER, SqlTypeName.DOUBLE), MODE("mode", SqlTypeName.OTHER, SqlTypeName.DOUBLE), + ANYVALUE("anyValue", ReturnTypes.ARG0, OperandTypes.ANY, SqlTypeName.OTHER), Review Comment: It should work with MSE as is, and the integration test added here should be configured to run against both the engines IMO. ########## pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/OfflineClusterIntegrationTest.java: ########## @@ -4268,4 +4268,177 @@ private void runQueryAndAssert(String query, String newAddedColumn, FieldSpec fi } Assert.assertTrue(columnPresent, "Column " + newAddedColumn + " not present in result set"); } + + @Test(dataProvider = "useBothQueryEngines") + public void testAdaptiveRegexpLike(boolean useMultiStageQueryEngine) + throws Exception { + setUseMultiStageQueryEngine(useMultiStageQueryEngine); + testRegexpLikeSmallDictionary(); + testRegexpLikeLargeDictionary(); + testRegexpLikeWithCaseSensitivity(); + testRegexpLikeConfigurableThreshold(); + } + + private void testRegexpLikeSmallDictionary() + throws Exception { + JsonNode response = postQuery("SELECT COUNT(*) FROM mytable WHERE REGEXP_LIKE(Origin, '^[A-C].*')"); + int matchCount = response.get("resultTable").get("rows").get(0).get(0).asInt(); + assertTrue(matchCount > 0, "Should find matches for origins starting with A-C"); + + int entriesScanned = response.get("numEntriesScannedInFilter").asInt(); + assertTrue(entriesScanned < 1000, "Dictionary-based evaluation should scan few entries"); + + response = postQuery("SELECT COUNT(*) FROM mytable WHERE REGEXP_LIKE(Origin, '^ZZZ.*')"); + assertEquals(response.get("resultTable").get("rows").get(0).get(0).asInt(), 0); + entriesScanned = response.get("numEntriesScannedInFilter").asInt(); + assertEquals(entriesScanned, 0, "Non-matching pattern should scan 0 entries"); + } + + private void testRegexpLikeLargeDictionary() + throws Exception { + JsonNode response = postQuery("SELECT COUNT(*) FROM mytable WHERE REGEXP_LIKE(TailNum, '^N[0-9]{3}.*')"); + int matchCount = response.get("resultTable").get("rows").get(0).get(0).asInt(); + assertTrue(matchCount >= 0, "Should handle tail number pattern matching"); + + int entriesScanned = response.get("numEntriesScannedInFilter").asInt(); + assertTrue(entriesScanned >= matchCount, "Scan-based evaluation should scan at least as many entries as matches"); + + response = postQuery("SELECT COUNT(*) FROM mytable WHERE REGEXP_LIKE(TailNum, '^N[1-5][0-9][0-9][A-Z]{2}$')"); + matchCount = response.get("resultTable").get("rows").get(0).get(0).asInt(); + assertTrue(matchCount >= 0, "Should handle complex tail number pattern"); + } + + private void testRegexpLikeWithCaseSensitivity() + throws Exception { + JsonNode response = postQuery("SELECT COUNT(*) FROM mytable WHERE REGEXP_LIKE(Origin, '.*')"); + int totalDocs = (int) getCountStarResult(); + assertEquals(response.get("resultTable").get("rows").get(0).get(0).asInt(), totalDocs); + int entriesScanned = response.get("numEntriesScannedInFilter").asInt(); + assertTrue(entriesScanned <= totalDocs, "Universal match should be optimized"); + + response = postQuery("SELECT COUNT(*) FROM mytable WHERE REGEXP_LIKE(Origin, 'NONEXISTENT_PATTERN_XYZ123')"); + assertEquals(response.get("resultTable").get("rows").get(0).get(0).asInt(), 0); + entriesScanned = response.get("numEntriesScannedInFilter").asInt(); + assertTrue(entriesScanned <= 100, "Non-matching pattern should scan minimal entries"); + + response = postQuery("SELECT COUNT(*) FROM mytable WHERE REGEXP_LIKE(Origin, '^lax$')"); + int caseSensitiveCount = response.get("resultTable").get("rows").get(0).get(0).asInt(); + + response = postQuery("SELECT COUNT(*) FROM mytable WHERE REGEXP_LIKE(Origin, '^lax$', 'i')"); + int caseInsensitiveCount = response.get("resultTable").get("rows").get(0).get(0).asInt(); + + assertTrue(caseInsensitiveCount >= caseSensitiveCount, "Case-insensitive should match at least as many"); + } + + private void testRegexpLikeConfigurableThreshold() + throws Exception { + String queryLowThreshold = "SELECT COUNT(*) FROM mytable WHERE REGEXP_LIKE(Origin, '^[A-C].*') " + + "OPTION(regexpLikeAdaptiveThreshold=0.01)"; + JsonNode response = postQuery(queryLowThreshold); + int matchCount = response.get("resultTable").get("rows").get(0).get(0).asInt(); + assertTrue(matchCount > 0, "Should find matches with low threshold"); + + String queryHighThreshold = "SELECT COUNT(*) FROM mytable WHERE REGEXP_LIKE(Origin, '^[A-C].*') " + + "OPTION(regexpLikeAdaptiveThreshold=0.99)"; + response = postQuery(queryHighThreshold); + int matchCountHighThreshold = response.get("resultTable").get("rows").get(0).get(0).asInt(); + assertEquals(matchCountHighThreshold, matchCount, "Results should be same regardless of threshold"); + + try { + String queryInvalidThreshold = "SELECT COUNT(*) FROM mytable WHERE REGEXP_LIKE(Origin, '^[A-C].*') " + + "OPTION(regexpLikeAdaptiveThreshold=1.5)"; + response = postQuery(queryInvalidThreshold); + assertTrue(response.has("resultTable") || response.has("exceptions"), + "Should handle invalid threshold gracefully"); + } catch (Exception e) { + assertTrue(e.getMessage().contains("threshold") || e.getMessage().contains("1.5"), + "Error should mention threshold"); + } + + String queryZeroThreshold = "SELECT COUNT(*) FROM mytable WHERE REGEXP_LIKE(Origin, '^[A-C].*') " + + "OPTION(regexpLikeAdaptiveThreshold=0.0)"; + response = postQuery(queryZeroThreshold); + int matchCountZeroThreshold = response.get("resultTable").get("rows").get(0).get(0).asInt(); + assertEquals(matchCountZeroThreshold, matchCount, "Results should be same with zero threshold"); + + String queryMaxThreshold = "SELECT COUNT(*) FROM mytable WHERE REGEXP_LIKE(Origin, '^[A-C].*') " + + "OPTION(regexpLikeAdaptiveThreshold=1.0)"; + response = postQuery(queryMaxThreshold); + int matchCountMaxThreshold = response.get("resultTable").get("rows").get(0).get(0).asInt(); + assertEquals(matchCountMaxThreshold, matchCount, "Results should be same with max threshold"); + } + + @Test + public void testAnyValueFunctionality() throws Exception { Review Comment: We should configure this test to run with both query engines (SSE / MSE). -- 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]
