-
Notifications
You must be signed in to change notification settings - Fork 139
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
Add Nested Function Support In SELECT Clause #1490
Merged
MaxKsyunz
merged 9 commits into
opensearch-project:main
from
Bit-Quill:integ-nested-select-clause
Apr 12, 2023
Merged
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
3f02344
Adding nested function support in SELECT clause.
forestmvey 2774934
Fixing flakey tests for UnnestOperator.
forestmvey 5aa7a3f
Simplifying flattening logic. Removed nested function classes making …
forestmvey 20a4cb3
Fixing bug for missing collection values not showing as null.
forestmvey 2552ca0
Adding comments and updating visit functions for nested.
forestmvey 1fe7c12
Adding exception error messages to analyzer tests for Nested function.
forestmvey 9ae39a1
Fix filter push down with nested push down with added IT and UT tests…
forestmvey 77fbd0e
Fixing rebase errors.
forestmvey fcb3558
Adding explain and test for Nested.
forestmvey File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
111 changes: 111 additions & 0 deletions
111
core/src/main/java/org/opensearch/sql/analysis/NestedAnalyzer.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,111 @@ | ||
/* | ||
* Copyright OpenSearch Contributors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
package org.opensearch.sql.analysis; | ||
|
||
import static org.opensearch.sql.data.type.ExprCoreType.STRING; | ||
|
||
import java.util.ArrayList; | ||
import java.util.Arrays; | ||
import java.util.List; | ||
import java.util.Map; | ||
import lombok.RequiredArgsConstructor; | ||
import org.opensearch.sql.ast.AbstractNodeVisitor; | ||
import org.opensearch.sql.ast.expression.Alias; | ||
import org.opensearch.sql.ast.expression.Function; | ||
import org.opensearch.sql.ast.expression.QualifiedName; | ||
import org.opensearch.sql.ast.expression.UnresolvedExpression; | ||
import org.opensearch.sql.expression.NamedExpression; | ||
import org.opensearch.sql.expression.ReferenceExpression; | ||
import org.opensearch.sql.expression.function.BuiltinFunctionName; | ||
import org.opensearch.sql.planner.logical.LogicalNested; | ||
import org.opensearch.sql.planner.logical.LogicalPlan; | ||
|
||
/** | ||
* Analyze the Nested Function in the {@link AnalysisContext} to construct the {@link | ||
* LogicalPlan}. | ||
*/ | ||
@RequiredArgsConstructor | ||
public class NestedAnalyzer extends AbstractNodeVisitor<LogicalPlan, AnalysisContext> { | ||
private final List<NamedExpression> namedExpressions; | ||
private final ExpressionAnalyzer expressionAnalyzer; | ||
private final LogicalPlan child; | ||
|
||
public LogicalPlan analyze(UnresolvedExpression projectItem, AnalysisContext context) { | ||
LogicalPlan nested = projectItem.accept(this, context); | ||
return (nested == null) ? child : nested; | ||
} | ||
|
||
@Override | ||
public LogicalPlan visitAlias(Alias node, AnalysisContext context) { | ||
return node.getDelegated().accept(this, context); | ||
} | ||
|
||
@Override | ||
public LogicalPlan visitFunction(Function node, AnalysisContext context) { | ||
if (node.getFuncName().equalsIgnoreCase(BuiltinFunctionName.NESTED.name())) { | ||
|
||
List<UnresolvedExpression> expressions = node.getFuncArgs(); | ||
validateArgs(expressions); | ||
ReferenceExpression nestedField = | ||
(ReferenceExpression)expressionAnalyzer.analyze(expressions.get(0), context); | ||
Map<String, ReferenceExpression> args; | ||
if (expressions.size() == 2) { | ||
args = Map.of( | ||
"field", nestedField, | ||
"path", (ReferenceExpression)expressionAnalyzer.analyze(expressions.get(1), context) | ||
); | ||
} else { | ||
args = Map.of( | ||
"field", (ReferenceExpression)expressionAnalyzer.analyze(expressions.get(0), context), | ||
"path", generatePath(nestedField.toString()) | ||
); | ||
} | ||
if (child instanceof LogicalNested) { | ||
((LogicalNested)child).addFields(args); | ||
return child; | ||
} else { | ||
return new LogicalNested(child, new ArrayList<>(Arrays.asList(args)), namedExpressions); | ||
} | ||
} | ||
return null; | ||
} | ||
|
||
/** | ||
* Validate each parameter used in nested function in SELECT clause. Any supplied parameter | ||
* for a nested function in a SELECT statement must be a valid qualified name, and the field | ||
* parameter must be nested at least one level. | ||
* @param args : Arguments in nested function. | ||
*/ | ||
private void validateArgs(List<UnresolvedExpression> args) { | ||
if (args.size() < 1 || args.size() > 2) { | ||
throw new IllegalArgumentException( | ||
"on nested object only allowed 2 parameters (field,path) or 1 parameter (field)" | ||
); | ||
} | ||
|
||
for (int i = 0; i < args.size(); i++) { | ||
if (!(args.get(i) instanceof QualifiedName)) { | ||
throw new IllegalArgumentException( | ||
String.format("Illegal nested field name: %s", args.get(i).toString()) | ||
); | ||
} | ||
if (i == 0 && ((QualifiedName)args.get(i)).getParts().size() < 2) { | ||
throw new IllegalArgumentException( | ||
String.format("Illegal nested field name: %s", args.get(i).toString()) | ||
); | ||
} | ||
} | ||
} | ||
|
||
/** | ||
* Generate nested path dynamically. Assumes at least one level of nesting in supplied string. | ||
* @param field : Nested field to generate path of. | ||
* @return : Path of field derived from last level of nesting. | ||
*/ | ||
private ReferenceExpression generatePath(String field) { | ||
return new ReferenceExpression(field.substring(0, field.lastIndexOf(".")), STRING); | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
49 changes: 49 additions & 0 deletions
49
core/src/main/java/org/opensearch/sql/planner/logical/LogicalNested.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,49 @@ | ||
/* | ||
* Copyright OpenSearch Contributors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
package org.opensearch.sql.planner.logical; | ||
|
||
import java.util.Collections; | ||
import java.util.List; | ||
import java.util.Map; | ||
import lombok.EqualsAndHashCode; | ||
import lombok.Getter; | ||
import lombok.ToString; | ||
import org.opensearch.sql.expression.NamedExpression; | ||
import org.opensearch.sql.expression.ReferenceExpression; | ||
|
||
/** | ||
* Logical Nested plan. | ||
*/ | ||
@EqualsAndHashCode(callSuper = true) | ||
@Getter | ||
@ToString | ||
public class LogicalNested extends LogicalPlan { | ||
forestmvey marked this conversation as resolved.
Show resolved
Hide resolved
|
||
private List<Map<String, ReferenceExpression>> fields; | ||
private final List<NamedExpression> projectList; | ||
|
||
/** | ||
* Constructor of LogicalNested. | ||
* | ||
*/ | ||
public LogicalNested( | ||
LogicalPlan childPlan, | ||
List<Map<String, ReferenceExpression>> fields, | ||
List<NamedExpression> projectList | ||
) { | ||
super(Collections.singletonList(childPlan)); | ||
this.fields = fields; | ||
this.projectList = projectList; | ||
} | ||
|
||
public void addFields(Map<String, ReferenceExpression> fields) { | ||
this.fields.add(fields); | ||
} | ||
|
||
@Override | ||
public <R, C> R accept(LogicalPlanNodeVisitor<R, C> visitor, C context) { | ||
return visitor.visitNested(this, context); | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
How about we generate a single
LogicalNested
here rather than get them merged later inMergeNestedAndNested
?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.
I will update to create the single LogicalNested here rather than merging further down query execution.
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.
Thanks. I'm just thinking if we have all the project items and always merge multiple LogicalNested later, we may be able to save that optimizer rule.
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.
Checked the latest revision and wondering if the for loop is needed? Because we generate single nested operator with all
namedExpressions
right?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.
Yes the for loop is still needed. The
namedExpressions
are passed in each time for project push down, but we are fulfilling theLogicalNested
nested fields by eachnested
Function
in theprojectList
. BothnamedExpressions
and nested functions are required.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.
using for-loop inside NesteAnalyzer, and change interface?
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 would be my preference to keep the for-loop as is so we don't have to complicate adding additional arguments to an already formed
LogicalPlan
. If we move the for-loop into theNestedAnalyzer
we will have to do some not-so-nice logic in NestedAnalyzer:Analyze to aggregate the arguments from the analyzedLogicalPlan
's.NestedAnalyzer.java#L36-L74
If you would prefer this implementation I can make the necessary revisions!