abhishekagarwal87 commented on code in PR #14004:
URL: https://github.com/apache/druid/pull/14004#discussion_r1231791313
##########
processing/src/main/java/org/apache/druid/common/exception/DruidException.java:
##########
@@ -22,6 +22,7 @@
/**
* A generic exception thrown by Druid.
*/
+@Deprecated
Review Comment:
can you also add a note that it's deprecated in favor of the other
DruidException?
##########
extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/tuple/sql/ArrayOfDoublesSketchSqlAggregatorTest.java:
##########
@@ -429,24 +435,30 @@ public void testNullInputs()
@Test
public void testArrayOfDoublesSketchIntersectOnScalarExpression()
{
- assertQueryIsUnplannable("SELECT DS_TUPLE_DOUBLES_INTERSECT(NULL, NULL)
FROM foo",
- "Possible error: DS_TUPLE_DOUBLES_INTERSECT can only be used on
aggregates. " +
- "It cannot be used directly on a column or on a scalar
expression.");
+ assertQueryIsUnplannable(
+ "SELECT DS_TUPLE_DOUBLES_INTERSECT(NULL, NULL) FROM foo",
+ "DS_TUPLE_DOUBLES_INTERSECT can only be used on aggregates. " +
Review Comment:
btw we had "Possible error" as a prefix in query planning errors because
they are in a way best-effort guesses. During planning phase, we record
instances of the planner trying to do something unsupported. and if there is
nothing else to show, we just return that instance as an error. But its
possible that planner didn't really need to go down that path or it tried to
same very fancy planning and we end up surfacing that error instead.
##########
extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/tuple/sql/ArrayOfDoublesSketchSqlAggregatorTest.java:
##########
@@ -429,24 +435,30 @@ public void testNullInputs()
@Test
public void testArrayOfDoublesSketchIntersectOnScalarExpression()
{
- assertQueryIsUnplannable("SELECT DS_TUPLE_DOUBLES_INTERSECT(NULL, NULL)
FROM foo",
- "Possible error: DS_TUPLE_DOUBLES_INTERSECT can only be used on
aggregates. " +
- "It cannot be used directly on a column or on a scalar
expression.");
+ assertQueryIsUnplannable(
+ "SELECT DS_TUPLE_DOUBLES_INTERSECT(NULL, NULL) FROM foo",
+ "DS_TUPLE_DOUBLES_INTERSECT can only be used on aggregates. " +
Review Comment:
ok. I think that this got replaced with "Query planning failed for unknown
reason, our best guess is this"
##########
extensions-core/multi-stage-query/src/test/java/org/apache/druid/msq/exec/MSQInsertTest.java:
##########
@@ -958,14 +955,14 @@ public void testInsertWrongTypeTimestamp()
@Test
public void testIncorrectInsertQuery()
{
- testIngestQuery().setSql(
- "insert into foo1 select __time, dim1 , count(*) as
cnt from foo where dim1 is not null group by 1, 2 clustered by dim1")
- .setExpectedValidationErrorMatcher(CoreMatchers.allOf(
- CoreMatchers.instanceOf(SqlPlanningException.class),
-
ThrowableMessageMatcher.hasMessage(CoreMatchers.startsWith(
- "CLUSTERED BY found before PARTITIONED BY. In
Druid, the CLUSTERED BY clause must follow the PARTITIONED BY clause"))
- ))
- .verifyPlanningErrors();
+ testIngestQuery()
+ .setSql(
+ "insert into foo1 select __time, dim1 , count(*) as cnt from foo
where dim1 is not null group by 1, 2 clustered by dim1"
+ )
+ .setExpectedValidationErrorMatcher(invalidSqlContains(
+ "LUSTERED BY found before PARTITIONED BY, CLUSTERED BY must come
after the PARTITIONED BY clause"
Review Comment:
```suggestion
"CLUSTERED BY found before PARTITIONED BY, CLUSTERED BY must
come after the PARTITIONED BY clause"
```
##########
extensions-core/multi-stage-query/src/test/java/org/apache/druid/msq/exec/MSQInsertTest.java:
##########
@@ -1079,7 +1074,7 @@ public void
testInsertLimitWithPeriodGranularityThrowsException()
+ "LIMIT 50 "
+ "PARTITIONED BY MONTH")
.setExpectedValidationErrorMatcher(CoreMatchers.allOf(
- CoreMatchers.instanceOf(SqlPlanningException.class),
+ CoreMatchers.instanceOf(DruidException.class),
ThrowableMessageMatcher.hasMessage(CoreMatchers.containsString(
"INSERT and REPLACE queries cannot have a LIMIT
unless PARTITIONED BY is \"ALL\""))
Review Comment:
could these be simplified into invalidSqlContains as well? It's find if that
refactoring was a bit too much and you gave up on it after a few :P
##########
extensions-core/multi-stage-query/src/test/java/org/apache/druid/msq/exec/MSQSelectTest.java:
##########
@@ -1673,7 +1659,7 @@ public void
testGroupByWithComplexColumnThrowsUnsupportedException()
.setSql("select unique_dim1 from foo2 group by unique_dim1")
.setQueryContext(context)
.setExpectedExecutionErrorMatcher(CoreMatchers.allOf(
- CoreMatchers.instanceOf(UnsupportedSQLQueryException.class),
+ CoreMatchers.instanceOf(DruidException.class),
Review Comment:
As we use one exception class to rule them all, were there places where
there was exception-class-specific handling? I am wondering how you dealt with
those. Or maybe I will find out myself as I move below.
##########
extensions-core/multi-stage-query/src/test/java/org/apache/druid/msq/exec/MSQSelectTest.java:
##########
@@ -1641,8 +1622,13 @@ public void testTimeColumnAggregationFromExtern() throws
IOException
+ "FROM kttm_data "
+ "GROUP BY 1")
.setExpectedValidationErrorMatcher(
- ThrowableMessageMatcher.hasMessage(CoreMatchers.containsString(
- "LATEST() aggregator depends on __time column"))
+ new DruidExceptionMatcher(DruidException.Persona.ADMIN,
DruidException.Category.INVALID_INPUT, "adhoc")
+ .expectMessageIs(
+ "Query planning failed for unknown reason, our best guess
is this "
Review Comment:
How about "Query planning failed for unknown reason, most likely the reason
is this"
##########
processing/src/main/java/org/apache/druid/error/QueryExceptionCompat.java:
##########
@@ -0,0 +1,72 @@
+/*
+ * 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.error;
+
+import org.apache.druid.query.QueryException;
+
+public class QueryExceptionCompat extends DruidException.Failure
Review Comment:
this class could use some javadocs.
--
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]
