Improve Error Reporting for New SQL Engine #88

forestmvey · 2022-07-12T15:58:16Z

Description

When a query fails both legacy and new SQL engines, only the legacy engine error is reported to the user. On failure in both the legacy and new SQL engines, error messages from both should be reported to the end user.

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.

Signed-off-by: forestmvey <forestv@bitquilltech.com>

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

acarbonetto · 2022-07-12T16:27:30Z

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

+    private void reportError(final RestChannel channel, final Exception e, final RestStatus status, String newSqlEngineError) {
+        sendResponse(channel, ErrorMessageFactory.createErrorMessage(e, status.getStatus()).toString() + (newSqlEngineError.isEmpty()
+            ? "" : "\nQuery failed both legacy and new SQL engines, see error message below for new SQL engine error.\n"
+            + newSqlEngineError), status);


Putting a conditional within a concat is clunky. I would suggest saving the result of the conditional to a String variable would be cleaner.

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

acarbonetto · 2022-07-12T16:29:38Z

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

+   * This member variable and it's usage can be deleted once the
+   * legacy SQL engine is deprecated.
+   */
+  private String ErrorStr;


errorString - classes capitalize the first letter, not private variables.

forestmvey · 2022-07-12T17:21:34Z

Failing tests will require updates with error outputs. Prior to me fixing all the tests and revising this PR could we confirm this error output format is appropriate:

Signed-off-by: forestmvey <forestv@bitquilltech.com>

codecov · 2022-07-12T21:29:29Z

Codecov Report

Merging #88 (0467d13) into integ-sql_engine_error_improvements (58cfa88) will increase coverage by 0.02%.
The diff coverage is n/a.

@@                            Coverage Diff                            @@
##             integ-sql_engine_error_improvements      #88      +/-   ##
=========================================================================
+ Coverage                                  97.72%   97.74%   +0.02%     
- Complexity                                  2816     2859      +43     
=========================================================================
  Files                                        271      272       +1     
  Lines                                       6934     7006      +72     
  Branches                                     439      443       +4     
=========================================================================
+ Hits                                        6776     6848      +72     
  Misses                                       157      157              
  Partials                                       1        1

Flag	Coverage Δ
sql-engine	`97.74% <ø> (+0.02%)`	⬆️

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

Impacted Files	Coverage Δ
...c/main/java/org/opensearch/sql/expression/DSL.java	`100.00% <0.00%> (ø)`
...h/sql/expression/function/BuiltinFunctionName.java	`100.00% <0.00%> (ø)`
...h/sql/expression/function/OpenSearchFunctions.java	`100.00% <0.00%> (ø)`
...arch/storage/script/filter/FilterQueryBuilder.java	`100.00% <0.00%> (ø)`
...ript/filter/lucene/relevance/MatchPhraseQuery.java	`100.00% <0.00%> (ø)`
...ilter/lucene/relevance/MatchPhrasePrefixQuery.java	`100.00% <0.00%> (ø)`

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 58cfa88...0467d13. Read the comment docs.

…race separately in log to avoid logging un-anonymized data. Added function comments Signed-off-by: forestmvey <forestv@bitquilltech.com>

acarbonetto · 2022-07-13T21:27:06Z

docs/user/admin/settings.rst

@@ -109,6 +109,15 @@ Result set::
 	  },
 	  "status" : 400
 	}
+	Query failed on both V1 and V2 SQL parser engines. V2 SQL parser error following:
+	{
+      "error": {


I think you whitespace is off here

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

acarbonetto · 2022-07-13T21:34:29Z

legacy/src/main/java/org/opensearch/sql/legacy/utils/QueryDataAnonymizer.java

@@ -39,7 +39,7 @@ public static String anonymizeData(String query) {
                    .replaceAll("[\\n][\\t]+", " ");
        } catch (Exception e) {
            LOG.warn("Caught an exception when anonymizing sensitive data");
-            resultQuery = query;
+            resultQuery = "Failed to anonymize data.";


We should comment why we aren't returning the query any longer

acarbonetto · 2022-07-13T21:36:33Z

legacy/src/main/java/org/opensearch/sql/legacy/utils/QueryDataAnonymizer.java

@@ -39,7 +39,7 @@ public static String anonymizeData(String query) {
                    .replaceAll("[\\n][\\t]+", " ");
        } catch (Exception e) {
            LOG.warn("Caught an exception when anonymizing sensitive data");


Can we add a LOG.debug statement and output the failed query?

…ction Signed-off-by: forestmvey <forestv@bitquilltech.com>

Yury-Fridlyand · 2022-07-14T01:32:06Z

legacy/src/main/java/org/opensearch/sql/legacy/utils/QueryDataAnonymizer.java

-            LOG.warn("Caught an exception when anonymizing sensitive data");
-            resultQuery = query;
+            LOG.warn("Caught an exception when anonymizing sensitive data.");
+            LOG.debug("String {} failed anonymization.", query);


You still print the query on failure in the log.

Only in debug level which shouldn't be enabled in a production build right?

@nonnull

* Apply lombok Update compile task to recognize lombok Use lombok wherever applicable Use @nonnull notation Use var for local variables Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com>

forestmvey added 2 commits July 11, 2022 16:18

Initial Implementation

38d3819

Signed-off-by: forestmvey <forestv@bitquilltech.com>

Adding JAVADOC and code cleanup

d9bfd6c

Signed-off-by: forestmvey <forestv@bitquilltech.com>

acarbonetto reviewed Jul 12, 2022

View reviewed changes

Fixing output errors on tests, variable name formatting, minor revisions

9866cce

Signed-off-by: forestmvey <forestv@bitquilltech.com>

MitchellGale approved these changes Jul 12, 2022

View reviewed changes

Fixed so only successfully anonymized data is logged. Handled stack t…

05b4c90

…race separately in log to avoid logging un-anonymized data. Added function comments Signed-off-by: forestmvey <forestv@bitquilltech.com>

acarbonetto approved these changes Jul 13, 2022

View reviewed changes

Fix whitespace and add comments on updates for anonymizing string fun…

0467d13

…ction Signed-off-by: forestmvey <forestv@bitquilltech.com>

Yury-Fridlyand reviewed Jul 14, 2022

View reviewed changes

Yury-Fridlyand approved these changes Jul 14, 2022

View reviewed changes

forestmvey merged commit ffe8995 into integ-sql_engine_error_improvements Jul 14, 2022

forestmvey deleted the dev-sql_engine_error_improvements branch July 14, 2022 17:04

Yury-Fridlyand mentioned this pull request Aug 18, 2022

Improve error reporting #108

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve Error Reporting for New SQL Engine #88

Improve Error Reporting for New SQL Engine #88

forestmvey commented Jul 12, 2022

acarbonetto Jul 12, 2022

acarbonetto Jul 12, 2022

forestmvey commented Jul 12, 2022

codecov bot commented Jul 12, 2022 •

edited

Loading

acarbonetto Jul 13, 2022

acarbonetto Jul 13, 2022

acarbonetto Jul 13, 2022

Yury-Fridlyand Jul 14, 2022

forestmvey Jul 14, 2022

Improve Error Reporting for New SQL Engine #88

Improve Error Reporting for New SQL Engine #88

Conversation

forestmvey commented Jul 12, 2022

Description

Check List

acarbonetto Jul 12, 2022

Choose a reason for hiding this comment

acarbonetto Jul 12, 2022

Choose a reason for hiding this comment

forestmvey commented Jul 12, 2022

codecov bot commented Jul 12, 2022 • edited Loading

Codecov Report

acarbonetto Jul 13, 2022

Choose a reason for hiding this comment

acarbonetto Jul 13, 2022

Choose a reason for hiding this comment

acarbonetto Jul 13, 2022

Choose a reason for hiding this comment

Yury-Fridlyand Jul 14, 2022

Choose a reason for hiding this comment

forestmvey Jul 14, 2022

Choose a reason for hiding this comment

codecov bot commented Jul 12, 2022 •

edited

Loading