Support match_phrase filter function in SQL and PPL #604

MaxKsyunz · 2022-05-11T00:53:02Z

Description

Support match_phrase filter function in SQL.
Support match_phrase filter function in PPL.
Support matchphrase filter function in SQL to maintain backwards compatibility with the legacy engine.

Issues Resolved

Resolves #185.

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: MaxKsyunz <[email protected]>

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

Signed-off-by: MaxKsyunz <[email protected]>

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

@ignore

Added a couple test for match_test in PPL. The tests are currently @ignore'd until more of the issue is complete. Signed-off-by: MaxKsyunz <[email protected]>

Signed-off-by: MaxKsyunz <[email protected]>

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

Signed-off-by: MaxKsyunz <[email protected]>

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

Create a parametrized test for PPLSyntax parser. Signed-off-by: MaxKsyunz <[email protected]>

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

…for `MATCH` are also updated. Signed-off-by: Yury Fridlyand <[email protected]>

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

Signed-off-by: MaxKsyunz <[email protected]>

1. Use PPL instead of SQL as samples. 2. Use data that doctest runs with. Signed-off-by: MaxKsyunz <[email protected]>

… samples. This fixes doctest. Signed-off-by: Yury Fridlyand <[email protected]>

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

codecov-commenter · 2022-05-11T01:36:13Z

Codecov Report

Merging #604 (15775e8) into main (760e377) will decrease coverage by 31.82%.
The diff coverage is n/a.

@@              Coverage Diff              @@
##               main     #604       +/-   ##
=============================================
- Coverage     94.59%   62.76%   -31.83%     
=============================================
  Files           276       10      -266     
  Lines          7457      658     -6799     
  Branches        552      119      -433     
=============================================
- Hits           7054      413     -6641     
+ Misses          349      192      -157     
+ Partials         54       53        -1

Flag	Coverage Δ
query-workbench	`62.76% <ø> (ø)`
sql-engine	`?`

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
...h/sql/expression/function/BuiltinFunctionName.java
...h/sql/expression/function/OpenSearchFunctions.java
...arch/storage/script/filter/FilterQueryBuilder.java
...rg/opensearch/sql/ppl/config/PPLServiceConfig.java
.../opensearch/sql/planner/logical/LogicalRemove.java
...ssion/operator/arthmetic/MathematicalFunction.java
...ensearch/sql/exception/SemanticCheckException.java
...search/response/agg/NoBucketAggregationParser.java
...org/opensearch/sql/data/model/ExprStringValue.java
... and 255 more

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 760e377...15775e8. Read the comment docs.

Add support for matchphrase as an alternative spelling of match_phrase. Mention in both SQL and PPL documentation that matchphrase is accepted as well as match_phrase. Signed-off-by: MaxKsyunz <[email protected]>

…update-v2 Support legacy syntax for match_phrase in the new SQL engine.

joshuali925

thanks, minor comments

joshuali925 · 2022-05-19T16:36:25Z

docs/user/dql/functions.rst

@@ -2195,3 +2195,43 @@ Another example to show how to set custom values for the optional parameters::
    | Bond       |
    +------------+

+MATCH_PHRASE


not sure what should be done but should we distinguish between this one and https://github.com/opensearch-project/sql/blob/main/docs/user/beyond/fulltext.rst#match-phrase-query

Is that match_phrase from the legacy SQL engine?

i believe so, i'm thinking maybe add a note to the old one, or link it after "For backward compatibility" in the new one, so users know what the difference is

@joshuali925 that's a good point.

Distinction between old and new SQL engines was explicitly removed as part of #58

I'd like to add a note to fulltext.rst that the document describes the legacy SQL engine and to refer to functions.rst for the new engine.

Once the new engine is on par with the old one, we can update fulltext.rst to describe the default engine.

What do you think?

joshuali925 · 2022-05-19T16:45:33Z

ppl/src/test/java/org/opensearch/sql/ppl/antlr/PPLSyntaxParserMatchPhraseSamplesTest.java

@@ -0,0 +1,40 @@
+package org.opensearch.sql.ppl.antlr;


missing license header

See 1fae665

joshuali925 · 2022-05-19T16:45:52Z

...st/java/org/opensearch/sql/opensearch/storage/script/filter/lucene/MatchPhraseQueryTest.java

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


license header

See 1fae665

penghuo · 2022-05-19T17:30:55Z

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

+  public QueryBuilder build(FunctionExpression func) {
+    List<Expression> arguments = func.getArguments();
+    if (arguments.size() < 2) {
+      throw new SemanticCheckException("match_phrase requires at least two parameters");


ANTLR will check the syntax, right? If you want to assert that, it should be syntax exception.

Please see 7101d07

penghuo · 2022-05-19T17:39:18Z

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

+/**
+ * Lucene query that builds a match_phrase query.
+ */
+public class MatchPhraseQuery extends LuceneQuery {


Looks like MatchPhraseQuery and MatchQuery have duplicate code, could it been simplified?

Please see RelevanceQuery added in d3667ce

Signed-off-by: MaxKsyunz <[email protected]>

Also, use getWriteableName() to get the name of the query in the error message. Signed-off-by: MaxKsyunz <[email protected]>

This dummy instance of MatchPhraseQueryBuilder is used in RelevanceQuery when creating an exception to get the name of the query. Signed-off-by: MaxKsyunz <[email protected]>

Signed-off-by: MaxKsyunz <[email protected]>

This branch changed RelevanceQuery.build to throw SyntaxCheckException if it is called with less than two arguments. Previously, build would through SemanticCheckException. Signed-off-by: MaxKsyunz <[email protected]>

joshuali925

lgtm

penghuo · 2022-05-24T22:38:47Z

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

+    List<Expression> arguments = func.getArguments();
+    if (arguments.size() < 2) {
+      String queryName = createQueryBuilder("dummy_field", "").getWriteableName();
+      throw new SyntaxCheckException(


add a UT to assert the exception message is expected?

penghuo · 2022-05-24T22:40:16Z

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

+/**
+ * Base class for query abstraction that builds a relevance query from function expression.
+ */
+public abstract class RelevanceQuery<T extends QueryBuilder> extends LuceneQuery {


Signed-off-by: MaxKsyunz <[email protected]>

acarbonetto · 2022-05-25T21:16:58Z

...pensearch/sql/opensearch/storage/script/filter/lucene/relevance/RelevanceQueryBuildTest.java

-    assertThrows(SemanticCheckException.class, () -> query.build(expr));
+    SemanticCheckException exception =
+        assertThrows(SemanticCheckException.class, () -> query.build(expr));
+    assertEquals("Parameter wrongArg is invalid for mock_query function.", exception.getMessage());


I'm not sure you want to tie the test that closely to the code (so that if someone changes a single character in the string, the test fails).
Maybe you can check that the messages contains "wrongArg" and "mock_query" and leave it at that?

assertThat(exception.getMessages(), containsString("wrongArg")); assertThat(exception.getMessages(), containsString("mock_query"));

@acarbonetto that's in line with pre-existing convention in this codebase.

acarbonetto · 2022-05-25T21:17:19Z

...pensearch/sql/opensearch/storage/script/filter/lucene/relevance/RelevanceQueryBuildTest.java

-    assertThrows(SyntaxCheckException.class, () -> query.build(createCall(arguments)));
+    SyntaxCheckException exception = assertThrows(SyntaxCheckException.class,
+        () -> query.build(createCall(arguments)));
+    assertEquals("mock_query requires at least two parameters", exception.getMessage());


penghuo

Thanks for the change!

joshuali925

thanks, merging in

See opensearch-project#604 (comment) for reference Signed-off-by: MaxKsyunz <[email protected]>

- Update QueryStringTest to check for SyntaxCheckException. SyntaxCheckException is correct when incorrect # of parameters See opensearch-project#604 (comment) for reference. - Introduce MultiFieldQuery and SingleFieldQuery base classes. - Extract FunctionResolver interface. FunctionResolver is now DefaultFunctionResolver. RelevanceFunctionResolver is a simplified function resolver for relevance search functions. - Removed tests from FilterQueryBuilderTest that verified exceptions thrown for invalid function calls. These scenarios are now handled by RelevanceQuery::build. Signed-off-by: MaxKsyunz <[email protected]>

- Update QueryStringTest to check for SyntaxCheckException. SyntaxCheckException is correct when incorrect # of parameters See #604 (comment) for reference. - Introduce MultiFieldQuery and SingleFieldQuery base classes. - Extract FunctionResolver interface. FunctionResolver is now DefaultFunctionResolver. RelevanceFunctionResolver is a simplified function resolver for relevance search functions. - Removed tests from FilterQueryBuilderTest that verified exceptions thrown for invalid function calls. These scenarios are now handled by RelevanceQuery::build. Signed-off-by: MaxKsyunz <[email protected]> Signed-off-by: MaxKsyunz <[email protected]>

MaxKsyunz and others added 30 commits May 10, 2022 12:51

Support match_phrase in AST expressions

e7c45f5

Signed-off-by: MaxKsyunz <[email protected]>

Support match_phrase in PPL syntax

a39f423

Signed-off-by: MaxKsyunz <[email protected]>

Added MATCH_QUERY to the list of supported functions in SQL parser.

8d0cd91

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

Support slop parameter for relevancy functions

7d451e5

Signed-off-by: MaxKsyunz <[email protected]>

Typo fix for previous commit. Expanded list of MATCH_* function argum…

7001b87

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

Integration tests for match_test in PPL

f0ff81c

Added a couple test for match_test in PPL. The tests are currently @ignore'd until more of the issue is complete. Signed-off-by: MaxKsyunz <[email protected]>

Support slop option

b883254

Signed-off-by: MaxKsyunz <[email protected]>

Tests to verify that PPL parser supports match_phrase

e1c071a

Signed-off-by: MaxKsyunz <[email protected]>

Refactor and style fixes

9d4f378

Signed-off-by: MaxKsyunz <[email protected]>

Refactor and fix style in OpenSearchFunctions.java

3c8bd62

Signed-off-by: MaxKsyunz <[email protected]>

Style fixes for OpenSearchFunctionsTest.java

4a0fe8b

Signed-off-by: MaxKsyunz <[email protected]>

Add tests for MATCH and MATCH_PHRASE functions.

659c4d7

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

Typo fix.

017fbb7

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

Fix style and minor refactor in PPLSyntaxParserTest.java

e93aa2b

Signed-off-by: MaxKsyunz <[email protected]>

Minor test improvements

f67b1ac

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

Parameterized test for match_phrase parsing

0e48992

Create a parametrized test for PPLSyntax parser. Signed-off-by: MaxKsyunz <[email protected]>

Add MATCH_PHRASE function to the function repository.

cc2100b

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

Finally added MATCH_PHASE query support to FilterQueryBuilder.

23f8fe6

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

Typo fix + EOL fix

775cb20

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

Typo fix

2aaf047

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

Adding tests for FilterQueryBuilder for MATCH_PHRASE. Some tests …

2316173

…for `MATCH` are also updated. Signed-off-by: Yury Fridlyand <[email protected]>

Address PR feedback.

e22879a

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

Add documentation for MATCH_PHRASE function.

a46d6ac

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

Remove duplicate definition of match_phrase in DSL.java

30ef3ba

Signed-off-by: MaxKsyunz <[email protected]>

Generate supported signatures for match and match_phrases

39ac1dd

Signed-off-by: MaxKsyunz <[email protected]>

Simplify match_phrase test.

0ec0aec

Signed-off-by: MaxKsyunz <[email protected]>

Some sample queries with match_phrase for parser test

a987aa3

Signed-off-by: MaxKsyunz <[email protected]>

Update PPL documentation.

14cb1a5

1. Use PPL instead of SQL as samples. 2. Use data that doctest runs with. Signed-off-by: MaxKsyunz <[email protected]>

Update query samples in docs. Add a new table to doctest to match the…

b4f5ba9

… samples. This fixes doctest. Signed-off-by: Yury Fridlyand <[email protected]>

Recover FUZZY_REWRITE - was deleted by mistake.

e4d155b

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

MaxKsyunz requested a review from a team as a code owner May 11, 2022 00:53

MaxKsyunz mentioned this pull request May 12, 2022

Support match_phrase function in SQL/PPL query engine #185

Closed

6 tasks

MaxKsyunz and others added 2 commits May 16, 2022 13:33

Support legacy syntax for match_phrase in the new SQL engine.

d6ea633

Add support for matchphrase as an alternative spelling of match_phrase. Mention in both SQL and PPL documentation that matchphrase is accepted as well as match_phrase. Signed-off-by: MaxKsyunz <[email protected]>

Merge pull request #59 from Bit-Quill/integ-match_phrase-#185-legacy-…

fedbbed

…update-v2 Support legacy syntax for match_phrase in the new SQL engine.

joshuali925 reviewed May 19, 2022

View reviewed changes

penghuo reviewed May 19, 2022

View reviewed changes

MaxKsyunz and others added 7 commits May 20, 2022 17:48

Added missed license headers.

1fae665

Signed-off-by: MaxKsyunz <[email protected]>

Add RelevanceQuery -- a base class for MatchQuery and MatchPhraseQuery.

d3667ce

Signed-off-by: MaxKsyunz <[email protected]>

Use SyntaxCheckException from RelevanceQuery.build

7101d07

Also, use getWriteableName() to get the name of the query in the error message. Signed-off-by: MaxKsyunz <[email protected]>

MatchPhraseQueryBuilder constructor requires a non-empty field name.

b5bf44e

This dummy instance of MatchPhraseQueryBuilder is used in RelevanceQuery when creating an exception to get the name of the query. Signed-off-by: MaxKsyunz <[email protected]>

Add missing newline at the end of the file.

17f0ded

Signed-off-by: MaxKsyunz <[email protected]>

Merge branch 'main' into integ-match_phrase-#185

80f0deb

Update tests from upstream/main to verify new behavior.

15775e8

This branch changed RelevanceQuery.build to throw SyntaxCheckException if it is called with less than two arguments. Previously, build would through SemanticCheckException. Signed-off-by: MaxKsyunz <[email protected]>

joshuali925 previously approved these changes May 24, 2022

View reviewed changes

penghuo reviewed May 24, 2022

View reviewed changes

Verify exception messages in RelevanceQueryBuildTest

ee4e319

Signed-off-by: MaxKsyunz <[email protected]>

MaxKsyunz dismissed joshuali925’s stale review via ee4e319 May 25, 2022 21:10

acarbonetto reviewed May 25, 2022

View reviewed changes

penghuo approved these changes May 25, 2022

View reviewed changes

joshuali925 approved these changes May 25, 2022

View reviewed changes

joshuali925 merged commit 26058b8 into opensearch-project:main May 25, 2022

MaxKsyunz pushed a commit to Bit-Quill/opensearch-project-sql that referenced this pull request Jul 26, 2022

SyntaxCheckException is correct when incorrect # of parameters

e7a87c6

See opensearch-project#604 (comment) for reference Signed-off-by: MaxKsyunz <[email protected]>

MaxKsyunz pushed a commit to Bit-Quill/opensearch-project-sql that referenced this pull request Aug 3, 2022

SyntaxCheckException is correct when incorrect # of parameters

52ce504

See opensearch-project#604 (comment) for reference Signed-off-by: MaxKsyunz <[email protected]>

MaxKsyunz pushed a commit to Bit-Quill/opensearch-project-sql that referenced this pull request Aug 10, 2022

SyntaxCheckException is correct when incorrect # of parameters

aafafe2

See opensearch-project#604 (comment) for reference Signed-off-by: MaxKsyunz <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support match_phrase filter function in SQL and PPL #604

Support match_phrase filter function in SQL and PPL #604

MaxKsyunz commented May 11, 2022 •

edited

Loading

codecov-commenter commented May 11, 2022 •

edited

Loading

joshuali925 left a comment

joshuali925 May 19, 2022

MaxKsyunz May 20, 2022

joshuali925 May 23, 2022

MaxKsyunz May 25, 2022

joshuali925 May 19, 2022

MaxKsyunz May 21, 2022

joshuali925 May 19, 2022

MaxKsyunz May 21, 2022

penghuo May 19, 2022 •

edited

Loading

MaxKsyunz May 21, 2022

penghuo May 19, 2022

MaxKsyunz May 21, 2022 •

edited

Loading

joshuali925 left a comment

penghuo May 24, 2022 •

edited

Loading

penghuo May 24, 2022

acarbonetto May 25, 2022

MaxKsyunz May 25, 2022

acarbonetto May 25, 2022

MaxKsyunz May 25, 2022

penghuo left a comment

joshuali925 left a comment

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

Support match_phrase filter function in SQL and PPL #604

Support match_phrase filter function in SQL and PPL #604

Conversation

MaxKsyunz commented May 11, 2022 • edited Loading

Description

Issues Resolved

Check List

codecov-commenter commented May 11, 2022 • edited Loading

Codecov Report

joshuali925 left a 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

penghuo May 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaxKsyunz May 21, 2022 • edited Loading

Choose a reason for hiding this comment

joshuali925 left a comment

Choose a reason for hiding this comment

penghuo May 24, 2022 • edited Loading

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

penghuo left a comment

Choose a reason for hiding this comment

joshuali925 left a comment

Choose a reason for hiding this comment

MaxKsyunz commented May 11, 2022 •

edited

Loading

codecov-commenter commented May 11, 2022 •

edited

Loading

penghuo May 19, 2022 •

edited

Loading

MaxKsyunz May 21, 2022 •

edited

Loading

penghuo May 24, 2022 •

edited

Loading