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.

Sign up for GitHub

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

Disable join types in validators #3056

Merged
merged 2 commits into from
Oct 8, 2024
Merged

Disable join types in validators #3056

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
12f2801
Disable join types in validators
ykmr1224 Oct 4, 2024
36858e3
Removed methods from SQLQueryValidationVisitor due to grammar file ch…
ykmr1224 Oct 4, 2024
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
10 changes: 10 additions & 0 deletions ...-core/src/main/java/org/opensearch/sql/spark/validator/S3GlueGrammarElementValidator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,20 +9,25 @@
import static org.opensearch.sql.spark.validator.GrammarElement.CLUSTER_BY;
import static org.opensearch.sql.spark.validator.GrammarElement.CREATE_FUNCTION;
import static org.opensearch.sql.spark.validator.GrammarElement.CREATE_VIEW;
import static org.opensearch.sql.spark.validator.GrammarElement.CROSS_JOIN;
import static org.opensearch.sql.spark.validator.GrammarElement.DESCRIBE_FUNCTION;
import static org.opensearch.sql.spark.validator.GrammarElement.DISTRIBUTE_BY;
import static org.opensearch.sql.spark.validator.GrammarElement.DROP_FUNCTION;
import static org.opensearch.sql.spark.validator.GrammarElement.DROP_VIEW;
import static org.opensearch.sql.spark.validator.GrammarElement.FILE;
import static org.opensearch.sql.spark.validator.GrammarElement.FULL_OUTER_JOIN;
import static org.opensearch.sql.spark.validator.GrammarElement.HINTS;
import static org.opensearch.sql.spark.validator.GrammarElement.INLINE_TABLE;
import static org.opensearch.sql.spark.validator.GrammarElement.INSERT;
import static org.opensearch.sql.spark.validator.GrammarElement.LEFT_ANTI_JOIN;
import static org.opensearch.sql.spark.validator.GrammarElement.LEFT_SEMI_JOIN;
import static org.opensearch.sql.spark.validator.GrammarElement.LOAD;
import static org.opensearch.sql.spark.validator.GrammarElement.MANAGE_RESOURCE;
import static org.opensearch.sql.spark.validator.GrammarElement.MISC_FUNCTIONS;
import static org.opensearch.sql.spark.validator.GrammarElement.REFRESH_FUNCTION;
import static org.opensearch.sql.spark.validator.GrammarElement.REFRESH_RESOURCE;
import static org.opensearch.sql.spark.validator.GrammarElement.RESET;
import static org.opensearch.sql.spark.validator.GrammarElement.RIGHT_OUTER_JOIN;
import static org.opensearch.sql.spark.validator.GrammarElement.SET;
import static org.opensearch.sql.spark.validator.GrammarElement.SHOW_FUNCTIONS;
import static org.opensearch.sql.spark.validator.GrammarElement.SHOW_VIEWS;
Expand Down Expand Up @@ -50,6 +55,11 @@ public class S3GlueGrammarElementValidator extends DenyListGrammarElementValidat
HINTS,
INLINE_TABLE,
FILE,
CROSS_JOIN,
LEFT_SEMI_JOIN,
RIGHT_OUTER_JOIN,
FULL_OUTER_JOIN,
LEFT_ANTI_JOIN,
Comment on lines +58 to +62
Copy link
Collaborator

Choose a reason for hiding this comment

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

These JOIN types are disallowed even for S3 data source?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, limiting regardless of the data source.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So we only limit these JOIN types due to performance/cost? Others like INNER JOIN, LEFT JOIN can still work?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, intention is only allowing INNER JOIN and LEFT OUTER JOIN.

TABLESAMPLE,
TABLE_VALUED_FUNCTION,
TRANSFORM,
Expand Down
28 changes: 0 additions & 28 deletions ...uery-core/src/main/java/org/opensearch/sql/spark/validator/SQLQueryValidationVisitor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,12 @@
import org.opensearch.sql.spark.antlr.parser.SqlBaseParser;
import org.opensearch.sql.spark.antlr.parser.SqlBaseParser.AddTableColumnsContext;
import org.opensearch.sql.spark.antlr.parser.SqlBaseParser.AddTablePartitionContext;
import org.opensearch.sql.spark.antlr.parser.SqlBaseParser.AlterClusterByContext;
import org.opensearch.sql.spark.antlr.parser.SqlBaseParser.AlterTableAlterColumnContext;
import org.opensearch.sql.spark.antlr.parser.SqlBaseParser.AlterViewQueryContext;
import org.opensearch.sql.spark.antlr.parser.SqlBaseParser.AlterViewSchemaBindingContext;
import org.opensearch.sql.spark.antlr.parser.SqlBaseParser.AnalyzeContext;
import org.opensearch.sql.spark.antlr.parser.SqlBaseParser.AnalyzeTablesContext;
import org.opensearch.sql.spark.antlr.parser.SqlBaseParser.CacheTableContext;
import org.opensearch.sql.spark.antlr.parser.SqlBaseParser.ClearCacheContext;
import org.opensearch.sql.spark.antlr.parser.SqlBaseParser.ClusterBySpecContext;
import org.opensearch.sql.spark.antlr.parser.SqlBaseParser.CreateNamespaceContext;
import org.opensearch.sql.spark.antlr.parser.SqlBaseParser.CreateTableContext;
import org.opensearch.sql.spark.antlr.parser.SqlBaseParser.CreateTableLikeContext;
Expand Down Expand Up @@ -81,7 +78,6 @@
import org.opensearch.sql.spark.antlr.parser.SqlBaseParser.TransformClauseContext;
import org.opensearch.sql.spark.antlr.parser.SqlBaseParser.TruncateTableContext;
import org.opensearch.sql.spark.antlr.parser.SqlBaseParser.UncacheTableContext;
import org.opensearch.sql.spark.antlr.parser.SqlBaseParser.UnsetNamespacePropertiesContext;
import org.opensearch.sql.spark.antlr.parser.SqlBaseParserBaseVisitor;

/** This visitor validate grammar using GrammarElementValidator */
Expand All @@ -101,12 +97,6 @@ public Void visitSetNamespaceProperties(SetNamespacePropertiesContext ctx) {
return super.visitSetNamespaceProperties(ctx);
}

@Override
public Void visitUnsetNamespaceProperties(UnsetNamespacePropertiesContext ctx) {
validateAllowed(GrammarElement.ALTER_NAMESPACE);
return super.visitUnsetNamespaceProperties(ctx);
}

@Override
public Void visitAddTableColumns(AddTableColumnsContext ctx) {
validateAllowed(GrammarElement.ALTER_NAMESPACE);
Expand Down Expand Up @@ -173,12 +163,6 @@ public Void visitRecoverPartitions(RecoverPartitionsContext ctx) {
return super.visitRecoverPartitions(ctx);
}

@Override
public Void visitAlterClusterBy(AlterClusterByContext ctx) {
validateAllowed(GrammarElement.ALTER_NAMESPACE);
return super.visitAlterClusterBy(ctx);
}

@Override
public Void visitSetNamespaceLocation(SetNamespaceLocationContext ctx) {
validateAllowed(GrammarElement.ALTER_NAMESPACE);
Expand All @@ -191,12 +175,6 @@ public Void visitAlterViewQuery(AlterViewQueryContext ctx) {
return super.visitAlterViewQuery(ctx);
}

@Override
public Void visitAlterViewSchemaBinding(AlterViewSchemaBindingContext ctx) {
validateAllowed(GrammarElement.ALTER_VIEW);
return super.visitAlterViewSchemaBinding(ctx);
}

@Override
public Void visitRenameTable(RenameTableContext ctx) {
if (ctx.VIEW() != null) {
Expand Down Expand Up @@ -337,12 +315,6 @@ public Void visitCtes(CtesContext ctx) {
return super.visitCtes(ctx);
}

@Override
public Void visitClusterBySpec(ClusterBySpecContext ctx) {
validateAllowed(GrammarElement.CLUSTER_BY);
return super.visitClusterBySpec(ctx);
}

@Override
public Void visitQueryOrganization(QueryOrganizationContext ctx) {
if (ctx.CLUSTER() != null) {
Expand Down
10 changes: 10 additions & 0 deletions ...src/main/java/org/opensearch/sql/spark/validator/SecurityLakeGrammarElementValidator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import static org.opensearch.sql.spark.validator.GrammarElement.CREATE_FUNCTION;
import static org.opensearch.sql.spark.validator.GrammarElement.CREATE_NAMESPACE;
import static org.opensearch.sql.spark.validator.GrammarElement.CREATE_VIEW;
import static org.opensearch.sql.spark.validator.GrammarElement.CROSS_JOIN;
import static org.opensearch.sql.spark.validator.GrammarElement.CSV_FUNCTIONS;
import static org.opensearch.sql.spark.validator.GrammarElement.DESCRIBE_FUNCTION;
import static org.opensearch.sql.spark.validator.GrammarElement.DESCRIBE_NAMESPACE;
Expand All @@ -24,9 +25,12 @@
import static org.opensearch.sql.spark.validator.GrammarElement.DROP_NAMESPACE;
import static org.opensearch.sql.spark.validator.GrammarElement.DROP_VIEW;
import static org.opensearch.sql.spark.validator.GrammarElement.FILE;
import static org.opensearch.sql.spark.validator.GrammarElement.FULL_OUTER_JOIN;
import static org.opensearch.sql.spark.validator.GrammarElement.HINTS;
import static org.opensearch.sql.spark.validator.GrammarElement.INLINE_TABLE;
import static org.opensearch.sql.spark.validator.GrammarElement.INSERT;
import static org.opensearch.sql.spark.validator.GrammarElement.LEFT_ANTI_JOIN;
import static org.opensearch.sql.spark.validator.GrammarElement.LEFT_SEMI_JOIN;
import static org.opensearch.sql.spark.validator.GrammarElement.LOAD;
import static org.opensearch.sql.spark.validator.GrammarElement.MANAGE_RESOURCE;
import static org.opensearch.sql.spark.validator.GrammarElement.MISC_FUNCTIONS;
Expand All @@ -35,6 +39,7 @@
import static org.opensearch.sql.spark.validator.GrammarElement.REFRESH_TABLE;
import static org.opensearch.sql.spark.validator.GrammarElement.REPAIR_TABLE;
import static org.opensearch.sql.spark.validator.GrammarElement.RESET;
import static org.opensearch.sql.spark.validator.GrammarElement.RIGHT_OUTER_JOIN;
import static org.opensearch.sql.spark.validator.GrammarElement.SET;
import static org.opensearch.sql.spark.validator.GrammarElement.SHOW_COLUMNS;
import static org.opensearch.sql.spark.validator.GrammarElement.SHOW_CREATE_TABLE;
Expand Down Expand Up @@ -76,6 +81,11 @@ public class SecurityLakeGrammarElementValidator extends DenyListGrammarElementV
HINTS,
INLINE_TABLE,
FILE,
CROSS_JOIN,
LEFT_SEMI_JOIN,
RIGHT_OUTER_JOIN,
FULL_OUTER_JOIN,
LEFT_ANTI_JOIN,
TABLESAMPLE,
TABLE_VALUED_FUNCTION,
TRANSFORM,
Expand Down
29 changes: 12 additions & 17 deletions async-query-core/src/test/java/org/opensearch/sql/spark/validator/SQLQueryValidatorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,9 @@ private enum TestElement {
// DDL Statements
ALTER_DATABASE(
"ALTER DATABASE inventory SET DBPROPERTIES ('Edit-date' = '01/01/2001');",
"ALTER DATABASE dbx.tab1 UNSET PROPERTIES ('winner');",
"ALTER DATABASE dbx.tab1 SET LOCATION '/path/to/part/ways';"),
ALTER_TABLE(
"ALTER TABLE default.StudentInfo PARTITION (age='10') RENAME TO PARTITION (age='15');",
"ALTER TABLE dbx.tab1 UNSET TBLPROPERTIES ('winner');",
"ALTER TABLE StudentInfo ADD columns (LastName string, DOB timestamp);",
"ALTER TABLE StudentInfo ADD IF NOT EXISTS PARTITION (age=18);",
"ALTER TABLE StudentInfo RENAME COLUMN name TO FirstName;",
Expand All @@ -50,12 +48,10 @@ private enum TestElement {
"ALTER TABLE StudentInfo DROP IF EXISTS PARTITION (age=18);",
"ALTER TABLE dbx.tab1 PARTITION (a='1', b='2') SET LOCATION '/path/to/part/ways';",
"ALTER TABLE dbx.tab1 RECOVER PARTITIONS;",
"ALTER TABLE dbx.tab1 CLUSTER BY NONE;",
"ALTER TABLE dbx.tab1 SET LOCATION '/path/to/part/ways';"),
ALTER_VIEW(
"ALTER VIEW tempdb1.v1 RENAME TO tempdb1.v2;",
"ALTER VIEW tempdb1.v2 AS SELECT * FROM tempdb1.v1;",
"ALTER VIEW tempdb1.v2 WITH SCHEMA BINDING"),
"ALTER VIEW tempdb1.v2 AS SELECT * FROM tempdb1.v1;"),
CREATE_DATABASE("CREATE DATABASE IF NOT EXISTS customer_db;\n"),
CREATE_FUNCTION("CREATE FUNCTION simple_udf AS 'SimpleUdf' USING JAR '/tmp/SimpleUdf.jar';"),
CREATE_TABLE(
Expand Down Expand Up @@ -94,8 +90,7 @@ private enum TestElement {
EXPLAIN("EXPLAIN SELECT * FROM my_table;"),
COMMON_TABLE_EXPRESSION(
"WITH cte AS (SELECT * FROM my_table WHERE age > 30) SELECT * FROM cte;"),
CLUSTER_BY_CLAUSE(
"SELECT * FROM my_table CLUSTER BY age;", "ALTER TABLE testTable CLUSTER BY (age);"),
CLUSTER_BY_CLAUSE("SELECT * FROM my_table CLUSTER BY age;"),
DISTRIBUTE_BY_CLAUSE("SELECT * FROM my_table DISTRIBUTE BY name;"),
GROUP_BY_CLAUSE("SELECT name, count(*) FROM my_table GROUP BY name;"),
HAVING_CLAUSE("SELECT name, count(*) FROM my_table GROUP BY name HAVING count(*) > 1;"),
Expand Down Expand Up @@ -370,12 +365,12 @@ void testS3glueQueries() {
v.ng(TestElement.INLINE_TABLE);
v.ng(TestElement.FILE);
v.ok(TestElement.INNER_JOIN);
v.ok(TestElement.CROSS_JOIN);
v.ng(TestElement.CROSS_JOIN);
v.ok(TestElement.LEFT_OUTER_JOIN);
v.ok(TestElement.LEFT_SEMI_JOIN);
v.ok(TestElement.RIGHT_OUTER_JOIN);
v.ok(TestElement.FULL_OUTER_JOIN);
v.ok(TestElement.LEFT_ANTI_JOIN);
v.ng(TestElement.LEFT_SEMI_JOIN);
v.ng(TestElement.RIGHT_OUTER_JOIN);
v.ng(TestElement.FULL_OUTER_JOIN);
v.ng(TestElement.LEFT_ANTI_JOIN);
v.ok(TestElement.LIKE_PREDICATE);
v.ok(TestElement.LIMIT_CLAUSE);
v.ok(TestElement.OFFSET_CLAUSE);
Expand Down Expand Up @@ -487,12 +482,12 @@ void testSecurityLakeQueries() {
v.ng(TestElement.INLINE_TABLE);
v.ng(TestElement.FILE);
v.ok(TestElement.INNER_JOIN);
v.ok(TestElement.CROSS_JOIN);
v.ng(TestElement.CROSS_JOIN);
v.ok(TestElement.LEFT_OUTER_JOIN);
v.ok(TestElement.LEFT_SEMI_JOIN);
v.ok(TestElement.RIGHT_OUTER_JOIN);
v.ok(TestElement.FULL_OUTER_JOIN);
v.ok(TestElement.LEFT_ANTI_JOIN);
v.ng(TestElement.LEFT_SEMI_JOIN);
v.ng(TestElement.RIGHT_OUTER_JOIN);
v.ng(TestElement.FULL_OUTER_JOIN);
v.ng(TestElement.LEFT_ANTI_JOIN);
v.ok(TestElement.LIKE_PREDICATE);
v.ok(TestElement.LIMIT_CLAUSE);
v.ok(TestElement.OFFSET_CLAUSE);
Expand Down
Loading