diff --git a/api/src/main/java/org/opensearch/sql/api/UnifiedQueryContext.java b/api/src/main/java/org/opensearch/sql/api/UnifiedQueryContext.java index 7a885edd65a..38c6561b409 100644 --- a/api/src/main/java/org/opensearch/sql/api/UnifiedQueryContext.java +++ b/api/src/main/java/org/opensearch/sql/api/UnifiedQueryContext.java @@ -122,6 +122,11 @@ public static class Builder { * Setting values with defaults from SysLimit.DEFAULT. Only includes planning-required settings * to avoid coupling with OpenSearchSettings. * + *

{@link Settings.Key#PPL_JOIN_SUBSEARCH_MAXOUT} defaults to {@code 0} to avoid injecting + * {@code LogicalSystemLimit} into the logical plan, which is an OpenSearch-specific operational + * concern irrelevant to external consumers of the unified query API. {@link + * Settings.Key#PPL_SUBSEARCH_MAXOUT} is set to {@code 0} for the same reason. + * *

{@link Settings.Key#CALCITE_ENGINE_ENABLED} defaults to {@code true} here because the * unified query path is by definition Calcite-based — every query reaching this context flows * through Calcite's planner, never the v2 engine. The PPL {@link @@ -142,8 +147,8 @@ public static class Builder { new HashMap( Map.of( QUERY_SIZE_LIMIT, SysLimit.DEFAULT.querySizeLimit(), - PPL_SUBSEARCH_MAXOUT, SysLimit.DEFAULT.subsearchLimit(), - PPL_JOIN_SUBSEARCH_MAXOUT, SysLimit.DEFAULT.joinSubsearchLimit(), + PPL_SUBSEARCH_MAXOUT, SysLimit.UNLIMITED_SUBSEARCH.subsearchLimit(), + PPL_JOIN_SUBSEARCH_MAXOUT, SysLimit.UNLIMITED_SUBSEARCH.joinSubsearchLimit(), CALCITE_ENGINE_ENABLED, true, PPL_REX_MAX_MATCH_LIMIT, 10)); diff --git a/api/src/main/java/org/opensearch/sql/api/parser/SqlV2QueryParser.java b/api/src/main/java/org/opensearch/sql/api/parser/SqlV2QueryParser.java index 827278a2119..30e88e5ce3a 100644 --- a/api/src/main/java/org/opensearch/sql/api/parser/SqlV2QueryParser.java +++ b/api/src/main/java/org/opensearch/sql/api/parser/SqlV2QueryParser.java @@ -5,11 +5,18 @@ package org.opensearch.sql.api.parser; +import static org.opensearch.sql.ast.dsl.AstDSL.join; + +import java.util.Optional; import org.antlr.v4.runtime.tree.ParseTree; +import org.opensearch.sql.ast.expression.UnresolvedExpression; import org.opensearch.sql.ast.statement.Query; import org.opensearch.sql.ast.statement.Statement; +import org.opensearch.sql.ast.tree.Join.JoinType; import org.opensearch.sql.ast.tree.UnresolvedPlan; import org.opensearch.sql.sql.antlr.SQLSyntaxParser; +import org.opensearch.sql.sql.antlr.parser.OpenSearchSQLParser; +import org.opensearch.sql.sql.antlr.parser.OpenSearchSQLParser.JoinClauseContext; import org.opensearch.sql.sql.parser.AstBuilder; import org.opensearch.sql.sql.parser.AstStatementBuilder; @@ -24,7 +31,8 @@ public UnresolvedPlan parse(String query) { ParseTree cst = syntaxParser.parse(query); AstStatementBuilder astStmtBuilder = new AstStatementBuilder( - new AstBuilder(query), AstStatementBuilder.StatementBuilderContext.builder().build()); + new ExtendedAstBuilder(query), + AstStatementBuilder.StatementBuilderContext.builder().build()); Statement statement = cst.accept(astStmtBuilder); if (statement instanceof Query) { @@ -33,4 +41,33 @@ public UnresolvedPlan parse(String query) { throw new UnsupportedOperationException( "Only query statements are supported but got " + statement.getClass().getSimpleName()); } + + /** + * Extends the V2 AstBuilder with JOIN support that the base AstBuilder rejects with + * SyntaxCheckException to trigger legacy engine fallback. + */ + private static class ExtendedAstBuilder extends AstBuilder { + + ExtendedAstBuilder(String query) { + super(query); + } + + @Override + public UnresolvedPlan visitJoinClause(JoinClauseContext ctx) { + JoinType joinType = toJoinType(ctx); + UnresolvedPlan right = visit(ctx.relation()); + Optional condition = + Optional.ofNullable(ctx.expression()).map(this::visitAstExpression); + return join(right, joinType, condition); + } + + private JoinType toJoinType(JoinClauseContext ctx) { + return switch (ctx.getStart().getType()) { + case OpenSearchSQLParser.LEFT -> JoinType.LEFT; + case OpenSearchSQLParser.RIGHT -> JoinType.RIGHT; + case OpenSearchSQLParser.CROSS -> JoinType.CROSS; + default -> JoinType.INNER; + }; + } + } } diff --git a/api/src/test/java/org/opensearch/sql/api/UnifiedQueryContextTest.java b/api/src/test/java/org/opensearch/sql/api/UnifiedQueryContextTest.java index f0111d06363..3960e8a6206 100644 --- a/api/src/test/java/org/opensearch/sql/api/UnifiedQueryContextTest.java +++ b/api/src/test/java/org/opensearch/sql/api/UnifiedQueryContextTest.java @@ -30,8 +30,8 @@ public void testContextCreationWithDefaults() { assertNotNull("PlanContext should be created", context.getPlanContext()); assertNotNull("Settings should be created", context.getSettings()); assertEquals( - "Settings should have default system limits", - SysLimit.DEFAULT, + "Settings should have unlimited subsearch limits for clean logical plans", + new SysLimit(SysLimit.DEFAULT.querySizeLimit(), 0, 0), SysLimit.fromSettings(context.getSettings())); assertEquals( "PPL_REX_MAX_MATCH_LIMIT default should be 10", diff --git a/api/src/test/java/org/opensearch/sql/api/UnifiedQueryPlannerSqlV2Test.java b/api/src/test/java/org/opensearch/sql/api/UnifiedQueryPlannerSqlV2Test.java index b9913bf450b..afe08e3a2ad 100644 --- a/api/src/test/java/org/opensearch/sql/api/UnifiedQueryPlannerSqlV2Test.java +++ b/api/src/test/java/org/opensearch/sql/api/UnifiedQueryPlannerSqlV2Test.java @@ -5,12 +5,19 @@ package org.opensearch.sql.api; +import static org.apache.calcite.sql.type.SqlTypeName.INTEGER; +import static org.apache.calcite.sql.type.SqlTypeName.VARCHAR; + +import java.util.Map; +import org.apache.calcite.schema.Table; +import org.apache.calcite.schema.impl.AbstractSchema; +import org.junit.Before; import org.junit.Test; import org.opensearch.sql.executor.QueryType; /** - * Tests for basic SQL query planning through the V2 ANTLR parser path. Covers SELECT, WHERE, ORDER - * BY operations that produce valid RelNode plans. + * Tests for SQL query planning through the V2 ANTLR parser path. Covers SELECT, WHERE, ORDER BY, + * and JOIN operations that produce valid RelNode plans. */ public class UnifiedQueryPlannerSqlV2Test extends UnifiedQueryTestBase { @@ -19,17 +26,35 @@ protected QueryType queryType() { return QueryType.SQL; } - @Test - public void selectStar() { - givenQuery("SELECT * FROM catalog.employees") - .assertPlan( - """ - LogicalTableScan(table=[[catalog, employees]]) - """); + @Before + @Override + public void setUp() { + testSchema = + new AbstractSchema() { + @Override + protected Map getTableMap() { + return Map.of( + "employees", createEmployeesTable(), + "departments", createDepartmentsTable()); + } + }; + + context = contextBuilder().build(); + planner = new UnifiedQueryPlanner(context); + } + + private Table createDepartmentsTable() { + return SimpleTable.builder() + .col("dept_id", INTEGER) + .col("dept_name", VARCHAR) + .row(new Object[] {1, "Engineering"}) + .row(new Object[] {2, "Sales"}) + .row(new Object[] {3, "Marketing"}) + .build(); } @Test - public void selectStarFields() { + public void selectStar() { givenQuery("SELECT * FROM catalog.employees") .assertPlan( """ @@ -68,4 +93,53 @@ public void testFilterAndOrderBy() { LogicalTableScan(table=[[catalog, employees]]) """); } + + @Test + public void testJoinTypes() { + Map.of("JOIN", "inner", "LEFT JOIN", "left", "RIGHT JOIN", "right") + .forEach( + (syntax, type) -> + givenQuery( + """ + SELECT * FROM catalog.employees %s catalog.departments + ON employees.department = departments.dept_name + """ + .formatted(syntax)) + .assertPlan( + """ + LogicalJoin(condition=[=($3, $5)], joinType=[%s]) + LogicalTableScan(table=[[catalog, employees]]) + LogicalTableScan(table=[[catalog, departments]]) + """ + .formatted(type))); + } + + @Test + public void testCrossJoin() { + givenQuery("SELECT * FROM catalog.employees CROSS JOIN catalog.departments") + .assertPlan( + """ + LogicalJoin(condition=[true], joinType=[inner]) + LogicalTableScan(table=[[catalog, employees]]) + LogicalTableScan(table=[[catalog, departments]]) + """); + } + + @Test + public void testJoinWithFilterAndOrderBy() { + givenQuery( + """ + SELECT * FROM catalog.employees JOIN catalog.departments + ON employees.department = departments.dept_name + WHERE employees.age > 30 ORDER BY employees.name + """) + .assertPlan( + """ + LogicalSort(sort0=[$1], dir0=[ASC-nulls-first]) + LogicalFilter(condition=[>($2, 30)]) + LogicalJoin(condition=[=($3, $5)], joinType=[inner]) + LogicalTableScan(table=[[catalog, employees]]) + LogicalTableScan(table=[[catalog, departments]]) + """); + } } diff --git a/core/src/main/java/org/opensearch/sql/ast/dsl/AstDSL.java b/core/src/main/java/org/opensearch/sql/ast/dsl/AstDSL.java index b2731ebbd40..f38e46377dc 100644 --- a/core/src/main/java/org/opensearch/sql/ast/dsl/AstDSL.java +++ b/core/src/main/java/org/opensearch/sql/ast/dsl/AstDSL.java @@ -60,6 +60,7 @@ import org.opensearch.sql.ast.tree.FillNull; import org.opensearch.sql.ast.tree.Filter; import org.opensearch.sql.ast.tree.Head; +import org.opensearch.sql.ast.tree.Join; import org.opensearch.sql.ast.tree.Limit; import org.opensearch.sql.ast.tree.MinSpanBin; import org.opensearch.sql.ast.tree.MvCombine; @@ -757,4 +758,17 @@ public static Bin bin(UnresolvedExpression field, Argument... arguments) { public static Field implicitTimestampField() { return AstDSL.field(OpenSearchConstants.IMPLICIT_FIELD_TIMESTAMP); } + + public static UnresolvedPlan join( + UnresolvedPlan right, Join.JoinType joinType, Optional condition) { + return new Join( + right, + Optional.empty(), + Optional.empty(), + joinType, + condition, + new Join.JoinHint(), + Optional.empty(), + Argument.ArgumentMap.empty()); + } } diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/SqlLegacyEngineSanityIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/SqlLegacyEngineSanityIT.java new file mode 100644 index 00000000000..d13df7e2fe7 --- /dev/null +++ b/integ-test/src/test/java/org/opensearch/sql/legacy/SqlLegacyEngineSanityIT.java @@ -0,0 +1,48 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.sql.legacy; + +import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_DOG; +import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_PEOPLE; +import static org.opensearch.sql.util.MatcherUtils.rows; +import static org.opensearch.sql.util.MatcherUtils.verifyDataRows; + +import java.io.IOException; +import org.json.JSONObject; +import org.junit.Test; + +/** + * Sanity tests for the legacy SQL engine. Many legacy integration tests (JoinIT, SubqueryIT, + * MultiQueryIT, etc.) are @Ignored because they assert on the deprecated JSON response format + * removed in 3.0. These tests provide minimal coverage that the legacy engine still executes + * correctly for queries that fall back from the V2 engine. + */ +public class SqlLegacyEngineSanityIT extends SQLIntegTestCase { + + @Override + protected void init() throws Exception { + loadIndex(Index.DOG); + loadIndex(Index.PEOPLE); + } + + @Test + public void testInnerJoinFallback() throws IOException { + JSONObject result = + executeQuery( + "SELECT a.firstname, d.dog_name FROM %s a JOIN %s d ON d.holdersName = a.firstname WHERE a.age > 35" + .formatted(TEST_INDEX_PEOPLE, TEST_INDEX_DOG)); + verifyDataRows(result, rows("Hattie", "snoopy")); + } + + @Test + public void testLeftJoinFallback() throws IOException { + JSONObject result = + executeQuery( + "SELECT a.firstname, d.dog_name FROM %s a LEFT JOIN %s d ON d.holdersName = a.firstname WHERE a.firstname = 'Daenerys'" + .formatted(TEST_INDEX_PEOPLE, TEST_INDEX_DOG)); + verifyDataRows(result, rows("Daenerys", "rex")); + } +} diff --git a/sql/src/main/antlr/OpenSearchSQLParser.g4 b/sql/src/main/antlr/OpenSearchSQLParser.g4 index 6b34507eacc..f0fbec498f9 100644 --- a/sql/src/main/antlr/OpenSearchSQLParser.g4 +++ b/sql/src/main/antlr/OpenSearchSQLParser.g4 @@ -32,6 +32,20 @@ parser grammar OpenSearchSQLParser; options { tokenVocab = OpenSearchSQLLexer; } + +@members { + /** + * Returns true if the next token is a join keyword that should not be consumed as a table alias. + * LEFT/RIGHT are valid identifiers (text function names), so without this predicate ANTLR4 + * greedily consumes them as implicit table aliases (e.g., FROM t1 LEFT becomes alias='LEFT' + * instead of starting a LEFT JOIN clause). + */ + private boolean isJoinKeyword() { + int t = _input.LT(1).getType(); + return t == LEFT || t == RIGHT || t == INNER || t == CROSS || t == JOIN; + } +} + // Top Level Description // Root rule @@ -104,12 +118,18 @@ selectElement ; fromClause - : FROM relation (whereClause)? (groupByClause)? (havingClause)? (orderByClause)? // Place it under FROM for now but actually not necessary ex. A UNION B ORDER BY + : FROM relation joinClause* (whereClause)? (groupByClause)? (havingClause)? (orderByClause)? // Place it under FROM for now but actually not necessary ex. A UNION B ORDER BY ; +joinClause + : (INNER | CROSS)? JOIN relation (ON expression)? + | (LEFT | RIGHT) OUTER? JOIN relation (ON expression)? + ; + relation - : tableName (AS? alias)? # tableAsRelation + // The predicate guarantees only match implicit alias if next token is NOT a join keyword + : tableName (AS alias | {!isJoinKeyword()}? alias)? # tableAsRelation | LR_BRACKET subquery = querySpecification RR_BRACKET AS? alias # subqueryAsRelation | qualifiedName LR_BRACKET tableFunctionArgs RR_BRACKET (AS? alias)? # tableFunctionRelation ; diff --git a/sql/src/main/java/org/opensearch/sql/sql/parser/AstBuilder.java b/sql/src/main/java/org/opensearch/sql/sql/parser/AstBuilder.java index 5250ab7fb0f..3575aa8919a 100644 --- a/sql/src/main/java/org/opensearch/sql/sql/parser/AstBuilder.java +++ b/sql/src/main/java/org/opensearch/sql/sql/parser/AstBuilder.java @@ -139,6 +139,10 @@ public UnresolvedPlan visitLimitClause(OpenSearchSQLParser.LimitClauseContext ct public UnresolvedPlan visitFromClause(FromClauseContext ctx) { UnresolvedPlan result = visit(ctx.relation()); + for (var joinCtx : ctx.joinClause()) { + result = visit(joinCtx).attach(result); + } + if (ctx.whereClause() != null) { result = visit(ctx.whereClause()).attach(result); } @@ -250,6 +254,12 @@ public UnresolvedPlan visitWhereClause(WhereClauseContext ctx) { return new Filter(visitAstExpression(ctx.expression())); } + @Override + public UnresolvedPlan visitJoinClause(OpenSearchSQLParser.JoinClauseContext ctx) { + throw new SyntaxCheckException( + "JOIN is not supported in the V2 SQL engine. Falling back to legacy engine."); + } + @Override public UnresolvedPlan visitHavingClause(HavingClauseContext ctx) { AstHavingFilterBuilder builder = new AstHavingFilterBuilder(context.peek()); @@ -261,7 +271,11 @@ protected UnresolvedPlan aggregateResult(UnresolvedPlan aggregate, UnresolvedPla return nextResult != null ? nextResult : aggregate; } - private UnresolvedExpression visitAstExpression(ParseTree tree) { + /** + * Visit expression tree node and convert to UnresolvedExpression. Protected to allow subclass + * access (e.g., ExtendedAstBuilder for join conditions). + */ + protected UnresolvedExpression visitAstExpression(ParseTree tree) { return expressionBuilder.visit(tree); } diff --git a/sql/src/test/java/org/opensearch/sql/sql/antlr/SQLSyntaxParserV2ExtensionTest.java b/sql/src/test/java/org/opensearch/sql/sql/antlr/SQLSyntaxParserV2ExtensionTest.java new file mode 100644 index 00000000000..cb56dfbe0a9 --- /dev/null +++ b/sql/src/test/java/org/opensearch/sql/sql/antlr/SQLSyntaxParserV2ExtensionTest.java @@ -0,0 +1,37 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.sql.sql.antlr; + +import org.junit.jupiter.api.Test; + +/** Tests for V2 grammar extensions (JOIN). */ +class SQLSyntaxParserV2ExtensionTest extends SQLParserTest { + + @Test + void canParseInnerJoin() { + acceptQuery("SELECT * FROM t1 JOIN t2 ON t1.id = t2.id"); + } + + @Test + void canParseLeftJoin() { + acceptQuery("SELECT * FROM t1 LEFT JOIN t2 ON t1.id = t2.id"); + } + + @Test + void canParseRightJoin() { + acceptQuery("SELECT * FROM t1 RIGHT JOIN t2 ON t1.id = t2.id"); + } + + @Test + void canParseCrossJoin() { + acceptQuery("SELECT * FROM t1 CROSS JOIN t2"); + } + + @Test + void canParseLeftOuterJoin() { + acceptQuery("SELECT * FROM t1 LEFT OUTER JOIN t2 ON t1.id = t2.id"); + } +} diff --git a/sql/src/test/java/org/opensearch/sql/sql/parser/AstBuilderTest.java b/sql/src/test/java/org/opensearch/sql/sql/parser/AstBuilderTest.java index 695cf85b144..d6897230b40 100644 --- a/sql/src/test/java/org/opensearch/sql/sql/parser/AstBuilderTest.java +++ b/sql/src/test/java/org/opensearch/sql/sql/parser/AstBuilderTest.java @@ -9,6 +9,7 @@ import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.containsString; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.opensearch.sql.ast.dsl.AstDSL.agg; import static org.opensearch.sql.ast.dsl.AstDSL.aggregate; @@ -22,6 +23,7 @@ import static org.opensearch.sql.ast.dsl.AstDSL.function; import static org.opensearch.sql.ast.dsl.AstDSL.highlight; import static org.opensearch.sql.ast.dsl.AstDSL.intLiteral; +import static org.opensearch.sql.ast.dsl.AstDSL.join; import static org.opensearch.sql.ast.dsl.AstDSL.limit; import static org.opensearch.sql.ast.dsl.AstDSL.project; import static org.opensearch.sql.ast.dsl.AstDSL.qualifiedName; @@ -36,6 +38,7 @@ import com.google.common.collect.ImmutableList; import java.util.HashMap; import java.util.Map; +import java.util.Optional; import org.junit.jupiter.api.Test; import org.opensearch.sql.ast.dsl.AstDSL; import org.opensearch.sql.ast.expression.AllFields; @@ -43,10 +46,14 @@ import org.opensearch.sql.ast.expression.Literal; import org.opensearch.sql.ast.expression.NestedAllTupleFields; import org.opensearch.sql.ast.expression.UnresolvedArgument; +import org.opensearch.sql.ast.tree.Join; import org.opensearch.sql.ast.tree.SubqueryAlias; import org.opensearch.sql.ast.tree.TableFunction; +import org.opensearch.sql.ast.tree.UnresolvedPlan; import org.opensearch.sql.common.antlr.SyntaxCheckException; import org.opensearch.sql.exception.SemanticCheckException; +import org.opensearch.sql.sql.antlr.SQLSyntaxParser; +import org.opensearch.sql.sql.antlr.parser.OpenSearchSQLParser; class AstBuilderTest extends AstBuilderTestBase { @@ -729,4 +736,24 @@ public void can_build_string_literal_highlight() { alias("highlight(\"fieldA\")", highlight(AstDSL.stringLiteral("fieldA"), args))), buildAST("SELECT highlight(\"fieldA\") FROM test")); } + + @Test + public void join_throws_syntax_check_exception() { + assertThrows( + SyntaxCheckException.class, () -> buildAST("SELECT * FROM t1 JOIN t2 ON t1.id = t2.id")); + } + + @Test + public void join_iteration_in_from_clause_dispatches_to_visit_join_clause() { + // Covers the join iteration loop body in visitFromClause for test coverage + String query = "SELECT * FROM t1 JOIN t2 ON t1.id = t2.id"; + AstBuilder builder = + new AstBuilder(query) { + @Override + public UnresolvedPlan visitJoinClause(OpenSearchSQLParser.JoinClauseContext ctx) { + return join(visit(ctx.relation()), Join.JoinType.INNER, Optional.empty()); + } + }; + assertNotNull(new SQLSyntaxParser().parse(query).accept(builder)); + } }