You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2021/12/07 13:29:17 UTC
[GitHub] [druid] abhishekagarwal87 opened a new pull request #12033: Return 400 when SQL query cannot be planned
abhishekagarwal87 opened a new pull request #12033:
URL: https://github.com/apache/druid/pull/12033
This is a follow-up to https://github.com/apache/druid/pull/11911.
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. I have added tests in SqlResourceTest to catch any regression.
This PR has:
- [ ] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] added documentation for new or modified features or behaviors.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/dev/license.md)
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [ ] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [ ] been tested in a test Druid cluster.
--
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
[GitHub] [druid] abhishekagarwal87 commented on a change in pull request #12033: Return 400 when SQL query cannot be planned
Posted by 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
[GitHub] [druid] abhishekagarwal87 commented on a change in pull request #12033: Return 400 when SQL query cannot be planned
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on a change in pull request #12033:
URL: https://github.com/apache/druid/pull/12033#discussion_r764614500
##########
File path: sql/src/test/java/org/apache/druid/sql/calcite/CalciteJoinQueryTest.java
##########
@@ -3122,23 +3121,8 @@ public void testInnerJoinOnTwoInlineDataSources_withLeftDirectAccess(Map<String,
@Parameters(source = QueryContextForJoinProvider.class)
public void testJoinOnConstantShouldFail(Map<String, Object> queryContext) throws Exception
{
- 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 have unsupported operand types.");
Review comment:
agreed. will fix that
--
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
[GitHub] [druid] clintropolis commented on a change in pull request #12033: Return 400 when SQL query cannot be planned
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #12033:
URL: https://github.com/apache/druid/pull/12033#discussion_r764601254
##########
File path: sql/src/test/java/org/apache/druid/sql/calcite/CalciteJoinQueryTest.java
##########
@@ -3122,23 +3121,8 @@ public void testInnerJoinOnTwoInlineDataSources_withLeftDirectAccess(Map<String,
@Parameters(source = QueryContextForJoinProvider.class)
public void testJoinOnConstantShouldFail(Map<String, Object> queryContext) throws Exception
{
- 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 have unsupported operand types.");
Review comment:
nit: i know this isn't new, but this error message should maybe be "... in a join that _has_ unsupported operand ..." or "SQL has join conditions that have unsupported operand types." or something
##########
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 had to think a bit on this because I was a bit torn. One the one hand, it's not necessarily the users fault that we don't support some SQL syntax they might have tried so a server error seems fair in that sense, but on the other hand being a client error might inspire the user to try to rewrite their query where a server error might not, so I've come around to being :+1: on this
--
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
[GitHub] [druid] abhishekagarwal87 merged pull request #12033: Return 400 when SQL query cannot be planned
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 merged pull request #12033:
URL: https://github.com/apache/druid/pull/12033
--
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
[GitHub] [druid] abhishekagarwal87 commented on pull request #12033: Return 400 when SQL query cannot be planned
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on pull request #12033:
URL: https://github.com/apache/druid/pull/12033#issuecomment-988113099
no worries @capistrant. Thank you for thumbs up :)
--
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