Relevance Field Validation for single-field Relevance Functions #186

forestmvey · 2022-12-09T00:44:41Z

Description

Handle fields validation for single-field relevance functions. Treating field argument as qualified name we can validate as an identifier. This does remove some tests as the type is now interpreted as identifier. Removed tests should be inconsequential as their coverage is done through the V2 parser and now fields identifier validation. Field parameter can be quoted or unquoted and both will be treated as qualified names.

Example Queries

select * from stackexchange_beer where match(invalid, 'beer');
select * from stackexchange_beer where match_bool_prefix(invalid, 'beer');
select * from stackexchange_beer where match_phrase(invalid, 'beer');
select * from stackexchange_beer where match_phrase_prefix(invalid, 'beer');

Issues Resolved

Issue: 613

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

…ass OpenSearchFunctions. Signed-off-by: forestmvey <[email protected]>

codecov · 2022-12-09T00:56:45Z

Codecov Report

Merging #186 (534f52b) into integ-relevance-fields-validation (2af7321) will decrease coverage by 2.50%.
The diff coverage is 100.00%.

@@                           Coverage Diff                           @@
##             integ-relevance-fields-validation     #186      +/-   ##
=======================================================================
- Coverage                                98.30%   95.80%   -2.51%     
+ Complexity                                3518     3517       -1     
=======================================================================
  Files                                      342      352      +10     
  Lines                                     8694     9348     +654     
  Branches                                   554      672     +118     
=======================================================================
+ Hits                                      8547     8956     +409     
- Misses                                     142      334     +192     
- Partials                                     5       58      +53

Flag	Coverage Δ
query-workbench	`62.76% <ø> (?)`
sql-engine	`98.30% <100.00%> (-0.01%)`	⬇️

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

Impacted Files	Coverage Δ
...expression/function/RelevanceFunctionResolver.java	`100.00% <ø> (ø)`
...h/sql/expression/function/OpenSearchFunctions.java	`100.00% <100.00%> (ø)`
...ript/filter/lucene/relevance/SingleFieldQuery.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%> (ø)`
...ch/public/components/QueryResults/QueryResults.tsx	`61.60% <0.00%> (ø)`
...h/public/components/QueryLanguageSwitch/Switch.tsx	`85.71% <0.00%> (ø)`
workbench/public/components/SQLPage/SQLPage.tsx	`100.00% <0.00%> (ø)`
workbench/public/utils/PanelWrapper.tsx	`100.00% <0.00%> (ø)`
workbench/public/components/PPLPage/PPLPage.tsx	`56.52% <0.00%> (ø)`
... and 5 more

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

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

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

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

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

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

...g/opensearch/sql/opensearch/storage/script/filter/lucene/relevance/SingleFieldQueryTest.java

…onAnalyzer. Signed-off-by: forestmvey <[email protected]>

Yury-Fridlyand · 2022-12-09T21:05:31Z

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

@@ -431,7 +435,8 @@ void should_build_match_phrase_query_with_default_parameters() {
            + "}",
        buildQuery(
            DSL.match_phrase(
-                DSL.namedArgument("field", literal("message")),
+                DSL.namedArgument("field",
+                    new ReferenceExpression("message", OPENSEARCH_TEXT_KEYWORD)),


Try different types in all tests, just for fun.

acarbonetto · 2022-12-13T20:32:43Z

Field parameter can be quoted or unquoted and both will be treated as qualified names.

Do you mean with backticks?

acarbonetto · 2022-12-12T18:55:59Z

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

@@ -475,7 +475,7 @@ private List<UnresolvedExpression> singleFieldRelevanceArguments(
    // to skip environment resolving and function signature resolving
    ImmutableList.Builder<UnresolvedExpression> builder = ImmutableList.builder();
    builder.add(new UnresolvedArgument("field",
-        new Literal(StringUtils.unquoteText(ctx.field.getText()), DataType.STRING)));
+        new QualifiedName(StringUtils.unquoteText(ctx.field.getText()))));


this should work for string literals as well as qualifiednames

Works for both string literals and qualifiedNames. The relevanceField is treated as a QualifiedName in the AstExpressionBuilder and later in the Analyzer can be visited as a QualifiedName.

acarbonetto · 2022-12-12T18:56:15Z

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

@@ -463,7 +463,7 @@ public void filteredDistinctCount() {
  public void matchPhraseQueryAllParameters() {
    assertEquals(
        AstDSL.function("matchphrasequery",
-            unresolvedArg("field", stringLiteral("test")),
+            unresolvedArg("field", qualifiedName("test")),


this should work for string literals as well as qualifiednames
we should test both, right?

acarbonetto · 2022-12-13T20:35:02Z

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

@@ -36,6 +37,18 @@ public void match_in_having() throws IOException {
    verifyDataRows(result, rows("Bates"));
  }

+  @Test
+  public void missing_field_test() {


include test with backticks -> SemanticCheckException
include test with quotes -> no error

forestmvey · 2022-12-13T21:10:03Z

Field parameter can be quoted or unquoted and both will be treated as qualified names.

Do you mean with backticks?

backticks are supported as well!

MaxKsyunz

LGTM!

…ance function field parameters. Signed-off-by: forestmvey <[email protected]>

forestmvey added 2 commits December 8, 2022 11:36

Fix relevance function fields are permissive when fields are missing.

73a043f

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

Move fields validation to FunctionExpression interface and derived cl…

63750a1

…ass OpenSearchFunctions. Signed-off-by: forestmvey <[email protected]>

forestmvey requested a review from a team December 9, 2022 00:50

MaxKsyunz reviewed Dec 9, 2022

View reviewed changes

Yury-Fridlyand reviewed Dec 9, 2022

View reviewed changes

forestmvey force-pushed the draft-relevance-fields-validation branch from e32b733 to f292f81 Compare December 9, 2022 17:02

Validating field and fields parameters in TypeEnv as part of Expressi…

2d25614

…onAnalyzer. Signed-off-by: forestmvey <[email protected]>

forestmvey force-pushed the draft-relevance-fields-validation branch from f292f81 to 2d25614 Compare December 9, 2022 17:15

forestmvey changed the title ~~Draft relevance fields validation~~ [DRAFT] Relevance Field Validation for single-field Relevance Functions Dec 9, 2022

Yury-Fridlyand reviewed Dec 9, 2022

View reviewed changes

forestmvey marked this pull request as ready for review December 12, 2022 23:56

forestmvey changed the title ~~[DRAFT] Relevance Field Validation for single-field Relevance Functions~~ Relevance Field Validation for single-field Relevance Functions Dec 13, 2022

acarbonetto approved these changes Dec 13, 2022

View reviewed changes

Yury-Fridlyand requested review from Yury-Fridlyand, GabeFernandez310, GumpacG, margarit-h and MitchellGale December 14, 2022 01:34

MaxKsyunz approved these changes Dec 15, 2022

View reviewed changes

Yury-Fridlyand approved these changes Dec 15, 2022

View reviewed changes

Updating tests to reflect using qualified name for single-field relev…

534f52b

…ance function field parameters. Signed-off-by: forestmvey <[email protected]>

forestmvey force-pushed the draft-relevance-fields-validation branch from cfd3144 to 534f52b Compare December 15, 2022 23:40

margarit-h approved these changes Dec 15, 2022

View reviewed changes

forestmvey closed this Dec 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Relevance Field Validation for single-field Relevance Functions #186

Relevance Field Validation for single-field Relevance Functions #186

forestmvey commented Dec 9, 2022 •

edited

Loading

codecov bot commented Dec 9, 2022 •

edited

Loading

Yury-Fridlyand Dec 9, 2022

acarbonetto commented Dec 13, 2022

acarbonetto Dec 12, 2022

forestmvey Dec 13, 2022

acarbonetto Dec 12, 2022

acarbonetto Dec 13, 2022

forestmvey commented Dec 13, 2022

MaxKsyunz left a comment

Relevance Field Validation for single-field Relevance Functions #186

Relevance Field Validation for single-field Relevance Functions #186

Conversation

forestmvey commented Dec 9, 2022 • edited Loading

Description

Example Queries

Issues Resolved

Check List

codecov bot commented Dec 9, 2022 • edited Loading

Codecov Report

Yury-Fridlyand Dec 9, 2022

Choose a reason for hiding this comment

acarbonetto commented Dec 13, 2022

acarbonetto Dec 12, 2022

Choose a reason for hiding this comment

forestmvey Dec 13, 2022

Choose a reason for hiding this comment

acarbonetto Dec 12, 2022

Choose a reason for hiding this comment

acarbonetto Dec 13, 2022

Choose a reason for hiding this comment

forestmvey commented Dec 13, 2022

MaxKsyunz left a comment

Choose a reason for hiding this comment

forestmvey commented Dec 9, 2022 •

edited

Loading

codecov bot commented Dec 9, 2022 •

edited

Loading