Skip to content

Support UDT for BINARY #3549

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

Merged
penghuo merged 5 commits into from
Apr 16, 2025
Merged

Support UDT for BINARY #3549

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c2b3508
Support UDT for BINARY
qianheng-aws Apr 15, 2025
3180d0b
Fix IT
qianheng-aws Apr 15, 2025
1d47586
Fix UT
qianheng-aws Apr 16, 2025
2227325
Merge remote-tracking branch 'refs/remotes/origin/main' into UDT-BINARY
qianheng-aws Apr 16, 2025
0225ae5
Address comments and change after merging
qianheng-aws Apr 16, 2025
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
16 changes: 16 additions & 0 deletions core/src/main/java/org/opensearch/sql/calcite/type/ExprBinaryType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/

package org.opensearch.sql.calcite.type;

import org.apache.calcite.sql.type.SqlTypeName;
import org.opensearch.sql.calcite.utils.OpenSearchTypeFactory;
import org.opensearch.sql.calcite.utils.OpenSearchTypeFactory.ExprUDT;

public class ExprBinaryType extends ExprSqlType {
public ExprBinaryType(OpenSearchTypeFactory typeFactory) {
super(typeFactory, ExprUDT.EXPR_BINARY, SqlTypeName.VARCHAR);
}
}
24 changes: 9 additions & 15 deletions core/src/main/java/org/opensearch/sql/calcite/utils/OpenSearchTypeFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
package org.opensearch.sql.calcite.utils;

import static org.opensearch.sql.data.type.ExprCoreType.ARRAY;
import static org.opensearch.sql.data.type.ExprCoreType.BINARY;
import static org.opensearch.sql.data.type.ExprCoreType.BOOLEAN;
import static org.opensearch.sql.data.type.ExprCoreType.BYTE;
import static org.opensearch.sql.data.type.ExprCoreType.DATE;
Expand Down Expand Up @@ -41,6 +42,7 @@
import org.apache.calcite.sql.type.SqlTypeName;
import org.apache.calcite.sql.type.SqlTypeUtil;
import org.opensearch.sql.calcite.type.AbstractExprRelDataType;
import org.opensearch.sql.calcite.type.ExprBinaryType;
import org.opensearch.sql.calcite.type.ExprDateType;
import org.opensearch.sql.calcite.type.ExprIPType;
import org.opensearch.sql.calcite.type.ExprTimeStampType;
Expand All @@ -67,6 +69,7 @@ public enum ExprUDT {
EXPR_DATE(DATE),
EXPR_TIME(TIME),
EXPR_TIMESTAMP(TIMESTAMP),
EXPR_BINARY(BINARY),
EXPR_IP(IP);

// Associated `ExprCoreType`
Expand Down Expand Up @@ -120,6 +123,8 @@ public RelDataType createUDT(ExprUDT typeName) {
yield new ExprTimeType(this);
case EXPR_TIMESTAMP:
yield new ExprTimeStampType(this);
case EXPR_BINARY:
yield new ExprBinaryType(this);
case EXPR_IP:
yield new ExprIPType(this);
};
Expand Down Expand Up @@ -180,7 +185,7 @@ public static RelDataType convertExprTypeToRelDataType(ExprType fieldType, boole
}
} else {
if (fieldType.legacyTypeName().equalsIgnoreCase("binary")) {
return TYPE_FACTORY.createSqlType(SqlTypeName.BINARY, nullable);
return TYPE_FACTORY.createUDT(ExprUDT.EXPR_BINARY, nullable);
} else if (fieldType.legacyTypeName().equalsIgnoreCase("timestamp")) {
return TYPE_FACTORY.createUDT(ExprUDT.EXPR_TIMESTAMP, nullable);
} else if (fieldType.legacyTypeName().equalsIgnoreCase("date")) {
Expand Down Expand Up @@ -243,20 +248,9 @@ public static ExprType convertSqlTypeNameToExprType(SqlTypeName sqlTypeName) {

/** Get legacy name for a RelDataType. */
public static String getLegacyTypeName(RelDataType relDataType, QueryType queryType) {
if (relDataType instanceof AbstractExprRelDataType<?> udt) {
return udt.getExprType().legacyTypeName();
}
switch (relDataType.getSqlTypeName()) {
case BINARY:
case VARBINARY:
return "BINARY";
case GEOMETRY:
return "GEO_POINT";
default:
ExprType type = convertSqlTypeNameToExprType(relDataType.getSqlTypeName());
return (queryType == PPL ? PPL_SPEC.typeName(type) : type.legacyTypeName())
.toUpperCase(Locale.ROOT);
}
ExprType type = convertRelDataTypeToExprType(relDataType);
return (queryType == PPL ? PPL_SPEC.typeName(type) : type.legacyTypeName())
.toUpperCase(Locale.ROOT);
}

/** Converts a Calcite data type to OpenSearch ExprCoreType. */
Expand Down
2 changes: 2 additions & 0 deletions core/src/main/java/org/opensearch/sql/data/type/ExprCoreType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@ public enum ExprCoreType implements ExprType {
/** Geometry. Only support point now. */
GEO_POINT(UNDEFINED),

BINARY(UNDEFINED),
Copy link
Copy Markdown
Collaborator

@penghuo penghuo Apr 15, 2025

Choose a reason for hiding this comment

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

Does Calcite impl required ExprCoreType mapping? Should we consider deprecated ExprCoreType in future?

Copy link
Copy Markdown
Collaborator Author

@qianheng-aws qianheng-aws Apr 16, 2025

Choose a reason for hiding this comment

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

For the current implementation it requires, because we will always populate the final results with ExprValue transformed from Calcite's results.

sql/opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java

Line 215 in 3c1591a

ExprType exprType = convertRelDataTypeToExprType(fieldType);


/** Struct. */
STRUCT(UNDEFINED),

Expand Down
7 changes: 1 addition & 6 deletions core/src/test/java/org/opensearch/sql/analysis/AnalyzerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,12 +161,7 @@ public void filter_relation_with_invalid_qualifiedName_ExpressionEvaluationExcep

ExpressionEvaluationException exception =
assertThrows(ExpressionEvaluationException.class, () -> analyze(typeMismatchPlan));
assertEquals(
"= function expected {[BYTE,BYTE],[SHORT,SHORT],[INTEGER,INTEGER],[LONG,LONG],"
+ "[FLOAT,FLOAT],[DOUBLE,DOUBLE],[STRING,STRING],[BOOLEAN,BOOLEAN],[DATE,DATE],"
+ "[TIME,TIME],[TIMESTAMP,TIMESTAMP],[INTERVAL,INTERVAL],[IP,IP],[GEO_POINT,GEO_POINT],"
+ "[STRUCT,STRUCT],[ARRAY,ARRAY]}, but got [STRING,INTEGER]",
exception.getMessage());
assertEquals(getIncompatibleTypeErrMsg(STRING, INTEGER), exception.getMessage());
}

@Test
Expand Down
11 changes: 11 additions & 0 deletions core/src/test/java/org/opensearch/sql/analysis/AnalyzerTestBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import org.opensearch.sql.analysis.symbol.SymbolTable;
import org.opensearch.sql.ast.tree.UnresolvedPlan;
import org.opensearch.sql.config.TestConfig;
import org.opensearch.sql.data.type.ExprCoreType;
import org.opensearch.sql.data.type.ExprType;
import org.opensearch.sql.datasource.DataSourceService;
import org.opensearch.sql.datasource.RequestContext;
Expand Down Expand Up @@ -274,4 +275,14 @@ public Table applyArguments() {
return table;
}
}

public static String getIncompatibleTypeErrMsg(ExprType lType, ExprType rType) {
return String.format(
"= function expected %s, but got [%s,%s]",
ExprCoreType.coreTypes().stream()
.map(type -> String.format("[%s,%s]", type.typeName(), type.typeName()))
.collect(Collectors.joining(",", "{", "}")),
lType,
rType);
}
}
14 changes: 0 additions & 14 deletions integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteDataTypeIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@

package org.opensearch.sql.calcite.remote;

import java.io.IOException;
import org.opensearch.sql.ppl.DataTypeIT;

public class CalciteDataTypeIT extends DataTypeIT {
Expand All @@ -15,17 +14,4 @@ public void init() throws Exception {
enableCalcite();
disallowCalciteFallback();
}

@Override
public void test_nonnumeric_data_types() throws IOException {
withFallbackEnabled(
() -> {
try {
super.test_nonnumeric_data_types();
} catch (IOException e) {
throw new RuntimeException(e);
}
},
"ignore this class since IP type is unsupported in calcite engine");
}
}
32 changes: 25 additions & 7 deletions integ-test/src/test/java/org/opensearch/sql/ppl/DataTypeIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,12 @@
import static org.opensearch.sql.util.MatcherUtils.rows;
import static org.opensearch.sql.util.MatcherUtils.schema;
import static org.opensearch.sql.util.MatcherUtils.verifyDataRows;
import static org.opensearch.sql.util.MatcherUtils.verifyDataRowsInOrder;
import static org.opensearch.sql.util.MatcherUtils.verifySchema;
import static org.opensearch.sql.util.MatcherUtils.verifySchemaInOrder;

import java.io.IOException;
import org.json.JSONArray;
import org.json.JSONObject;
import org.junit.Test;

Expand Down Expand Up @@ -48,18 +51,33 @@ public void test_numeric_data_types() throws IOException {
@Test
public void test_nonnumeric_data_types() throws IOException {
JSONObject result = executeQuery(String.format("source=%s", TEST_INDEX_DATATYPE_NONNUMERIC));
verifySchema(
verifySchemaInOrder(
result,
schema("boolean_value", "boolean"),
schema("keyword_value", "string"),
schema("text_value", "string"),
schema("binary_value", "binary"),
schema("date_value", "timestamp"),
schema("date_nanos_value", "timestamp"),
schema("date_value", "timestamp"),
schema("boolean_value", "boolean"),
schema("ip_value", "ip"),
schema("object_value", "struct"),
schema("nested_value", "array"),
schema("geo_point_value", "geo_point"));
schema("object_value", "struct"),
schema("keyword_value", "string"),
schema("geo_point_value", "geo_point"),
schema("binary_value", "binary"));
verifyDataRowsInOrder(
result,
rows(
"text",
"2019-03-24 01:34:46.123456789",
"2020-10-13 13:00:00",
true,
"127.0.0.1",
new JSONArray(
"[{\"last\": \"Smith\", \"first\": \"John\"}, {\"last\": \"White\", \"first\":"
+ " \"Alice\"}]"),
new JSONObject("{\"last\": \"Dale\", \"first\": \"Dale\"}"),
"keyword",
new JSONObject("{\"lon\": 74, \"lat\": 40.71}"),
"U29tZSBiaW5hcnkgYmxvYg=="));
Copy link
Copy Markdown
Collaborator

@penghuo penghuo Apr 15, 2025

Choose a reason for hiding this comment

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

what is current value without this PR?

Copy link
Copy Markdown
Collaborator Author

@qianheng-aws qianheng-aws Apr 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This is the value V2 returned.

For Calcite, it will throw exception. But this test was ignored in Calcite's IT previously so CI can pass.

Copy link
Copy Markdown
Collaborator Author

@qianheng-aws qianheng-aws Apr 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The fallback reason in Calcite's IT is outdated actually, we already have supported IP type. It's true reason is the incompatible BIANRY type.

sql/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteDataTypeIT.java

Line 29 in 3c1591a

"ignore this class since IP type is unsupported in calcite engine");

}

@Test
Expand Down
11 changes: 8 additions & 3 deletions integ-test/src/test/java/org/opensearch/sql/ppl/WhereCommandIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,11 @@
import static org.opensearch.sql.util.MatcherUtils.verifyDataRows;

import java.io.IOException;
import java.util.stream.Collectors;
import org.hamcrest.MatcherAssert;
import org.json.JSONObject;
import org.junit.jupiter.api.Test;
import org.opensearch.sql.data.type.ExprCoreType;

public class WhereCommandIT extends PPLIntegTestCase {

Expand Down Expand Up @@ -175,8 +177,11 @@ public void testInWithIncompatibleType() {
}

protected String getIncompatibleTypeErrMsg() {
return "function expected"
+ " {[BYTE,BYTE],[SHORT,SHORT],[INTEGER,INTEGER],[LONG,LONG],[FLOAT,FLOAT],[DOUBLE,DOUBLE],[STRING,STRING],[BOOLEAN,BOOLEAN],[DATE,DATE],[TIME,TIME],[TIMESTAMP,TIMESTAMP],[INTERVAL,INTERVAL],[IP,IP],[GEO_POINT,GEO_POINT],[STRUCT,STRUCT],[ARRAY,ARRAY]},"
+ " but got [LONG,STRING]";
return String.format(
"function expected %s, but got %s",
ExprCoreType.coreTypes().stream()
.map(type -> String.format("[%s,%s]", type.typeName(), type.typeName()))
.collect(Collectors.joining(",", "{", "}")),
"[LONG,STRING]");
}
}
Loading