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.

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;

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<T> implements Iterator<T>
   {
     private final Iterator<T> it;

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();
       }
     }

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
       );
     }

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());
   }
 
   /**

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<String, Object> queryContext) throws Exception
+  public void testJoinOnConstantShouldFail(Map<String, Object> 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

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,27 +5398,16 @@ 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(
+    assertQueryIsUnplannable(
         "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()
+        "Possible error: Illegal TIMESTAMP constant: CAST('z2000-01-01 00:00:00'):TIMESTAMP(3) NOT NULL"
     );
   }
-    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()
-      );
-    }
-  }
 
   @Test
   public void testCountStarWithSinglePointInTime() throws Exception

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,

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());