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

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

Conversation

qianheng-aws
Copy link
Contributor

@qianheng-aws qianheng-aws commented Feb 18, 2025

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.

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

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?

};
}

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
Contributor 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?

Signed-off-by: Heng Qian <qianheng@amazon.com>
@qianheng-aws
Copy link
Contributor 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

@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

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.

4 participants