-
Notifications
You must be signed in to change notification settings - Fork 0
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
Legacy fall back with JSON format #237
Changes from 29 commits
9f2ad1d
3167ee5
5e7a28d
ab98db4
26920d0
543773e
3f23122
1365ae0
8e43ba6
799a4ce
a82de60
f11274f
16af6ea
7a782c7
52a3c99
b676acd
f7c5ee6
bdb0ff6
7b4fea1
581e7fe
d882830
7ecc76e
1a85ff0
bb2d8e4
ce02e9d
dbe5067
4ff206b
44fd436
a31dc27
bee2102
74f88ae
6785f23
9e733aa
232196d
e0e2365
b60da69
c2cb0f3
3bd7865
9233d7c
3335354
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,80 @@ | ||
/* | ||
* Copyright OpenSearch Contributors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
package org.opensearch.sql.analysis; | ||
|
||
import org.apache.commons.lang3.StringUtils; | ||
import org.opensearch.sql.ast.AbstractNodeVisitor; | ||
import org.opensearch.sql.ast.Node; | ||
import org.opensearch.sql.ast.expression.Alias; | ||
import org.opensearch.sql.ast.expression.Cast; | ||
import org.opensearch.sql.ast.expression.Function; | ||
import org.opensearch.sql.ast.expression.Literal; | ||
import org.opensearch.sql.ast.tree.Aggregation; | ||
import org.opensearch.sql.ast.tree.Filter; | ||
import org.opensearch.sql.ast.tree.Project; | ||
|
||
public class JsonSupportAnalyzer extends AbstractNodeVisitor<Boolean, Object> { | ||
@Override | ||
public Boolean visit(Node node, Object context) { | ||
// A node is supported if all of its children are supported. | ||
return node.getChild().stream().allMatch(c -> c.accept(this, null)); | ||
} | ||
|
||
@Override | ||
public Boolean visitChildren(Node node, Object context) { | ||
for (Node child : node.getChild()) { | ||
if (!child.accept(this, null)) | ||
return false; | ||
} | ||
return true; | ||
} | ||
|
||
@Override | ||
public Boolean visitFunction(Function node, Object context) { | ||
// queries with function calls are not supported. | ||
return false; | ||
} | ||
|
||
@Override | ||
public Boolean visitLiteral(Literal node, Object context) { | ||
// queries with literal values are not supported | ||
return false; | ||
} | ||
|
||
@Override | ||
public Boolean visitCast(Cast node, Object context) { | ||
// Queries with cast are not supported | ||
return false; | ||
} | ||
|
||
@Override | ||
public Boolean visitAlias(Alias node, Object context) { | ||
// Alias node is accepted if it does not have a user-defined alias | ||
// and if the delegated expression is accepted. | ||
if (!StringUtils.isEmpty(node.getAlias())) | ||
return false; | ||
else { | ||
return node.getDelegated().accept(this, null); | ||
} | ||
} | ||
|
||
@Override | ||
public Boolean visitProject(Project node, Object context) { | ||
return visit(node, null) | ||
&& node.getProjectList().stream().allMatch(e -> e.accept(this, null)); | ||
} | ||
|
||
@Override | ||
public Boolean visitAggregation(Aggregation node, Object context) { | ||
return node.getGroupExprList().isEmpty(); | ||
} | ||
|
||
@Override | ||
public Boolean visitFilter(Filter node, Object context) { | ||
return visit(node, null) | ||
&& node.getCondition().accept(this, null); | ||
} | ||
} |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,8 +7,11 @@ | |
package org.opensearch.sql.sql; | ||
|
||
import java.util.Optional; | ||
|
||
import lombok.RequiredArgsConstructor; | ||
import org.antlr.v4.runtime.tree.ParseTree; | ||
import org.opensearch.sql.analysis.JsonSupportAnalyzer; | ||
import org.opensearch.sql.ast.statement.Query; | ||
import org.opensearch.sql.ast.statement.Statement; | ||
import org.opensearch.sql.common.response.ResponseListener; | ||
import org.opensearch.sql.executor.ExecutionEngine.ExplainResponse; | ||
|
@@ -65,6 +68,9 @@ private AbstractPlan plan( | |
SQLQueryRequest request, | ||
Optional<ResponseListener<QueryResponse>> queryListener, | ||
Optional<ResponseListener<ExplainResponse>> explainListener) { | ||
if (parser.containsHints(request.getQuery())) | ||
throw new UnsupportedOperationException("Hints are not yet supported in the new engine."); | ||
|
||
// 1.Parse query and convert parse tree (CST) to abstract syntax tree (AST) | ||
ParseTree cst = parser.parse(request.getQuery()); | ||
Statement statement = | ||
|
@@ -75,6 +81,18 @@ private AbstractPlan plan( | |
.isExplain(request.isExplainRequest()) | ||
.build())); | ||
|
||
// There is no full support for JSON format yet for in memory operations, aliases, literals, and casts | ||
// Aggregation has differences with legacy results | ||
if (request.format().getFormatName().equals("json") && statement instanceof Query) { | ||
Boolean isJsonSupported = ((Query) statement).getPlan().accept(new JsonSupportAnalyzer(), null); | ||
|
||
if (!isJsonSupported) { | ||
throw new UnsupportedOperationException( | ||
"Queries with aggregation and in memory operations (cast, literals, alias, math, etc.)" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Curious what of these are supported in legacy engine with JSON format? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All of those are supported in legacy because they are pushed down. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For slightly better error messaging, throw the errors in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've made each unsupported node visitor throw the exception in 74f88ae. Thanks! |
||
+ " are not yet supported with json format in the new engine"); | ||
} | ||
} | ||
|
||
return queryExecutionFactory.create(statement, queryListener, explainListener); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -32,4 +32,9 @@ public ParseTree parse(String query) { | |
return parser.root(); | ||
} | ||
|
||
public boolean containsHints(String query) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Longer term, we want to return hints... not whether the parser contains hints. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks. Changed this in 74f88ae |
||
OpenSearchSQLLexer lexer = new OpenSearchSQLLexer(new CaseInsensitiveCharStream(query)); | ||
Yury-Fridlyand marked this conversation as resolved.
Show resolved
Hide resolved
|
||
OpenSearchSQLParser hintsParser = new OpenSearchSQLParser(new CommonTokenStream(lexer, OpenSearchSQLLexer.SQLCOMMENT)); | ||
return hintsParser.root().getChildCount() > 1; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,7 @@ | |
package org.opensearch.sql.sql; | ||
|
||
import static org.junit.jupiter.api.Assertions.assertNotNull; | ||
import static org.junit.jupiter.api.Assertions.assertTrue; | ||
import static org.junit.jupiter.api.Assertions.fail; | ||
import static org.mockito.ArgumentMatchers.any; | ||
import static org.mockito.Mockito.doAnswer; | ||
|
@@ -83,6 +84,114 @@ public void onFailure(Exception e) { | |
}); | ||
} | ||
|
||
@Test | ||
public void canExecuteJsonFormatRequest() { | ||
doAnswer(invocation -> { | ||
ResponseListener<QueryResponse> listener = invocation.getArgument(1); | ||
listener.onResponse(new QueryResponse(schema, Collections.emptyList(), "")); | ||
return null; | ||
}).when(queryService).execute(any(), any()); | ||
|
||
sqlService.execute( | ||
new SQLQueryRequest(new JSONObject(), "SELECT FIELD FROM TABLE", QUERY, "json"), | ||
new ResponseListener<QueryResponse>() { | ||
@Override | ||
public void onResponse(QueryResponse response) { | ||
assertNotNull(response); | ||
} | ||
|
||
@Override | ||
public void onFailure(Exception e) { | ||
fail(e); | ||
} | ||
}); | ||
} | ||
|
||
@Test | ||
public void canThrowUnsupportedExceptionForAggregationJsonQuery() { | ||
sqlService.execute( | ||
new SQLQueryRequest(new JSONObject(), "SELECT FIELD FROM TABLE GROUP BY FIELD", QUERY, "json"), | ||
new ResponseListener<QueryResponse>() { | ||
@Override | ||
public void onResponse(QueryResponse response) { | ||
assertNotNull(response); | ||
} | ||
|
||
@Override | ||
public void onFailure(Exception e) { | ||
assertTrue(e instanceof UnsupportedOperationException); | ||
} | ||
}); | ||
} | ||
|
||
@Test | ||
public void canThrowUnsupportedExceptionForAliasJsonQuery() { | ||
sqlService.execute( | ||
new SQLQueryRequest(new JSONObject(), "SELECT FIELD as FIELD FROM TABLE", QUERY, "json"), | ||
new ResponseListener<QueryResponse>() { | ||
@Override | ||
public void onResponse(QueryResponse response) { | ||
assertNotNull(response); | ||
} | ||
|
||
@Override | ||
public void onFailure(Exception e) { | ||
assertTrue(e instanceof UnsupportedOperationException); | ||
} | ||
}); | ||
} | ||
|
||
@Test | ||
public void canThrowUnsupportedExceptionForFunctionJsonQuery() { | ||
sqlService.execute( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This block of code is common to all the tests, isn't it? They only differ in query sent. Can you put it in a utility method that's call by the tests? You can make it even more compact by creating a parameterized test that takes a list of sql statement to test. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, changed in e0e2365. Please let me know if you had something else in mind. |
||
new SQLQueryRequest(new JSONObject(), "SELECT 1 + 1", QUERY, "json"), | ||
new ResponseListener<QueryResponse>() { | ||
@Override | ||
public void onResponse(QueryResponse response) { | ||
assertNotNull(response); | ||
} | ||
|
||
@Override | ||
public void onFailure(Exception e) { | ||
assertTrue(e instanceof UnsupportedOperationException); | ||
} | ||
}); | ||
} | ||
|
||
@Test | ||
public void canThrowUnsupportedExceptionForLiteralJsonQuery() { | ||
sqlService.execute( | ||
new SQLQueryRequest(new JSONObject(), "SELECT 123", QUERY, "json"), | ||
new ResponseListener<QueryResponse>() { | ||
@Override | ||
public void onResponse(QueryResponse response) { | ||
assertNotNull(response); | ||
} | ||
|
||
@Override | ||
public void onFailure(Exception e) { | ||
assert(e instanceof UnsupportedOperationException); | ||
} | ||
}); | ||
} | ||
|
||
@Test | ||
public void canThrowUnsupportedExceptionForCastJsonQuery() { | ||
sqlService.execute( | ||
new SQLQueryRequest(new JSONObject(), "SELECT CAST(FIELD AS DOUBLE)", QUERY, "json"), | ||
new ResponseListener<QueryResponse>() { | ||
@Override | ||
public void onResponse(QueryResponse response) { | ||
assertNotNull(response); | ||
} | ||
|
||
@Override | ||
public void onFailure(Exception e) { | ||
assert(e instanceof UnsupportedOperationException); | ||
} | ||
}); | ||
} | ||
|
||
@Test | ||
public void canExecuteCsvFormatRequest() { | ||
doAnswer(invocation -> { | ||
|
@@ -106,6 +215,23 @@ public void onFailure(Exception e) { | |
}); | ||
} | ||
|
||
@Test | ||
public void canThrowUnsupportedExceptionForHintsQuery() { | ||
sqlService.execute( | ||
new SQLQueryRequest(new JSONObject(), "SELECT /*! HINTS */ 123", QUERY, "jdbc"), | ||
new ResponseListener<QueryResponse>() { | ||
@Override | ||
public void onResponse(QueryResponse response) { | ||
assertNotNull(response); | ||
} | ||
|
||
@Override | ||
public void onFailure(Exception e) { | ||
assert(e instanceof UnsupportedOperationException); | ||
} | ||
}); | ||
} | ||
|
||
@Test | ||
public void canExplainSqlQuery() { | ||
doAnswer(invocation -> { | ||
|
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.
JsonSupportNodeVisitor
? I'm not sure why we call things "analyzers" when they're node visitors.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.
<Boolean, Object>
doesn't seem correct, but I suppose we don't really have a ned for a context yet, nor a return type (which is usually a Plan or OptimizedTree.If the purpose of the node visitor is just to find unsupported nodes, then I suppose this is fine but we should include a javadoc comment that details 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.
Longer term, we may consider using this to return a JSON object or a tree that can be turned into a JSON object.
We can remove the Boolean return type if we throw Exceptions in the node visitors.
If not, we should be returning Boolean types, not boolean types. Example,
Boolean.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.
Thanks. Changed this in 74f88ae and 6785f23.
I've made each unsupported node visitor throw an exception so there is no more use for return type boolean. However The abstract class requires a return type so I left it as Boolean.