Skip to content

Commit eab8b32

Browse files
committed
Accept plain Calcite types against UDT operand signatures
PPLOperandTypes.SCALAR_TYPES declares DATE/TIME/TIMESTAMP/IP/BINARY operands as UDTs, but typesMatch rejected a pair outright whenever only one side extended AbstractExprRelDataType. The analytics engine builds its row types from plain Calcite types (date -> TIMESTAMP(3), ip and binary -> VARBINARY) plus markers deriving from Calcite's AbstractSqlType, so every such operand failed the check. The result was a self-contradictory error, because getAllowedSignatures renders via the UDT tag while getActualSignature renders via convertRelDataTypeToExprType -- both print TIMESTAMP: Aggregation function LIST expects field type {...|[DATE]|[TIME]|[TIMESTAMP]|[IP]|[BINARY]}, but got [TIMESTAMP] Map the UDT tag to the SqlTypeNames a backend would emit for the same logical type. Comparing backing types would not work, since the UDTs are all VARCHAR-backed. The mapping is expressed over SqlTypeName rather than by calling convertAnalyticsEngineRelDataTypeToExprType, because analytics-api is a compileOnly dependency of core and loading those marker classes throws NoClassDefFoundError wherever it is off the runtime classpath. Signed-off-by: Kai Huang <ahkcs@amazon.com>
1 parent 8ca47ac commit eab8b32

2 files changed

Lines changed: 122 additions & 3 deletions

File tree

core/src/main/java/org/opensearch/sql/expression/function/PPLTypeChecker.java

Lines changed: 35 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -557,19 +557,51 @@ public List<List<RelDataType>> getParameterTypes() {
557557
* ExprUDT} tag — comparing {@code getClass()} is unsafe because addCharsetAndCollation collapses
558558
* ExprDateType/ExprTimeType/ExprTimeStampType/ExprBinaryType down to ExprSqlType, so different
559559
* UDTs would appear equal. Plain types match by SqlTypeName.
560+
*
561+
* <p>A UDT signature also accepts the equivalent plain Calcite type. Signatures such as {@code
562+
* PPLOperandTypes.ANY_SCALAR} declare temporal/IP/BINARY operands as UDTs, but the analytics
563+
* engine builds its row types from plain Calcite types plus its own markers, which extend
564+
* Calcite's {@code AbstractSqlType} rather than {@link AbstractExprRelDataType}. Matching on
565+
* class identity alone made {@code list(<date field>)} fail with an error that listed the very
566+
* type it had just rejected.
560567
*/
561568
private static boolean typesMatch(RelDataType expected, RelDataType actual) {
562569
if (expected instanceof AbstractExprRelDataType<?> expUdt
563570
&& actual instanceof AbstractExprRelDataType<?> actUdt) {
564571
return expUdt.getUdt() == actUdt.getUdt();
565572
}
566-
if (expected instanceof AbstractExprRelDataType<?>
567-
|| actual instanceof AbstractExprRelDataType<?>) {
568-
return false;
573+
if (expected instanceof AbstractExprRelDataType<?> expUdt) {
574+
return matchesPlainType(expUdt.getUdt(), actual);
575+
}
576+
if (actual instanceof AbstractExprRelDataType<?> actUdt) {
577+
return matchesPlainType(actUdt.getUdt(), expected);
569578
}
570579
return expected.getSqlTypeName() == actual.getSqlTypeName();
571580
}
572581

582+
/**
583+
* Whether a plain Calcite type is the non-UDT spelling of {@code udt}. The UDTs themselves are
584+
* all VARCHAR-backed, so this maps the tag to the {@link SqlTypeName}s a backend would produce
585+
* for the same logical type instead of comparing backing types.
586+
*/
587+
private static boolean matchesPlainType(ExprUDT udt, RelDataType plain) {
588+
return switch (udt) {
589+
case EXPR_DATE -> plain.getSqlTypeName() == SqlTypeName.DATE;
590+
case EXPR_TIME ->
591+
switch (plain.getSqlTypeName()) {
592+
case TIME, TIME_TZ, TIME_WITH_LOCAL_TIME_ZONE -> true;
593+
default -> false;
594+
};
595+
case EXPR_TIMESTAMP ->
596+
switch (plain.getSqlTypeName()) {
597+
case TIMESTAMP, TIMESTAMP_TZ, TIMESTAMP_WITH_LOCAL_TIME_ZONE -> true;
598+
default -> false;
599+
};
600+
// ip and binary both land as VARBINARY.
601+
case EXPR_IP, EXPR_BINARY -> SqlTypeName.BINARY_TYPES.contains(plain.getSqlTypeName());
602+
};
603+
}
604+
573605
// Util Functions
574606

575607
/**
Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,87 @@
1+
/*
2+
* Copyright OpenSearch Contributors
3+
* SPDX-License-Identifier: Apache-2.0
4+
*/
5+
6+
package org.opensearch.sql.expression.function;
7+
8+
import static org.junit.jupiter.api.Assertions.assertFalse;
9+
import static org.junit.jupiter.api.Assertions.assertTrue;
10+
11+
import java.util.List;
12+
import org.apache.calcite.rel.type.RelDataType;
13+
import org.apache.calcite.sql.type.SqlTypeName;
14+
import org.junit.jupiter.api.Test;
15+
import org.opensearch.sql.calcite.utils.OpenSearchTypeFactory;
16+
import org.opensearch.sql.calcite.utils.OpenSearchTypeFactory.ExprUDT;
17+
import org.opensearch.sql.calcite.utils.PPLOperandTypes;
18+
19+
/**
20+
* Exercises {@link PPLTypeChecker#wrapUDT} against plain Calcite types. Signatures declare
21+
* temporal/IP/BINARY operands as UDTs, but the analytics engine builds row types from plain Calcite
22+
* types, so a UDT signature must still accept the equivalent plain type.
23+
*/
24+
class PPLUdtSignatureMatchTest {
25+
26+
private static final OpenSearchTypeFactory TF = OpenSearchTypeFactory.TYPE_FACTORY;
27+
28+
/** The checker behind {@code list(<field>)}. */
29+
private static final PPLTypeChecker ANY_SCALAR =
30+
PPLTypeChecker.wrapUDT(
31+
((UDFOperandMetadata.UDTOperandMetadata) PPLOperandTypes.ANY_SCALAR).allowedParamTypes());
32+
33+
private static RelDataType nullable(RelDataType type) {
34+
return TF.createTypeWithNullability(type, true);
35+
}
36+
37+
private static boolean accepts(RelDataType type) {
38+
return ANY_SCALAR.checkOperandTypes(List.of(type));
39+
}
40+
41+
@Test
42+
void plainTimestampMatchesTimestampUdt() {
43+
// date -> TIMESTAMP(3), date_nanos -> TIMESTAMP(9) on the analytics route.
44+
assertTrue(accepts(nullable(TF.createSqlType(SqlTypeName.TIMESTAMP, 3))));
45+
assertTrue(accepts(nullable(TF.createSqlType(SqlTypeName.TIMESTAMP, 9))));
46+
}
47+
48+
@Test
49+
void plainDateAndTimeMatchTheirUdts() {
50+
assertTrue(accepts(nullable(TF.createSqlType(SqlTypeName.DATE))));
51+
assertTrue(accepts(nullable(TF.createSqlType(SqlTypeName.TIME))));
52+
}
53+
54+
@Test
55+
void plainVarbinaryMatchesBinaryUdt() {
56+
// ip and binary both map to VARBINARY on the analytics route.
57+
assertTrue(accepts(nullable(TF.createSqlType(SqlTypeName.VARBINARY))));
58+
}
59+
60+
@Test
61+
void udtOperandsStillMatch() {
62+
assertTrue(accepts(TF.createUDT(ExprUDT.EXPR_TIMESTAMP)));
63+
assertTrue(accepts(TF.createUDT(ExprUDT.EXPR_DATE)));
64+
assertTrue(accepts(TF.createUDT(ExprUDT.EXPR_TIME)));
65+
assertTrue(accepts(TF.createUDT(ExprUDT.EXPR_IP)));
66+
}
67+
68+
@Test
69+
void plainScalarsStillMatch() {
70+
assertTrue(accepts(TF.createSqlType(SqlTypeName.INTEGER)));
71+
assertTrue(accepts(TF.createSqlType(SqlTypeName.BIGINT)));
72+
assertTrue(accepts(TF.createSqlType(SqlTypeName.VARCHAR)));
73+
assertTrue(accepts(TF.createSqlType(SqlTypeName.BOOLEAN)));
74+
}
75+
76+
@Test
77+
void nonScalarsAreStillRejected() {
78+
assertFalse(
79+
accepts(TF.createArrayType(TF.createSqlType(SqlTypeName.INTEGER), -1)),
80+
"ANY_SCALAR must not accept arrays");
81+
assertFalse(
82+
accepts(
83+
TF.createMapType(
84+
TF.createSqlType(SqlTypeName.VARCHAR), TF.createSqlType(SqlTypeName.INTEGER))),
85+
"ANY_SCALAR must not accept maps");
86+
}
87+
}

0 commit comments

Comments
 (0)