From eab8b327f48a325bf3a9cfc2d82b0d48e5b00d1d Mon Sep 17 00:00:00 2001 From: Kai Huang Date: Tue, 4 Aug 2026 10:06:47 -0700 Subject: [PATCH] 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 --- .../expression/function/PPLTypeChecker.java | 38 +++++++- .../function/PPLUdtSignatureMatchTest.java | 87 +++++++++++++++++++ 2 files changed, 122 insertions(+), 3 deletions(-) create mode 100644 core/src/test/java/org/opensearch/sql/expression/function/PPLUdtSignatureMatchTest.java diff --git a/core/src/main/java/org/opensearch/sql/expression/function/PPLTypeChecker.java b/core/src/main/java/org/opensearch/sql/expression/function/PPLTypeChecker.java index c3de664443d..4925b35b649 100644 --- a/core/src/main/java/org/opensearch/sql/expression/function/PPLTypeChecker.java +++ b/core/src/main/java/org/opensearch/sql/expression/function/PPLTypeChecker.java @@ -557,19 +557,51 @@ public List> getParameterTypes() { * ExprUDT} tag — comparing {@code getClass()} is unsafe because addCharsetAndCollation collapses * ExprDateType/ExprTimeType/ExprTimeStampType/ExprBinaryType down to ExprSqlType, so different * UDTs would appear equal. Plain types match by SqlTypeName. + * + *

A UDT signature also accepts the equivalent plain Calcite type. Signatures such as {@code + * PPLOperandTypes.ANY_SCALAR} declare temporal/IP/BINARY operands as UDTs, but the analytics + * engine builds its row types from plain Calcite types plus its own markers, which extend + * Calcite's {@code AbstractSqlType} rather than {@link AbstractExprRelDataType}. Matching on + * class identity alone made {@code list()} fail with an error that listed the very + * type it had just rejected. */ private static boolean typesMatch(RelDataType expected, RelDataType actual) { if (expected instanceof AbstractExprRelDataType expUdt && actual instanceof AbstractExprRelDataType actUdt) { return expUdt.getUdt() == actUdt.getUdt(); } - if (expected instanceof AbstractExprRelDataType - || actual instanceof AbstractExprRelDataType) { - return false; + if (expected instanceof AbstractExprRelDataType expUdt) { + return matchesPlainType(expUdt.getUdt(), actual); + } + if (actual instanceof AbstractExprRelDataType actUdt) { + return matchesPlainType(actUdt.getUdt(), expected); } return expected.getSqlTypeName() == actual.getSqlTypeName(); } + /** + * Whether a plain Calcite type is the non-UDT spelling of {@code udt}. The UDTs themselves are + * all VARCHAR-backed, so this maps the tag to the {@link SqlTypeName}s a backend would produce + * for the same logical type instead of comparing backing types. + */ + private static boolean matchesPlainType(ExprUDT udt, RelDataType plain) { + return switch (udt) { + case EXPR_DATE -> plain.getSqlTypeName() == SqlTypeName.DATE; + case EXPR_TIME -> + switch (plain.getSqlTypeName()) { + case TIME, TIME_TZ, TIME_WITH_LOCAL_TIME_ZONE -> true; + default -> false; + }; + case EXPR_TIMESTAMP -> + switch (plain.getSqlTypeName()) { + case TIMESTAMP, TIMESTAMP_TZ, TIMESTAMP_WITH_LOCAL_TIME_ZONE -> true; + default -> false; + }; + // ip and binary both land as VARBINARY. + case EXPR_IP, EXPR_BINARY -> SqlTypeName.BINARY_TYPES.contains(plain.getSqlTypeName()); + }; + } + // Util Functions /** diff --git a/core/src/test/java/org/opensearch/sql/expression/function/PPLUdtSignatureMatchTest.java b/core/src/test/java/org/opensearch/sql/expression/function/PPLUdtSignatureMatchTest.java new file mode 100644 index 00000000000..2388721435f --- /dev/null +++ b/core/src/test/java/org/opensearch/sql/expression/function/PPLUdtSignatureMatchTest.java @@ -0,0 +1,87 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.sql.expression.function; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.List; +import org.apache.calcite.rel.type.RelDataType; +import org.apache.calcite.sql.type.SqlTypeName; +import org.junit.jupiter.api.Test; +import org.opensearch.sql.calcite.utils.OpenSearchTypeFactory; +import org.opensearch.sql.calcite.utils.OpenSearchTypeFactory.ExprUDT; +import org.opensearch.sql.calcite.utils.PPLOperandTypes; + +/** + * Exercises {@link PPLTypeChecker#wrapUDT} against plain Calcite types. Signatures declare + * temporal/IP/BINARY operands as UDTs, but the analytics engine builds row types from plain Calcite + * types, so a UDT signature must still accept the equivalent plain type. + */ +class PPLUdtSignatureMatchTest { + + private static final OpenSearchTypeFactory TF = OpenSearchTypeFactory.TYPE_FACTORY; + + /** The checker behind {@code list()}. */ + private static final PPLTypeChecker ANY_SCALAR = + PPLTypeChecker.wrapUDT( + ((UDFOperandMetadata.UDTOperandMetadata) PPLOperandTypes.ANY_SCALAR).allowedParamTypes()); + + private static RelDataType nullable(RelDataType type) { + return TF.createTypeWithNullability(type, true); + } + + private static boolean accepts(RelDataType type) { + return ANY_SCALAR.checkOperandTypes(List.of(type)); + } + + @Test + void plainTimestampMatchesTimestampUdt() { + // date -> TIMESTAMP(3), date_nanos -> TIMESTAMP(9) on the analytics route. + assertTrue(accepts(nullable(TF.createSqlType(SqlTypeName.TIMESTAMP, 3)))); + assertTrue(accepts(nullable(TF.createSqlType(SqlTypeName.TIMESTAMP, 9)))); + } + + @Test + void plainDateAndTimeMatchTheirUdts() { + assertTrue(accepts(nullable(TF.createSqlType(SqlTypeName.DATE)))); + assertTrue(accepts(nullable(TF.createSqlType(SqlTypeName.TIME)))); + } + + @Test + void plainVarbinaryMatchesBinaryUdt() { + // ip and binary both map to VARBINARY on the analytics route. + assertTrue(accepts(nullable(TF.createSqlType(SqlTypeName.VARBINARY)))); + } + + @Test + void udtOperandsStillMatch() { + assertTrue(accepts(TF.createUDT(ExprUDT.EXPR_TIMESTAMP))); + assertTrue(accepts(TF.createUDT(ExprUDT.EXPR_DATE))); + assertTrue(accepts(TF.createUDT(ExprUDT.EXPR_TIME))); + assertTrue(accepts(TF.createUDT(ExprUDT.EXPR_IP))); + } + + @Test + void plainScalarsStillMatch() { + assertTrue(accepts(TF.createSqlType(SqlTypeName.INTEGER))); + assertTrue(accepts(TF.createSqlType(SqlTypeName.BIGINT))); + assertTrue(accepts(TF.createSqlType(SqlTypeName.VARCHAR))); + assertTrue(accepts(TF.createSqlType(SqlTypeName.BOOLEAN))); + } + + @Test + void nonScalarsAreStillRejected() { + assertFalse( + accepts(TF.createArrayType(TF.createSqlType(SqlTypeName.INTEGER), -1)), + "ANY_SCALAR must not accept arrays"); + assertFalse( + accepts( + TF.createMapType( + TF.createSqlType(SqlTypeName.VARCHAR), TF.createSqlType(SqlTypeName.INTEGER))), + "ANY_SCALAR must not accept maps"); + } +}