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

Add BETWEEN expression in v2 engine #1163

Merged
merged 8 commits into from
Dec 16, 2022

Conversation

dai-chen
Copy link
Collaborator

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

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

Description

Support BETWEEN and NOT BETWEEN expression in v2 engine to avoid fallback to legacy code.

  • AST Layer: build Between AST node and wrap it by Not node in the case of NOT BETWEEN.
  • Analyzer: rewrite Between node to basic comparison predicate. Specifically, rewrite A BETWEEN X AND Y to X <= A AND A <= Y.

Benefits of the implementation:

  1. No need to add extra function of between and not_between
  2. No need to redefine the function signature: as @MaxKsyunz commented, BETWEEN boolean AND boolean is not a valid signature. The current implementation delegate this to LTE and GTE function signature.
  3. No need to implement Lucene push down (need extra work in optimizer or OpenSearch storage engine if we'd like to merge multiple Lucene range query together. This is out of scope of this PR)
  4. Decomposing BETWEEN to basic predicate unit gives logical optimizer more freedom to optimize further, such as simplify multiple comparison expression, ex. age <= 50 AND age BETWEEN 10 AND 60.

This approach maybe applied to other functions: NOT NULL, NOT LIKE etc.

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 enhancement New feature or request legacy Issues related to legacy query engine to be deprecated labels Dec 13, 2022
@dai-chen dai-chen self-assigned this Dec 13, 2022
@codecov-commenter
Copy link

codecov-commenter commented Dec 13, 2022

Codecov Report

Merging #1163 (7743e59) into 2.x (c5e8fc0) will decrease coverage by 2.49%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##                2.x    #1163      +/-   ##
============================================
- Coverage     98.31%   95.81%   -2.50%     
- Complexity     3521     3524       +3     
============================================
  Files           342      352      +10     
  Lines          8700     9371     +671     
  Branches        554      674     +120     
============================================
+ Hits           8553     8979     +426     
- Misses          142      334     +192     
- Partials          5       58      +53     
Flag Coverage Δ
query-workbench 62.76% <ø> (?)
sql-engine 98.31% <100.00%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
...rg/opensearch/sql/analysis/ExpressionAnalyzer.java 100.00% <100.00%> (ø)
...pensearch/sql/sql/parser/AstExpressionBuilder.java 100.00% <100.00%> (ø)
...n/java/org/opensearch/sql/utils/OperatorUtils.java 100.00% <0.00%> (ø)
...h/public/components/QueryLanguageSwitch/Switch.tsx 85.71% <0.00%> (ø)
workbench/public/components/PPLPage/PPLPage.tsx 56.52% <0.00%> (ø)
...ublic/components/QueryResults/QueryResultsBody.tsx 68.32% <0.00%> (ø)
...ch/public/components/QueryResults/QueryResults.tsx 61.60% <0.00%> (ø)
workbench/public/utils/PanelWrapper.tsx 100.00% <0.00%> (ø)
workbench/public/application.tsx 0.00% <0.00%> (ø)
workbench/public/components/app.tsx 0.00% <0.00%> (ø)
... and 3 more

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

Signed-off-by: Chen Dai <daichen@amazon.com>
Signed-off-by: Chen Dai <daichen@amazon.com>
Signed-off-by: Chen Dai <daichen@amazon.com>
Signed-off-by: Chen Dai <daichen@amazon.com>
Signed-off-by: Chen Dai <daichen@amazon.com>
Signed-off-by: Chen Dai <daichen@amazon.com>
Signed-off-by: Chen Dai <daichen@amazon.com>
@dai-chen dai-chen marked this pull request as ready for review December 14, 2022 00:15
@dai-chen dai-chen requested a review from a team as a code owner December 14, 2022 00:15
Copy link
Collaborator

@Yury-Fridlyand Yury-Fridlyand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does PPL have BETWEEN? Should it? Why not?

It is interesting how it works with Bit-Quill#129, e.g.

SELECT DATE('2022-11-22') BETWEEN DATETIME('1984-02-01 10:20:30') AND NOW()

@dai-chen
Copy link
Collaborator Author

dai-chen commented Dec 14, 2022

Does PPL have BETWEEN? Should it? Why not?

It is interesting how it works with Bit-Quill#129, e.g.

SELECT DATE('2022-11-22') BETWEEN DATETIME('1984-02-01 10:20:30') AND NOW()

Thanks for pointing this out! I did a quick check earlier but couldn't find anything in SPL. Please let me know if you're aware. Yeah, I think it will be much more convenient for users with more complete function and type cast. :)

@Yury-Fridlyand
Copy link
Collaborator

It works, I merged branches and run a test:

opensearchsql> SELECT DATE('2022-11-22') BETWEEN DATETIME('1984-02-01 10:20:30') AND NOW();
fetched rows / total rows = 1/1
+------------------------------------------------------------------------+
| DATE('2022-11-22') BETWEEN DATETIME('1984-02-01 10:20:30') AND NOW()   |
|------------------------------------------------------------------------|
| True                                                                   |
+------------------------------------------------------------------------+

I can't find it in SPL too, but we can add it to PPL regardless of that.

Copy link
Collaborator

@MaxKsyunz MaxKsyunz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM @dai-chen!

Should this be merged to 2.x branch or main now?

@dai-chen
Copy link
Collaborator Author

LGTM @dai-chen!

Should this be merged to 2.x branch or main now?

Yes, because this PR was raised during the transition, I've added backport main label.

@dai-chen dai-chen merged commit 6c0af83 into opensearch-project:2.x Dec 16, 2022
opensearch-trigger-bot bot pushed a commit that referenced this pull request Dec 16, 2022
* Add between grammar and in-memory impl

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

* Add comparison test for between

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

* Add doctest for between

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

* Add not between support

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

* Fix doctest failure

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

* Refactor to rewrite to basic comparison expression

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

* Clean up unused code

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

* Prepare to publish PR

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

Signed-off-by: Chen Dai <daichen@amazon.com>
(cherry picked from commit 6c0af83)
@dai-chen dai-chen deleted the add-between-expression-v2 branch December 16, 2022 18:41
dai-chen added a commit that referenced this pull request Dec 16, 2022
* Add between grammar and in-memory impl

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

* Add comparison test for between

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

* Add doctest for between

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

* Add not between support

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

* Fix doctest failure

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

* Refactor to rewrite to basic comparison expression

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

* Clean up unused code

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

* Prepare to publish PR

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

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

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 main enhancement New feature or request legacy Issues related to legacy query engine to be deprecated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants