[GitHub] [druid] abhishekagarwal87 commented on a change in pull request #12033: Return 400 when SQL query cannot be planned

[GitBox <gi...@apache.org>]

abhishekagarwal87 commented on a change in pull request #12033:
URL: https://github.com/apache/druid/pull/12033#discussion_r764614328



##########
File path: sql/src/main/java/org/apache/druid/sql/http/SqlResource.java
##########
@@ -205,22 +205,20 @@ public Response doPost(
       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);

Review comment:
       I think it is weird currently that the HTTP error code that the user gets depends on when can we detect that the query is bad or unsupported. I wanted the behaviour to be consistent irrespective of the stage, the query gets rejected. 
   If there is an unknown failure (NPE, ISE) during the detection path, that should be an internal server error but if we did detect it correctly that query is unplannable then we know we can't support the said query syntax. this is why 400 is returned only in the case of CannotPlanException. 
   
   Like a user trying to do max aggregation on a string or a user trying to use multiple exact distinct counts will get 500 right now even though it is documented that these features are not supported. these are indeed bad queries in that sense. Also, what matters more than the error code, is the info we are giving alongside in the response since that is more useful for the end-user. 




-- 
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: commits-unsubscribe@druid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org