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/01 01:12:09 UTC
[GitHub] [druid] jihoonson commented on a change in pull request #11911: Human-readable and actionable SQL error messages
jihoonson commented on a change in pull request #11911:
URL: https://github.com/apache/druid/pull/11911#discussion_r759744852
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/planner/UnsupportedSQLQueryException.java
##########
@@ -24,14 +24,14 @@
/**
* This class is different from {@link org.apache.calcite.plan.RelOptPlanner.CannotPlanException} in that the error
- * messages are user-friendly unlike it's parent class. This exception class be used instead of
+ * messages are user-friendly unlike its parent class. This exception class should be used instead of
* {@link org.apache.druid.java.util.common.ISE} or {@link org.apache.druid.java.util.common.IAE} when processing is
- * to be halted during planning. Similarly, Druid planner can catch these exception and know that the error
- * can be directly exposed to end-user.
+ * to be halted during planning. Similarly, Druid planner can catch this exception and know that the error
+ * can be directly exposed to end-users.
*/
-public class UnsupportedQueryFeatureException extends RelOptPlanner.CannotPlanException
+public class UnsupportedSQLQueryException extends RelOptPlanner.CannotPlanException
Review comment:
:+1:
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java
##########
@@ -209,23 +212,27 @@ public PlannerResult plan() throws SqlParseException, ValidationException, RelCo
// Not a CannotPlanningException, rethrow without trying with bindable
throw e;
}
+ if (parsed.getInsertNode() != null) {
+ // Cannot INSERT with BINDABLE.
+ throw e;
+ }
// Try again with BINDABLE convention. Used for querying Values and metadata tables.
try {
- return planWithBindableConvention(explain, root);
+ return planWithBindableConvention(rootQueryRel, parsed.getExplainNode());
}
catch (Exception e2) {
e.addSuppressed(e2);
- log.noStackTrace().warn(e, "Failed to plan the query '%s'", sql);
+ log.noStackTrace().warn(e, "Failed to plan the query '%s'", plannerContext.getSql());
Review comment:
https://github.com/apache/druid/pull/11911#discussion_r755584263
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java
##########
@@ -209,23 +212,27 @@ public PlannerResult plan() throws SqlParseException, ValidationException, RelCo
// Not a CannotPlanningException, rethrow without trying with bindable
throw e;
}
+ if (parsed.getInsertNode() != null) {
+ // Cannot INSERT with BINDABLE.
+ throw e;
+ }
// Try again with BINDABLE convention. Used for querying Values and metadata tables.
try {
- return planWithBindableConvention(explain, root);
+ return planWithBindableConvention(rootQueryRel, parsed.getExplainNode());
}
catch (Exception e2) {
e.addSuppressed(e2);
- log.noStackTrace().warn(e, "Failed to plan the query '%s'", sql);
+ log.noStackTrace().warn(e, "Failed to plan the query '%s'", plannerContext.getSql());
Review comment:
Also, warning seems reasonable to me as this is not a system failure but is inconsistent with https://github.com/apache/druid/blob/master/server/src/main/java/org/apache/druid/server/QueryLifecycle.java#L323
--
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