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

[POC] Make Calcite execute successfully #3258

Open
wants to merge 2 commits into
base: feature/calcite-engine
Choose a base branch
from

Conversation

qianheng-aws
Copy link
Contributor

@qianheng-aws qianheng-aws commented Jan 21, 2025

Description

  • Replace RelRunners with CalciteConnection to do execution
  • Add OpenSearchSchema to the rootSchema of above connection
  • Change ExprValue to Object in OpenSearchTable

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

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.

Copy link
Collaborator

@noCharger noCharger left a comment

Choose a reason for hiding this comment

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

@qianheng-aws is this PR plan for review?

Signed-off-by: Heng Qian <qianheng@amazon.com>
…able when visitRelation.

Signed-off-by: Heng Qian <qianheng@amazon.com>
@qianheng-aws qianheng-aws force-pushed the feature/calcite-engine branch from 148b79c to cf34fab Compare January 24, 2025 08:13
@qianheng-aws
Copy link
Contributor Author

@qianheng-aws is this PR plan for review?

Yeah, but it should only be merged to the feature branch calcite-engine. It requires the review from @LantaoJin as he initiated that feature branch.

@penghuo penghuo added the calcite calcite migration releated label Jan 28, 2025
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.

This PR ensure CalciteStandaloneIT passes?

  1. Could u refine the PR description? Make Calcite execute is generic.
  2. Could u enable CalciteStandaloneIT, and disable other PPL ITs. we expected IT should passed.

@@ -102,30 +113,50 @@ public ExplainResponseNode visitTableScan(
@Override
public void execute(
RelNode rel, CalcitePlanContext context, ResponseListener<QueryResponse> listener) {
try (PreparedStatement statement = RelRunners.run(rel)) {
Connection connection = context.connection;
final RelShuttle shuttle = new RelHomogeneousShuttle();
Copy link
Collaborator

Choose a reason for hiding this comment

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

does it required?

@Override
public Table get(Object key) {
if (!super.containsKey(key)) {
registerTable((String) key);
Copy link
Collaborator

Choose a reason for hiding this comment

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

could we do registerTable(new QualifiedName((String)key)) instead?

Comment on lines +46 to +54
try {
this.statement = connection.createStatement().unwrap(CalciteServerStatement.class);
} catch (Exception e) {
throw new RuntimeException("create statement failed", e);
}
this.prepare = new CalcitePrepareImpl();
this.relBuilder = prepare.perform(statement, config,
(cluster, relOptSchema, rootSchema, statement) -> new OSRelBuilder(config.getContext(),
cluster, relOptSchema));
Copy link
Collaborator

Choose a reason for hiding this comment

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

could u help explain the purpose this statement?

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