Add PPL Between functionality #758

dr-lilienthal · 2024-10-09T12:19:59Z

Description

Add PPL Between functionality

Issues Resolved

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.

YANG-DB · 2024-10-09T16:05:42Z

hi @dr-lilienthal
plz fix DCO ...

YANG-DB · 2024-10-11T16:27:29Z

@dr-lilienthal
please add documentation in the following locations:

../docs/ppl-lang/PPL-Example-Commands.md
../docs/ppl-lang/README.md
Thanks

dr-lilienthal · 2024-10-17T18:26:56Z

@YANG-DB ready for review CC: @salyh

...cala/org/opensearch/flint/spark/ppl/PPLLogicalPlanBetweenExpressionTranslatorTestSuite.scala

LantaoJin · 2024-10-25T11:58:13Z

ppl-spark-integration/src/main/antlr4/OpenSearchPPLParser.g4

@@ -371,6 +371,7 @@ logicalExpression
   | left = logicalExpression OR right = logicalExpression      # logicalOr
   | left = logicalExpression XOR right = logicalExpression     # logicalXor
   | booleanExpression                                          # booleanExpr
+   | expr1 = functionArg NOT? BETWEEN expr2 = functionArg AND expr3 = functionArg   # between


Move this new expression into ~~booleanExpression~~ comparisonExpression, then delete NOT?.

I changed this but the syntax also changes from

where a not between x and y -> where not a between x and y Is this desired?

Oh, I see. We can keep the NOT? here, and better to add two tests for a not between and not a between. They should have the same logic evaluation.

Actually, just like IN, a not in is ANSI SQL, but not a in is the original design in OpenSearch SQL. So yes, keep the NOT? symbol here.

Would be great if can discuss such things in the beginning of the PR next time ...

done, both "nots" are now supported and are tested

LantaoJin · 2024-10-25T12:02:02Z

ppl-spark-integration/src/main/antlr4/OpenSearchPPLLexer.g4

@@ -368,6 +368,7 @@ LIKE:                               'LIKE';
 ISNULL:                             'ISNULL';
 ISNOTNULL:                          'ISNOTNULL';
 ISPRESENT:                          'ISPRESENT';
+BETWEEN:                            'BETWEEN';


Add BETWEEN to keywordsCanBeId

LantaoJin · 2024-10-25T12:04:57Z

docs/ppl-lang/PPL-Example-Commands.md

@@ -49,6 +49,8 @@ _- **Limitation: new field added by eval command with a function cannot be dropp
 - `source = table | where isempty(a)`
 - `source = table | where isblank(a)`
 - `source = table | where case(length(a) > 6, 'True' else 'False') = 'True'`
+- `source = table | where a between 1 and 4`


I think we need explain the range of between is [1, 4] rather than (1, 4) in user doc. From the code implementation, I think it is [1, 4]. In anther word, when a = 1, the predicate returns True

LantaoJin · 2024-10-25T12:11:38Z

...-test/src/integration/scala/org/opensearch/flint/spark/ppl/FlintSparkPPLBetweenITSuite.scala

+    })
+  }
+
+  test("test between should return records NOT between two values") {


Can you add two tests (between and not between) with Date/Time in condition?
I think you can use the table by createTimeSeriesTable

Please add another test where age between 20 + 1 and 30 - 1. There is similar query in TPCH q6: l_discount between .06 - 0.01 and .06 + 0.01

YANG-DB · 2024-10-28T18:20:07Z

docs/ppl-lang/PPL-Example-Commands.md

@@ -55,6 +55,8 @@ _- **Limitation: new field added by eval command with a function cannot be dropp
 - `source = table | where isempty(a)`
 - `source = table | where isblank(a)`
 - `source = table | where case(length(a) > 6, 'True' else 'False') = 'True'`
+- `source = table | where a between 1 and 4` - Note: This returns a >= 1 and a <= 4, i.e. [1, 4]


@dr-lilienthal please also copy these comments to the ppl-between.md

YANG-DB · 2024-10-28T18:21:23Z

docs/ppl-lang/planning/ppl-between.md

+    - `... | where a between 1 and 4`
+    - `... | where b not between '2024-09-10' and '2025-09-10'`
+
+2. **Proposed impl**


you can remove the proposed impl here since there was no actual discussion about how it should be implemented...

LantaoJin

LGTM. Enabled auto-merge when CI passed.

Signed-off-by: Hendrik Saly <[email protected]>

Signed-off-by: Jens Schmidt <[email protected]>

Signed-off-by: Hendrik Saly <[email protected]>

Signed-off-by: Jens Schmidt <[email protected]>

Signed-off-by: Hendrik Saly <[email protected]>

* Implement between Signed-off-by: Hendrik Saly <[email protected]> * add integration test for between command to the straight and NOT usage Signed-off-by: Jens Schmidt <[email protected]> * Add docs Signed-off-by: Hendrik Saly <[email protected]> * Add proposed syntax to ppl planning Signed-off-by: Hendrik Saly <[email protected]> * adjust gitignore Signed-off-by: Jens Schmidt <[email protected]> * adjust gitignore: add spark-bin Signed-off-by: Jens Schmidt <[email protected]> * clean .gitignore: remove local adjustments Signed-off-by: Jens Schmidt <[email protected]> * update integration test to use between keyword Signed-off-by: Jens Schmidt <[email protected]> * Move to comparisonExpression Signed-off-by: Hendrik Saly <[email protected]> * Added to keywordsCanBeId Signed-off-by: Hendrik Saly <[email protected]> * Update docs Signed-off-by: Hendrik Saly <[email protected]> * Add additional tests Signed-off-by: Hendrik Saly <[email protected]> * Move to comparisonExpression -2- Signed-off-by: Hendrik Saly <[email protected]> * Fix docs Signed-off-by: Hendrik Saly <[email protected]> * Added IT tests Signed-off-by: Hendrik Saly <[email protected]> --------- Signed-off-by: Hendrik Saly <[email protected]> Signed-off-by: Jens Schmidt <[email protected]> Co-authored-by: Hendrik Saly <[email protected]>

YANG-DB added Lang:PPL Pipe Processing Language support 0.5 0.6 and removed 0.5 labels Oct 9, 2024

salyh force-pushed the between branch 3 times, most recently from 75e8dd4 to 6c13c69 Compare October 16, 2024 05:14

dr-lilienthal marked this pull request as ready for review October 16, 2024 06:56

dr-lilienthal requested review from dai-chen, rupal-bq, vmmusings, penghuo, seankao-az, anirudha, kaituo, YANG-DB and LantaoJin as code owners October 16, 2024 06:56

dr-lilienthal force-pushed the between branch 2 times, most recently from 1d82270 to 1622e1e Compare October 17, 2024 18:25

YANG-DB approved these changes Oct 17, 2024

View reviewed changes

YANG-DB requested changes Oct 17, 2024

View reviewed changes

...cala/org/opensearch/flint/spark/ppl/PPLLogicalPlanBetweenExpressionTranslatorTestSuite.scala Outdated Show resolved Hide resolved

dr-lilienthal requested a review from YANG-DB October 18, 2024 09:18

YANG-DB approved these changes Oct 24, 2024

View reviewed changes

salyh force-pushed the between branch 2 times, most recently from ac5b2a7 to f0d7aab Compare October 25, 2024 08:49

salyh requested review from mengweieric and noCharger as code owners October 25, 2024 08:49

salyh requested a review from ykmr1224 as a code owner October 25, 2024 08:49

LantaoJin reviewed Oct 25, 2024

View reviewed changes

salyh force-pushed the between branch from f0d7aab to 8f711e3 Compare October 28, 2024 14:18

YANG-DB requested changes Oct 28, 2024

View reviewed changes

LantaoJin approved these changes Oct 29, 2024

View reviewed changes

LantaoJin enabled auto-merge (squash) October 29, 2024 00:47

salyh and others added 15 commits October 29, 2024 10:01

Implement between

21af1a9

Signed-off-by: Hendrik Saly <[email protected]>

add integration test for between command to the straight and NOT usage

98d68fe

Signed-off-by: Jens Schmidt <[email protected]>

Add docs

08acaa2

Signed-off-by: Hendrik Saly <[email protected]>

Add proposed syntax to ppl planning

5596532

Signed-off-by: Hendrik Saly <[email protected]>

adjust gitignore

62c467d

Signed-off-by: Jens Schmidt <[email protected]>

adjust gitignore: add spark-bin

811ab18

Signed-off-by: Jens Schmidt <[email protected]>

clean .gitignore: remove local adjustments

3d705c5

Signed-off-by: Jens Schmidt <[email protected]>

update integration test to use between keyword

84b43b2

Signed-off-by: Jens Schmidt <[email protected]>

Move to comparisonExpression

2bef422

Signed-off-by: Hendrik Saly <[email protected]>

Added to keywordsCanBeId

f95f870

Signed-off-by: Hendrik Saly <[email protected]>

Update docs

1490db0

Signed-off-by: Hendrik Saly <[email protected]>

Add additional tests

a209f31

Signed-off-by: Hendrik Saly <[email protected]>

Move to comparisonExpression -2-

fadd2d2

Signed-off-by: Hendrik Saly <[email protected]>

Fix docs

f8be3a7

Signed-off-by: Hendrik Saly <[email protected]>

Added IT tests

a0f6af8

Signed-off-by: Hendrik Saly <[email protected]>

auto-merge was automatically disabled October 29, 2024 09:04
Head branch was pushed to by a user without write access

salyh force-pushed the between branch from 8dd9029 to a0f6af8 Compare October 29, 2024 09:04

YANG-DB self-requested a review October 29, 2024 16:46

YANG-DB approved these changes Oct 29, 2024

View reviewed changes

YANG-DB merged commit 82770ec into opensearch-project:main Oct 29, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add PPL Between functionality #758

Add PPL Between functionality #758

dr-lilienthal commented Oct 9, 2024

YANG-DB commented Oct 9, 2024

YANG-DB commented Oct 11, 2024

dr-lilienthal commented Oct 17, 2024

LantaoJin Oct 25, 2024 •

edited

Loading

salyh Oct 28, 2024

LantaoJin Oct 28, 2024

LantaoJin Oct 28, 2024

salyh Oct 28, 2024 •

edited

Loading

salyh Oct 28, 2024

LantaoJin Oct 25, 2024

salyh Oct 28, 2024

LantaoJin Oct 25, 2024

salyh Oct 28, 2024

LantaoJin Oct 25, 2024

LantaoJin Oct 28, 2024 •

edited

Loading

salyh Oct 28, 2024

YANG-DB Oct 28, 2024

salyh Oct 28, 2024

YANG-DB Oct 28, 2024

salyh Oct 28, 2024

LantaoJin left a comment

Add PPL Between functionality #758

Add PPL Between functionality #758

Conversation

dr-lilienthal commented Oct 9, 2024

Description

Issues Resolved

YANG-DB commented Oct 9, 2024

YANG-DB commented Oct 11, 2024

dr-lilienthal commented Oct 17, 2024

LantaoJin Oct 25, 2024 • edited Loading

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

salyh Oct 28, 2024 • edited Loading

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

Choose a reason for hiding this comment

LantaoJin Oct 28, 2024 • edited Loading

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

LantaoJin left a comment

Choose a reason for hiding this comment

LantaoJin Oct 25, 2024 •

edited

Loading

salyh Oct 28, 2024 •

edited

Loading

LantaoJin Oct 28, 2024 •

edited

Loading