From 989a8f787442beae8e8a3dfc83583313c198d1d1 Mon Sep 17 00:00:00 2001 From: Abhishek Agarwal <1477457+abhishekagarwal87@users.noreply.github.com> Date: Sat, 27 Jan 2024 11:22:39 +0530 Subject: [PATCH] Better error message for date_trunc operators (#15759) IAEs are not bubbled up and show up as a runtime failure to the user which are not helpful. See https://apachedruidworkspace.slack.com/archives/C0303FDCZEZ/p1706185796975109 for one such example. This change will fix that. --- .../apache/druid/error/InvalidSqlInput.java | 6 +++ .../builtin/DateTruncOperatorConversion.java | 13 ++++--- .../sql/calcite/planner/QueryHandler.java | 14 +++++-- .../planner/UnsupportedSQLQueryException.java | 38 ------------------- .../druid/sql/calcite/CalciteQueryTest.java | 24 ++++++++++++ .../druid/sql/http/SqlResourceTest.java | 5 +-- 6 files changed, 50 insertions(+), 50 deletions(-) delete mode 100644 sql/src/main/java/org/apache/druid/sql/calcite/planner/UnsupportedSQLQueryException.java diff --git a/processing/src/main/java/org/apache/druid/error/InvalidSqlInput.java b/processing/src/main/java/org/apache/druid/error/InvalidSqlInput.java index 17a392962f9..c8d073b4119 100644 --- a/processing/src/main/java/org/apache/druid/error/InvalidSqlInput.java +++ b/processing/src/main/java/org/apache/druid/error/InvalidSqlInput.java @@ -19,6 +19,12 @@ package org.apache.druid.error; +/** + * This exception class should be used instead of + * {@link org.apache.druid.java.util.common.ISE} or {@link org.apache.druid.java.util.common.IAE} when processing is + * to be halted during planning. There is wiring in place to bubble up the error message to the user when wrapped + * in this exception class. + */ public class InvalidSqlInput extends InvalidInput { public static DruidException exception(String msg, Object... args) diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/DateTruncOperatorConversion.java b/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/DateTruncOperatorConversion.java index 1fa5e8791d1..37f64cd225c 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/DateTruncOperatorConversion.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/DateTruncOperatorConversion.java @@ -27,7 +27,7 @@ import org.apache.calcite.sql.SqlFunctionCategory; import org.apache.calcite.sql.SqlOperator; import org.apache.calcite.sql.type.SqlTypeFamily; import org.apache.calcite.sql.type.SqlTypeName; -import org.apache.druid.java.util.common.IAE; +import org.apache.druid.error.InvalidSqlInput; import org.apache.druid.java.util.common.StringUtils; import org.apache.druid.math.expr.Expr; import org.apache.druid.segment.column.RowSignature; @@ -67,6 +67,7 @@ public class DateTruncOperatorConversion implements SqlOperatorConversion .operatorBuilder("DATE_TRUNC") .operandTypes(SqlTypeFamily.CHARACTER, SqlTypeFamily.TIMESTAMP) .requiredOperandCount(2) + .literalOperands(0) .returnTypeCascadeNullable(SqlTypeName.TIMESTAMP) .functionCategory(SqlFunctionCategory.TIMEDATE) .build(); @@ -92,15 +93,15 @@ public class DateTruncOperatorConversion implements SqlOperatorConversion final DruidExpression arg = inputExpressions.get(1); final Expr truncTypeExpr = plannerContext.parseExpression(inputExpressions.get(0).getExpression()); - if (!truncTypeExpr.isLiteral()) { - throw new IAE("Operator[%s] truncType must be a literal", calciteOperator().getName()); - } - final String truncType = (String) truncTypeExpr.getLiteralValue(); final Period truncPeriod = TRUNC_PERIOD_MAP.get(StringUtils.toLowerCase(truncType)); if (truncPeriod == null) { - throw new IAE("Operator[%s] cannot truncate to[%s]", calciteOperator().getName(), truncType); + throw InvalidSqlInput.exception( + "Operator[%s] cannot truncate to[%s]", + calciteOperator().getName(), + truncType + ); } return DruidExpression.ofFunctionCall( diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/planner/QueryHandler.java b/sql/src/main/java/org/apache/druid/sql/calcite/planner/QueryHandler.java index 357ec61c6cd..0a570efa32f 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/planner/QueryHandler.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/planner/QueryHandler.java @@ -239,6 +239,17 @@ public abstract class QueryHandler extends SqlStatementHandler.BaseStatementHand if (de != null) { throw de; } + + // Exceptions during rule evaluations could be wrapped inside a RuntimeException by VolcanoRuleCall class. + // This block will extract a user-friendly message from the exception chain. + if (e.getMessage() != null + && e.getCause() != null + && e.getCause().getMessage() != null + && e.getMessage().startsWith("Error while applying rule")) { + throw DruidException.forPersona(DruidException.Persona.ADMIN) + .ofCategory(DruidException.Category.UNCATEGORIZED) + .build(e, "%s", e.getCause().getMessage()); + } throw DruidPlanner.translateException(e); } catch (Exception e) { @@ -681,9 +692,6 @@ public abstract class QueryHandler extends SqlStatementHandler.BaseStatementHand private DruidException buildSQLPlanningError(RelOptPlanner.CannotPlanException exception) { String errorMessage = handlerContext.plannerContext().getPlanningError(); - if (null == errorMessage && exception instanceof UnsupportedSQLQueryException) { - errorMessage = exception.getMessage(); - } if (errorMessage == null) { throw DruidException.forPersona(DruidException.Persona.OPERATOR) .ofCategory(DruidException.Category.UNSUPPORTED) diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/planner/UnsupportedSQLQueryException.java b/sql/src/main/java/org/apache/druid/sql/calcite/planner/UnsupportedSQLQueryException.java deleted file mode 100644 index 8c4576f1fab..00000000000 --- a/sql/src/main/java/org/apache/druid/sql/calcite/planner/UnsupportedSQLQueryException.java +++ /dev/null @@ -1,38 +0,0 @@ -/* - * 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.sql.calcite.planner; - -import org.apache.calcite.plan.RelOptPlanner; -import org.apache.druid.java.util.common.StringUtils; - -/** - * This class is different from {@link RelOptPlanner.CannotPlanException} in that the error - * messages are user-friendly unlike its parent class. This exception class should be used instead of - * {@link org.apache.druid.java.util.common.ISE} or {@link org.apache.druid.java.util.common.IAE} when processing is - * to be halted during planning. Similarly, Druid planner can catch this exception and know that the error - * can be directly exposed to end-users. - */ -public class UnsupportedSQLQueryException extends RelOptPlanner.CannotPlanException -{ - public UnsupportedSQLQueryException(String formatText, Object... arguments) - { - super(StringUtils.nonStrictFormat(formatText, arguments)); - } -} diff --git a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java index c5e26ca56fc..b2ba5b96ae3 100644 --- a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java +++ b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java @@ -14759,6 +14759,30 @@ public class CalciteQueryTest extends BaseCalciteQueryTest ) ); } + @Test + public void testGroupByDateTrunc() + { + testQuery( + "select DATE_TRUNC('HOUR', __time), COUNT(*) from druid.foo group by DATE_TRUNC('HOUR', __time)", + ImmutableList.of( + Druids.newTimeseriesQueryBuilder() + .dataSource(CalciteTests.DATASOURCE1) + .intervals(querySegmentSpec(Filtration.eternity())) + .granularity(Granularities.HOUR) + .aggregators(aggregators(new CountAggregatorFactory("a0"))) + .context(QUERY_CONTEXT_DEFAULT) + .build() + ), + ImmutableList.of( + new Object[]{946684800000L, 1L}, + new Object[]{946771200000L, 1L}, + new Object[]{946857600000L, 1L}, + new Object[]{978307200000L, 1L}, + new Object[]{978393600000L, 1L}, + new Object[]{978480000000L, 1L} + ) + ); + } @Test public void testLatestByOnStringColumnWithoutMaxBytesSpecified() diff --git a/sql/src/test/java/org/apache/druid/sql/http/SqlResourceTest.java b/sql/src/test/java/org/apache/druid/sql/http/SqlResourceTest.java index aca833540b2..921be219b2a 100644 --- a/sql/src/test/java/org/apache/druid/sql/http/SqlResourceTest.java +++ b/sql/src/test/java/org/apache/druid/sql/http/SqlResourceTest.java @@ -98,7 +98,6 @@ import org.apache.druid.sql.calcite.planner.PlannerConfig; import org.apache.druid.sql.calcite.planner.PlannerContext; import org.apache.druid.sql.calcite.planner.PlannerFactory; import org.apache.druid.sql.calcite.planner.PlannerResult; -import org.apache.druid.sql.calcite.planner.UnsupportedSQLQueryException; import org.apache.druid.sql.calcite.run.NativeSqlEngine; import org.apache.druid.sql.calcite.schema.DruidSchemaCatalog; import org.apache.druid.sql.calcite.util.CalciteTestBase; @@ -1399,12 +1398,12 @@ public class SqlResourceTest extends CalciteTestBase } /** - * This test is for {@link UnsupportedSQLQueryException} exceptions that are thrown by druid rules during query + * This test is for {@link org.apache.druid.error.InvalidSqlInput} exceptions that are thrown by druid rules during query * planning. e.g. doing max aggregation on string type. The test checks that the API returns correct error messages * for such planning errors. */ @Test - public void testCannotConvert_UnsupportedSQLQueryException() throws Exception + public void testCannotConvert_InvalidSQL() throws Exception { // max(string) unsupported ErrorResponse errorResponse = postSyncForException(