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

Add where clause support in create statement #2366

Merged
merged 4 commits into from
Oct 25, 2023
Merged

Add where clause support in create statement #2366

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
794b174
Upload latest grammar with where clause support in create statement
dai-chen Oct 25, 2023
8838013
Fix MV index name bug
dai-chen Oct 25, 2023
7db15a0
Add UT for create index statement with where clause
dai-chen Oct 25, 2023
4ad4410
Fix spotless format
dai-chen Oct 25, 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
10 changes: 10 additions & 0 deletions spark/src/main/antlr/FlintSparkSqlExtensions.g4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ createSkippingIndexStatement
: CREATE SKIPPING INDEX (IF NOT EXISTS)?
ON tableName
LEFT_PAREN indexColTypeList RIGHT_PAREN
whereClause?
(WITH LEFT_PAREN propertyList RIGHT_PAREN)?
;

Expand Down Expand Up @@ -58,6 +59,7 @@ createCoveringIndexStatement
: CREATE INDEX (IF NOT EXISTS)? indexName
ON tableName
LEFT_PAREN indexColumns=multipartIdentifierPropertyList RIGHT_PAREN
whereClause?
(WITH LEFT_PAREN propertyList RIGHT_PAREN)?
;

Expand Down Expand Up @@ -115,6 +117,14 @@ materializedViewQuery
: .+?
;

whereClause
: WHERE filterCondition
;

filterCondition
: .+?
;

indexColTypeList
: indexColType (COMMA indexColType)*
;
Expand Down
1 change: 1 addition & 0 deletions spark/src/main/antlr/SparkSqlBase.g4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,7 @@ SHOW: 'SHOW';
TRUE: 'TRUE';
VIEW: 'VIEW';
VIEWS: 'VIEWS';
WHERE: 'WHERE';
WITH: 'WITH';


Expand Down
22 changes: 22 additions & 0 deletions spark/src/main/java/org/opensearch/sql/spark/dispatcher/model/FullyQualifiedTableName.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@

package org.opensearch.sql.spark.dispatcher.model;

import static org.apache.commons.lang3.StringUtils.strip;
import static org.opensearch.sql.spark.dispatcher.model.IndexQueryDetails.STRIP_CHARS;
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Should we bring this constant to this File.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sure, will merge this first. CI took so long every commit ...


import java.util.Arrays;
import lombok.Data;
import lombok.NoArgsConstructor;
Expand Down Expand Up @@ -40,4 +43,23 @@ public FullyQualifiedTableName(String fullyQualifiedName) {
tableName = parts[0];
}
}

/**
* Convert qualified name to Flint name concat by underscore.
*
* @return Flint name
*/
public String toFlintName() {
dai-chen marked this conversation as resolved.
Show resolved Hide resolved
StringBuilder builder = new StringBuilder();
if (datasourceName != null) {
builder.append(strip(datasourceName, STRIP_CHARS)).append("_");
}
if (schemaName != null) {
builder.append(strip(schemaName, STRIP_CHARS)).append("_");
}
if (tableName != null) {
builder.append(strip(tableName, STRIP_CHARS));
}
return builder.toString();
}
}
25 changes: 7 additions & 18 deletions spark/src/main/java/org/opensearch/sql/spark/dispatcher/model/IndexQueryDetails.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@

package org.opensearch.sql.spark.dispatcher.model;

import static org.apache.commons.lang3.StringUtils.strip;

import lombok.EqualsAndHashCode;
import lombok.Getter;
import org.apache.commons.lang3.StringUtils;
Expand Down Expand Up @@ -83,32 +85,19 @@ public String openSearchIndexName() {
switch (getIndexType()) {
case COVERING:
indexName =
"flint"
+ "_"
+ StringUtils.strip(fullyQualifiedTableName.getDatasourceName(), STRIP_CHARS)
+ "_"
+ StringUtils.strip(fullyQualifiedTableName.getSchemaName(), STRIP_CHARS)
"flint_"
+ fullyQualifiedTableName.toFlintName()
+ "_"
+ StringUtils.strip(fullyQualifiedTableName.getTableName(), STRIP_CHARS)
+ "_"
+ StringUtils.strip(getIndexName(), STRIP_CHARS)
+ strip(getIndexName(), STRIP_CHARS)
+ "_"
+ getIndexType().getSuffix();
break;
case SKIPPING:
indexName =
"flint"
+ "_"
+ StringUtils.strip(fullyQualifiedTableName.getDatasourceName(), STRIP_CHARS)
+ "_"
+ StringUtils.strip(fullyQualifiedTableName.getSchemaName(), STRIP_CHARS)
+ "_"
+ StringUtils.strip(fullyQualifiedTableName.getTableName(), STRIP_CHARS)
+ "_"
+ getIndexType().getSuffix();
"flint_" + fullyQualifiedTableName.toFlintName() + "_" + getIndexType().getSuffix();
break;
case MATERIALIZED_VIEW:
indexName = "flint" + "_" + StringUtils.strip(getMvName(), STRIP_CHARS).toLowerCase();
indexName = "flint_" + new FullyQualifiedTableName(mvName).toFlintName();
Copy link
Member

Choose a reason for hiding this comment

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

Let me know if this is the edge case still pending.

CREATE MATERIALIZED VIEW default.mv
Do we allow this without datasource name?

Copy link
Collaborator Author

@dai-chen dai-chen Oct 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think this is impossible from frontend. The reason I add check in toFlintName() is I found many UT pass such name and failed after my changes. So I make toFlintName() flexible.

But for API call, user can put any name. May need to validate this.

Copy link
Member

Choose a reason for hiding this comment

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

It can come from workbench but probably we can take that as backlog.

break;
}
return indexName.toLowerCase();
Expand Down
77 changes: 77 additions & 0 deletions spark/src/test/java/org/opensearch/sql/spark/flint/IndexQueryDetailsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,4 +26,81 @@ public void skippingIndexName() {
.build()
.openSearchIndexName());
}

@Test
public void coveringIndexName() {
assertEquals(
"flint_mys3_default_http_logs_idx_status_index",
IndexQueryDetails.builder()
.indexName("idx_status")
.fullyQualifiedTableName(new FullyQualifiedTableName("mys3.default.http_logs"))
.indexType(FlintIndexType.COVERING)
.build()
.openSearchIndexName());
}

@Test
public void materializedViewIndexName() {
assertEquals(
"flint_mys3_default_http_logs_metrics",
IndexQueryDetails.builder()
.mvName("mys3.default.http_logs_metrics")
.indexType(FlintIndexType.MATERIALIZED_VIEW)
.build()
.openSearchIndexName());
}

@Test
public void materializedViewIndexNameWithBackticks() {
assertEquals(
"flint_mys3_default_http_logs_metrics",
IndexQueryDetails.builder()
.mvName("`mys3`.`default`.`http_logs_metrics`")
.indexType(FlintIndexType.MATERIALIZED_VIEW)
.build()
.openSearchIndexName());
}

@Test
public void materializedViewIndexNameWithDots() {
assertEquals(
"flint_mys3_default_http_logs_metrics.1026",
IndexQueryDetails.builder()
.mvName("`mys3`.`default`.`http_logs_metrics.1026`")
.indexType(FlintIndexType.MATERIALIZED_VIEW)
.build()
.openSearchIndexName());
}

@Test
public void materializedViewIndexNameWithDotsInCatalogName() {
// FIXME: should not use ctx.getText which is hard to split
assertEquals(
"flint_mys3_1026_default`.`http_logs_metrics",
IndexQueryDetails.builder()
.mvName("`mys3.1026`.`default`.`http_logs_metrics`")
.indexType(FlintIndexType.MATERIALIZED_VIEW)
.build()
.openSearchIndexName());
}

@Test
public void materializedViewIndexNameNotFullyQualified() {
// Normally this should not happen and can add precondition check once confirmed.
assertEquals(
"flint_default_http_logs_metrics",
IndexQueryDetails.builder()
.mvName("default.http_logs_metrics")
.indexType(FlintIndexType.MATERIALIZED_VIEW)
.build()
.openSearchIndexName());

assertEquals(
"flint_http_logs_metrics",
IndexQueryDetails.builder()
.mvName("http_logs_metrics")
.indexType(FlintIndexType.MATERIALIZED_VIEW)
.build()
.openSearchIndexName());
}
}
64 changes: 51 additions & 13 deletions spark/src/test/java/org/opensearch/sql/spark/utils/SQLQueryUtilsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,19 +102,57 @@ void testErrorScenarios() {
}

@Test
void testExtractionFromFlintIndexQueries() {
String createCoveredIndexQuery =
"CREATE INDEX elb_and_requestUri ON myS3.default.alb_logs(l_orderkey, l_quantity) WITH"
+ " (auto_refresh = true)";
Assertions.assertTrue(SQLQueryUtils.isFlintExtensionQuery(createCoveredIndexQuery));
IndexQueryDetails indexQueryDetails =
SQLQueryUtils.extractIndexDetails(createCoveredIndexQuery);
FullyQualifiedTableName fullyQualifiedTableName =
indexQueryDetails.getFullyQualifiedTableName();
Assertions.assertEquals("elb_and_requestUri", indexQueryDetails.getIndexName());
Assertions.assertEquals("myS3", fullyQualifiedTableName.getDatasourceName());
Assertions.assertEquals("default", fullyQualifiedTableName.getSchemaName());
Assertions.assertEquals("alb_logs", fullyQualifiedTableName.getTableName());
void testExtractionFromFlintSkippingIndexQueries() {
String[] createSkippingIndexQueries = {
"CREATE SKIPPING INDEX ON myS3.default.alb_logs (l_orderkey VALUE_SET)",
"CREATE SKIPPING INDEX IF NOT EXISTS"
+ " ON myS3.default.alb_logs (l_orderkey VALUE_SET) "
+ " WITH (auto_refresh = true)",
"CREATE SKIPPING INDEX ON myS3.default.alb_logs(l_orderkey VALUE_SET)"
+ " WITH (auto_refresh = true)",
"CREATE SKIPPING INDEX ON myS3.default.alb_logs(l_orderkey VALUE_SET) "
+ " WHERE elb_status_code = 500 "
+ " WITH (auto_refresh = true)"
};

for (String query : createSkippingIndexQueries) {
Assertions.assertTrue(SQLQueryUtils.isFlintExtensionQuery(query), "Failed query: " + query);
IndexQueryDetails indexQueryDetails = SQLQueryUtils.extractIndexDetails(query);
FullyQualifiedTableName fullyQualifiedTableName =
indexQueryDetails.getFullyQualifiedTableName();

Assertions.assertNull(indexQueryDetails.getIndexName());
Assertions.assertEquals("myS3", fullyQualifiedTableName.getDatasourceName());
Assertions.assertEquals("default", fullyQualifiedTableName.getSchemaName());
Assertions.assertEquals("alb_logs", fullyQualifiedTableName.getTableName());
}
}

@Test
void testExtractionFromFlintCoveringIndexQueries() {
String[] createCoveredIndexQueries = {
"CREATE INDEX elb_and_requestUri ON myS3.default.alb_logs(l_orderkey, l_quantity)",
"CREATE INDEX IF NOT EXISTS elb_and_requestUri "
+ " ON myS3.default.alb_logs(l_orderkey, l_quantity) "
+ " WITH (auto_refresh = true)",
"CREATE INDEX elb_and_requestUri ON myS3.default.alb_logs(l_orderkey, l_quantity)"
+ " WITH (auto_refresh = true)",
"CREATE INDEX elb_and_requestUri ON myS3.default.alb_logs(l_orderkey, l_quantity) "
+ " WHERE elb_status_code = 500 "
+ " WITH (auto_refresh = true)"
};

for (String query : createCoveredIndexQueries) {
Assertions.assertTrue(SQLQueryUtils.isFlintExtensionQuery(query), "Failed query: " + query);
IndexQueryDetails indexQueryDetails = SQLQueryUtils.extractIndexDetails(query);
FullyQualifiedTableName fullyQualifiedTableName =
indexQueryDetails.getFullyQualifiedTableName();

Assertions.assertEquals("elb_and_requestUri", indexQueryDetails.getIndexName());
Assertions.assertEquals("myS3", fullyQualifiedTableName.getDatasourceName());
Assertions.assertEquals("default", fullyQualifiedTableName.getSchemaName());
Assertions.assertEquals("alb_logs", fullyQualifiedTableName.getTableName());
}
}

@Test
Expand Down