Skip to content

Conversation

qianheng-aws
Copy link
Collaborator

@qianheng-aws qianheng-aws commented Mar 19, 2025

Description

Support metadata field in Calcite. This PR also include changes:

  1. Fix the bug on v2 engine that cannot evaluate metadata field if it's not pushdown
  2. Add limitation of using metadata field on eval, alias and rename

Related Issues

Resolves #3333

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>
@qianheng-aws qianheng-aws marked this pull request as ready for review March 20, 2025 05:24
Signed-off-by: Heng Qian <qianheng@amazon.com>
@qianheng-aws
Copy link
Collaborator Author

flakey failure:
CalcitePPLStringBuiltinFunctionPushdownIT > testSubstring

@LantaoJin LantaoJin added the calcite calcite migration releated label Mar 20, 2025
Signed-off-by: Heng Qian <qianheng@amazon.com>
@qianheng-aws qianheng-aws changed the base branch from feature/calcite-engine to main March 25, 2025 02:39
…calcite-engine-metadata

# Conflicts:
#	core/src/main/java/org/opensearch/sql/ast/expression/Alias.java
#	integ-test/src/test/java/org/opensearch/sql/calcite/remote/nonfallback/NonFallbackCalciteFieldsCommandIT.java
#	ppl/src/main/java/org/opensearch/sql/ppl/parser/AstStatementBuilder.java
Signed-off-by: Heng Qian <qianheng@amazon.com>
Signed-off-by: Heng Qian <qianheng@amazon.com>
…calcite-engine-metadata

# Conflicts:
#	integ-test/src/test/java/org/opensearch/sql/calcite/remote/nonfallback/NonFallbackCalciteFieldsCommandIT.java
#	integ-test/src/test/java/org/opensearch/sql/calcite/remote/nonfallback/NonFallbackCalciteWhereCommandIT.java
Signed-off-by: Heng Qian <qianheng@amazon.com>
Copy link
Member

@LantaoJin LantaoJin left a comment

Choose a reason for hiding this comment

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

this(name, expr, alias, false);
}

public Alias(String name, UnresolvedExpression expr, boolean metaMetaFieldAllowed) {
Copy link
Member

Choose a reason for hiding this comment

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

Why double “meta”?

Copy link
Member

Choose a reason for hiding this comment

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

Alias is a public interface. Could you add some comments in code to illustrate the purpose of this parameter.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will make it private and add comments

return nodeVisitor.visitAllFields(this, context);
}

public UnresolvedPlan apply(UnresolvedPlan plan) {
Copy link
Member

Choose a reason for hiding this comment

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

can you add a java doc for this interface? can this method be moved to CalcitePlanContext and rename to wrapProject?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Doc added. We cannot do that because some of this wrapping happens in AstBuilder

checkRename();
}

private void checkRename() {
Copy link
Member

Choose a reason for hiding this comment

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

validate()

executeQuery(
String.format(
"""
source = %s | rename name as _id
Copy link
Member

Choose a reason for hiding this comment

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

what if rename name as _ID or rename name as _Id? if it is valid, could you add a test for these valid new names

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added another test case for it.

Signed-off-by: Heng Qian <qianheng@amazon.com>
Signed-off-by: Heng Qian <qianheng@amazon.com>
@qianheng-aws qianheng-aws requested a review from LantaoJin April 17, 2025 05:50
Comment on lines 27 to 28
public static final AllFields INSTANCE_OF_ALL = new AllFields(false);
public static final AllFields INSTANCE_EXCEPT_META = new AllFields(true);
Copy link
Member

Choose a reason for hiding this comment

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

Can we create a new derived class or classes for different semantics?

Signed-off-by: Heng Qian <qianheng@amazon.com>
Signed-off-by: Heng Qian <qianheng@amazon.com>
Signed-off-by: Heng Qian <qianheng@amazon.com>
LantaoJin
LantaoJin previously approved these changes Apr 17, 2025
return StringUtils.format("[ %s ] as %s", child, node.getAlias());
Node childNode = node.getChild().get(0);
String child = childNode.accept(this, context);
if (childNode instanceof Project project
Copy link
Collaborator

Choose a reason for hiding this comment

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

add an UT to cover this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's covered in PPLQueryDataAnonymizerTest::testJoin, there are several test cases with subquery alias

}

/** Whether include reserved fields in the schema of table */
default boolean includeReservedFieldTypes() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

not been used?

Copy link
Collaborator Author

@qianheng-aws qianheng-aws Apr 22, 2025

Choose a reason for hiding this comment

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

Yeah, will delete it.

Signed-off-by: Heng Qian <qianheng@amazon.com>
@qianheng-aws qianheng-aws requested a review from LantaoJin April 22, 2025 05:37
@LantaoJin LantaoJin merged commit 9092cc8 into opensearch-project:main Apr 22, 2025
22 checks passed
@LantaoJin
Copy link
Member

Found a bug after merged this PR:
source=[source=banks | where age>35] as t throws NPE when Calcite disabled.

@LantaoJin LantaoJin added bug Something isn't working backport 3.0 labels Apr 25, 2025
opensearch-trigger-bot bot pushed a commit that referenced this pull request Apr 25, 2025
* [Calcite Engine]Support metadata field

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

* Fix UT/IT

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

* Refine code

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

* [Calcite engine] Fix nested subquery

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

* Fix merging

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

* Refine Code

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

* Fix IT

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

* Address comments

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

* Address comments

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

* Address comments3

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

* Address comments

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

* Fix Anonymizer

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

* Address comments

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

---------

Signed-off-by: Heng Qian <qianheng@amazon.com>
(cherry picked from commit 9092cc8)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
LantaoJin pushed a commit that referenced this pull request Apr 25, 2025
* [Calcite Engine]Support metadata field



* Fix UT/IT



* Refine code



* [Calcite engine] Fix nested subquery



* Fix merging



* Refine Code



* Fix IT



* Address comments



* Address comments



* Address comments3



* Address comments



* Fix Anonymizer



* Address comments



---------


(cherry picked from commit 9092cc8)

Signed-off-by: Heng Qian <qianheng@amazon.com>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
penghuo pushed a commit that referenced this pull request Jun 16, 2025
* [Calcite Engine]Support metadata field

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

* Fix UT/IT

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

* Refine code

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

* [Calcite engine] Fix nested subquery

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

* Fix merging

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

* Refine Code

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

* Fix IT

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

* Address comments

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

* Address comments

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

* Address comments3

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

* Address comments

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

* Fix Anonymizer

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

* Address comments

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

Labels

backport 3.0 bug Something isn't working calcite calcite migration releated

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[FEATURE] Support metadata fields in fields command with Calcite

3 participants