Added Query Function As Query_string Function by GabeFernandez310 · Pull Request #143 · Bit-Quill/opensearch-project-sql

GabeFernandez310 · 2022-10-26T16:00:53Z

Description

Added query function to SQL plugin. Achieved by routing function calls from the SQL plugin to the query_string function which currently exists in OpenSearch.

Currently blocked by opensearch-project#839. PR to upstream will be made once PR opensearch-project#839 is merged.

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 · 2022-10-26T16:36:59Z

Codecov Report

Merging #143 (52ea1e5) into integ-add-query-function (89b3e1a) will increase coverage by 0.28%.
The diff coverage is 100.00%.

@@                      Coverage Diff                       @@
##             integ-add-query-function     #143      +/-   ##
==============================================================
+ Coverage                       94.95%   95.23%   +0.28%     
- Complexity                       3190     3395     +205     
==============================================================
  Files                             316      330      +14     
  Lines                            8646     9302     +656     
  Branches                          637      691      +54     
==============================================================
+ Hits                             8210     8859     +649     
- Misses                            382      386       +4     
- Partials                           54       57       +3

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

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

Impacted Files	Coverage Δ
...arch/storage/script/filter/FilterQueryBuilder.java	`100.00% <ø> (ø)`
...c/main/java/org/opensearch/sql/expression/DSL.java	`100.00% <100.00%> (ø)`
...h/sql/expression/function/OpenSearchFunctions.java	`100.00% <100.00%> (ø)`
...e/script/filter/lucene/relevance/NoFieldQuery.java	`100.00% <100.00%> (ø)`
...age/script/filter/lucene/relevance/QueryQuery.java	`100.00% <100.00%> (ø)`
...script/filter/lucene/relevance/RelevanceQuery.java	`100.00% <100.00%> (ø)`
...pensearch/sql/sql/parser/AstExpressionBuilder.java	`100.00% <100.00%> (ø)`
...ain/java/org/opensearch/sql/analysis/Analyzer.java	`100.00% <0.00%> (ø)`
...java/org/opensearch/sql/ppl/parser/AstBuilder.java	`100.00% <0.00%> (ø)`
...java/org/opensearch/sql/storage/StorageEngine.java	`100.00% <0.00%> (ø)`
... and 25 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

acarbonetto

Good job guys! Thanks for the work. Just a couple of comments

docs/user/dql/functions.rst

acarbonetto · 2022-10-26T18:04:57Z

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

        TestsConstants.TEST_INDEX_ACCOUNT));
    Assert.assertThat(result,
-        containsString("query_string\":{\"query\":\"address:880 Holmes Lane"));
+        containsString("query_string\\\":{\\\"query\\\":\\\"address:880 Holmes Lane"));


this wasn't working?

There were some issues with escaped characters causing it to fail.

docs/user/dql/functions.rst

acarbonetto · 2022-10-26T18:06:50Z

integ-test/src/test/java/org/opensearch/sql/sql/QueryIT.java

+    @Test
+    public void all_fields_test() throws IOException {
+        String query = "SELECT * FROM "
+                + TEST_INDEX_BEER + " WHERE query('*:taste')";


I thought the wildcard syntax doesn't work? Or does it work with just *?

I thought so too... I thought we determined that they didn't work when we had our 1:1. I played around with it trying different combinations...

The following all seem to work when passed in as the query for query_string:

"* : taste" "* : ?aste" "Tags : *aste" "Tags : Ta?te" "Tags : Ta*te"

I tried these according to this documentation for Lucene, but it doesn't seem to follow it exactly since it also allows * and ? to be the first character of a search. Should we add tests for all of these cases as well? I can't find a specific Doc to reference for what should/shouldn't be supported in the query so I'm not really sure what other behaviour we should be testing for that we might be missing.

Added Unit Tests in f2d77c6

I was more asking about the field name (* or Tags). It seems like those two cases are fine, but T?g* is not?

Honestly, the checks are handled by OpenSearch not us... so what additional tests are going to do is test another system. We can consider covering some of the general cases, but we do not need to be exhaustive here.

.../java/org/opensearch/sql/opensearch/storage/script/filter/lucene/relevance/NoFieldQuery.java

...arch/src/test/java/org/opensearch/sql/opensearch/storage/script/filter/lucene/QueryTest.java

sql/src/test/java/org/opensearch/sql/sql/antlr/SQLSyntaxParserTest.java

integ-test/src/test/java/org/opensearch/sql/sql/QueryIT.java

...arch/src/test/java/org/opensearch/sql/opensearch/storage/script/filter/lucene/QueryTest.java

sql/src/test/java/org/opensearch/sql/sql/parser/AstExpressionBuilderTest.java

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

docs/user/dql/functions.rst

.../java/org/opensearch/sql/opensearch/storage/script/filter/lucene/relevance/NoFieldQuery.java

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

.../java/org/opensearch/sql/opensearch/storage/script/filter/lucene/relevance/NoFieldQuery.java

sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java

integ-test/src/test/java/org/opensearch/sql/sql/QueryIT.java

Yury-Fridlyand · 2022-10-27T22:08:51Z

Hint:
Add Signed-off-by: ... line manually if you commit suggestion via GH to avoid DCO failure.

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

forestmvey · 2022-11-01T20:13:41Z

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

  protected interface QueryBuilderStep<T extends QueryBuilder> extends
      BiFunction<T, ExprValue, T> {
  }
+


Formatting change not needed, and really should only be included in a review when appropriate.

Fixed in 7add05f

forestmvey · 2022-11-01T20:14:18Z

sql/src/test/java/org/opensearch/sql/sql/parser/AstExpressionBuilderTest.java

            + "analyzer='keyword', operator='AND')"));
  }

+


formatting.

Fixed in 7add05f

forestmvey · 2022-11-01T20:14:37Z

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

-                String.format("Parameter '%s' can only be specified once.", k));
-          }
-        });
+            .forEach((k, v) -> {


Formatting change not needed.

Introduced by accident when I was trying to fix checkstyle issues. Fixed in 7add05f

forestmvey · 2022-11-01T20:14:59Z

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

-          queryBuildActions
-              .get(argNormalized)))
-          .apply(queryBuilder, arg.getValue().valueOf(null));
+              queryBuildActions


Formatting change not needed.

Introduced by accident when I was trying to fix checkstyle issues. Fixed in 7add05f

forestmvey

Looks good other than some unnecessary formatting changes.

acarbonetto · 2022-10-31T19:11:04Z

integ-test/src/test/java/org/opensearch/sql/sql/QueryIT.java

+    @Test
+    public void all_fields_test() throws IOException {
+        String query = "SELECT * FROM "
+                + TEST_INDEX_BEER + " WHERE query('*:taste')";


I was more asking about the field name (* or Tags). It seems like those two cases are fine, but T?g* is not?

Honestly, the checks are handled by OpenSearch not us... so what additional tests are going to do is test another system. We can consider covering some of the general cases, but we do not need to be exhaustive here.

acarbonetto · 2022-10-31T19:15:47Z

docs/user/dql/functions.rst

 ``query_string([field_expression+], query_expression[, option=<option_value>]*)``

-The query_string function maps to the query_string query used in search engine, to return the documents that match a provided text, number, date or boolean value with a given field or fields.
+The query_string function maps to the query_string query used in search engine (also used by `query`_), to return the documents that match a provided text, number, date or boolean value with a given field or fields.


please remove, this is confusing and covered below

Fixed in 7add05f

acarbonetto · 2022-10-31T19:17:16Z

docs/user/dql/functions.rst

+
+``query("query_expression" [, option=<option_value>]*)``
+
+This is an alternative syntax to the `query_string`_ function. The query function maps to the query_string query used in search engine, to return the documents that match a provided text, number, date or boolean value with a given field or fields.


Suggested change

This is an alternative syntax to the `query_string`_ function. The query function maps to the query_string query used in search engine, to return the documents that match a provided text, number, date or boolean value with a given field or fields.

The `query` function is an alternative syntax to the `query_string` function. It maps to the query_string query used in search engine, to return the documents that match a provided text, number, date or boolean value with a given query expression.

Do we want to remove the links entirely? They were originally added to address this comment

You can apply the change and restore the link.

I changed the language to match the suggestion, but kept the link to query_string. Please let me know if you are ok with this. Done in 7add05f

acarbonetto · 2022-11-01T20:19:17Z

core/src/test/java/org/opensearch/sql/analysis/ExpressionAnalyzerTest.java

+  void query_expression() {
+    assertAnalyzeEqual(
+            dsl.query(
+                    dsl.namedArgument("query", DSL.literal("query_value"))),


nit: change "query_value" to "sample query" or "field:query" or something similar.

Fixed in ef2f4c9

Signed-off-by: GabeFernandez310 <gabrielf@bitquilltech.com>

acarbonetto approved these changes Oct 26, 2022

View reviewed changes

forestmvey reviewed Oct 26, 2022

View reviewed changes

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

forestmvey reviewed Oct 26, 2022

View reviewed changes

...arch/src/test/java/org/opensearch/sql/opensearch/storage/script/filter/lucene/QueryTest.java Outdated Show resolved Hide resolved

forestmvey reviewed Oct 26, 2022

View reviewed changes

sql/src/test/java/org/opensearch/sql/sql/parser/AstExpressionBuilderTest.java Outdated Show resolved Hide resolved

GumpacG reviewed Oct 27, 2022

View reviewed changes

...a/org/opensearch/sql/opensearch/storage/script/filter/lucene/relevance/NoFieldQueryTest.java Show resolved Hide resolved

GumpacG requested review from MaxKsyunz, MitchellGale, Yury-Fridlyand, acarbonetto and forestmvey October 27, 2022 17:29

Yury-Fridlyand suggested changes Oct 27, 2022

View reviewed changes

Yury-Fridlyand reviewed Oct 27, 2022

View reviewed changes

...in/java/org/opensearch/sql/opensearch/storage/script/filter/lucene/relevance/QueryQuery.java Outdated Show resolved Hide resolved

Yury-Fridlyand mentioned this pull request Oct 28, 2022

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

Closed

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

GabeFernandez310 force-pushed the dev-add-query-function branch from d49c0d1 to 9d14975 Compare October 28, 2022 22:48

Yury-Fridlyand self-requested a review November 1, 2022 16:51

forestmvey reviewed Nov 1, 2022

View reviewed changes

forestmvey approved these changes Nov 1, 2022

View reviewed changes

acarbonetto approved these changes Nov 1, 2022

View reviewed changes

GabeFernandez310 changed the base branch from integ-improve-error-reporting to integ-add-query-function November 1, 2022 22:54

GabeFernandez310 requested review from GumpacG, acarbonetto and forestmvey November 1, 2022 22:58

Yury-Fridlyand approved these changes Nov 2, 2022

View reviewed changes

MitchellGale approved these changes Nov 2, 2022

View reviewed changes

forestmvey approved these changes Nov 2, 2022

View reviewed changes

GumpacG approved these changes Nov 2, 2022

View reviewed changes

Added Query Function As Alternate Syntax For Query String Function

52ea1e5

Signed-off-by: GabeFernandez310 <gabrielf@bitquilltech.com>

GabeFernandez310 force-pushed the dev-add-query-function branch from ef2f4c9 to 52ea1e5 Compare November 2, 2022 16:15

GabeFernandez310 merged commit 2776fdd into integ-add-query-function Nov 2, 2022

GabeFernandez310 deleted the dev-add-query-function branch December 1, 2022 05:35


		``query("query_expression" [, option=<option_value>]*)``

		This is an alternative syntax to the `query_string`_ function. The query function maps to the query_string query used in search engine, to return the documents that match a provided text, number, date or boolean value with a given field or fields.

	This is an alternative syntax to the `query_string`_ function. The query function maps to the query_string query used in search engine, to return the documents that match a provided text, number, date or boolean value with a given field or fields.
	The `query` function is an alternative syntax to the `query_string` function. It maps to the query_string query used in search engine, to return the documents that match a provided text, number, date or boolean value with a given query expression.

Conversation

GabeFernandez310 commented Oct 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Check List

Uh oh!

codecov bot commented Oct 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

acarbonetto left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Yury-Fridlyand commented Oct 27, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

forestmvey left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GabeFernandez310 commented Oct 26, 2022 •

edited

Loading

codecov bot commented Oct 26, 2022 •

edited

Loading