You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by GitBox <gi...@apache.org> on 2021/03/27 15:51:20 UTC
[GitHub] [drill] vvysotskyi opened a new pull request #2194: DRILL-7888: query.json returns an incorrect error message when the query fails
vvysotskyi opened a new pull request #2194:
URL: https://github.com/apache/drill/pull/2194
# [DRILL-7888](https://issues.apache.org/jira/browse/DRILL-7888): query.json returns an incorrect error message when the query fails
## Description
Updated StreamingHttpConnection.java to return query id and failed state instead of the incorrect message as stated in the Jira ticket.
Examples of the requests and responses:
Request:
```
{
"query": "SELECT * FROM cp.`employee123321123321.json` LIMIT 20",
"queryType": "SQL"
}
```
Response:
```
{
"queryId": "1fa0acc2-e507-6688-69fe-02fc627c8c47",
"queryState": "FAILED"
}
```
Please note, to preserve backward compatibility, default response has such a form, so now it is possible to obtain additional error info from the query profile.
To provide error details right in the response, the `exec.errors.verbose` option should be set in the following way:
```
{
"query": "SELECT * FROM cp.`employee123321123321.json` LIMIT 20",
"queryType": "SQL",
"options": {
"exec.errors.verbose": "true"
}
}
```
The result will be the following and includes a detailed error message and exception stack trace:
```
{
"queryId": "1fa0ac2e-19e7-a453-d2b4-6e775e57b4ca",
"exception": "org.apache.calcite.runtime.CalciteContextException",
"errorMessage": "From line 1, column 15 to line 1, column 16: Object 'employee123321123321.json' not found within 'cp': Object 'employee123321123321.json' not found within 'cp'",
"stackTrace": "org.apache.calcite.runtime.CalciteContextException: From line 1, column 15 to line 1, column 16: Object 'employee123321123321.json' not found within 'cp': Object 'employee123321123321.json' not found within 'cp'\n\tat .......(:0)\n\tat org.apache.calcite.runtime.Resources$ExInstWithCause.ex(Resources.java:463)\n\tat org.apache.calcite.sql.SqlUtil.newContextException(SqlUtil.java:835)\n\tat org.apache.calcite.sql.SqlUtil.newContextException(SqlUtil.java:820)\n\tat org.apache.calcite.sql.validate.SqlValidatorImpl.newValidationError(SqlValidatorImpl.java:4881)\n\tat org.apache.calcite.sql.validate.IdentifierNamespace.resolveImpl(IdentifierNamespace.java:127)\n\tat org.apache.calcite.sql.validate.IdentifierNamespace.validateImpl(IdentifierNamespace.java:177)\n\tat org.apache.calcite.sql.validate.AbstractNamespace.validate(AbstractNamespace.java:84)\n\tat org.apache.calcite.sql.validate.SqlValidatorImpl.validateNamespace(SqlValidatorImpl.java:1009)\n\tat org.apache.c
alcite.sql.validate.SqlValidatorImpl.validateQuery(SqlValidatorImpl.java:969)\n\tat org.apache.calcite.sql.validate.SqlValidatorImpl.validateFrom(SqlValidatorImpl.java:3129)\n\tat org.apache.drill.exec.planner.sql.conversion.DrillValidator.validateFrom(DrillValidator.java:63)\n\tat org.apache.calcite.sql.validate.SqlValidatorImpl.validateFrom(SqlValidatorImpl.java:3111)\n\tat org.apache.drill.exec.planner.sql.conversion.DrillValidator.validateFrom(DrillValidator.java:63)\n\tat org.apache.calcite.sql.validate.SqlValidatorImpl.validateSelect(SqlValidatorImpl.java:3383)\n\tat org.apache.calcite.sql.validate.SelectNamespace.validateImpl(SelectNamespace.java:60)\n\tat org.apache.calcite.sql.validate.AbstractNamespace.validate(AbstractNamespace.java:84)\n\tat org.apache.calcite.sql.validate.SqlValidatorImpl.validateNamespace(SqlValidatorImpl.java:1009)\n\tat org.apache.calcite.sql.validate.SqlValidatorImpl.validateQuery(SqlValidatorImpl.java:969)\n\tat org.apache.calcite.sql.SqlSelect.val
idate(SqlSelect.java:216)\n\tat org.apache.calcite.sql.validate.SqlValidatorImpl.validateScopedExpression(SqlValidatorImpl.java:944)\n\tat org.apache.calcite.sql.validate.SqlValidatorImpl.validate(SqlValidatorImpl.java:651)\n\tat org.apache.drill.exec.planner.sql.conversion.SqlConverter.validate(SqlConverter.java:189)\n\tat org.apache.drill.exec.planner.sql.handlers.DefaultSqlHandler.validateNode(DefaultSqlHandler.java:641)\n\tat org.apache.drill.exec.planner.sql.handlers.DefaultSqlHandler.validateAndConvert(DefaultSqlHandler.java:195)\n\tat org.apache.drill.exec.planner.sql.handlers.DefaultSqlHandler.getPlan(DefaultSqlHandler.java:169)\n\tat org.apache.drill.exec.planner.sql.DrillSqlWorker.getQueryPlan(DrillSqlWorker.java:283)\n\tat org.apache.drill.exec.planner.sql.DrillSqlWorker.getPhysicalPlan(DrillSqlWorker.java:163)\n\tat org.apache.drill.exec.planner.sql.DrillSqlWorker.convertPlan(DrillSqlWorker.java:128)\n\tat org.apache.drill.exec.planner.sql.DrillSqlWorker.getPlan(DrillSql
Worker.java:93)\n\tat org.apache.drill.exec.work.foreman.Foreman.runSQL(Foreman.java:592)\n\tat org.apache.drill.exec.work.foreman.Foreman.run(Foreman.java:273)\n\tat .......(:0)\nCaused by: org.apache.calcite.sql.validate.SqlValidatorException: Object 'employee123321123321.json' not found within 'cp'\n\tat .......(:0)\n\tat org.apache.calcite.runtime.Resources$ExInstWithCause.ex(Resources.java:463)\n\tat org.apache.calcite.runtime.Resources$ExInst.ex(Resources.java:572)\n\t... 31 more\n",
"queryState": "FAILED"
}
```
## Documentation
NA
## Testing
Added unit tests and checked manually.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] luocooong commented on pull request #2194: DRILL-7888: query.json returns an incorrect error message when the query fails
Posted by GitBox <gi...@apache.org>.
luocooong commented on pull request #2194:
URL: https://github.com/apache/drill/pull/2194#issuecomment-812850694
@vvysotskyi Hi. Do we have any new revisions ready to submit? Or It's only ready to request a review again?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] paul-rogers commented on a change in pull request #2194: DRILL-7888: query.json returns an incorrect error message when the query fails
Posted by GitBox <gi...@apache.org>.
paul-rogers commented on a change in pull request #2194:
URL: https://github.com/apache/drill/pull/2194#discussion_r605407739
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java
##########
@@ -257,6 +257,7 @@
new OptionDefinition(ClassCompilerSelector.JAVA_COMPILER_JANINO_MAXSIZE),
new OptionDefinition(ClassCompilerSelector.JAVA_COMPILER_DEBUG),
new OptionDefinition(ExecConstants.ENABLE_VERBOSE_ERRORS),
+ new OptionDefinition(ExecConstants.ENABLE_REST_VERBOSE_ERRORS),
Review comment:
There is one more step: add the default value to the `exec` version of `drill-module.conf`.
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/BaseQueryRunner.java
##########
@@ -45,7 +45,7 @@
protected final WorkManager workManager;
protected final WebUserConnection webUserConnection;
- private final OptionSet options;
+ protected final OptionSet options;
Review comment:
Probably not needed. See the `applyOptions` method and below.
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/ExecConstants.java
##########
@@ -1216,6 +1216,11 @@ public static String bootDefaultFor(String name) {
"the sender to send out its data more rapidly, but you should know that it has a risk to OOM when the system is solving parallel " +
"large queries until we have a more accurate resource manager."));
+
+ public static final String ENABLE_REST_VERBOSE_ERRORS_KEY = "rest.errors.verbose";
Review comment:
The name here should be consistent with other similar keys. For example:
```
public static final String HTTP_ENABLE = "drill.exec.http.enabled";
```
So, maybe, `drill.exec.http.rest.errors.verbose`. Kind of ugly.
We had talked about reworking the keys now that we know what they all are. But, until that is done as a unified task, best to maintain the existing naming conventions so we don't make things more of a mess than they already are.
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/stream/QueryRunner.java
##########
@@ -48,7 +48,7 @@ public void start(QueryWrapper query) throws ValidationException {
startQuery(QueryType.valueOf(query.getQueryType()),
query.getQuery(),
userConn);
- userConn.onStart(queryId, maxRows);
+ userConn.onStart(queryId, maxRows, options);
Review comment:
Not needed. The connection already has this member:
```
protected WebSessionResources webSessionResources;
```
Which provides:
```
private final UserSession webUserSession;
```
Which provides `getOptions()`
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/stream/StreamingHttpConnection.java
##########
@@ -80,9 +85,10 @@ public StreamingHttpConnection(WebSessionResources webSessionResources) {
* Provide query info once the query starts. Sent from the REST request
* thread.
*/
- public void onStart(QueryId queryId, int rowLimit) {
+ public void onStart(QueryId queryId, int rowLimit, OptionSet options) {
Review comment:
Not needed. See above.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] vvysotskyi commented on a change in pull request #2194: DRILL-7888: query.json returns an incorrect error message when the query fails
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on a change in pull request #2194:
URL: https://github.com/apache/drill/pull/2194#discussion_r605846389
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/ExecConstants.java
##########
@@ -1216,6 +1216,11 @@ public static String bootDefaultFor(String name) {
"the sender to send out its data more rapidly, but you should know that it has a risk to OOM when the system is solving parallel " +
"large queries until we have a more accurate resource manager."));
+
+ public static final String ENABLE_REST_VERBOSE_ERRORS_KEY = "rest.errors.verbose";
Review comment:
Thanks, renamed
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java
##########
@@ -257,6 +257,7 @@
new OptionDefinition(ClassCompilerSelector.JAVA_COMPILER_JANINO_MAXSIZE),
new OptionDefinition(ClassCompilerSelector.JAVA_COMPILER_DEBUG),
new OptionDefinition(ExecConstants.ENABLE_VERBOSE_ERRORS),
+ new OptionDefinition(ExecConstants.ENABLE_REST_VERBOSE_ERRORS),
Review comment:
Yes, without this step everything will fail.
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/stream/QueryRunner.java
##########
@@ -48,7 +48,7 @@ public void start(QueryWrapper query) throws ValidationException {
startQuery(QueryType.valueOf(query.getQueryType()),
query.getQuery(),
userConn);
- userConn.onStart(queryId, maxRows);
+ userConn.onStart(queryId, maxRows, options);
Review comment:
Thanks, used this method
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] luocooong merged pull request #2194: DRILL-7888: query.json returns an incorrect error message when the query fails
Posted by GitBox <gi...@apache.org>.
luocooong merged pull request #2194:
URL: https://github.com/apache/drill/pull/2194
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] vvysotskyi commented on pull request #2194: DRILL-7888: query.json returns an incorrect error message when the query fails
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on pull request #2194:
URL: https://github.com/apache/drill/pull/2194#issuecomment-812851057
Hi @luocooong, yes, it is ready to review.
@paul-rogers, could you please look one more time?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] paul-rogers commented on a change in pull request #2194: DRILL-7888: query.json returns an incorrect error message when the query fails
Posted by GitBox <gi...@apache.org>.
paul-rogers commented on a change in pull request #2194:
URL: https://github.com/apache/drill/pull/2194#discussion_r603747584
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/stream/StreamingHttpConnection.java
##########
@@ -219,12 +230,39 @@ private void emitRows(RowSetReader batchReader) throws IOException {
*/
public void finish() throws IOException {
JsonOutput gen = writer.jsonOutput();
- gen.writeEndArray();
- writeNewline(gen);
+ if (batchCount == 0) {
+ startHeader();
+ if (options != null &&
+ Boolean.parseBoolean(options.get(ExecConstants.ENABLE_VERBOSE_ERRORS_KEY))) {
Review comment:
Do we want to reuse the rather verbose key here? It might be confusing as I would think, from the name, that I can also get the new behavior by setting the given config item.
In fact, I think we have a way to pass session options in a JSON query. Would it be better to simply use that mechanism here? Check if a new "drill.rest.errors.verbose" (or some such) option is set? This would let a site set the option by default, and would allow a future version of Drill make the verbose option the default.
Otherwise, if we want to add the one-off options here, maybe use short names such as "verbose". Someone might add a "schema-first" option to export the schema before the data to avoid client-side buffering.
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/stream/StreamingHttpConnection.java
##########
@@ -219,12 +230,39 @@ private void emitRows(RowSetReader batchReader) throws IOException {
*/
public void finish() throws IOException {
JsonOutput gen = writer.jsonOutput();
- gen.writeEndArray();
- writeNewline(gen);
+ if (batchCount == 0) {
+ startHeader();
+ if (options != null &&
+ Boolean.parseBoolean(options.get(ExecConstants.ENABLE_VERBOSE_ERRORS_KEY))) {
+ emitErrorInfo();
+ }
+ } else {
+ gen.writeEndArray();
+ writeNewline(gen);
+ }
gen.writeFieldName("queryState");
gen.writeVarChar(getQueryState());
writeNewline(gen);
gen.writeEndObject();
writeNewline(gen);
}
+
+ private void emitErrorInfo() throws IOException {
+ JsonOutput gen = writer.jsonOutput();
+ Throwable exception = DrillExceptionUtil.getThrowable(error.getException());
+ if (exception != null) {
+ gen.writeFieldName("exception");
+ gen.writeVarChar(exception.getClass().getName());
+ writeNewline(gen);
+ gen.writeFieldName("errorMessage");
+ gen.writeVarChar(exception.getMessage());
+ writeNewline(gen);
+ gen.writeFieldName("stackTrace");
Review comment:
Stack traces are generally very large. Do you want to a) represent the items as a list, or b) prune away the uninteresting top-level bits?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] vvysotskyi commented on a change in pull request #2194: DRILL-7888: query.json returns an incorrect error message when the query fails
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on a change in pull request #2194:
URL: https://github.com/apache/drill/pull/2194#discussion_r604409604
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/stream/StreamingHttpConnection.java
##########
@@ -219,12 +230,39 @@ private void emitRows(RowSetReader batchReader) throws IOException {
*/
public void finish() throws IOException {
JsonOutput gen = writer.jsonOutput();
- gen.writeEndArray();
- writeNewline(gen);
+ if (batchCount == 0) {
+ startHeader();
+ if (options != null &&
+ Boolean.parseBoolean(options.get(ExecConstants.ENABLE_VERBOSE_ERRORS_KEY))) {
+ emitErrorInfo();
+ }
+ } else {
+ gen.writeEndArray();
+ writeNewline(gen);
+ }
gen.writeFieldName("queryState");
gen.writeVarChar(getQueryState());
writeNewline(gen);
gen.writeEndObject();
writeNewline(gen);
}
+
+ private void emitErrorInfo() throws IOException {
+ JsonOutput gen = writer.jsonOutput();
+ Throwable exception = DrillExceptionUtil.getThrowable(error.getException());
+ if (exception != null) {
+ gen.writeFieldName("exception");
+ gen.writeVarChar(exception.getClass().getName());
+ writeNewline(gen);
+ gen.writeFieldName("errorMessage");
+ gen.writeVarChar(exception.getMessage());
+ writeNewline(gen);
+ gen.writeFieldName("stackTrace");
Review comment:
Good point, thanks, updated the code to pass stack trace items as a list and updated the PR description with the example of how it will look like.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] vvysotskyi commented on a change in pull request #2194: DRILL-7888: query.json returns an incorrect error message when the query fails
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on a change in pull request #2194:
URL: https://github.com/apache/drill/pull/2194#discussion_r604409604
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/stream/StreamingHttpConnection.java
##########
@@ -219,12 +230,39 @@ private void emitRows(RowSetReader batchReader) throws IOException {
*/
public void finish() throws IOException {
JsonOutput gen = writer.jsonOutput();
- gen.writeEndArray();
- writeNewline(gen);
+ if (batchCount == 0) {
+ startHeader();
+ if (options != null &&
+ Boolean.parseBoolean(options.get(ExecConstants.ENABLE_VERBOSE_ERRORS_KEY))) {
+ emitErrorInfo();
+ }
+ } else {
+ gen.writeEndArray();
+ writeNewline(gen);
+ }
gen.writeFieldName("queryState");
gen.writeVarChar(getQueryState());
writeNewline(gen);
gen.writeEndObject();
writeNewline(gen);
}
+
+ private void emitErrorInfo() throws IOException {
+ JsonOutput gen = writer.jsonOutput();
+ Throwable exception = DrillExceptionUtil.getThrowable(error.getException());
+ if (exception != null) {
+ gen.writeFieldName("exception");
+ gen.writeVarChar(exception.getClass().getName());
+ writeNewline(gen);
+ gen.writeFieldName("errorMessage");
+ gen.writeVarChar(exception.getMessage());
+ writeNewline(gen);
+ gen.writeFieldName("stackTrace");
Review comment:
Good point, thanks, updated the code to pass stack trace items as a list.
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/stream/StreamingHttpConnection.java
##########
@@ -219,12 +230,39 @@ private void emitRows(RowSetReader batchReader) throws IOException {
*/
public void finish() throws IOException {
JsonOutput gen = writer.jsonOutput();
- gen.writeEndArray();
- writeNewline(gen);
+ if (batchCount == 0) {
+ startHeader();
+ if (options != null &&
+ Boolean.parseBoolean(options.get(ExecConstants.ENABLE_VERBOSE_ERRORS_KEY))) {
Review comment:
Agree, thanks. Updated the code to use a new session option which also can be passed in the query request.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org