Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

Add Error Message for Explain and Run #872

Conversation

davidcui1225
Copy link
Contributor

Issue #, if available:
#863
Description of changes:

  • Added a generic error message in Explain for invalid queries. The error message consists of <API Response>: this query is not explainable.
  • Added this query is not runnable to error messages for Run on invalid queries.

Invalid Explain on SQL:
Screen Shot 2020-12-01 at 12 25 09 PM

Invalid Explain on PPL:
Screen Shot 2020-12-01 at 12 38 00 PM

Invalid Run on SQL:
Screen Shot 2020-12-01 at 1 14 26 PM

Invalid Run on PPL:
Screen Shot 2020-12-01 at 1 14 41 PM

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Copy link
Contributor

@joshuali925 joshuali925 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@codecov
Copy link

codecov bot commented Dec 1, 2020

Codecov Report

Merging #872 (8240742) into develop (370ff6d) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             develop     #872   +/-   ##
==========================================
  Coverage      99.85%   99.85%           
  Complexity      2132     2132           
==========================================
  Files            215      215           
  Lines           4774     4774           
  Branches         313      313           
==========================================
  Hits            4767     4767           
  Misses             5        5           
  Partials           2        2           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 370ff6d...8240742. Read the comment docs.

@dai-chen
Copy link
Member

dai-chen commented Dec 1, 2020

Is there any chance to add UT for this change?

For Run error message, it would be more helpful to show the "reason" or "details" in message from backend if "status"=400.

@davidcui1225
Copy link
Contributor Author

Is there any chance to add UT for this change?

For Run error message, it would be more helpful to show the "reason" or "details" in message from backend if "status"=400.

Do you mean display the response code in the output as well? Also I can try to add some UTs to check output matching on invalid queries/Explain

@dai-chen
Copy link
Member

dai-chen commented Dec 1, 2020

Is there any chance to add UT for this change?
For Run error message, it would be more helpful to show the "reason" or "details" in message from backend if "status"=400.

Do you mean display the response code in the output as well? Also I can try to add some UTs to check output matching on invalid queries/Explain

I meant when the "status" as below is 400, the words in "reason" or "details" would help user figure out why:

{
  "error": {
    "reason": "Invalid SQL query",
    "details": "Failed to parse query due to offending symbol [60] at: '\n         SELECT ((YEAR(DATE_ADD(FROM_DAYS(FLOOR(NULL) + 693961), INTERVAL 60' <--- HERE... More details: Expecting tokens in {'SELECT', '('}",
    "type": "SyntaxAnalysisException"
  },
  "status": 400
}

@davidcui1225
Copy link
Contributor Author

Added more detailed error output for Run on invalid queries to include API response.

SQL example:
Screen Shot 2020-12-01 at 4 12 53 PM

PPL example:
Screen Shot 2020-12-01 at 4 18 32 PM

@chloe-zh
Copy link
Member

chloe-zh commented Dec 2, 2020

Added more detailed error output for Run on invalid queries to include API response.

SQL example:

Screen Shot 2020-12-01 at 4 12 53 PM

PPL example:

Screen Shot 2020-12-01 at 4 18 32 PM

This looks good enough, but I wonder would it be better if the error reason, detail, type and status are parsed and displayed into a paragraph than put the raw exception to the console log, since the output panel looks not a json style but only a plain text box. For example:

SHOW TABLES LIKE: Bad request, this query is not explainable.

reason: "Invalid SQL query"
details: "Expected syntax example: 'SHOW TABLES LIKE <table pattern>'"
type: "IllegalArgumentException"
status: 400

@davidcui1225
Copy link
Contributor Author

Added more detailed error output for Run on invalid queries to include API response.
SQL example:
Screen Shot 2020-12-01 at 4 12 53 PM
PPL example:
Screen Shot 2020-12-01 at 4 18 32 PM

This looks good enough, but I wonder would it be better if the error reason, detail, type and status are parsed and displayed into a paragraph than put the raw exception to the console log, since the output panel looks not a json style but only a plain text box. For example:

SHOW TABLES LIKE: Bad request, this query is not explainable.

reason: "Invalid SQL query"
details: "Expected syntax example: 'SHOW TABLES LIKE <table pattern>'"
type: "IllegalArgumentException"
status: 400

I like this idea, will add that

Copy link
Member

@chloe-zh chloe-zh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for the changes!

@davidcui1225 davidcui1225 merged commit f47e9fc into opendistro-for-elasticsearch:develop Dec 2, 2020
@davidcui1225 davidcui1225 deleted the explain-error-message branch December 2, 2020 22:16
joshuali925 pushed a commit that referenced this pull request Dec 3, 2020
Added error messages for `Explain` aside from empty box. More detail in error messages for invalid queries on `Run`
chloe-zh pushed a commit to chloe-zh/sql that referenced this pull request Dec 15, 2020
Added error messages for `Explain` aside from empty box. More detail in error messages for invalid queries on `Run`
chloe-zh pushed a commit to chloe-zh/sql that referenced this pull request Dec 15, 2020
Added error messages for `Explain` aside from empty box. More detail in error messages for invalid queries on `Run`
chloe-zh pushed a commit to chloe-zh/sql that referenced this pull request Dec 15, 2020
Added error messages for `Explain` aside from empty box. More detail in error messages for invalid queries on `Run`
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants