This is an automated email from the ASF dual-hosted git repository.
abhishek pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/druid.git
The following commit(s) were added to refs/heads/master by this push:
new 989a8f78744 Better error message for date_trunc operators (#15759)
989a8f78744 is described below
commit 989a8f787442beae8e8a3dfc83583313c198d1d1
Author: Abhishek Agarwal <[email protected]>
AuthorDate: Sat Jan 27 11:22:39 2024 +0530
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.
---
.../org/apache/druid/error/InvalidSqlInput.java | 6 ++++
.../builtin/DateTruncOperatorConversion.java | 13 ++++----
.../druid/sql/calcite/planner/QueryHandler.java | 14 ++++++--
.../planner/UnsupportedSQLQueryException.java | 38 ----------------------
.../apache/druid/sql/calcite/CalciteQueryTest.java | 24 ++++++++++++++
.../org/apache/druid/sql/http/SqlResourceTest.java | 5 ++-
6 files changed, 50 insertions(+), 50 deletions(-)
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(
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
