From 7abf847eaefbab84500b7fa245106a30f10bf5c8 Mon Sep 17 00:00:00 2001 From: Abhishek Agarwal <1477457+abhishekagarwal87@users.noreply.github.com> Date: Wed, 8 Dec 2021 21:49:54 +0530 Subject: [PATCH] Return 400 when SQL query cannot be planned (#12033) In this PR, we will now return 400 instead of 500 when SQL query cannot be planned. I also fixed a bug where error messages were not getting sent to the users in case the rules throw UnsupportSQLQueryException. --- .../druid/sql/SqlPlanningException.java | 4 ++- .../sql/calcite/planner/DruidPlanner.java | 28 ++++++++++------ .../druid/sql/calcite/rule/DruidJoinRule.java | 2 +- .../apache/druid/sql/http/SqlResource.java | 18 +++++----- .../sql/calcite/BaseCalciteQueryTest.java | 5 ++- .../sql/calcite/CalciteJoinQueryTest.java | 22 ++----------- .../druid/sql/calcite/CalciteQueryTest.java | 24 ++++---------- .../druid/sql/calcite/util/CalciteTests.java | 16 --------- .../druid/sql/http/SqlResourceTest.java | 33 +++++++++++++++++-- 9 files changed, 73 insertions(+), 79 deletions(-) diff --git a/sql/src/main/java/org/apache/druid/sql/SqlPlanningException.java b/sql/src/main/java/org/apache/druid/sql/SqlPlanningException.java index 8dcdbfd15eb..a08ad53674a 100644 --- a/sql/src/main/java/org/apache/druid/sql/SqlPlanningException.java +++ b/sql/src/main/java/org/apache/druid/sql/SqlPlanningException.java @@ -21,6 +21,7 @@ package org.apache.druid.sql; import com.fasterxml.jackson.annotation.JsonCreator; import com.fasterxml.jackson.annotation.JsonProperty; +import org.apache.calcite.plan.RelOptPlanner; import org.apache.calcite.sql.parser.SqlParseException; import org.apache.calcite.tools.ValidationException; import org.apache.druid.query.BadQueryException; @@ -33,7 +34,8 @@ public class SqlPlanningException extends BadQueryException public enum PlanningError { SQL_PARSE_ERROR("SQL parse failed", SqlParseException.class.getName()), - VALIDATION_ERROR("Plan validation failed", ValidationException.class.getName()); + VALIDATION_ERROR("Plan validation failed", ValidationException.class.getName()), + UNSUPPORTED_SQL_ERROR("SQL query is unsupported", RelOptPlanner.CannotPlanException.class.getName()); private final String errorCode; private final String errorClass; diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java b/sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java index eeb4e4f8254..621653591ed 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java @@ -227,16 +227,8 @@ public class DruidPlanner implements Closeable if (!QueryContexts.isDebug(plannerContext.getQueryContext())) { logger = log.noStackTrace(); } - logger.warn(e, "Failed to plan the query '%s'", plannerContext.getSql()); - String errorMessage = plannerContext.getPlanningError(); - if (null == errorMessage && cannotPlanException instanceof UnsupportedSQLQueryException) { - errorMessage = cannotPlanException.getMessage(); - } - if (null == errorMessage) { - errorMessage = "Please check broker logs for more details"; - } else { - errorMessage = "Possible error: " + errorMessage; - } + String errorMessage = buildSQLPlanningErrorMessage(cannotPlanException); + logger.warn(e, errorMessage); throw new UnsupportedSQLQueryException(errorMessage); } } @@ -692,6 +684,22 @@ public class DruidPlanner implements Closeable } } + private String buildSQLPlanningErrorMessage(Throwable exception) + { + String errorMessage = plannerContext.getPlanningError(); + if (null == errorMessage && exception instanceof UnsupportedSQLQueryException) { + errorMessage = exception.getMessage(); + } + if (null == errorMessage) { + errorMessage = "Please check broker logs for more details"; + } else { + // Re-phrase since planning errors are more like hints + errorMessage = "Possible error: " + errorMessage; + } + // Finally, add the query itself to error message that user will get. + return StringUtils.format("Cannot build plan for query: %s. %s", plannerContext.getSql(), errorMessage); + } + private static class EnumeratorIterator implements Iterator { private final Iterator it; diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/rule/DruidJoinRule.java b/sql/src/main/java/org/apache/druid/sql/calcite/rule/DruidJoinRule.java index 8844b00d57e..9441c8bcf7e 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/rule/DruidJoinRule.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/rule/DruidJoinRule.java @@ -284,7 +284,7 @@ public class DruidJoinRule extends RelOptRule rightColumns.add((RexInputRef) operands.get(0)); } else { // Cannot handle this condition. - plannerContext.setPlanningError("SQL is resulting in a join that have unsupported operand types."); + plannerContext.setPlanningError("SQL is resulting in a join that has unsupported operand types."); return Optional.empty(); } } diff --git a/sql/src/main/java/org/apache/druid/sql/http/SqlResource.java b/sql/src/main/java/org/apache/druid/sql/http/SqlResource.java index 4e239c94a62..8445a4053c4 100644 --- a/sql/src/main/java/org/apache/druid/sql/http/SqlResource.java +++ b/sql/src/main/java/org/apache/druid/sql/http/SqlResource.java @@ -27,7 +27,6 @@ import com.google.inject.Inject; import org.apache.calcite.plan.RelOptPlanner; import org.apache.druid.common.exception.SanitizableException; import org.apache.druid.guice.annotations.Json; -import org.apache.druid.java.util.common.ISE; import org.apache.druid.java.util.common.StringUtils; import org.apache.druid.java.util.common.guava.Sequence; import org.apache.druid.java.util.common.guava.Yielder; @@ -64,6 +63,7 @@ import javax.ws.rs.core.MediaType; import javax.ws.rs.core.Response; import javax.ws.rs.core.Response.Status; import javax.ws.rs.core.StreamingOutput; + import java.io.IOException; import java.util.List; import java.util.Set; @@ -205,22 +205,20 @@ public class SqlResource throw (ForbiddenException) serverConfig.getErrorResponseTransformStrategy() .transformIfNeeded(e); // let ForbiddenExceptionMapper handle this } + catch (RelOptPlanner.CannotPlanException e) { + endLifecycle(sqlQueryId, lifecycle, e, remoteAddr, -1); + SqlPlanningException spe = new SqlPlanningException(SqlPlanningException.PlanningError.UNSUPPORTED_SQL_ERROR, + e.getMessage()); + return buildNonOkResponse(BadQueryException.STATUS_CODE, spe, sqlQueryId); + } // calcite throws a java.lang.AssertionError which is type error not exception. using throwable will catch all catch (Throwable e) { log.warn(e, "Failed to handle query: %s", sqlQuery); endLifecycle(sqlQueryId, lifecycle, e, remoteAddr, -1); - final Throwable exceptionToReport; - - if (e instanceof RelOptPlanner.CannotPlanException) { - exceptionToReport = new ISE("Cannot build plan for query: %s. %s", sqlQuery.getQuery(), e.getMessage()); - } else { - exceptionToReport = e; - } - return buildNonOkResponse( Status.INTERNAL_SERVER_ERROR.getStatusCode(), - QueryInterruptedException.wrapIfNeeded(exceptionToReport), + QueryInterruptedException.wrapIfNeeded(e), sqlQueryId ); } diff --git a/sql/src/test/java/org/apache/druid/sql/calcite/BaseCalciteQueryTest.java b/sql/src/test/java/org/apache/druid/sql/calcite/BaseCalciteQueryTest.java index bd821ae4d94..5adaa127892 100644 --- a/sql/src/test/java/org/apache/druid/sql/calcite/BaseCalciteQueryTest.java +++ b/sql/src/test/java/org/apache/druid/sql/calcite/BaseCalciteQueryTest.java @@ -109,6 +109,7 @@ import org.junit.rules.ExpectedException; import org.junit.rules.TemporaryFolder; import javax.annotation.Nullable; + import java.io.IOException; import java.util.ArrayList; import java.util.Arrays; @@ -590,7 +591,9 @@ public class BaseCalciteQueryTest extends CalciteTestBase log.error(e, "Expected CannotPlanException for query: %s", sql); Assert.fail(sql); } - Assert.assertEquals(sql, expectedError, e.getMessage()); + Assert.assertEquals(sql, + StringUtils.format("Cannot build plan for query: %s. %s", sql, expectedError), + e.getMessage()); } /** diff --git a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteJoinQueryTest.java b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteJoinQueryTest.java index ac5890dd203..7f31fc94500 100644 --- a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteJoinQueryTest.java +++ b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteJoinQueryTest.java @@ -23,7 +23,6 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import junitparams.JUnitParamsRunner; import junitparams.Parameters; -import org.apache.calcite.plan.RelOptPlanner; import org.apache.druid.common.config.NullHandling; import org.apache.druid.java.util.common.DateTimes; import org.apache.druid.java.util.common.Intervals; @@ -3120,25 +3119,10 @@ public class CalciteJoinQueryTest extends BaseCalciteQueryTest // TODO: Remove expected Exception when https://github.com/apache/druid/issues/9942 is fixed @Test @Parameters(source = QueryContextForJoinProvider.class) - public void testJoinOnConstantShouldFail(Map queryContext) throws Exception + public void testJoinOnConstantShouldFail(Map queryContext) { - cannotVectorize(); - - final String query = "SELECT t1.dim1 from foo as t1 LEFT JOIN foo as t2 on t1.dim1 = '10.1'"; - - try { - testQuery( - query, - queryContext, - ImmutableList.of(), - ImmutableList.of() - ); - } - catch (RelOptPlanner.CannotPlanException cpe) { - Assert.assertEquals(cpe.getMessage(), "Possible error: SQL is resulting in a join that have unsupported operand types."); - return; - } - Assert.fail("Expected an exception of type: " + RelOptPlanner.CannotPlanException.class); + assertQueryIsUnplannable("SELECT t1.dim1 from foo as t1 LEFT JOIN foo as t2 on t1.dim1 = '10.1'", + "Possible error: SQL is resulting in a join that has unsupported operand types."); } @Test 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 02abe1398bf..90b29128990 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 @@ -111,7 +111,6 @@ import org.apache.druid.sql.calcite.filtration.Filtration; import org.apache.druid.sql.calcite.planner.Calcites; import org.apache.druid.sql.calcite.planner.PlannerConfig; import org.apache.druid.sql.calcite.planner.PlannerContext; -import org.apache.druid.sql.calcite.planner.UnsupportedSQLQueryException; import org.apache.druid.sql.calcite.rel.CannotBuildQueryException; import org.apache.druid.sql.calcite.util.CalciteTests; import org.joda.time.DateTime; @@ -5399,26 +5398,15 @@ public class CalciteQueryTest extends BaseCalciteQueryTest } @Test - public void testCountStarWithTimeFilterUsingStringLiteralsInvalid() + public void testCountStarWithTimeFilterUsingStringLiteralsInvalid_isUnplannable() { // Strings are implicitly cast to timestamps. Test an invalid string. // This error message isn't ideal but it is at least better than silently ignoring the problem. - try { - testQuery( - "SELECT COUNT(*) FROM druid.foo\n" - + "WHERE __time >= 'z2000-01-01 00:00:00' AND __time < '2001-01-01 00:00:00'\n", - ImmutableList.of(), - ImmutableList.of() - ); - } - catch (Throwable t) { - Throwable rootException = CalciteTests.getRootCauseFromInvocationTargetExceptionChain(t); - Assert.assertEquals(UnsupportedSQLQueryException.class, rootException.getClass()); - Assert.assertEquals( - "Possible error: Illegal TIMESTAMP constant: CAST('z2000-01-01 00:00:00'):TIMESTAMP(3) NOT NULL", - rootException.getMessage() - ); - } + assertQueryIsUnplannable( + "SELECT COUNT(*) FROM druid.foo\n" + + "WHERE __time >= 'z2000-01-01 00:00:00' AND __time < '2001-01-01 00:00:00'\n", + "Possible error: Illegal TIMESTAMP constant: CAST('z2000-01-01 00:00:00'):TIMESTAMP(3) NOT NULL" + ); } @Test diff --git a/sql/src/test/java/org/apache/druid/sql/calcite/util/CalciteTests.java b/sql/src/test/java/org/apache/druid/sql/calcite/util/CalciteTests.java index 25137a47b08..a91d345c7cb 100644 --- a/sql/src/test/java/org/apache/druid/sql/calcite/util/CalciteTests.java +++ b/sql/src/test/java/org/apache/druid/sql/calcite/util/CalciteTests.java @@ -145,7 +145,6 @@ import org.joda.time.chrono.ISOChronology; import javax.annotation.Nullable; import java.io.File; -import java.lang.reflect.InvocationTargetException; import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; @@ -1214,21 +1213,6 @@ public class CalciteTests return catalog; } - /** - * Some Calcite exceptions (such as that thrown by - * {@link org.apache.druid.sql.calcite.CalciteQueryTest#testCountStarWithTimeFilterUsingStringLiteralsInvalid)}, - * are structured as a chain of RuntimeExceptions caused by InvocationTargetExceptions. To get the root exception - * it is necessary to make getTargetException calls on the InvocationTargetExceptions. - */ - public static Throwable getRootCauseFromInvocationTargetExceptionChain(Throwable t) - { - Throwable curThrowable = t; - while (curThrowable.getCause() instanceof InvocationTargetException) { - curThrowable = ((InvocationTargetException) curThrowable.getCause()).getTargetException(); - } - return curThrowable; - } - private static DruidSchema createMockSchema( final QueryRunnerFactoryConglomerate conglomerate, final SpecificSegmentsQuerySegmentWalker walker, 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 bc99f878d84..1a4aab25332 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 @@ -74,6 +74,7 @@ import org.apache.druid.sql.calcite.planner.DruidOperatorTable; 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.UnsupportedSQLQueryException; import org.apache.druid.sql.calcite.schema.DruidSchemaCatalog; import org.apache.druid.sql.calcite.util.CalciteTestBase; import org.apache.druid.sql.calcite.util.CalciteTests; @@ -1110,11 +1111,37 @@ public class SqlResourceTest extends CalciteTestBase ).lhs; Assert.assertNotNull(exception); - Assert.assertEquals(QueryInterruptedException.UNKNOWN_EXCEPTION, exception.getErrorCode()); - Assert.assertEquals(ISE.class.getName(), exception.getErrorClass()); + Assert.assertEquals(PlanningError.UNSUPPORTED_SQL_ERROR.getErrorCode(), exception.getErrorCode()); + Assert.assertEquals(PlanningError.UNSUPPORTED_SQL_ERROR.getErrorClass(), exception.getErrorClass()); Assert.assertTrue( exception.getMessage() - .contains("Cannot build plan for query: SELECT dim1 FROM druid.foo ORDER BY dim1") + .contains("Cannot build plan for query: SELECT dim1 FROM druid.foo ORDER BY dim1. " + + "Possible error: SQL query requires order by non-time column [dim1 ASC] that is not supported.") + ); + checkSqlRequestLog(false); + Assert.assertTrue(lifecycleManager.getAll("id").isEmpty()); + } + + /** + * This test is for {@link UnsupportedSQLQueryException} 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 + { + // max(string) unsupported + final QueryException exception = doPost( + createSimpleQueryWithId("id", "SELECT max(dim1) FROM druid.foo") + ).lhs; + + Assert.assertNotNull(exception); + Assert.assertEquals(PlanningError.UNSUPPORTED_SQL_ERROR.getErrorCode(), exception.getErrorCode()); + Assert.assertEquals(PlanningError.UNSUPPORTED_SQL_ERROR.getErrorClass(), exception.getErrorClass()); + Assert.assertTrue( + exception.getMessage() + .contains("Cannot build plan for query: SELECT max(dim1) FROM druid.foo. " + + "Possible error: Max aggregation is not supported for 'STRING' type") ); checkSqlRequestLog(false); Assert.assertTrue(lifecycleManager.getAll("id").isEmpty());