Rework on error reporting to make it more verbose and human-friendly. #839

Yury-Fridlyand · 2022-09-17T02:34:08Z

Rework on #691

Co-authored-by: MaxKsyunz [email protected]
Co-authored-by: forestmvey [email protected]
Signed-off-by: Yury-Fridlyand [email protected]

Description

1.
Added cast validation and user-friendly error messages

SELECT phrase, insert_time2 from phrase WHERE match_phrase(phrase, 'brown fox', slop = 0.5);

before

{'reason': 'Invalid SQL query', 'details': 'For input string: "0.5"', 'type': 'NumberFormatException'}

after

{'reason': 'There was internal problem at backend', 'details': "Invalid slop value: '0.5'. Accepts only integer values.", 'type': 'RuntimeException'}

2.
Cast validation and user-friendly errors for all enum types

select `key` from calcs where multi_match([str1], 'DVD', zero_terms_query=meow);

before

{'reason': 'Invalid SQL query', 'details': 'No enum constant org.opensearch.index.search.MatchQuery.ZeroTermsQuery.MEOW', 'type': 'IllegalArgumentException'}

after

{'reason': 'There was internal problem at backend', 'details': "Invalid zero_terms_query value: 'meow'. Available values are: NONE, ALL, NULL.", 'type': 'RuntimeException'}

3.
Extended error message when incorrect parameters specified

SELECT phrase, insert_time2 from phrase WHERE match(phrase, 'brown fox', slop = 0);

before

{'reason': 'Invalid SQL query', 'details': 'Parameter slop is invalid for match function', 'type': 'SemanticCheckException'}

after

{'reason': 'Invalid SQL query', 'details': 'Parameter slop is invalid for match function. Available parameters are: analyzer, auto_generate_synonyms_phrase_query, boost, fuzziness, fuzzy_rewrite, fuzzy_transpositions, lenient, minimum_should_match, max_expansions, operator, prefix_length, zero_terms_query', 'type': 'SemanticCheckException'}

4.
~~Error aggregation from V1 and V2 engines~~ Reverted

Issues Resolved

#838

Check List

New functionality includes testing.
- All tests pass, including unit test, integration test and doctest
New functionality has been documented.
- New functionality has javadoc added
- New functionality has user manual doc added
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

codecov-commenter · 2022-09-17T02:41:03Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.95%. Comparing base (3fd53ca) to head (89b3e1a).
Report is 478 commits behind head on 2.x.

Additional details and impacted files

@@             Coverage Diff              @@
##                2.x     #839      +/-   ##
============================================
- Coverage     97.58%   94.95%   -2.63%     
- Complexity     3162     3190      +28     
============================================
  Files           305      316      +11     
  Lines          7902     8646     +744     
  Branches        515      637     +122     
============================================
+ Hits           7711     8210     +499     
- Misses          190      382     +192     
- Partials          1       54      +53

Flag	Coverage Δ
query-workbench	`62.76% <ø> (?)`
sql-engine	`97.60% <100.00%> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

penghuo · 2022-09-27T16:03:22Z

integ-test/src/test/java/org/opensearch/sql/legacy/PluginIT.java

@@ -70,8 +70,12 @@ public void sqlDeleteSettingsTest() throws IOException {
            "{\n"
                + "  \"error\": {\n"
                + "    \"reason\": \"Invalid SQL query\",\n"
-                + "    \"details\": \"DELETE clause is disabled by default and will be deprecated. Using "
-                + "the plugins.sql.delete.enabled setting to enable it\",\n"
+                + "    \"details\": \""


comparing old one, the message is more confuse for customer.

#839 (comment)

penghuo · 2022-09-27T16:04:28Z

docs/user/admin/settings.rst

@@ -305,7 +305,7 @@ SQL query::
    {
      "error": {
        "reason": "Invalid SQL query",
-        "details": "DELETE clause is disabled by default and will be deprecated. Using the plugins.sql.delete.enabled setting to enable it",
+        "details": "Failed to parse query due to offending symbol [DELETE] at: 'DELETE' <--- HERE... More details: Expecting tokens in {<EOF>, 'DESCRIBE', 'SELECT', 'SHOW', ';'}\nQuery failed on both V1 and V2 SQL engines. V1 SQL engine error following: \nDELETE clause is disabled by default and will be deprecated. Using the plugins.sql.delete.enabled setting to enable it",


any reason we want to expose V1/V2 engine info for customer?

It is already exposed, see #830/#1111.
Any ideas how to do this better?
I placed V2 error first, because it is more relevant for most cases (except this one).

as discussed, we may want to simplify error message exposed to customer and log this kind of detailed message for our developer use.

Fixed in de104bf.

common/src/main/java/org/opensearch/sql/common/utils/QueryContext.java

legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java

Yury-Fridlyand · 2022-09-28T21:01:32Z

Rebased to resolve conflicts.

joshuali925 · 2022-09-27T19:07:05Z

...earch/sql/opensearch/storage/script/filter/lucene/relevance/FunctionParameterRepository.java

@@ -0,0 +1,209 @@
+package org.opensearch.sql.opensearch.storage.script.filter.lucene.relevance;


missing license header. any reason why there are a lot of relevance related changes in this PR?

Thank you, fixed in a0f9970.
I updated all relevance functions to use this FunctionParameterRepository to validate functions' arguments' values and generate user-friendly error messages.
I also updated base RelevanceQuery class to do better argument check.

just wondering did Github action fail on missing license header?

We don't have such validation yet.

penghuo · 2022-10-13T15:49:40Z

docs/user/admin/settings.rst

@@ -305,7 +305,7 @@ SQL query::
    {
      "error": {
        "reason": "Invalid SQL query",
-        "details": "DELETE clause is disabled by default and will be deprecated. Using the plugins.sql.delete.enabled setting to enable it",
+        "details": "Failed to parse query due to offending symbol [DELETE] at: 'DELETE' <--- HERE... More details: Expecting tokens in {<EOF>, 'DESCRIBE', 'SELECT', 'SHOW', ';'}",


I think the originl message is much clear

#839 (comment)

Revered in b5ad3c9.

penghuo · 2022-10-13T15:50:45Z

common/src/main/java/org/opensearch/sql/common/utils/QueryContext.java

+   * Can be deleted once the
+   * legacy SQL engine is deprecated.
+   */
+  public static void setError(String error) {


do we have better way to pass error messsage around instead of using threadcontext? we should consider deprecated threadcontext instead of add more capability to it.

This feature (aggregating error messages from V1 and V2) will be removed soon along with V1. I can create/use a thread-safe singleton Map instead.

Revered in b5ad3c9.

MaxKsyunz · 2022-10-18T19:02:52Z

...ava/org/opensearch/sql/opensearch/storage/script/filter/lucene/relevance/RelevanceQuery.java

+    var field = arguments.stream()
+        .filter(a -> a.getArgName().equalsIgnoreCase("field"))
+        .findFirst()
+        .orElse(null);
+    var fields = arguments.stream()
+        .filter(a -> a.getArgName().equalsIgnoreCase("fields"))
+        .findFirst()
+        .orElse(null);
+    if (fields == null && field == null) {
+      throw new SemanticCheckException("Both 'field' and 'fields' parameters are missing.");
+    }
+    if (fields != null && field != null) {
+      throw new SemanticCheckException("Both 'field' and 'fields' parameters are given.");
+    }
+
+    var query = arguments.stream()
+        .filter(a -> a.getArgName().equalsIgnoreCase("query"))
+        .findFirst()
+        .orElseThrow(() -> new SemanticCheckException("'query' parameter is missing"));
+    T queryBuilder = createQueryBuilder((field != null ? field : fields), query);


Code specific to single field functions belongs in SingleFieldQuery class, multi-field functions -- MultiFieldQuery.

Thank you, fixed in 2f1e302.
Notes:

we can't check whether field and fields present both

query check could be moved to a separate method to the superclass, but the benefit is rather low

Yury-Fridlyand · 2022-10-28T00:16:47Z

What about to move all queryBuildActions from CTORS of all inheritors of ...FieldQuery to FunctionParameterRepository?

Yury-Fridlyand · 2022-10-28T19:51:03Z

What about to move all queryBuildActions from CTORS of all inheritors of ...FieldQuery to FunctionParameterRepository?

Updated in 827a0cf.

…#116) Co-authored-by: MaxKsyunz <[email protected]> Co-authored-by: forestmvey <[email protected]> Signed-off-by: Yury-Fridlyand <[email protected]>

Yury-Fridlyand · 2022-10-28T20:13:15Z

Rebased to resolve conflicts.

penghuo

Thanks for the change!

opensearch-trigger-bot · 2022-11-01T22:17:53Z

The backport to 2.4 failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.4 2.4
# Navigate to the new working tree
cd .worktrees/backport-2.4
# Create a new branch
git switch --create backport/backport-839-to-2.4
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 21373b9380dea321063dc6a5532b84a2ebec32a4
# Push it to GitHub
git push --set-upstream origin backport/backport-839-to-2.4
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.4

Then, create a pull request where the base branch is 2.4 and the compare/head branch is backport/backport-839-to-2.4.

…e-error-reporting Rework on error reporting to make it more verbose and human-friendly. (cherry picked from commit 21373b9) Signed-off-by: Yury-Fridlyand <[email protected]>

…1008) Rework on error reporting to make it more verbose and human-friendly. (cherry picked from commit 21373b9) Signed-off-by: Yury-Fridlyand <[email protected]>

Yury-Fridlyand requested a review from a team as a code owner September 17, 2022 02:34

Yury-Fridlyand mentioned this pull request Sep 20, 2022

SQL ingestion and ops - standup #791

Open

MaxKsyunz previously approved these changes Sep 24, 2022

View reviewed changes

penghuo reviewed Sep 27, 2022

View reviewed changes

Yury-Fridlyand requested a review from penghuo September 27, 2022 22:32

dai-chen reviewed Sep 27, 2022

View reviewed changes

common/src/main/java/org/opensearch/sql/common/utils/QueryContext.java Outdated Show resolved Hide resolved

dai-chen reviewed Sep 27, 2022

View reviewed changes

legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java Outdated Show resolved Hide resolved

Yury-Fridlyand dismissed MaxKsyunz’s stale review via 4412631 September 28, 2022 00:37

Yury-Fridlyand requested review from MaxKsyunz and dai-chen September 28, 2022 00:38

Yury-Fridlyand mentioned this pull request Sep 28, 2022

[FEATURE] Add option to specify custom argument description/list #860

Open

acarbonetto previously approved these changes Sep 28, 2022

View reviewed changes

Yury-Fridlyand dismissed acarbonetto’s stale review via 03b0bcb September 28, 2022 20:47

Yury-Fridlyand force-pushed the integ-improve-error-reporting branch from 03b0bcb to bb91d5a Compare September 28, 2022 21:01

joshuali925 reviewed Sep 28, 2022

View reviewed changes

joshuali925 previously approved these changes Sep 29, 2022

View reviewed changes

dai-chen added the enhancement New feature or request label Sep 29, 2022

Yury-Fridlyand dismissed joshuali925’s stale review via de104bf October 4, 2022 05:37

penghuo reviewed Oct 13, 2022

View reviewed changes

MaxKsyunz requested changes Oct 18, 2022

View reviewed changes

MaxKsyunz self-requested a review October 18, 2022 19:57

Yury-Fridlyand requested review from penghuo and joshuali925 October 19, 2022 20:51

GumpacG mentioned this pull request Oct 27, 2022

Added Query Function As Query_string Function Bit-Quill/opensearch-project-sql#143

Merged

6 tasks

Rework on error reporting to make it more verbose and human-friendly. (…

89b3e1a

…#116) Co-authored-by: MaxKsyunz <[email protected]> Co-authored-by: forestmvey <[email protected]> Signed-off-by: Yury-Fridlyand <[email protected]>

Yury-Fridlyand force-pushed the integ-improve-error-reporting branch from 827a0cf to 89b3e1a Compare October 28, 2022 20:10

penghuo approved these changes Oct 28, 2022

View reviewed changes

dai-chen approved these changes Oct 28, 2022

View reviewed changes

MaxKsyunz approved these changes Nov 1, 2022

View reviewed changes

Yury-Fridlyand merged commit 21373b9 into opensearch-project:2.x Nov 1, 2022

Yury-Fridlyand added the backport 2.4 label Nov 1, 2022

Yury-Fridlyand mentioned this pull request Nov 1, 2022

[Backport 2.4] Merge pull request #839 from Bit-Quill/integ-improve-error-reporting #1008

Merged

6 tasks

Yury-Fridlyand deleted the integ-improve-error-reporting branch November 1, 2022 23:39

Yury-Fridlyand mentioned this pull request Nov 1, 2022

Bump backport version. #1009

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework on error reporting to make it more verbose and human-friendly. #839

Rework on error reporting to make it more verbose and human-friendly. #839

Yury-Fridlyand commented Sep 17, 2022 •

edited

Loading

codecov-commenter commented Sep 17, 2022 •

edited by codecov bot

Loading

penghuo Sep 27, 2022

Yury-Fridlyand Sep 27, 2022

penghuo Sep 27, 2022

Yury-Fridlyand Sep 27, 2022

dai-chen Sep 29, 2022

Yury-Fridlyand Oct 4, 2022

Yury-Fridlyand commented Sep 28, 2022

joshuali925 Sep 27, 2022

Yury-Fridlyand Sep 29, 2022

dai-chen Sep 29, 2022

Yury-Fridlyand Oct 4, 2022

penghuo Oct 13, 2022

Yury-Fridlyand Oct 13, 2022

Yury-Fridlyand Oct 19, 2022

penghuo Oct 13, 2022

Yury-Fridlyand Oct 13, 2022

Yury-Fridlyand Oct 19, 2022

MaxKsyunz Oct 18, 2022

Yury-Fridlyand Oct 19, 2022

Yury-Fridlyand commented Oct 28, 2022

Yury-Fridlyand commented Oct 28, 2022

Yury-Fridlyand commented Oct 28, 2022

penghuo left a comment

opensearch-trigger-bot bot commented Nov 1, 2022

		@@ -0,0 +1,209 @@
		package org.opensearch.sql.opensearch.storage.script.filter.lucene.relevance;

Rework on error reporting to make it more verbose and human-friendly. #839

Rework on error reporting to make it more verbose and human-friendly. #839

Conversation

Yury-Fridlyand commented Sep 17, 2022 • edited Loading

Description

Issues Resolved

Check List

codecov-commenter commented Sep 17, 2022 • edited by codecov bot Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yury-Fridlyand commented Sep 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yury-Fridlyand commented Oct 28, 2022

Yury-Fridlyand commented Oct 28, 2022

Yury-Fridlyand commented Oct 28, 2022

penghuo left a comment

Choose a reason for hiding this comment

opensearch-trigger-bot bot commented Nov 1, 2022

Yury-Fridlyand commented Sep 17, 2022 •

edited

Loading

codecov-commenter commented Sep 17, 2022 •

edited by codecov bot

Loading