-
Notifications
You must be signed in to change notification settings - Fork 176
[Calcite Engine]Support metadata field #3445
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]Support metadata field #3445
Conversation
Signed-off-by: Heng Qian <qianheng@amazon.com>
Signed-off-by: Heng Qian <qianheng@amazon.com>
flakey failure: |
Signed-off-by: Heng Qian <qianheng@amazon.com>
…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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please describe the limitation in https://github.com/opensearch-project/sql/blob/main/docs/dev/intro-v3-engine.md#33-limitations
this(name, expr, alias, false); | ||
} | ||
|
||
public Alias(String name, UnresolvedExpression expr, boolean metaMetaFieldAllowed) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why double “meta”?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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() { |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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>
public static final AllFields INSTANCE_OF_ALL = new AllFields(false); | ||
public static final AllFields INSTANCE_EXCEPT_META = new AllFields(true); |
There was a problem hiding this comment.
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>
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not been used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, will delete it.
integ-test/src/test/resources/expectedOutput/calcite/explain_filter_push.json
Show resolved
Hide resolved
Signed-off-by: Heng Qian <qianheng@amazon.com>
Found a bug after merged this PR: |
* [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>
* [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>
* [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>
Description
Support metadata field in Calcite. This PR also include changes:
Related Issues
Resolves #3333
Check List
--signoff
.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.