-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix sql syntax error user #16583
Fix sql syntax error user #16583
Changes from 1 commit
ccdbc74
3173bb9
abe1092
5d73047
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -380,13 +380,7 @@ public static DruidException translateException(Exception e) | |
} | ||
} | ||
|
||
return DruidException.forPersona(DruidException.Persona.DEVELOPER) | ||
.ofCategory(DruidException.Category.UNCATEGORIZED) | ||
.build( | ||
inner, | ||
"Unable to parse the SQL, unrecognized error from calcite: [%s]", | ||
inner.getMessage() | ||
); | ||
return InvalidSqlInput.exception(inner.getMessage()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this exception creation clips the stacktrace - it only retains the source exceptions message |
||
} | ||
catch (RelOptPlanner.CannotPlanException inner) { | ||
return DruidException.forPersona(DruidException.Persona.USER) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -684,16 +684,7 @@ private DruidException buildSQLPlanningError(RelOptPlanner.CannotPlanException e | |
.ofCategory(DruidException.Category.UNSUPPORTED) | ||
.build(exception, "Unhandled Query Planning Failure, see broker logs for details"); | ||
} else { | ||
// Planning errors are more like hints: it isn't guaranteed that the planning error is actually what went wrong. | ||
// For this reason, we consider these as targetting a more expert persona, i.e. the admin instead of the actual | ||
// user. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The removed comment provides the reason why it was not target to the if there is a particular set of exceptions which should be hardened; maybe those should be targeted directly There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I reverted this part of the change, thanks for taking a look! |
||
throw DruidException.forPersona(DruidException.Persona.ADMIN) | ||
.ofCategory(DruidException.Category.INVALID_INPUT) | ||
.build( | ||
exception, | ||
"Query could not be planned. A possible reason is [%s]", | ||
errorMessage | ||
); | ||
throw InvalidSqlInput.exception("Query could not be planned. A possible reason is [%s]", errorMessage); | ||
} | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes sense to me since this is still a case of
SqlParseException
.@zachjsh, do you mind adding a test with a bad SQL that would cause the exception to be targeted towards a user?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added.