This is an automated email from the ASF dual-hosted git repository. jackie pushed a commit to branch handle_non_standard_sql in repository https://gitbox.apache.org/repos/asf/pinot.git
commit 741712158c2eb3ab4296aba4fb613b8cde743858 Author: Xiaotian (Jackie) Jiang <jackie....@gmail.com> AuthorDate: Wed Nov 8 18:20:27 2023 -0800 Handle legacy query and log error --- .../pinot/sql/parsers/rewriter/AliasApplier.java | 28 ++++++++++++++++++ .../pinot/sql/parsers/CalciteSqlCompilerTest.java | 30 -------------------- .../transform/function/CaseTransformFunction.java | 19 ++++++++++--- .../function/CaseTransformFunctionTest.java | 33 ---------------------- .../tests/OfflineClusterIntegrationTest.java | 17 ----------- 5 files changed, 43 insertions(+), 84 deletions(-) diff --git a/pinot-common/src/main/java/org/apache/pinot/sql/parsers/rewriter/AliasApplier.java b/pinot-common/src/main/java/org/apache/pinot/sql/parsers/rewriter/AliasApplier.java index de8d06f338..729b9df374 100644 --- a/pinot-common/src/main/java/org/apache/pinot/sql/parsers/rewriter/AliasApplier.java +++ b/pinot-common/src/main/java/org/apache/pinot/sql/parsers/rewriter/AliasApplier.java @@ -26,9 +26,12 @@ import org.apache.pinot.common.request.Function; import org.apache.pinot.common.request.Identifier; import org.apache.pinot.common.request.PinotQuery; import org.apache.pinot.sql.parsers.SqlCompilationException; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; public class AliasApplier implements QueryRewriter { + private static final Logger LOGGER = LoggerFactory.getLogger(AliasApplier.class); @Override public PinotQuery rewrite(PinotQuery pinotQuery) { @@ -53,6 +56,10 @@ public class AliasApplier implements QueryRewriter { } private static void applyAlias(Map<String, Expression> aliasMap, PinotQuery pinotQuery) { + Expression filterExpression = pinotQuery.getFilterExpression(); + if (filterExpression != null) { + applyAliasForFilter(aliasMap, filterExpression); + } List<Expression> groupByList = pinotQuery.getGroupByList(); if (groupByList != null) { for (Expression expression : groupByList) { @@ -90,4 +97,25 @@ public class AliasApplier implements QueryRewriter { } } } + + private static void applyAliasForFilter(Map<String, Expression> aliasMap, Expression expression) { + Identifier identifier = expression.getIdentifier(); + if (identifier != null) { + Expression aliasExpression = aliasMap.get(identifier.getName()); + if (aliasExpression != null) { + LOGGER.error("LEGACY QUERY: Cannot apply alias for filter: {}", expression); + expression.setType(aliasExpression.getType()); + expression.setIdentifier(aliasExpression.getIdentifier()); + expression.setFunctionCall(aliasExpression.getFunctionCall()); + expression.setLiteral(aliasExpression.getLiteral()); + } + return; + } + Function function = expression.getFunctionCall(); + if (function != null) { + for (Expression operand : function.getOperands()) { + applyAliasForFilter(aliasMap, operand); + } + } + } } diff --git a/pinot-common/src/test/java/org/apache/pinot/sql/parsers/CalciteSqlCompilerTest.java b/pinot-common/src/test/java/org/apache/pinot/sql/parsers/CalciteSqlCompilerTest.java index 3576c19660..ef49ade2a8 100644 --- a/pinot-common/src/test/java/org/apache/pinot/sql/parsers/CalciteSqlCompilerTest.java +++ b/pinot-common/src/test/java/org/apache/pinot/sql/parsers/CalciteSqlCompilerTest.java @@ -1467,27 +1467,6 @@ public class CalciteSqlCompilerTest { pinotQuery.getOrderByList().get(1).getFunctionCall().getOperands().get(0).getFunctionCall().getOperands().get(0) .getIdentifier().getName(), "rsvp_count"); - sql = "select secondsSinceEpoch/86400 AS daysSinceEpoch, sum(rsvp_count) as sum_rsvp_count, count(*) as cnt" - + " from meetupRsvp where daysSinceEpoch = 18523 group by daysSinceEpoch order by cnt, sum_rsvp_count DESC" - + " limit 50"; - pinotQuery = compileToPinotQuery(sql); - Assert.assertEquals(pinotQuery.getSelectListSize(), 3); - // Alias should not be applied to filter - Assert.assertEquals(pinotQuery.getFilterExpression().getFunctionCall().getOperator(), FilterKind.EQUALS.name()); - Assert.assertEquals( - pinotQuery.getFilterExpression().getFunctionCall().getOperands().get(0).getIdentifier().getName(), - "daysSinceEpoch"); - Assert.assertEquals( - pinotQuery.getFilterExpression().getFunctionCall().getOperands().get(1).getLiteral().getIntValue(), 18523); - Assert.assertEquals(pinotQuery.getGroupByListSize(), 1); - Assert.assertEquals(pinotQuery.getGroupByList().get(0).getFunctionCall().getOperator(), "divide"); - Assert.assertEquals( - pinotQuery.getGroupByList().get(0).getFunctionCall().getOperands().get(0).getIdentifier().getName(), - "secondsSinceEpoch"); - Assert.assertEquals( - pinotQuery.getGroupByList().get(0).getFunctionCall().getOperands().get(1).getLiteral().getIntValue(), 86400); - Assert.assertEquals(pinotQuery.getOrderByListSize(), 2); - // Invalid groupBy clause shouldn't contain aggregate expression, like sum(rsvp_count), count(*). try { sql = "select sum(rsvp_count), count(*) as cnt from meetupRsvp group by group_country, cnt limit 50"; @@ -1535,15 +1514,6 @@ public class CalciteSqlCompilerTest { Assert.assertEquals(pinotQuery.getSelectList().get(1).getIdentifier().getName(), "C2"); } - @Test - public void testAliasInFilter() { - // Alias should not be applied - String sql = "SELECT C1 AS ALIAS_CI FROM Foo WHERE ALIAS_CI > 10"; - PinotQuery pinotQuery = compileToPinotQuery(sql); - Assert.assertEquals( - pinotQuery.getFilterExpression().getFunctionCall().getOperands().get(0).getIdentifier().getName(), "ALIAS_CI"); - } - @Test public void testColumnOverride() { String sql = "SELECT C1 + 1 AS C1, COUNT(*) AS cnt FROM Foo GROUP BY 1"; diff --git a/pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/CaseTransformFunction.java b/pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/CaseTransformFunction.java index 0d008c212e..318a1a394a 100644 --- a/pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/CaseTransformFunction.java +++ b/pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/CaseTransformFunction.java @@ -19,6 +19,7 @@ package org.apache.pinot.core.operator.transform.function; import com.google.common.base.Preconditions; +import com.google.common.util.concurrent.RateLimiter; import java.math.BigDecimal; import java.util.ArrayList; import java.util.Arrays; @@ -37,6 +38,8 @@ import org.apache.pinot.spi.utils.BytesUtils; import org.apache.pinot.spi.utils.CommonConstants.NullValuePlaceHolder; import org.apache.pinot.spi.utils.TimestampUtils; import org.roaringbitmap.RoaringBitmap; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; /** @@ -59,6 +62,9 @@ import org.roaringbitmap.RoaringBitmap; * PostgreSQL documentation: <a href="https://www.postgresql.org/docs/current/typeconv-union-case.html">CASE</a> */ public class CaseTransformFunction extends ComputeDifferentlyWhenNullHandlingEnabledTransformFunction { + private static final Logger LOGGER = LoggerFactory.getLogger(CaseTransformFunction.class); + private static final RateLimiter LOG_RATE_LIMITER = RateLimiter.create(1.0); + public static final String FUNCTION_NAME = "case"; private List<TransformFunction> _whenStatements = new ArrayList<>(); @@ -147,10 +153,15 @@ public class CaseTransformFunction extends ComputeDifferentlyWhenNullHandlingEna checkLiteral(currentType, ((LiteralTransformFunction) newFunction).getStringLiteral()); } else { // Only allow upcast from numeric to numeric: INT -> LONG -> FLOAT -> DOUBLE -> BIG_DECIMAL - Preconditions.checkArgument(currentType.isNumeric() && newType.isNumeric(), "Cannot upcast from %s to %s", - currentType, newType); - if (newType.ordinal() > currentType.ordinal()) { - currentTypeAndUnresolvedLiterals.setLeft(newType); + if (currentType.isNumeric() && newType.isNumeric()) { + if (newType.ordinal() > currentType.ordinal()) { + currentTypeAndUnresolvedLiterals.setLeft(newType); + } + } else { + if (LOG_RATE_LIMITER.tryAcquire()) { + LOGGER.error("LEGACY QUERY: Cannot upcast from {} to {}", currentType, newType); + } + currentTypeAndUnresolvedLiterals.setLeft(DataType.STRING); } } } diff --git a/pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/CaseTransformFunctionTest.java b/pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/CaseTransformFunctionTest.java index 315b53e9e0..90dfd455ee 100644 --- a/pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/CaseTransformFunctionTest.java +++ b/pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/CaseTransformFunctionTest.java @@ -145,39 +145,6 @@ public class CaseTransformFunctionTest extends BaseTransformFunctionTest { } } - @DataProvider - public static String[] illegalExpressions() { - //@formatter:off - return new String[] { - // '10.0' cannot be parsed as INT/LONG/TIMESTAMP/BYTES - String.format("CASE WHEN true THEN %s ELSE '10.0' END", INT_SV_COLUMN), - String.format("CASE WHEN true THEN %s ELSE '10.0' END", LONG_SV_COLUMN), - String.format("CASE WHEN true THEN %s ELSE '10.0' END", TIMESTAMP_COLUMN), - String.format("CASE WHEN true THEN %s ELSE '10.0' END", BYTES_SV_COLUMN), - // 'abc' cannot be parsed as any type other than STRING - String.format("CASE WHEN true THEN %s ELSE 'abc' END", INT_SV_COLUMN), - String.format("CASE WHEN true THEN %s ELSE 'abc' END", LONG_SV_COLUMN), - String.format("CASE WHEN true THEN %s ELSE 'abc' END", FLOAT_SV_COLUMN), - String.format("CASE WHEN true THEN %s ELSE 'abc' END", DOUBLE_SV_COLUMN), - String.format("CASE WHEN true THEN %s ELSE 'abc' END", BIG_DECIMAL_SV_COLUMN), - String.format("CASE WHEN true THEN %s ELSE 'abc' END", TIMESTAMP_COLUMN), - String.format("CASE WHEN true THEN %s ELSE 'abc' END", BYTES_SV_COLUMN), - // Cannot mix 2 types that are not both numeric - String.format("CASE WHEN true THEN %s ELSE %s END", INT_SV_COLUMN, TIMESTAMP_COLUMN), - String.format("CASE WHEN true THEN %s ELSE %s END", INT_SV_COLUMN, STRING_SV_COLUMN), - String.format("CASE WHEN true THEN %s ELSE %s END", INT_SV_COLUMN, BYTES_SV_COLUMN), - String.format("CASE WHEN true THEN 100 ELSE %s END", TIMESTAMP_COLUMN), - String.format("CASE WHEN true THEN 100 ELSE %s END", STRING_SV_COLUMN), - String.format("CASE WHEN true THEN 100 ELSE %s END", BYTES_SV_COLUMN) - }; - //@formatter:on - } - - @Test(dataProvider = "illegalExpressions", expectedExceptions = Exception.class) - public void testInvalidCaseTransformFunction(String expression) { - TransformFunctionFactory.get(RequestContextUtils.getExpression(expression), _dataSourceMap); - } - @Test public void testCaseTransformationWithNullColumn() { ExpressionContext expression = RequestContextUtils.getExpression( diff --git a/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/OfflineClusterIntegrationTest.java b/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/OfflineClusterIntegrationTest.java index c6018cacc1..2307264ca4 100644 --- a/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/OfflineClusterIntegrationTest.java +++ b/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/OfflineClusterIntegrationTest.java @@ -2418,13 +2418,6 @@ public class OfflineClusterIntegrationTest extends BaseClusterIntegrationTestSet public void testQueryWithAlias(boolean useMultiStageQueryEngine) throws Exception { setUseMultiStageQueryEngine(useMultiStageQueryEngine); - { - String pinotQuery = "SELECT count(*), DaysSinceEpoch as d FROM mytable WHERE d = 16138 GROUP BY d"; - JsonNode jsonNode = postQuery(pinotQuery); - JsonNode exceptions = jsonNode.get("exceptions"); - assertFalse(exceptions.isEmpty()); - assertEquals(exceptions.get(0).get("errorCode").asInt(), 710); - } { //test same alias name with column name String query = "SELECT ArrTime AS ArrTime, Carrier AS Carrier, DaysSinceEpoch AS DaysSinceEpoch FROM mytable " @@ -2449,16 +2442,6 @@ public class OfflineClusterIntegrationTest extends BaseClusterIntegrationTestSet query = "SELECT count(*) AS cnt, Carrier AS CarrierName FROM mytable GROUP BY CarrierName ORDER BY cnt"; testQuery(query); - - // Test: 1. Alias should not be applied to filter; 2. Ordinal can be properly applied - query = - "SELECT DaysSinceEpoch + 100 AS DaysSinceEpoch, COUNT(*) AS cnt FROM mytable WHERE DaysSinceEpoch <= 16312 " - + "GROUP BY 1 ORDER BY 1 DESC"; - // NOTE: H2 does not support ordinal in GROUP BY - String h2Query = - "SELECT DaysSinceEpoch + 100 AS DaysSinceEpoch, COUNT(*) AS cnt FROM mytable WHERE DaysSinceEpoch <= 16312 " - + "GROUP BY DaysSinceEpoch ORDER BY 1 DESC"; - testQuery(query, h2Query); } { //test multiple alias --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org