-
Notifications
You must be signed in to change notification settings - Fork 180
[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 <[email protected]>
Signed-off-by: Heng Qian <[email protected]>
Signed-off-by: Heng Qian <[email protected]>
|
flakey failure: |
Signed-off-by: Heng Qian <[email protected]>
…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 <[email protected]>
Signed-off-by: Heng Qian <[email protected]>
…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 <[email protected]>
LantaoJin
left a comment
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 <[email protected]>
Signed-off-by: Heng Qian <[email protected]>
| 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 <[email protected]>
Signed-off-by: Heng Qian <[email protected]>
Signed-off-by: Heng Qian <[email protected]>
| 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 <[email protected]>
|
Found a bug after merged this PR: |
* [Calcite Engine]Support metadata field Signed-off-by: Heng Qian <[email protected]> * Fix UT/IT Signed-off-by: Heng Qian <[email protected]> * Refine code Signed-off-by: Heng Qian <[email protected]> * [Calcite engine] Fix nested subquery Signed-off-by: Heng Qian <[email protected]> * Fix merging Signed-off-by: Heng Qian <[email protected]> * Refine Code Signed-off-by: Heng Qian <[email protected]> * Fix IT Signed-off-by: Heng Qian <[email protected]> * Address comments Signed-off-by: Heng Qian <[email protected]> * Address comments Signed-off-by: Heng Qian <[email protected]> * Address comments3 Signed-off-by: Heng Qian <[email protected]> * Address comments Signed-off-by: Heng Qian <[email protected]> * Fix Anonymizer Signed-off-by: Heng Qian <[email protected]> * Address comments Signed-off-by: Heng Qian <[email protected]> --------- Signed-off-by: Heng Qian <[email protected]> (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 <[email protected]> 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 <[email protected]> * Fix UT/IT Signed-off-by: Heng Qian <[email protected]> * Refine code Signed-off-by: Heng Qian <[email protected]> * [Calcite engine] Fix nested subquery Signed-off-by: Heng Qian <[email protected]> * Fix merging Signed-off-by: Heng Qian <[email protected]> * Refine Code Signed-off-by: Heng Qian <[email protected]> * Fix IT Signed-off-by: Heng Qian <[email protected]> * Address comments Signed-off-by: Heng Qian <[email protected]> * Address comments Signed-off-by: Heng Qian <[email protected]> * Address comments3 Signed-off-by: Heng Qian <[email protected]> * Address comments Signed-off-by: Heng Qian <[email protected]> * Fix Anonymizer Signed-off-by: Heng Qian <[email protected]> * Address comments Signed-off-by: Heng Qian <[email protected]> --------- Signed-off-by: Heng Qian <[email protected]> Signed-off-by: xinyual <[email protected]>
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.