Skip to content

Commit

Permalink
Merge pull request #260 from Bit-Quill/dev-nested-fallback
Browse files Browse the repository at this point in the history
Ensure Nested Function Falls Back to Legacy Engine Where Not Supported
  • Loading branch information
forestmvey authored Apr 18, 2023
2 parents 4e9ca01 + be853e6 commit e574dd2
Show file tree
Hide file tree
Showing 3 changed files with 102 additions and 2 deletions.
29 changes: 28 additions & 1 deletion core/src/main/java/org/opensearch/sql/analysis/Analyzer.java
Original file line number Diff line number Diff line change
Expand Up @@ -60,18 +60,21 @@
import org.opensearch.sql.ast.tree.TableFunction;
import org.opensearch.sql.ast.tree.UnresolvedPlan;
import org.opensearch.sql.ast.tree.Values;
import org.opensearch.sql.common.antlr.SyntaxCheckException;
import org.opensearch.sql.data.model.ExprMissingValue;
import org.opensearch.sql.data.type.ExprCoreType;
import org.opensearch.sql.data.type.ExprType;
import org.opensearch.sql.datasource.DataSourceService;
import org.opensearch.sql.exception.SemanticCheckException;
import org.opensearch.sql.expression.DSL;
import org.opensearch.sql.expression.Expression;
import org.opensearch.sql.expression.FunctionExpression;
import org.opensearch.sql.expression.LiteralExpression;
import org.opensearch.sql.expression.NamedExpression;
import org.opensearch.sql.expression.ReferenceExpression;
import org.opensearch.sql.expression.aggregation.Aggregator;
import org.opensearch.sql.expression.aggregation.NamedAggregator;
import org.opensearch.sql.expression.function.BuiltinFunctionName;
import org.opensearch.sql.expression.function.BuiltinFunctionRepository;
import org.opensearch.sql.expression.function.FunctionName;
import org.opensearch.sql.expression.function.TableFunctionImplementation;
Expand Down Expand Up @@ -217,13 +220,35 @@ public LogicalPlan visitLimit(Limit node, AnalysisContext context) {
public LogicalPlan visitFilter(Filter node, AnalysisContext context) {
LogicalPlan child = node.getChild().get(0).accept(this, context);
Expression condition = expressionAnalyzer.analyze(node.getCondition(), context);
verifySupportsCondition(condition);

ExpressionReferenceOptimizer optimizer =
new ExpressionReferenceOptimizer(expressionAnalyzer.getRepository(), child);
Expression optimized = optimizer.optimize(condition, context);
return new LogicalFilter(child, optimized);
}

/**
* Ensure NESTED function is not used in WHERE clause. Fallback to legacy engine. Can remove when
* support is added for NESTED function in WHERE and GROUP BY clauses.
* @param condition : Filter condition
*/
private void verifySupportsCondition(Expression condition) {
if (condition instanceof FunctionExpression) {
if (((FunctionExpression) condition).getFunctionName().getFunctionName().equalsIgnoreCase(
BuiltinFunctionName.NESTED.name()
)) {
throw new SyntaxCheckException(
"Falling back to legacy engine. Nested function is not supported in WHERE"
+ " and GROUP BY clauses."
);
}
((FunctionExpression)condition).getArguments().stream()
.forEach(e -> verifySupportsCondition(e)
);
}
}

/**
* Build {@link LogicalRename}.
*/
Expand Down Expand Up @@ -273,7 +298,9 @@ public LogicalPlan visitAggregation(Aggregation node, AnalysisContext context) {
}

for (UnresolvedExpression expr : node.getGroupExprList()) {
groupbyBuilder.add(namedExpressionAnalyzer.analyze(expr, context));
NamedExpression resolvedExpr = namedExpressionAnalyzer.analyze(expr, context);
verifySupportsCondition(resolvedExpr.getDelegated());
groupbyBuilder.add(resolvedExpr);
}
ImmutableList<NamedExpression> groupBys = groupbyBuilder.build();

Expand Down
48 changes: 48 additions & 0 deletions core/src/test/java/org/opensearch/sql/analysis/AnalyzerTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@
import org.opensearch.sql.ast.tree.ML;
import org.opensearch.sql.ast.tree.RareTopN.CommandType;
import org.opensearch.sql.ast.tree.UnresolvedPlan;
import org.opensearch.sql.common.antlr.SyntaxCheckException;
import org.opensearch.sql.exception.ExpressionEvaluationException;
import org.opensearch.sql.exception.SemanticCheckException;
import org.opensearch.sql.expression.DSL;
Expand Down Expand Up @@ -1016,6 +1017,53 @@ public void select_all_from_subquery() {
);
}

/**
* Ensure Nested function falls back to legacy engine when used in GROUP BY clause.
* TODO Remove this test when support is added.
*/
@Test
public void nested_group_by_clause_throws_syntax_exception() {
SyntaxCheckException exception = assertThrows(SyntaxCheckException.class,
() -> analyze(
AstDSL.project(
AstDSL.agg(
AstDSL.relation("schema"),
emptyList(),
emptyList(),
ImmutableList.of(alias("nested(message.info)",
function("nested",
qualifiedName("message", "info")))),
emptyList()
)))
);
assertEquals("Falling back to legacy engine. Nested function is not supported in WHERE"
+ " and GROUP BY clauses.",
exception.getMessage());
}

/**
* Ensure Nested function falls back to legacy engine when used in WHERE clause.
* TODO Remove this test when support is added.
*/
@Test
public void nested_where_clause_throws_syntax_exception() {
SyntaxCheckException exception = assertThrows(SyntaxCheckException.class,
() -> analyze(
AstDSL.filter(
AstDSL.relation("schema"),
AstDSL.equalTo(
AstDSL.function("nested", qualifiedName("message", "info")),
AstDSL.stringLiteral("str")
)
)
)
);
assertEquals("Falling back to legacy engine. Nested function is not supported in WHERE"
+ " and GROUP BY clauses.",
exception.getMessage());
}


/**
* SELECT name, AVG(age) FROM test GROUP BY name.
*/
Expand Down
27 changes: 26 additions & 1 deletion integ-test/src/test/java/org/opensearch/sql/sql/NestedIT.java
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
import java.io.IOException;
import java.util.List;
import java.util.Map;

import org.json.JSONArray;
import org.json.JSONObject;
import org.junit.Test;
Expand Down Expand Up @@ -171,6 +170,32 @@ public void nested_function_mixed_with_non_nested_type_test() {
rows("zz", "a"));
}

@Test
public void nested_function_with_where_clause() {
String query =
"SELECT nested(message.info) FROM " + TEST_INDEX_NESTED_TYPE + " WHERE nested(message.info) = 'a'";
JSONObject result = executeJdbcRequest(query);

assertEquals(2, result.getInt("total"));
verifyDataRows(result,
rows("a"),
rows("a"));
}

// Nested function in GROUP BY clause is not yet implemented for JDBC format. This test ensures
// that the V2 engine falls back to legacy implementation.
// TODO Fix the test when NESTED is supported in GROUP BY in the V2 engine.
@Test
public void nested_function_with_group_by_clause() {
String query =
"SELECT count(*) FROM " + TEST_INDEX_NESTED_TYPE + " GROUP BY nested(message.info)";
JSONObject result = executeJdbcRequest(query);

assertTrue(result.getJSONObject("error").get("details").toString().contains(
"Aggregation type nested is not yet implemented"
));
}

@Test
public void nested_function_mixed_with_non_nested_types_test() {
String query =
Expand Down

0 comments on commit e574dd2

Please sign in to comment.