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] Build integration test framework #3342

Merged

Conversation

LantaoJin
Copy link
Member

@LantaoJin LantaoJin commented Feb 21, 2025

Description

Build integration test framework, includes

  1. Add two kinds of integration tests: standalone and remote. standalone IT can debug in IDE, remote IT run tests in the min OS cluster.
  2. Add two configs plugins.calcite.enabled and plugins.calcite.fallback.allowed
  3. Add CalciteToolsHelper.java, this class is used to create customized Connection, JavaTypeFactory, RelBuilder and RelRunner
  4. Add a method valueForCalcite() in ExprValue, calcite read opensearch data by valueForCalcite() instead of value()
  5. Add EnumerableIndexScanRule to convert a CalciteLogicalTableScan to CalciteOpenSearchIndexScan`

Related Issues

Resolves #3330

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>
Signed-off-by: Lantao Jin <ltjin@amazon.com>
@LantaoJin LantaoJin added the calcite calcite migration releated label Feb 21, 2025
Signed-off-by: Lantao Jin <ltjin@amazon.com>
@LantaoJin
Copy link
Member Author

Current CI will still fail due to missing security policy in job-schedule plugin. cc @xinyuan

Review required cc @qianheng-aws @penghuo @dai-chen

fallbackAllowed = settings.getSettingValue(Settings.Key.CALCITE_FALLBACK_ALLOWED);
}
if (!fallbackAllowed) {
throw e;
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we use listener to handle this exception?

listener.onFailure(e);

* This abstract test case provide a standalone env to run PPL query, IT extends this class could
* debug the service side execution of PPL in IDE.
*/
public abstract class CalcitePPLTestCase extends PPLIntegTestCase {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious why do we change this name? The current name doesn't explicitly indicate that this's an integ test.

Copy link
Member Author

Choose a reason for hiding this comment

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

reverted

@@ -0,0 +1,185 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should add calcite's license header after ours since we copy code from that repo.

There is similar example in

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

Signed-off-by: Lantao Jin <ltjin@amazon.com>
@LantaoJin LantaoJin merged commit 4ba78d3 into opensearch-project:feature/calcite-engine Feb 26, 2025
8 of 13 checks passed
@LantaoJin
Copy link
Member Author

Thanks, merging to dev branch.

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.

3 participants