Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow common keywords and scalar function name used as identifier #1191

Merged
merged 3 commits into from
Jan 3, 2023

Conversation

dai-chen
Copy link
Collaborator

@dai-chen dai-chen commented Dec 16, 2022

Signed-off-by: Chen Dai daichen@amazon.com

Description

  1. score is not used by any grammar rule. It maybe defined as keyword due to copy-paste from legacy grammar file. Remove it from lexer file to fix [BUG] score cannot be used in field name #788. Pending on Allow common keywords and scalar function name used as identifier #1191 (comment)
  2. type is reserved by relevancy function argument. Added it to keywordsCanBeId rule temporarily to fix [BUG] GROUP BY Missing field returns error #743. Can be removed once all relevancy function moved out of ANTLR and core engine.
  3. Some of scalar function names and datetime literals are added to keywordsCanBeId so users can use them without backquotes, such as DATE, CURDATE etc.
    a. However, I double checked MySQL grammar and our legacy grammar that all scalar function names should be allowed. In this way we don't need to add function name to identifier or keywordsCanBeId rule one by one in future.
    b. To make this happen, I have to remove OpenSearchSQLIdentifierParser.g4 to avoid circular reference. Previously it maybe intended to be shared by different language. However it is not necessary now.

Note that PPL identifier doesn't have this issue and thus no change required.

Reference

simpleId
    : ID
    | DOT_ID
    | DOUBLE_QUOTE_ID
    | BACKTICK_QUOTE_ID
    | keywordsCanBeId
    | functionNameBase
    ;

Testing

Add comparison test for the second ignored test.

$ ./gradlew :integ-test:comparisonTest -Dqueries=queries/aggregation.txt
{
  "summary": {
    "total": 15,
    "failure": 0,
    "success": 15
  },
  "tests": [
    ...
    {
      "result": "Success",
      "id": 15,
      "sql": "SELECT LOG(MAX(AvgTicketPrice) + MIN(AvgTicketPrice)) FROM opensearch_dashboards_sample_data_flights"
    }
  ]
}

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.

Signed-off-by: Chen Dai <daichen@amazon.com>
@dai-chen dai-chen added the bug Something isn't working label Dec 16, 2022
@dai-chen dai-chen self-assigned this Dec 16, 2022
@codecov-commenter
Copy link

codecov-commenter commented Dec 16, 2022

Codecov Report

Merging #1191 (e8b924e) into main (eb595c7) will decrease coverage by 2.47%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##               main    #1191      +/-   ##
============================================
- Coverage     98.31%   95.83%   -2.48%     
- Complexity     3524     3552      +28     
============================================
  Files           342      356      +14     
  Lines          8711     9414     +703     
  Branches        555      673     +118     
============================================
+ Hits           8564     9022     +458     
- Misses          142      334     +192     
- Partials          5       58      +53     
Flag Coverage Δ
query-workbench 62.76% <ø> (?)
sql-engine 98.32% <ø> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
...rc/main/java/org/opensearch/sql/storage/Table.java 100.00% <0.00%> (ø)
...c/main/java/org/opensearch/sql/expression/DSL.java 100.00% <0.00%> (ø)
...org/opensearch/sql/planner/DefaultImplementor.java 100.00% <0.00%> (ø)
...opensearch/sql/planner/logical/LogicalPlanDSL.java 100.00% <0.00%> (ø)
...search/sql/planner/optimizer/pattern/Patterns.java 100.00% <0.00%> (ø)
...arch/sql/expression/datetime/DateTimeFunction.java 100.00% <0.00%> (ø)
...ch/sql/planner/logical/LogicalPlanNodeVisitor.java 100.00% <0.00%> (ø)
...ch/sql/planner/optimizer/LogicalPlanOptimizer.java 100.00% <0.00%> (ø)
...h/sql/expression/function/BuiltinFunctionName.java 100.00% <0.00%> (ø)
...h/sql/expression/function/OpenSearchFunctions.java 100.00% <0.00%> (ø)
... and 17 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@dai-chen
Copy link
Collaborator Author

dai-chen commented Dec 19, 2022

4 integration tests failed. Checked one of them and found it was related to #1133. Basically it was caused by _score become score after parsing and thus fallback to legacy engine didn't happen as expect.

@dai-chen
Copy link
Collaborator Author

@acarbonetto This changes seem depend on the underscore issue in #1133 (which maybe depend on adding meta field in v2). I will pause the work now and wait for your changes. Thanks!

Signed-off-by: Chen Dai <daichen@amazon.com>
@dai-chen dai-chen changed the title Allow score, type and scalar function name used as identifier Allow common keywords and scalar function name used as identifier Dec 20, 2022
@dai-chen
Copy link
Collaborator Author

I reverted the changes on score. Will publish separate PR once the pending item done.

@dai-chen dai-chen marked this pull request as ready for review December 20, 2022 21:10
@dai-chen dai-chen requested a review from a team as a code owner December 20, 2022 21:10
@acarbonetto
Copy link
Collaborator

@acarbonetto This changes seem depend on the underscore issue in #1133 (which maybe depend on adding meta field in v2). I will pause the work now and wait for your changes. Thanks!

Thanks. Working on it today. I'll let you know how it'll work out. If the score function is obvious to include, I'll do that too.

@dai-chen
Copy link
Collaborator Author

@acarbonetto This changes seem depend on the underscore issue in #1133 (which maybe depend on adding meta field in v2). I will pause the work now and wait for your changes. Thanks!

Thanks. Working on it today. I'll let you know how it'll work out. If the score function is obvious to include, I'll do that too.

Thanks! No rush. I've reverted score changes and sending this PR out first. :)

acarbonetto
acarbonetto previously approved these changes Dec 21, 2022
penghuo
penghuo previously approved these changes Dec 22, 2022
Signed-off-by: Chen Dai <daichen@amazon.com>
@dai-chen dai-chen merged commit 2f4924a into opensearch-project:main Jan 3, 2023
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jan 3, 2023
)

* Allow score, type and scalar function name as identifier

Signed-off-by: Chen Dai <daichen@amazon.com>

* Revert score and ignore failed IT

Signed-off-by: Chen Dai <daichen@amazon.com>

* Add comparison test to address PR comment

Signed-off-by: Chen Dai <daichen@amazon.com>

Signed-off-by: Chen Dai <daichen@amazon.com>
(cherry picked from commit 2f4924a)
dai-chen added a commit that referenced this pull request Jan 3, 2023
) (#1212)

* Allow score, type and scalar function name as identifier

Signed-off-by: Chen Dai <daichen@amazon.com>

* Revert score and ignore failed IT

Signed-off-by: Chen Dai <daichen@amazon.com>

* Add comparison test to address PR comment

Signed-off-by: Chen Dai <daichen@amazon.com>

Signed-off-by: Chen Dai <daichen@amazon.com>
(cherry picked from commit 2f4924a)

Co-authored-by: Chen Dai <daichen@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x bug Something isn't working SQL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] score cannot be used in field name [BUG] GROUP BY Missing field returns error
5 participants