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

Yury-Fridlyand · 2022-09-13T20:27:47Z

Co-authored-by: MaxKsyunz maxk@bitquilltech.com
Co-authored-by: forestmvey forestv@bitquilltech.com
Signed-off-by: Yury-Fridlyand yuryf@bitquilltech.com

Description

Re-creation of PR #108.

Issues Resolved

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.
A bit better error on typo in function name

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

before

TransportError(503, 'Exception', {'error': {'reason': 'There was internal problem at backend', 'details': '[701f78b8-da61-4eb1-a00a-18f089f5644d] Request SQLQueryRequest(jsonContent={"query":"SELECT phrase, insert_time2 from phrase WHERE match_phrase2(phrase, \'brown fox\', slop = 0)"}, query=SELECT phrase, insert_time2 from phrase WHERE match_phrase2(phrase, \'brown fox\', slop = 0), path=/_plugins/_sql/, format=jdbc, params={format=jdbc}, sanitize=true) is not supported and falling back to old SQL engine', 'type': 'Exception'}, 'status': 503})

after

{'reason': 'Invalid SQL query', 'details': "unsupported method: match_phrase2\nQuery failed on both V1 and V2 SQL parser engines. V2 SQL parser error following: \nFailed to parse query due to offending symbol [(] at: 'SELECT phrase, insert_time2 from phrase WHERE match_phrase2(' <--- HERE... More details: Expecting tokens in {<EOF>, ';'}", 'type': 'SqlParseException'}

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.

Co-authored-by: MaxKsyunz <maxk@bitquilltech.com> Co-authored-by: forestmvey <forestv@bitquilltech.com> Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com>

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

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

acarbonetto · 2022-09-13T20:40:23Z

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

-    Set<String> visitedParms = new HashSet();
+    // Aggregate parameters by name, so getting a Map<Name:String, List>
+    arguments.stream().collect(Collectors.groupingBy(a -> a.getArgName().toLowerCase()))
+        .forEach((k, v) -> {


@Yury-Fridlyand asked https://github.com/Bit-Quill/opensearch-project-sql/pull/108/files#r966554640
Do we need to aggregate all duplicates into one exception? Currently, function throws on first found duplicated parameter.

What do we do other places...? I think we always throw on the first failure, but maybe you have an example where we aggregate?

I can't find any examples. I don't think that we should worry about that, the probability that a user will specify multiple arguments more than once is extremely low I suppose. I just pointed that we have a room for further improvement.

acarbonetto · 2022-09-13T22:19:38Z

legacy/src/main/java/org/opensearch/sql/legacy/executor/format/ErrorMessage.java

    private String details;

+    public String getDetails() {


why don't you use @Getter too?

Thank you, fixed in 8423187.

acarbonetto · 2022-09-13T22:31:29Z

...in/java/org/opensearch/sql/opensearch/storage/script/filter/lucene/relevance/MatchQuery.java

-        .put("lenient", (b, v) -> b.lenient(Boolean.parseBoolean(v.stringValue())))
-        .put("operator", (b, v) -> b.operator(Operator.fromString(v.stringValue())))
+        .put("auto_generate_synonyms_phrase_query", (b, v) -> b.autoGenerateSynonymsPhraseQuery(
+            FunctionParameterRepository.convertBoolValue(v,"auto_generate_synonyms_phrase_query")))


Suggested change

FunctionParameterRepository.convertBoolValue(v,"auto_generate_synonyms_phrase_query")))

FunctionParameterRepository.convertBoolValue(v, "auto_generate_synonyms_phrase_query")))

acarbonetto · 2022-09-13T22:31:54Z

...a/org/opensearch/sql/opensearch/storage/script/filter/lucene/relevance/QueryStringQuery.java

+        .put("analyze_wildcard", (b, v) -> b.analyzeWildcard(
+            FunctionParameterRepository.convertBoolValue(v, "analyze_wildcard")))
+        .put("auto_generate_synonyms_phrase_query", (b, v) -> b.autoGenerateSynonymsPhraseQuery(
+            FunctionParameterRepository.convertBoolValue(v,"auto_generate_synonyms_phrase_query")))


Suggested change

FunctionParameterRepository.convertBoolValue(v,"auto_generate_synonyms_phrase_query")))

FunctionParameterRepository.convertBoolValue(v, "auto_generate_synonyms_phrase_query")))

If I add the space, line pass 100 symbols limit and I have to break it. I would like to avoid it, but if you insist...

Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com>

forestmvey · 2022-09-14T16:19:32Z

The query
select Title, Body from stackexchange_beer where match(Title, 'Cicerone', blah=0);
returns a useful error message of

{
  "error": {
    "reason": "Invalid SQL query",
    "details": "Failed to parse query due to offending symbol [blah] at: 'select Title, Body from stackexchange_beer where match(Title, Cicerone, blah' <--- HERE... More details: Expecting tokens in {'ALLOW_LEADING_WILDCARD', 'ANALYZER', 'ANALYZE_WILDCARD', 'AUTO_GENERATE_SYNONYMS_PHRASE_QUERY', 'BOOST', 'CUTOFF_FREQUENCY', 'DEFAULT_FIELD', 'DEFAULT_OPERATOR', 'ESCAPE', 'ENABLE_POSITION_INCREMENTS', 'FIELDS', 'FLAGS', 'FUZZINESS', 'FUZZY_MAX_EXPANSIONS', 'FUZZY_PREFIX_LENGTH', 'FUZZY_REWRITE', 'FUZZY_TRANSPOSITIONS', 'LENIENT', 'LOW_FREQ_OPERATOR', 'MAX_DETERMINIZED_STATES', 'MAX_EXPANSIONS', 'MINIMUM_SHOULD_MATCH', 'OPERATOR', 'PHRASE_SLOP', 'PREFIX_LENGTH', 'QUOTE_ANALYZER', 'QUOTE_FIELD_SUFFIX', 'REWRITE', 'SLOP', 'TIE_BREAKER', 'TIME_ZONE', 'TYPE', 'ZERO_TERMS_QUERY'}\nQuery failed on both V1 and V2 SQL engines. V1 SQL engine error following: \nsyntax error, expect AGAINST, actual EOF",
    "type": "ParserException"
  },
  "status": 400
}

but the query

select Title, Body from stackexchange_beer where match_phrase(Title, 'Cicerone', blah=0);

returns

{
  "error": {
    "reason": "There was internal problem at backend",
    "details": "[match_phrase] requires query value",
    "type": "IllegalArgumentException"
  },
  "status": 500
}

EDIT: Seems we could not re-produce this on another machine

😕

…#116) Co-authored-by: MaxKsyunz <maxk@bitquilltech.com> Co-authored-by: forestmvey <forestv@bitquilltech.com> Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com>

…#116) Co-authored-by: MaxKsyunz <maxk@bitquilltech.com> Co-authored-by: forestmvey <forestv@bitquilltech.com> Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com> Solved Merge Conflicts

…#116) Co-authored-by: MaxKsyunz <maxk@bitquilltech.com> Co-authored-by: forestmvey <forestv@bitquilltech.com> Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com>

Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com>

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

9b0e4c9

Co-authored-by: MaxKsyunz <maxk@bitquilltech.com> Co-authored-by: forestmvey <forestv@bitquilltech.com> Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com>

Yury-Fridlyand requested review from acarbonetto, forestmvey, MaxKsyunz and MitchellGale September 13, 2022 20:27

Yury-Fridlyand mentioned this pull request Sep 13, 2022

Improve error reporting #108

Closed

6 tasks

This comment was marked as spam.

Sign in to view

forestmvey reviewed Sep 13, 2022

View reviewed changes

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

forestmvey reviewed Sep 13, 2022

View reviewed changes

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

acarbonetto approved these changes Sep 13, 2022

View reviewed changes

Address PR feedback.

8423187

Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com>

MaxKsyunz approved these changes Sep 14, 2022

View reviewed changes

Yury-Fridlyand requested a review from forestmvey September 15, 2022 16:32

Yury-Fridlyand mentioned this pull request Sep 15, 2022

SQL ingestion and ops - standup opensearch-project/sql#791

Open

forestmvey approved these changes Sep 16, 2022

View reviewed changes

Yury-Fridlyand merged commit aaa4fda into integ-improve-error-reporting Sep 17, 2022

Yury-Fridlyand deleted the dev-improve-error-reporting branch September 17, 2022 02:28

andy-k-improving pushed a commit that referenced this pull request Nov 16, 2024

Adding gradle task for running integ tests in remote cluster (#116)

a81ea44

Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com>

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. #116

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

Yury-Fridlyand commented Sep 13, 2022

This comment was marked as spam.

acarbonetto Sep 13, 2022

acarbonetto Sep 13, 2022

Yury-Fridlyand Sep 13, 2022

acarbonetto Sep 13, 2022

Yury-Fridlyand Sep 13, 2022

acarbonetto Sep 13, 2022

acarbonetto Sep 13, 2022

Yury-Fridlyand Sep 13, 2022

forestmvey commented Sep 14, 2022 •

edited

Loading

	FunctionParameterRepository.convertBoolValue(v,"auto_generate_synonyms_phrase_query")))
	FunctionParameterRepository.convertBoolValue(v, "auto_generate_synonyms_phrase_query")))

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

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

Conversation

Yury-Fridlyand commented Sep 13, 2022

Description

Issues Resolved

Check List

This comment was marked as spam.

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

forestmvey commented Sep 14, 2022 • edited Loading

😕

forestmvey commented Sep 14, 2022 •

edited

Loading