Support match_phrase filter function in SQL and PPL by MaxKsyunz · Pull Request #604 · opensearch-project/sql

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 <maxk@bitquilltech.com>

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

Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>

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

@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 <maxk@bitquilltech.com>

Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>

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

Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>

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

Create a parametrized test for PPLSyntax parser. Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>

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

…for `MATCH` are also updated. Signed-off-by: Yury Fridlyand <yuryf@bitquilltech.com>

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

Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>

1. Use PPL instead of SQL as samples. 2. Use data that doctest runs with. Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>

… samples. This fixes doctest. Signed-off-by: Yury Fridlyand <yuryf@bitquilltech.com>

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

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 <maxk@bitquilltech.com>

…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

    | 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 <maxk@bitquilltech.com>

Also, use getWriteableName() to get the name of the query in the error message. Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>

This dummy instance of MatchPhraseQueryBuilder is used in RelevanceQuery when creating an exception to get the name of the query. Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>

Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>

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 <maxk@bitquilltech.com>

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 <maxk@bitquilltech.com>

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 <maxk@bitquilltech.com>

- 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 <maxk@bitquilltech.com>

- 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 <maxk@bitquilltech.com> Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>

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

Support match_phrase in AST expressions

e7c45f5

Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>

Support match_phrase in PPL syntax

a39f423

Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>

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

8d0cd91

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

Support slop parameter for relevancy functions

7d451e5

Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>

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

7001b87

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

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 <maxk@bitquilltech.com>

Support slop option

b883254

Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>

Tests to verify that PPL parser supports match_phrase

e1c071a

Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>

Refactor and style fixes

9d4f378

Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>

Refactor and fix style in OpenSearchFunctions.java

3c8bd62

Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>

Style fixes for OpenSearchFunctionsTest.java

4a0fe8b

Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>

Add tests for MATCH and MATCH_PHRASE functions.

659c4d7

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

Typo fix.

017fbb7

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

Fix style and minor refactor in PPLSyntaxParserTest.java

e93aa2b

Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>

Minor test improvements

f67b1ac

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

Parameterized test for match_phrase parsing

0e48992

Create a parametrized test for PPLSyntax parser. Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>

Add MATCH_PHRASE function to the function repository.

cc2100b

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

Finally added MATCH_PHASE query support to FilterQueryBuilder.

23f8fe6

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

Typo fix + EOL fix

775cb20

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

Typo fix

2aaf047

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

Adding tests for FilterQueryBuilder for MATCH_PHRASE. Some tests …

2316173

…for `MATCH` are also updated. Signed-off-by: Yury Fridlyand <yuryf@bitquilltech.com>

Address PR feedback.

e22879a

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

Add documentation for MATCH_PHRASE function.

a46d6ac

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

Remove duplicate definition of match_phrase in DSL.java

30ef3ba

Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>

Generate supported signatures for match and match_phrases

39ac1dd

Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>

Simplify match_phrase test.

0ec0aec

Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>

Some sample queries with match_phrase for parser test

a987aa3

Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>

Update PPL documentation.

14cb1a5

1. Use PPL instead of SQL as samples. 2. Use data that doctest runs with. Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>

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

b4f5ba9

… samples. This fixes doctest. Signed-off-by: Yury Fridlyand <yuryf@bitquilltech.com>

Recover FUZZY_REWRITE - was deleted by mistake.

e4d155b

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

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 <maxk@bitquilltech.com>

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 <maxk@bitquilltech.com>

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

d3667ce

Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>

Use SyntaxCheckException from RelevanceQuery.build

7101d07

Also, use getWriteableName() to get the name of the query in the error message. Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>

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 <maxk@bitquilltech.com>

Add missing newline at the end of the file.

17f0ded

Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>

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 <maxk@bitquilltech.com>

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 <maxk@bitquilltech.com>

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 <maxk@bitquilltech.com>

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 <maxk@bitquilltech.com>

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 <maxk@bitquilltech.com>

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

Conversation

MaxKsyunz commented May 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issues Resolved

Check List

Uh oh!

codecov-commenter commented May 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

joshuali925 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!

penghuo May 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

MaxKsyunz May 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joshuali925 left a comment

Choose a reason for hiding this comment

Uh oh!

penghuo May 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

penghuo left a comment

Choose a reason for hiding this comment

Uh oh!

joshuali925 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

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