Skip to content

[Calcite Engine] Push down project and filter operator into index scan #3327

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Feb 21, 2025
Merged

[Calcite Engine] Push down project and filter operator into index scan #3327

merged 4 commits into from
Feb 21, 2025

Conversation

@qianheng-aws
Copy link
Collaborator

@qianheng-aws qianheng-aws commented Feb 18, 2025 •
edited
Loading

Description

Push down project and filter operator into index scan. This PR includes change:

  1. OpenSearchIndex change to use its specific scan operator, CalciteOpenSearchIndexScan
  2. Simplify to use RelRunners to do execution since the current implementation of CalciteOpenSearchIndexScan does not require a "schema" as context.
  3. Add OpenSearchFilterIndexScanRule and OpenSearchProjectIndexScanRule to do filter/project operator push down.
  4. Add PredicateAnalyzer to build QueryBuilder for filter condition.

Related Issues

Part Resolves #3331

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.

@qianheng-aws
Support Filter and Project pushdown
6ae6391 
Signed-off-by: Heng Qian <qianheng@amazon.com>
@qianheng-aws
Support Filter and Project pushdown v2
31c89c4 
Signed-off-by: Heng Qian <qianheng@amazon.com>
@penghuo penghuo added the calcite calcite migration releated label Feb 18, 2025
penghuo
penghuo reviewed Feb 18, 2025
View reviewed changes
@qianheng-aws
Address comments
8c40495 
Signed-off-by: Heng Qian <qianheng@amazon.com>
penghuo
penghuo approved these changes Feb 19, 2025
View reviewed changes
Copy link
Collaborator

@penghuo penghuo left a comment

Choose a reason for hiding this comment

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

Thx!

dai-chen
dai-chen approved these changes Feb 20, 2025
View reviewed changes
Copy link
Collaborator

@dai-chen dai-chen left a comment

Choose a reason for hiding this comment

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

Thanks for the changes! Btw, do we have a high level diagram for Calcite integration? Just want to learn and get more context. As I understand, we're trying to adapt v2 OS domain classes to Calcite connector, right?

...rch/src/main/java/org/opensearch/sql/opensearch/storage/scan/CalciteOpenSearchIndexScan.java
};
}

public boolean pushDownFilter(Filter filter) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This assumes the given filter can be fully pushed down?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If analyze exception occurs in PredicateAnalyzer, it will return false and won't push down this filter.

In v2 engine, will there any other case except aggregation block filter push down?

@qianheng-aws
Add original license for PredicateAnalyzer
558eed9 
Signed-off-by: Heng Qian <qianheng@amazon.com>
@qianheng-aws
Copy link
Collaborator Author

Thanks for the changes! Btw, do we have a high level diagram for Calcite integration? Just want to learn and get more context. As I understand, we're trying to adapt v2 OS domain classes to Calcite connector, right?

This RFC should be the root issue and should have the overview: #3229

@LantaoJin LantaoJin merged commit cb103d7 into opensearch-project:feature/calcite-engine Feb 21, 2025
8 of 13 checks passed
@LantaoJin
Copy link
Member

LantaoJin commented Feb 21, 2025 •
edited
Loading

@qianheng-aws seems many ITs fail with this PR in my local.
For example

  • Predicates with OR
String.format(
                "source=%s | where (account_number = 25 or balance > 10000) and gender = 'M' |"
                    + " fields firstname, lastname",
                TEST_INDEX_BANK)
  • Multiple Tables and Filters
String.format(
                "source=%s, %s gender = 'F' | stats count() as c",
                TEST_INDEX_BANK, TEST_INDEX_BANK)

could you add at least two tests to verify each PR in future:

  1. Add a UT such as CalcitePPLBasicTest extends CalcitePPLAbstractTest
  2. Add a IT such as CalcitePPLBasicIT extends CalcitePPLTestCase

penghuo pushed a commit to penghuo/os-sql that referenced this pull request Mar 12, 2025
@qianheng-aws @penghuo
[Calcite Engine] Push down project and filter operator into index scan (
f136008 
opensearch-project#3327)

---------

Signed-off-by: Heng Qian <qianheng@amazon.com>
@LantaoJin LantaoJin mentioned this pull request Apr 2, 2025
Closed
penghuo pushed a commit that referenced this pull request Jun 16, 2025
@qianheng-aws @xinyual
[Calcite Engine] Push down project and filter operator into index scan (
30639c5 
#3327)

* Support Filter and Project pushdown

Signed-off-by: Heng Qian <qianheng@amazon.com>

* Support Filter and Project pushdown v2

Signed-off-by: Heng Qian <qianheng@amazon.com>

* Address comments

Signed-off-by: Heng Qian <qianheng@amazon.com>

* Add original license for PredicateAnalyzer

Signed-off-by: Heng Qian <qianheng@amazon.com>

---------

Signed-off-by: Heng Qian <qianheng@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

Reviewers

@penghuo penghuo penghuo approved these changes

@dai-chen dai-chen dai-chen approved these changes

@ps48 ps48 Awaiting requested review from ps48 ps48 is a code owner

@kavithacm kavithacm Awaiting requested review from kavithacm kavithacm is a code owner

@derek-ho derek-ho Awaiting requested review from derek-ho derek-ho is a code owner

@joshuali925 joshuali925 Awaiting requested review from joshuali925 joshuali925 is a code owner

@YANG-DB YANG-DB Awaiting requested review from YANG-DB YANG-DB is a code owner

@mengweieric mengweieric Awaiting requested review from mengweieric mengweieric is a code owner

@Swiddis Swiddis Awaiting requested review from Swiddis Swiddis is a code owner

@seankao-az seankao-az Awaiting requested review from seankao-az seankao-az is a code owner

@MaxKsyunz MaxKsyunz Awaiting requested review from MaxKsyunz MaxKsyunz is a code owner

@Yury-Fridlyand Yury-Fridlyand Awaiting requested review from Yury-Fridlyand Yury-Fridlyand is a code owner

@anirudha anirudha Awaiting requested review from anirudha anirudha is a code owner

@forestmvey forestmvey Awaiting requested review from forestmvey forestmvey is a code owner

@acarbonetto acarbonetto Awaiting requested review from acarbonetto acarbonetto is a code owner

@GumpacG GumpacG Awaiting requested review from GumpacG GumpacG is a code owner

@ykmr1224 ykmr1224 Awaiting requested review from ykmr1224 ykmr1224 is a code owner

@LantaoJin LantaoJin Awaiting requested review from LantaoJin LantaoJin is a code owner

@noCharger noCharger Awaiting requested review from noCharger noCharger is a code owner

Assignees

No one assigned

Labels

calcite calcite migration releated

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@qianheng-aws @LantaoJin @penghuo @dai-chen