Skip to content

Support ppl BETWEEN operator within Calcite#3433

Merged
LantaoJin merged 2 commits intoopensearch-project:feature/calcite-enginefrom
LantaoJin:pr/issues/3432
Mar 17, 2025
Merged

Support ppl BETWEEN operator within Calcite#3433
LantaoJin merged 2 commits intoopensearch-project:feature/calcite-enginefrom
LantaoJin:pr/issues/3432

Conversation

@LantaoJin
Copy link
Copy Markdown
Member

@LantaoJin LantaoJin commented Mar 14, 2025

Description

  1. Support ppl BETWEEN operator within Calcite
  2. Add all unsupported visit callings in code (PPL only)
  3. Support REGEXP function
  4. Fix xor operator issue.

Related Issues

Resolves #3432

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • New functionality has javadoc added.
  • New functionality has a user manual doc added.
  • API changes companion pull request created.
  • Commits are signed per the DCO using --signoff.
  • Public documentation issue/PR created.

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: Lantao Jin <ltjin@amazon.com>
@LantaoJin
Copy link
Copy Markdown
Member Author

cc @penghuo @qianheng-aws

String expectedLogical =
""
+ "LogicalProject(EMPNO=[$0], ENAME=[$1])\n"
+ " LogicalFilter(condition=[SEARCH($7, Sarg[[20..30]])])\n"
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Add tests for between 20 and 30.0. Calcite has bug on Sarg: #3437, it may affect between as well.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

check testBetweenWithDifferentTypes2() in IT

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Shall we check the plan and final spark sql for that case as well? Seems we performs cast on the literal.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

sure, will add it.

Copy link
Copy Markdown
Member Author

@LantaoJin LantaoJin Mar 17, 2025

Choose a reason for hiding this comment

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

The cast is implicit actually:

source=EMP | where DEPTNO between 20 and 30.0 | fields EMPNO, ENAME

convert to

 "SELECT `EMPNO`, `ENAME` FROM `scott`.`EMP` WHERE `DEPTNO` >= 2.00E1 AND `DEPTNO` <= 3.00E1"

JSONObject actual =
executeQuery(
String.format(
"source=%s | where not age between 30 and 39 | fields firstname, age",
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Shall we add test like "where not age not between 30 and 39"? Though I think it should work well.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

not necessary IMO. But no harm to add it as it's simply enough.

Signed-off-by: Lantao Jin <ltjin@amazon.com>
@LantaoJin LantaoJin requested a review from qianheng-aws March 17, 2025 08:36
@LantaoJin LantaoJin merged commit 1cefc8c into opensearch-project:feature/calcite-engine Mar 17, 2025
9 of 13 checks passed
penghuo pushed a commit that referenced this pull request Jun 16, 2025
* Support ppl BETWEEN operation within Calcite

Signed-off-by: Lantao Jin <ltjin@amazon.com>

* add more tests

Signed-off-by: Lantao Jin <ltjin@amazon.com>

---------

Signed-off-by: Lantao Jin <ltjin@amazon.com>
Signed-off-by: xinyual <xinyual@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

calcite calcite migration releated

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants