`simple_query_string` implementation. #66

Yury-Fridlyand · 2022-06-02T23:06:31Z

Implementation for simple_query_string copied from dev-simple_query_string-#192-impl2, since that branch was corrupted by incorrect merge.
See also #61.
All PR review notes have been addressed.

Signed-off-by: Yury Fridlyand [email protected]

Description

Issues Resolved

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.

#192-impl2`, since that branch was corrupted by incorrect merge. Signed-off-by: Yury Fridlyand <[email protected]>

codecov · 2022-06-02T23:11:36Z

Codecov Report

Merging #66 (b3bca7c) into dev-simple_query_string-#192 (26058b8) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@                        Coverage Diff                         @@
##             dev-simple_query_string-#192      #66      +/-   ##
==================================================================
+ Coverage                           97.66%   97.69%   +0.02%     
- Complexity                           2743     2777      +34     
==================================================================
  Files                                 268      269       +1     
  Lines                                6772     6859      +87     
  Branches                              435      437       +2     
==================================================================
+ Hits                                 6614     6701      +87     
  Misses                                157      157              
  Partials                                1        1

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

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

Impacted Files	Coverage Δ
...ensearch/sql/expression/ExpressionNodeVisitor.java	`100.00% <ø> (ø)`
...arch/storage/script/filter/FilterQueryBuilder.java	`100.00% <ø> (ø)`
...rg/opensearch/sql/analysis/ExpressionAnalyzer.java	`100.00% <100.00%> (ø)`
...opensearch/sql/data/model/ExprCollectionValue.java	`100.00% <100.00%> (ø)`
...c/main/java/org/opensearch/sql/expression/DSL.java	`100.00% <100.00%> (ø)`
...h/sql/expression/function/BuiltinFunctionName.java	`100.00% <100.00%> (ø)`
...h/sql/expression/function/OpenSearchFunctions.java	`100.00% <100.00%> (ø)`
...ilter/lucene/relevance/SimpleQueryStringQuery.java	`100.00% <100.00%> (ø)`
...pensearch/sql/ppl/parser/AstExpressionBuilder.java	`100.00% <100.00%> (ø)`
...pensearch/sql/sql/parser/AstExpressionBuilder.java	`100.00% <100.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 26058b8...b3bca7c. Read the comment docs.

…ing-#192-impl3 Signed-off-by: Yury Fridlyand <[email protected]>

MaxKsyunz · 2022-06-03T19:47:18Z

...opensearch/sql/opensearch/storage/script/filter/lucene/relevance/SimpleQueryStringQuery.java

+import org.opensearch.sql.expression.NamedArgumentExpression;
+import org.opensearch.sql.opensearch.storage.script.filter.lucene.LuceneQuery;
+
+public class SimpleQueryStringQuery extends LuceneQuery {


There's a base class for common code for relevance queries -- RelevanceQuery.

Let's refactor it so that SimpleQueryStringQuery can use it as well before we send this to upstream.

Fixed in 521beca.

MaxKsyunz · 2022-06-03T19:48:12Z

...rc/test/java/org/opensearch/sql/opensearch/storage/script/filter/FilterQueryBuilderTest.java

+  // 1) DSL reverses order of the `fields`
+  // 2) `flags` are printed by OpenSearch (not by the plugin) as an integer
+  // 3) parameters are printed -//- not in lexicographical order
+  @Disabled


If the test does not work, can we remove it? If not, then let's fix it.

Fixed in 2ac159f.

MaxKsyunz · 2022-06-03T19:49:32Z

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

-    return new Function(
-        ctx.relevanceFunctionName().getText().toLowerCase(),
-        relevanceArguments(ctx));
+    if (ctx.singleFieldRelevanceFunction() != null) {


There is visitSingleFieldRelevanceFunction in the base class that would be more appropriate place for this code.

Fixed in 0817d9e.

MaxKsyunz · 2022-06-03T19:49:54Z

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

+              .singleFieldRelevanceFunctionName().getText().toLowerCase(),
+          singleFieldRelevanceArguments(ctx.singleFieldRelevanceFunction()));
+    } else {
+      return new Function(


There is visitMultiFieldRelevanceFunction in the base class that would be more appropriate place for this code.

Fixed in 0817d9e.

MaxKsyunz · 2022-06-03T19:52:40Z

...rc/test/java/org/opensearch/sql/opensearch/storage/script/filter/FilterQueryBuilderTest.java

+  // - fields' weights inside quotes
+  // - `flags` as an integer, but not as an enum
+  // - parameters not in alphabetical order
+  @Disabled


Let's make this test work or remove it.

Fixed in 2ac159f.

MaxKsyunz · 2022-06-03T19:58:02Z

core/src/main/java/org/opensearch/sql/ast/expression/RelevanceFieldList.java

+@AllArgsConstructor
+public class RelevanceFieldList extends UnresolvedExpression {
+  @Getter
+  private java.util.Map<UnresolvedExpression, UnresolvedExpression> fieldList;


I believe we can make this into a string-float map.
This would improve and simplify the places that use the class as well.

We talked about using the Field class to store the string/float parameters. If that doesn't work, we should consider creating an Object to store this information.

Fixed in 015aa06.

MaxKsyunz · 2022-06-03T19:59:53Z

core/src/main/java/org/opensearch/sql/analysis/ExpressionAnalyzer.java

+        .stream()
+        .collect(ImmutableMap.toImmutableMap(
+            n -> n.getKey().toString(),
+            n -> ExprValueUtils.floatValue((Float) ((Literal) n.getValue()).getValue())


Without simplified RelevanceFieldList, this will fail if RelevanceFieldList is anything other than string-float map.

Fixed in 015aa06.

MaxKsyunz · 2022-06-03T20:00:40Z

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

+        + Index.BEER.getName() + " WHERE simple_query_string(['Tags'], 'taste')";
+    var result1 = new JSONObject(executeQuery(query1, "jdbc"));
+    String query2 = "SELECT count(*) FROM "
+        + Index.BEER.getName() + " WHERE simple_query_string(['T*'], 'taste')";


Couldn't TestConstants.TEST_INDEX_BEER be used here?

Fixed in c1a91fc.

MaxKsyunz · 2022-06-03T20:04:24Z

sql/src/main/antlr/OpenSearchSQLParser.g4

+// Field is a list of columns
+multiFieldRelevanceFunction
+    : multiFieldRelevanceFunctionName LR_BRACKET
+        LT_SQR_PRTHS field=relevanceFieldAndWeight (COMMA field=relevanceFieldAndWeight)* RT_SQR_PRTHS


I believe if we add a separate rule for field list (everything between square brackets), we can simplify AstExpressionBuilder.

MaxKsyunz · 2022-06-03T20:04:49Z

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

-    return new Function(
-        ctx.relevanceFunctionName().getText().toLowerCase(),
-        relevanceArguments(ctx));
+    if (ctx.singleFieldRelevanceFunction() != null) {


Similar comments I made for PPL AstExpressionBuilder apply here.

Fixed in 0817d9e.

…ace. Signed-off-by: Yury Fridlyand <[email protected]>

Signed-off-by: Yury Fridlyand <[email protected]>

…y processing. Signed-off-by: Yury Fridlyand <[email protected]>

Signed-off-by: Yury Fridlyand <[email protected]>

...rc/test/java/org/opensearch/sql/opensearch/storage/script/filter/FilterQueryBuilderTest.java

acarbonetto · 2022-06-06T22:39:53Z

ppl/src/main/antlr/OpenSearchPPLLexer.g4

+// Operators. Bit
+
+BIT_NOT_OP:                         '~';
+//BIT_OR_OP:                          '|';


commented code can be removed

Fixed in b3bca7c.

acarbonetto · 2022-06-06T22:40:04Z

ppl/src/main/antlr/OpenSearchPPLLexer.g4

+REWRITE:                            'REWRITE';
+SLOP:                               'SLOP';
+TIE_BREAKER:                        'TIE_BREAKER';
+//TIME_ZONE:                          'TIME_ZONE'; // already defined on line 63


commented code can be removed

Fixed in b3bca7c.

Signed-off-by: Yury Fridlyand <[email protected]>

Copy simple_query_string implementation from `dev-simple_query_string-

d837b60

#192-impl2`, since that branch was corrupted by incorrect merge. Signed-off-by: Yury Fridlyand <[email protected]>

Yury-Fridlyand requested a review from a team June 2, 2022 23:06

Merge branch 'dev-simple_query_string-#192' into dev-simple_query_str…

5564fd0

…ing-#192-impl3 Signed-off-by: Yury Fridlyand <[email protected]>

forestmvey approved these changes Jun 3, 2022

View reviewed changes

MaxKsyunz reviewed Jun 3, 2022

View reviewed changes

Yury-Fridlyand added 6 commits June 3, 2022 19:39

Rework SimpleQueryStringQuery to use the RelevanceFunction interf…

521beca

…ace. Signed-off-by: Yury Fridlyand <[email protected]>

Optimize AstExpressionBuilder.

0817d9e

Signed-off-by: Yury Fridlyand <[email protected]>

Update RelevanceFieldList to store Map<String, Float> and simplif…

015aa06

…y processing. Signed-off-by: Yury Fridlyand <[email protected]>

Update SimpleQueryStringQuery to use ImmutableMap.

9814fa4

Signed-off-by: Yury Fridlyand <[email protected]>

Rework on DSL tests.

2ac159f

Signed-off-by: Yury Fridlyand <[email protected]>

Rework on integration tests. Add tests for PPL.

c1a91fc

Signed-off-by: Yury Fridlyand <[email protected]>

MaxKsyunz approved these changes Jun 6, 2022

View reviewed changes

Yury-Fridlyand mentioned this pull request Jun 6, 2022

Implementation for simple_query_string. #61

Closed

6 tasks

acarbonetto reviewed Jun 6, 2022

View reviewed changes

...rc/test/java/org/opensearch/sql/opensearch/storage/script/filter/FilterQueryBuilderTest.java Show resolved Hide resolved

acarbonetto reviewed Jun 6, 2022

View reviewed changes

acarbonetto approved these changes Jun 6, 2022

View reviewed changes

Remove commented code.

b3bca7c

Signed-off-by: Yury Fridlyand <[email protected]>

Yury-Fridlyand merged commit 30bc724 into dev-simple_query_string-#192 Jun 6, 2022

Yury-Fridlyand deleted the dev-simple_query_string-#192-impl3 branch June 6, 2022 23:41

Yury-Fridlyand mentioned this pull request Jun 15, 2022

Add implementation of multi_match similar to simple_query_string. #71

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`simple_query_string` implementation. #66

`simple_query_string` implementation. #66

Yury-Fridlyand commented Jun 2, 2022

codecov bot commented Jun 2, 2022 •

edited

Loading

MaxKsyunz Jun 3, 2022

Yury-Fridlyand Jun 6, 2022

MaxKsyunz Jun 3, 2022

Yury-Fridlyand Jun 6, 2022

MaxKsyunz Jun 3, 2022

Yury-Fridlyand Jun 6, 2022

MaxKsyunz Jun 3, 2022

Yury-Fridlyand Jun 6, 2022

MaxKsyunz Jun 3, 2022

Yury-Fridlyand Jun 6, 2022

MaxKsyunz Jun 3, 2022

acarbonetto Jun 3, 2022

Yury-Fridlyand Jun 6, 2022

MaxKsyunz Jun 3, 2022

Yury-Fridlyand Jun 6, 2022

MaxKsyunz Jun 3, 2022

Yury-Fridlyand Jun 6, 2022

MaxKsyunz Jun 3, 2022

MaxKsyunz Jun 3, 2022

Yury-Fridlyand Jun 6, 2022

acarbonetto Jun 6, 2022

Yury-Fridlyand Jun 6, 2022

acarbonetto Jun 6, 2022

Yury-Fridlyand Jun 6, 2022

simple_query_string implementation. #66

simple_query_string implementation. #66

Conversation

Yury-Fridlyand commented Jun 2, 2022

Description

Issues Resolved

Check List

codecov bot commented Jun 2, 2022 • edited 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

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

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

`simple_query_string` implementation. #66

`simple_query_string` implementation. #66

codecov bot commented Jun 2, 2022 •

edited

Loading