Update for SQL and PPL parser to accept `simple_query_string` #53

Yury-Fridlyand · 2022-05-12T17:57:40Z

Description

SQL and PPL parser update to support simple_query_string, see opensearch-project#192
Require #54 to support end-to-end flow.

TODO:

tests
rework on AstExpressionBuilder.java x2

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.

… field list as the first argument. Signed-off-by: Yury Fridlyand <[email protected]>

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

Yury-Fridlyand · 2022-05-12T18:39:15Z

ppl/src/main/antlr/OpenSearchPPLParser.g4

-        field=relevanceArgValue COMMA query=relevanceArgValue
+        field=relevanceField COMMA query=relevanceQuery
+        (COMMA relevanceArg)* RT_PRTHS
+    | relevanceFunctionNameEx LT_PRTHS


Should we rename it? Like relevanceFunctionType1Name and relevanceFunctionType2Name.
Or maybe to make relevanceExpressionType1 and relevanceExpressionType2.

Please be explicit with your naming as possible. Type1 and Type2 don't mean anything. If there's an order associated with the types in the list (and no other way to differentiate the types) we can call them first and second or something similar.

Let's change it to have separate rule for single-field relevance functions and a separate rule for multi-field relevance functions.

This way we can avoid the cumbersome ?: in AstExpressionBuilder. Instead there will be one method to visit single field relevance functions and another to visit multi-field relevance functions.

Yury-Fridlyand · 2022-05-12T18:41:01Z

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

    // all the arguments are defaulted to string values
    // to skip environment resolving and function signature resolving
    ImmutableList.Builder<UnresolvedExpression> builder = ImmutableList.builder();
+    ParserRuleContext field = ctx.field;
+    if (field == null)
+      field = ctx.fields;
    builder.add(new UnresolvedArgument("field",
-        new Literal(StringUtils.unquoteText(ctx.field.getText()), DataType.STRING)));
+        new Literal(StringUtils.unquoteText(field.getText()), DataType.STRING)));
    builder.add(new UnresolvedArgument("query",
        new Literal(StringUtils.unquoteText(ctx.query.getText()), DataType.STRING)));
    ctx.relevanceArg().forEach(v -> builder.add(new UnresolvedArgument(


Should we create 2 different functions as relevanceArguments?

Should we skip StringUtils.unquoteText for fields?

MaxKsyunz · 2022-05-12T18:42:09Z

Could you please make the SQL Java CI actions pass?

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

Yury-Fridlyand · 2022-05-12T20:53:44Z

Could you please make the SQL Java CI actions pass?

dc75ce8 fixes code style, rest failures require tests.

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

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

@@ -332,8 +336,12 @@ private List<UnresolvedExpression> relevanceArguments(RelevanceExpressionContext
    // all the arguments are defaulted to string values
    // to skip environment resolving and function signature resolving
    ImmutableList.Builder<UnresolvedExpression> builder = ImmutableList.builder();
+    ParserRuleContext field = ctx.field;
+    if (field == null) {


use conditions to define the ParseRuleContext, for example:
ParserRuleContext field = ctx.field != null ? ctx.field : ctx.fields;

acarbonetto · 2022-05-12T21:28:30Z

sql/src/main/antlr/OpenSearchSQLLexer.g4

@@ -357,6 +358,8 @@ BIT_XOR_OP:                         '^';
 DOT:                                '.';
 LR_BRACKET:                         '(';
 RR_BRACKET:                         ')';
+LT_SQR_PRTHS:                       '[';


where's this name coming from?
Just wondering why not something like: LR_SQUARE_BRACKET and RR_SQUARE_BRACKET

acarbonetto · 2022-05-12T21:29:50Z

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

@@ -362,8 +364,12 @@ public UnresolvedExpression visitConvertedDataType(

  @Override
  public UnresolvedExpression visitRelevanceFunction(RelevanceFunctionContext ctx) {
+    ParserRuleContext func = ctx.relevanceFunctionNameEx();


ParserRuleContext func = ctx.relevanceFunctionNameEx() != null ctx.relevanceFunctionNameEx() : ctx.relevanceFunctionName();

acarbonetto · 2022-05-12T21:30:47Z

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

@@ -393,8 +399,12 @@ private List<UnresolvedExpression> relevanceArguments(RelevanceFunctionContext c
    // all the arguments are defaulted to string values
    // to skip environment resolving and function signature resolving
    ImmutableList.Builder<UnresolvedExpression> builder = ImmutableList.builder();
+    ParserRuleContext field = ctx.field;


ParserRuleContext field = ctx.field != null ctx.field : ctx.field2;

MaxKsyunz · 2022-05-12T21:39:56Z

ppl/src/main/antlr/OpenSearchPPLParser.g4

-        field=relevanceArgValue COMMA query=relevanceArgValue
+        field=relevanceField COMMA query=relevanceQuery
+        (COMMA relevanceArg)* RT_PRTHS
+    | relevanceFunctionNameEx LT_PRTHS


Let's change it to have separate rule for single-field relevance functions and a separate rule for multi-field relevance functions.

This way we can avoid the cumbersome ?: in AstExpressionBuilder. Instead there will be one method to visit single field relevance functions and another to visit multi-field relevance functions.

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

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

…hub.com/Bit-Quill/opensearch-project-sql into dev-simple_query_string-#192-sql-simple

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

rejectMultipleStars test is failing right now. Run it only if WIP_TEST environment variable is set to something. Signed-off-by: MaxKsyunz <[email protected]>

It now only accepts ['*'] or ['*'] or a list of quoted identifiers. Skip a test for an existing bug in match implementation unless env variable GITHUB_ISSUE is set to 182 -- the ticket # it was added under. Signed-off-by: MaxKsyunz <[email protected]>

Yury-Fridlyand · 2022-05-18T00:03:14Z

Closed as of #61.

Yury-Fridlyand added 2 commits May 6, 2022 17:07

Add simple_query_string to SQL and PPL parsers. Modify it to accept…

6e082af

… field list as the first argument. Signed-off-by: Yury Fridlyand <[email protected]>

Update parser to allow multiple fields for simple_query_string only.

607b330

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

Yury-Fridlyand mentioned this pull request May 12, 2022

Implementation of simple_query_string #54

Closed

6 tasks

Yury-Fridlyand requested a review from a team May 12, 2022 18:27

Yury-Fridlyand commented May 12, 2022

View reviewed changes

Fix code styling.

dc75ce8

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

acarbonetto reviewed May 12, 2022

View reviewed changes

MaxKsyunz requested changes May 12, 2022

View reviewed changes

MaxKsyunz and others added 7 commits May 13, 2022 14:40

Added SQL parser tests for simple_query_string.

70a7457

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

Update parser - separate parsing function with one and multiple fields.

6c1d9bb

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

Merge branch 'dev-simple_query_string-#192-sql-simple' of https://git…

3a2ff8a

…hub.com/Bit-Quill/opensearch-project-sql into dev-simple_query_string-#192-sql-simple

Improve error message in assertRejected

a013d64

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

Re-arrange unit tests to highlight what's important.

d9617ae

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

Hide failing test

0256386

rejectMultipleStars test is failing right now. Run it only if WIP_TEST environment variable is set to something. Signed-off-by: MaxKsyunz <[email protected]>

Yury-Fridlyand mentioned this pull request May 18, 2022

Implementation for simple_query_string. #61

Closed

6 tasks

Yury-Fridlyand closed this May 18, 2022

Yury-Fridlyand deleted the dev-simple_query_string-#192-sql-simple branch May 31, 2022 21:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update for SQL and PPL parser to accept `simple_query_string` #53

Update for SQL and PPL parser to accept `simple_query_string` #53

Yury-Fridlyand commented May 12, 2022 •

edited

Loading

Yury-Fridlyand May 12, 2022

acarbonetto May 12, 2022

MaxKsyunz May 12, 2022

Yury-Fridlyand May 12, 2022

MaxKsyunz commented May 12, 2022

Yury-Fridlyand commented May 12, 2022

acarbonetto May 12, 2022

acarbonetto May 12, 2022

acarbonetto May 12, 2022 •

edited

Loading

acarbonetto May 12, 2022

MaxKsyunz May 12, 2022

Yury-Fridlyand commented May 18, 2022

Update for SQL and PPL parser to accept simple_query_string #53

Update for SQL and PPL parser to accept simple_query_string #53

Conversation

Yury-Fridlyand commented May 12, 2022 • edited Loading

Description

Issues Resolved

Check List

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 commented May 12, 2022

Yury-Fridlyand commented May 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acarbonetto May 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yury-Fridlyand commented May 18, 2022

Update for SQL and PPL parser to accept `simple_query_string` #53

Update for SQL and PPL parser to accept `simple_query_string` #53

Yury-Fridlyand commented May 12, 2022 •

edited

Loading

acarbonetto May 12, 2022 •

edited

Loading