Skip to content
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

Merged
merged 40 commits into from
Mar 17, 2023
Merged
Show file tree
Hide file tree
Changes from 36 commits
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
9f2ad1d
Implement json support for V2 engine
margarit-h Feb 2, 2023
3167ee5
Reverted some changes
margarit-h Feb 2, 2023
5e7a28d
Removed some fields
margarit-h Feb 3, 2023
ab98db4
minor fix
margarit-h Feb 3, 2023
26920d0
Added a unit test, cleaned up
margarit-h Feb 6, 2023
543773e
Returning raw OpenSearch response when type is json
margarit-h Feb 9, 2023
3f23122
Add an integration test, fix checkstyle errors
margarit-h Feb 10, 2023
1365ae0
Made new engine fallback to legacy for in memory operations for json …
GumpacG Feb 22, 2023
8e43ba6
Address build failures
Feb 22, 2023
799a4ce
Added legacy fall back
GumpacG Feb 27, 2023
a82de60
Refactored fall back logic to use visitor design pattern
GumpacG Mar 1, 2023
f11274f
Added unit tests
GumpacG Mar 1, 2023
16af6ea
Revert "Address build failures"
GumpacG Mar 1, 2023
7a782c7
Removed unnecessary IT
GumpacG Mar 2, 2023
52a3c99
Addressed PR feedback
GumpacG Mar 2, 2023
b676acd
Removed unnecessary context
GumpacG Mar 3, 2023
f7c5ee6
Added fall back for Filter functions
GumpacG Mar 3, 2023
bdb0ff6
Made new engine fallback to legacy for in memory operations for json …
GumpacG Feb 22, 2023
7b4fea1
Address build failures
Feb 22, 2023
581e7fe
Added legacy fall back
GumpacG Feb 27, 2023
d882830
Refactored fall back logic to use visitor design pattern
GumpacG Mar 1, 2023
7ecc76e
Added unit tests
GumpacG Mar 1, 2023
1a85ff0
Revert "Address build failures"
GumpacG Mar 1, 2023
bb2d8e4
Removed unnecessary IT
GumpacG Mar 2, 2023
ce02e9d
Addressed PR feedback
GumpacG Mar 2, 2023
dbe5067
Removed unnecessary context
GumpacG Mar 3, 2023
4ff206b
Added fall back for Filter functions
GumpacG Mar 3, 2023
44fd436
Merge branch 'dev-json-fallback' of github.com:Bit-Quill/opensearch-p…
GumpacG Mar 3, 2023
a31dc27
Address build failures (#1366)
Mar 3, 2023
bee2102
Fixed checkstyle errors
GumpacG Mar 6, 2023
74f88ae
Addressed PR comments and fixed the visitor
GumpacG Mar 7, 2023
6785f23
Added comment to visitor class
GumpacG Mar 7, 2023
9e733aa
Addressed PR comments to improve visitor class
GumpacG Mar 9, 2023
232196d
Added unit tests for JsonSupportVisitor
GumpacG Mar 10, 2023
e0e2365
Added helper function for SQLServiceTest
GumpacG Mar 10, 2023
b60da69
Added expected failures
GumpacG Mar 13, 2023
c2cb0f3
Reworked the visitor class to have type Void instead of Boolean
GumpacG Mar 13, 2023
3bd7865
Resolved merge conflicts
GumpacG Mar 13, 2023
9233d7c
Fixed typo
GumpacG Mar 13, 2023
3335354
Added github link for tracking issue
GumpacG Mar 14, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
119 changes: 119 additions & 0 deletions core/src/main/java/org/opensearch/sql/analysis/JsonSupportVisitor.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
/*
* 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.Project;

/**
* This visitor's sole purpose is to throw UnsupportedOperationExceptions
* for unsupported features in the new engine when JSON format is specified.
* Unsupported features in V2 are ones the produce results that differ from
* legacy results.
*/
public class JsonSupportVisitor extends AbstractNodeVisitor<Boolean, JsonSupportVisitorContext> {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change type to Void and only throw exceptions.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in c2cb0f3

@Override
public Boolean visit(Node node, JsonSupportVisitorContext context) {
return visitChildren(node, context);
}

@Override
public Boolean visitChildren(Node node, JsonSupportVisitorContext context) {
for (Node child : node.getChild()) {
if (!child.accept(this, context)) {
return Boolean.FALSE;
}
}
return Boolean.TRUE;
}

@Override
public Boolean visitAggregation(Aggregation node, JsonSupportVisitorContext context) {
if (node.getGroupExprList().isEmpty()) {
return Boolean.TRUE;
} else {
context.setUnsupportedOperationException(new UnsupportedOperationException(
"Queries with aggregation are not yet supported with json format in the new engine"));
return Boolean.FALSE;
}
}

@Override
public Boolean visitFunction(Function node, JsonSupportVisitorContext context) {
// Supported if outside of Project
if (!context.isVisitingProject()) {
return Boolean.TRUE;
}

// queries with function calls are not supported.
context.setUnsupportedOperationException(new UnsupportedOperationException(
"Queries with functions are not yet supported with json format in the new engine"));
return Boolean.FALSE;
}

@Override
public Boolean visitLiteral(Literal node, JsonSupportVisitorContext context) {
// Supported if outside of Project
if (!context.isVisitingProject()) {
return Boolean.TRUE;
}

// queries with literal values are not supported
context.setUnsupportedOperationException(new UnsupportedOperationException(
"Queries with literals are not yet supported with json format in the new engine"));
return Boolean.FALSE;
}

@Override
public Boolean visitCast(Cast node, JsonSupportVisitorContext context) {
// Supported if outside of Project
if (!context.isVisitingProject()) {
return Boolean.TRUE;
}

// Queries with cast are not supported
context.setUnsupportedOperationException(new UnsupportedOperationException(
"Queries with casts are not yet supported with json format in the new engine"));
return Boolean.FALSE;
}

@Override
public Boolean visitAlias(Alias node, JsonSupportVisitorContext context) {
// Supported if outside of Project
if (!context.isVisitingProject()) {
return Boolean.TRUE;
}

// 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 node.getDelegated().accept(this, context);
} else {
context.setUnsupportedOperationException(new UnsupportedOperationException(
"Queries with aliases are not yet supported with json format in the new engine"));
return Boolean.FALSE;
}
}

@Override
public Boolean visitProject(Project node, JsonSupportVisitorContext context) {
boolean isSupported = visit(node, context);

context.setVisitingProject(true);
isSupported = isSupported ? node.getProjectList().stream()
.allMatch(e -> e.accept(this, context)) : Boolean.FALSE;
context.setVisitingProject(false);

return isSupported;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/

package org.opensearch.sql.analysis;

import lombok.Getter;
import lombok.Setter;

/**
* The context used for JsonSupportVisitor.
*/
public class JsonSupportVisitorContext {
@Getter
@Setter
private boolean isVisitingProject = false;

@Getter
@Setter
private UnsupportedOperationException unsupportedOperationException;
}
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.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.opensearch.sql.ast.dsl.AstDSL.qualifiedName;

import com.google.common.collect.ImmutableList;
import java.util.Collections;
import org.junit.jupiter.api.Test;
import org.opensearch.sql.ast.dsl.AstDSL;
import org.opensearch.sql.ast.expression.Literal;
import org.opensearch.sql.ast.expression.UnresolvedExpression;
import org.opensearch.sql.ast.tree.UnresolvedPlan;

class JsonSupportVisitorTest {
@Test
public void visitLiteralInProject() {
UnresolvedPlan project = AstDSL.project(
AstDSL.relation("table", "table"),
AstDSL.intLiteral(1));
assertFalse(project.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext()));
}

@Test
public void visitLiteralOutsideProject() {
Literal intLiteral = AstDSL.intLiteral(1);
assertTrue(intLiteral.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext()));
}

@Test
public void visitCastInProject() {
UnresolvedPlan project = AstDSL.project(
AstDSL.relation("table", "table"),
AstDSL.cast(AstDSL.intLiteral(1), AstDSL.stringLiteral("INT")));
assertFalse(project.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext()));
}

@Test
public void visitCastOutsideProject() {
UnresolvedExpression intCast = AstDSL.cast(
AstDSL.intLiteral(1),
AstDSL.stringLiteral("INT"));
assertTrue(intCast.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext()));
}

@Test
public void visitAliasInProject() {
UnresolvedPlan project = AstDSL.project(
AstDSL.relation("table", "table"),
AstDSL.alias("alias", AstDSL.intLiteral(1)));
assertFalse(project.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext()));
}

@Test
public void visitAliasInProjectWithDelegated() {
UnresolvedPlan project = AstDSL.project(
AstDSL.relation("table", "table"),
AstDSL.alias("alias", AstDSL.intLiteral(1), "alias"));
assertFalse(project.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext()));
}

@Test
public void visitAliasOutsideProject() {
UnresolvedExpression alias = AstDSL.alias("alias", AstDSL.intLiteral(1));
assertTrue(alias.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext()));
}

@Test
public void visitFunctionInProject() {
UnresolvedPlan function = AstDSL.project(
AstDSL.relation("table", "table"),
AstDSL.function("abs", AstDSL.intLiteral(-1)));
assertFalse(function.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext()));
}

@Test
public void visitFunctionOutsideProject() {
UnresolvedExpression function = AstDSL.function("abs", AstDSL.intLiteral(-1));
assertTrue(function.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext()));
}

@Test
public void visitAggregationWithGroupExprList() {
UnresolvedPlan projectAggr = AstDSL.project(AstDSL.agg(
AstDSL.relation("table", "table"),
Collections.emptyList(),
Collections.emptyList(),
ImmutableList.of(AstDSL.alias("alias", qualifiedName("integer_value"))),
Collections.emptyList()));
assertFalse(projectAggr.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext()));
}

@Test
public void visitAggregationWithAggExprList() {
UnresolvedPlan aggregation = AstDSL.agg(
AstDSL.relation("table", "table"),
ImmutableList.of(
AstDSL.alias("AVG(alias)",
AstDSL.aggregate("AVG",
qualifiedName("integer_value")))),
Collections.emptyList(),
Collections.emptyList(),
Collections.emptyList());
assertTrue(aggregation.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext()));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import org.json.JSONObject;
import org.junit.Ignore;
import org.junit.Test;
import org.opensearch.client.ResponseException;
import org.opensearch.sql.legacy.exception.SqlParseException;

public class DateFormatIT extends SQLIntegTestCase {
Expand All @@ -50,7 +51,8 @@ protected void init() throws Exception {
* this ends up excluding some of the expected values causing the assertion to fail. LIMIT overrides this.
*/

@Test
// DATE_FORMAT with timezone argument is not yet supported in V2
@Test(expected = SqlParseException.class)
public void equalTo() throws SqlParseException {
assertThat(
dateQuery(
Expand All @@ -59,7 +61,8 @@ public void equalTo() throws SqlParseException {
);
}

@Test
// DATE_FORMAT with timezone argument is not yet supported in V2
@Test(expected = SqlParseException.class)
public void lessThan() throws SqlParseException {
assertThat(
dateQuery(
Expand All @@ -68,7 +71,8 @@ public void lessThan() throws SqlParseException {
);
}

@Test
// DATE_FORMAT with timezone argument is not yet supported in V2
@Test(expected = SqlParseException.class)
public void lessThanOrEqualTo() throws SqlParseException {
assertThat(
dateQuery(
Expand All @@ -79,7 +83,8 @@ public void lessThanOrEqualTo() throws SqlParseException {
);
}

@Test
// DATE_FORMAT with timezone argument is not yet supported in V2
@Test(expected = SqlParseException.class)
public void greaterThan() throws SqlParseException {
assertThat(
dateQuery(
Expand All @@ -88,7 +93,8 @@ public void greaterThan() throws SqlParseException {
);
}

@Test
// DATE_FORMAT with timezone argument is not yet supported in V2
@Test(expected = SqlParseException.class)
public void greaterThanOrEqualTo() throws SqlParseException {
assertThat(
dateQuery(
Expand All @@ -99,7 +105,8 @@ public void greaterThanOrEqualTo() throws SqlParseException {
);
}

@Test
// DATE_FORMAT with timezone argument is not yet supported in V2
@Test(expected = SqlParseException.class)
public void and() throws SqlParseException {
assertThat(
dateQuery(SELECT_FROM +
Expand All @@ -122,7 +129,8 @@ public void andWithDefaultTimeZone() throws SqlParseException {
);
}

@Test
// DATE_FORMAT with timezone argument is not yet supported in V2
@Test(expected = SqlParseException.class)
public void or() throws SqlParseException {
assertThat(
dateQuery(SELECT_FROM +
Expand All @@ -134,8 +142,8 @@ public void or() throws SqlParseException {
);
}


@Test
// DATE_FORMAT with timezone argument is not yet supported in V2
@Test(expected = ResponseException.class)
public void sortByDateFormat() throws IOException {
// Sort by expression in descending order, but sort inside in ascending order, so we increase our confidence
// that successful test isn't just random chance.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import org.json.JSONObject;
import org.junit.Test;
import org.opensearch.action.search.SearchResponse;
import org.opensearch.client.ResponseException;
import org.opensearch.common.xcontent.LoggingDeprecationHandler;
import org.opensearch.common.xcontent.XContentFactory;
import org.opensearch.common.xcontent.XContentType;
Expand Down Expand Up @@ -133,7 +134,9 @@ public void scoreQueryWithNestedField() throws IOException {
);
}

@Test
// Specifying city.keyword is not supported in V2
// Rework on data types is in progress and tracked with https://github.com/opensearch-project/sql/pull/1314
@Test(expected = ResponseException.class)
public void wildcardQuery() throws IOException {
assertThat(
query(
Expand Down
Loading