Add MatchPhraseQuery As Alternate Syntax for Match_Phrase Function #165

GabeFernandez310 · 2022-11-14T16:51:20Z

Description

Adds matchphrasequery as alternate syntax for match_phrase function which currently exists in opensearch

Queries can be performed using the following syntax :

matchphrasequery(field, 'query')

Example:

SELECT field FROM index WHERE matchphrasequery(field, 'query');

This query should return the same result as match_phrase(field, 'query')

Issues Resolved

AOS-765

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-11-14T16:57:41Z

Codecov Report

Merging #165 (fbcee86) into integ-add-legacy-syntax-for-matchphrase-function (e280866) will decrease coverage by 35.52%.
The diff coverage is n/a.

@@                                   Coverage Diff                                   @@
##             integ-add-legacy-syntax-for-matchphrase-function     #165       +/-   ##
=======================================================================================
- Coverage                                               98.28%   62.76%   -35.53%     
=======================================================================================
  Files                                                     345       10      -335     
  Lines                                                    8580      658     -7922     
  Branches                                                  547      119      -428     
=======================================================================================
- Hits                                                     8433      413     -8020     
- Misses                                                    142      192       +50     
- Partials                                                    5       53       +48

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 Δ
...h/sql/expression/function/BuiltinFunctionName.java
...h/sql/expression/function/OpenSearchFunctions.java
...arch/storage/script/filter/FilterQueryBuilder.java
...l/response/format/SimpleJsonResponseFormatter.java
...pensearch/sql/protocol/response/format/Format.java
...s/request/system/PrometheusListMetricsRequest.java
...ript/filter/lucene/relevance/QueryStringQuery.java
...a/org/opensearch/sql/utils/DateTimeFormatters.java
.../org/opensearch/sql/ppl/utils/ArgumentFactory.java
...a/org/opensearch/sql/data/model/ExprNullValue.java
... and 345 more

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

docs/user/dql/functions.rst

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

acarbonetto · 2022-11-14T17:26:59Z

sql/src/main/antlr/OpenSearchSQLParser.g4

@@ -426,7 +426,8 @@ systemFunctionName

 singleFieldRelevanceFunctionName
    : MATCH | MATCH_PHRASE | MATCHPHRASE
-    | MATCH_BOOL_PREFIX | MATCH_PHRASE_PREFIX
+    | MATCHPHRASEQUERY | MATCH_BOOL_PREFIX


nit: add it to the previous line with MATCH_PHRASE | MATCHPHRASE

Fixed in d04916b

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

MaxKsyunz

Please make sure SQL Java CI workflow passes.
Looks to be failing on checkstyle.

GabeFernandez310 · 2022-11-15T15:53:19Z

Please make sure SQL Java CI workflow passes. Looks to be failing on checkstyle.

Fixed!

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

MaxKsyunz · 2022-11-17T18:44:34Z

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

+    JSONObject result1 = executeJdbcRequest(String.format(query1, TEST_INDEX_PHRASE));
+    JSONObject result2 = executeJdbcRequest(String.format(query2, TEST_INDEX_PHRASE));
+    JSONObject result3 = executeJdbcRequest(String.format(query3, TEST_INDEX_PHRASE));
+    assertEquals(result1.toString(), result2.toString());


assertEquals(result1, result2) should work.

I tried to just do assertEquals(result1, result2) originally but the test was failing with the following message (I separated it onto different lines so it's more readable)...

expected: org.json.JSONObject<{"schema":[{"name":"phrase","type":"text"}],"total":2,"datarows":[["quick fox"],["quick fox here"]],"size":2,"status":200}> but was: org.json.JSONObject<{"schema":[{"name":"phrase","type":"text"}],"total":2,"datarows":[["quick fox"],["quick fox here"]],"size":2,"status":200}>

I wasn't sure why it was failing, but I assumed it may have something to do with the equality operator and how the objects might be stored in memory, so I added the .toString() as a hacky way to fix it. Is there a different way to compare the JSON objects that might be better?

I found that it is better to use JSONObject::similar instead of JSONObject::equals. So assert be like

assertTrue(result1.similar(result2));

This has been fixed. assertEquals was replaced with assertTrue.

MaxKsyunz · 2022-11-17T21:20:14Z

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

+  @Test
+  public void test_SyntaxCheckException_when_invalid_parameter_match_phrase_syntax() {
+    List<Expression> arguments = List.of(
+        dsl.namedArgument("field", "test"),


Please use DSL.namedArgument. Either works now, but dsl.namedArgument is going away immanently upstream.

This has been fixed

MaxKsyunz

As a feature, this looks good @GabeFernandez310!

I do have a few stylistic comments:

MatchPhraseQueryTest.this. ... looks odd. Is it necessary?
By convention, when writing an assert the first argument is the expected value. Some of your asserts have that flipped.
assertEquals(result1.toString(), result2.toString()) is unnecessary -- JSONObject overrides equals directly.
Use DSL.namedArgument instead of dsl.namedArgument. I realize the later is all over the codebase, but it is unnecessary -- namedArgument is a static method, and will go away in upstream imminently.

I flagged at least one example of each with a comment, but I see there are others instances.

MaxKsyunz · 2022-11-17T21:22:02Z

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

+    List<Expression> arguments = List.of();
+    assertThrows(SyntaxCheckException.class,
+        () -> matchPhraseQuery.build(new MatchPhraseExpression(
+            arguments, MatchPhraseQueryTest.this.matchPhraseWithUnderscoreName)));


Suggested change

arguments, MatchPhraseQueryTest.this.matchPhraseWithUnderscoreName)));

arguments, matchPhraseWithUnderscoreName)));

isn't that sufficient?

This has been fixed

MaxKsyunz · 2022-11-17T21:24:59Z

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

@@ -448,6 +466,15 @@ private static Stream<String> generateMatchPhraseQueries() {
    return generateQueries("match_phrase", matchPhraseArgs);
  }

+  private static Stream<String> generateMatchPhraseQueryQueries() {


Do we need both generateMatchPhraseQueryQueries and `matchPhraseQueryComplexQueries?

After looking closer I felt that they weren't both needed. I removed generateMatchPhraseQueryQueries

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

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

* Import h3 library (#154) Made following changes to make it compatible: 1. Rename package from elasticsearch to opensearch.geospatial 2. Update License headers 3. Update build file 4. Update settings to include sub projects * Use Transport Request (#164) Remove usage of deprecated BaseNodeRequest * Update http client package to resolve build failure (#168) (#171) * Introduce H3 min resolution constant (#165) H3 version 1 has 16 resolutions, numbered 0 through 15. Introduced a constant to represent min value, similar to max value. * Add geohex aggregation (#160) This aggregation will use uber's h3 to group coordinates into H3 cell. Created new aggregation type geohex_grid. The precision will be between 0 and 15. This aggreation has default precision as 5, similar to geohash and geotile. Signed-off-by: Vijayan Balasubramanian <[email protected]> * Add integration test (#176) Included integration test for geohex_grid. Signed-off-by: Vijayan Balasubramanian <[email protected]>

acarbonetto approved these changes Nov 14, 2022

View reviewed changes

MaxKsyunz reviewed Nov 14, 2022

View reviewed changes

GabeFernandez310 marked this pull request as ready for review November 15, 2022 15:07

GabeFernandez310 requested review from a team, forestmvey, GumpacG, MaxKsyunz, MitchellGale, margarit-h and Yury-Fridlyand and removed request for a team November 15, 2022 23:15

Yury-Fridlyand approved these changes Nov 15, 2022

View reviewed changes

GumpacG reviewed Nov 16, 2022

View reviewed changes

...st/java/org/opensearch/sql/opensearch/storage/script/filter/lucene/MatchPhraseQueryTest.java Outdated Show resolved Hide resolved

GumpacG approved these changes Nov 16, 2022

View reviewed changes

MaxKsyunz reviewed Nov 17, 2022

View reviewed changes

GabeFernandez310 force-pushed the dev-add-legacy-syntax-for-matchphrase-function branch from 38b7714 to 72580ed Compare November 21, 2022 15:59

GabeFernandez310 marked this pull request as draft November 21, 2022 16:01

GabeFernandez310 added 2 commits November 21, 2022 08:12

Added Tests

595ab32

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

Changed Documentation

5c48bb5

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

GabeFernandez310 force-pushed the dev-add-legacy-syntax-for-matchphrase-function branch from 72580ed to 9e112a3 Compare November 21, 2022 16:14

Added Implementation.

fbcee86

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

GabeFernandez310 force-pushed the dev-add-legacy-syntax-for-matchphrase-function branch from 9e112a3 to fbcee86 Compare November 21, 2022 17:00

GabeFernandez310 marked this pull request as ready for review November 21, 2022 17:15

forestmvey approved these changes Nov 21, 2022

View reviewed changes

margarit-h approved these changes Nov 21, 2022

View reviewed changes

GabeFernandez310 requested a review from MaxKsyunz November 23, 2022 16:38

MaxKsyunz approved these changes Nov 23, 2022

View reviewed changes

MitchellGale approved these changes Nov 24, 2022

View reviewed changes

GabeFernandez310 merged commit 9c769dd into integ-add-legacy-syntax-for-matchphrase-function Nov 24, 2022

GabeFernandez310 mentioned this pull request Nov 25, 2022

Add Week_Of_Year Function To OpenSearch #175

Merged

6 tasks

GabeFernandez310 deleted the dev-add-legacy-syntax-for-matchphrase-function branch December 1, 2022 05:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add MatchPhraseQuery As Alternate Syntax for Match_Phrase Function #165

Add MatchPhraseQuery As Alternate Syntax for Match_Phrase Function #165

GabeFernandez310 commented Nov 14, 2022 •

edited

Loading

codecov bot commented Nov 14, 2022 •

edited

Loading

acarbonetto Nov 14, 2022

GabeFernandez310 Nov 15, 2022

MaxKsyunz left a comment

GabeFernandez310 commented Nov 15, 2022

MaxKsyunz Nov 17, 2022

GabeFernandez310 Nov 18, 2022

Yury-Fridlyand Nov 19, 2022

GabeFernandez310 Nov 21, 2022

MaxKsyunz Nov 17, 2022

GabeFernandez310 Nov 21, 2022

MaxKsyunz left a comment

MaxKsyunz Nov 17, 2022

GabeFernandez310 Nov 21, 2022

MaxKsyunz Nov 17, 2022

GabeFernandez310 Nov 21, 2022

	arguments, MatchPhraseQueryTest.this.matchPhraseWithUnderscoreName)));
	arguments, matchPhraseWithUnderscoreName)));

Add MatchPhraseQuery As Alternate Syntax for Match_Phrase Function #165

Add MatchPhraseQuery As Alternate Syntax for Match_Phrase Function #165

Conversation

GabeFernandez310 commented Nov 14, 2022 • edited Loading

Description

Issues Resolved

Check List

codecov bot commented Nov 14, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaxKsyunz left a comment

Choose a reason for hiding this comment

GabeFernandez310 commented Nov 15, 2022

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

MaxKsyunz 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

GabeFernandez310 commented Nov 14, 2022 •

edited

Loading

codecov bot commented Nov 14, 2022 •

edited

Loading