From a5694a68a034afc705b301ea3a4fa5e7e71ce355 Mon Sep 17 00:00:00 2001 From: Vinay Krishna Pudyodu Date: Thu, 14 May 2026 22:14:38 +0000 Subject: [PATCH 1/4] Added coercion rules and placeholder UDF to handle VARBINARY Signed-off-by: Vinay Krishna Pudyodu --- .../sql/calcite/ExtendedRexBuilder.java | 6 ++ .../calcite/utils/OpenSearchTypeFactory.java | 3 + .../expression/function/CoercionUtils.java | 11 ++- .../function/PPLBuiltinOperators.java | 4 + .../udf/conversion/BinaryFunction.java | 55 ++++++++++++ .../sql/calcite/ExtendedRexBuilderTest.java | 89 +++++++++++++++++++ .../utils/OpenSearchTypeFactoryTest.java | 36 ++++++++ .../function/CoercionUtilsTest.java | 5 +- 8 files changed, 207 insertions(+), 2 deletions(-) create mode 100644 core/src/main/java/org/opensearch/sql/expression/function/udf/conversion/BinaryFunction.java create mode 100644 core/src/test/java/org/opensearch/sql/calcite/ExtendedRexBuilderTest.java diff --git a/core/src/main/java/org/opensearch/sql/calcite/ExtendedRexBuilder.java b/core/src/main/java/org/opensearch/sql/calcite/ExtendedRexBuilder.java index 9b8ac7dfc97..3626c167d8e 100644 --- a/core/src/main/java/org/opensearch/sql/calcite/ExtendedRexBuilder.java +++ b/core/src/main/java/org/opensearch/sql/calcite/ExtendedRexBuilder.java @@ -183,6 +183,12 @@ else if ((SqlTypeUtil.isApproximateNumeric(sourceType) || SqlTypeUtil.isDecimal( // NUMBER_TO_STRING uses java's built-in method to get the string representation of a number return makeCall(type, PPLBuiltinOperators.NUMBER_TO_STRING, List.of(exp)); } + // VARCHAR → VARBINARY for ip/binary fields. Emit BINARY(varchar) as a placeholder + // RexCall the analytics backend adapter rewrites into a VARBINARY literal. + else if (sqlType == SqlTypeName.VARBINARY + && sourceType.getSqlTypeName() == SqlTypeName.VARCHAR) { + return makeCall(type, PPLBuiltinOperators.BINARY, List.of(exp)); + } return super.makeCast(pos, type, exp, matchNullability, safe, format); } } diff --git a/core/src/main/java/org/opensearch/sql/calcite/utils/OpenSearchTypeFactory.java b/core/src/main/java/org/opensearch/sql/calcite/utils/OpenSearchTypeFactory.java index cfed5804f25..9480cc717c9 100644 --- a/core/src/main/java/org/opensearch/sql/calcite/utils/OpenSearchTypeFactory.java +++ b/core/src/main/java/org/opensearch/sql/calcite/utils/OpenSearchTypeFactory.java @@ -174,6 +174,8 @@ public static RelDataType convertExprTypeToRelDataType(ExprType fieldType, boole return TYPE_FACTORY.createUDT(ExprUDT.EXPR_TIME, nullable); case TIMESTAMP: return TYPE_FACTORY.createUDT(ExprUDT.EXPR_TIMESTAMP, nullable); + case BINARY: + return TYPE_FACTORY.createSqlType(SqlTypeName.VARBINARY, nullable); case ARRAY: return TYPE_FACTORY.createArrayType( TYPE_FACTORY.createSqlType(SqlTypeName.ANY, nullable), -1); @@ -225,6 +227,7 @@ public static ExprType convertSqlTypeNameToExprType(SqlTypeName sqlTypeName) { case FLOAT, REAL -> FLOAT; case DOUBLE, DECIMAL -> DOUBLE; // TODO the decimal is only used for literal case CHAR, VARCHAR, MULTISET -> STRING; // call toString() for MULTISET + case VARBINARY, BINARY -> BINARY; case BOOLEAN -> BOOLEAN; case DATE -> DATE; case TIME, TIME_TZ, TIME_WITH_LOCAL_TIME_ZONE -> TIME; diff --git a/core/src/main/java/org/opensearch/sql/expression/function/CoercionUtils.java b/core/src/main/java/org/opensearch/sql/expression/function/CoercionUtils.java index ce78d6dec21..7562dac74d8 100644 --- a/core/src/main/java/org/opensearch/sql/expression/function/CoercionUtils.java +++ b/core/src/main/java/org/opensearch/sql/expression/function/CoercionUtils.java @@ -181,7 +181,12 @@ public static boolean hasString(List rexNodeList) { (left, right) -> ExprCoreType.TIMESTAMP), CoercionRule.of( (left, right) -> hasString(left, right) && hasNumber(left, right), - (left, right) -> ExprCoreType.DOUBLE)); + (left, right) -> ExprCoreType.DOUBLE), + // (BINARY, STRING) → BINARY: ip/binary columns compared with string literals. + // Triggers ExtendedRexBuilder.makeCast which wraps the literal with BINARY. + CoercionRule.of( + (left, right) -> hasString(left, right) && hasBinary(left, right), + (left, right) -> ExprCoreType.BINARY)); private static boolean hasString(ExprType left, ExprType right) { return left == ExprCoreType.STRING || right == ExprCoreType.STRING; @@ -195,6 +200,10 @@ private static boolean hasBoolean(ExprType left, ExprType right) { return left == ExprCoreType.BOOLEAN || right == ExprCoreType.BOOLEAN; } + private static boolean hasBinary(ExprType left, ExprType right) { + return left == ExprCoreType.BINARY || right == ExprCoreType.BINARY; + } + private record CoercionRule( BiPredicate predicate, BinaryOperator resolver) { diff --git a/core/src/main/java/org/opensearch/sql/expression/function/PPLBuiltinOperators.java b/core/src/main/java/org/opensearch/sql/expression/function/PPLBuiltinOperators.java index 3c144967ef7..4b7ee41f016 100644 --- a/core/src/main/java/org/opensearch/sql/expression/function/PPLBuiltinOperators.java +++ b/core/src/main/java/org/opensearch/sql/expression/function/PPLBuiltinOperators.java @@ -83,6 +83,7 @@ import org.opensearch.sql.expression.function.udf.condition.EarliestFunction; import org.opensearch.sql.expression.function.udf.condition.EnhancedCoalesceFunction; import org.opensearch.sql.expression.function.udf.condition.LatestFunction; +import org.opensearch.sql.expression.function.udf.conversion.BinaryFunction; import org.opensearch.sql.expression.function.udf.datetime.AddSubDateFunction; import org.opensearch.sql.expression.function.udf.datetime.CurrentFunction; import org.opensearch.sql.expression.function.udf.datetime.DateAddSubFunction; @@ -179,6 +180,9 @@ public class PPLBuiltinOperators extends ReflectiveSqlOperatorTable { public static final SqlOperator EARLIEST = new EarliestFunction().toUDF("EARLIEST"); public static final SqlOperator LATEST = new LatestFunction().toUDF("LATEST"); + // VARBINARY conversion (placeholder for ip/binary fields rewritten by analytics backend adapter) + public static final SqlOperator BINARY = new BinaryFunction().toUDF("BINARY"); + // Datetime function public static final SqlOperator TIMESTAMP = new TimestampFunction().toUDF("TIMESTAMP"); public static final SqlOperator DATE = diff --git a/core/src/main/java/org/opensearch/sql/expression/function/udf/conversion/BinaryFunction.java b/core/src/main/java/org/opensearch/sql/expression/function/udf/conversion/BinaryFunction.java new file mode 100644 index 00000000000..b2a39f627be --- /dev/null +++ b/core/src/main/java/org/opensearch/sql/expression/function/udf/conversion/BinaryFunction.java @@ -0,0 +1,55 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.sql.expression.function.udf.conversion; + +import static org.opensearch.sql.calcite.utils.OpenSearchTypeFactory.TYPE_FACTORY; + +import java.util.List; +import org.apache.calcite.adapter.enumerable.NotNullImplementor; +import org.apache.calcite.adapter.enumerable.NullPolicy; +import org.apache.calcite.adapter.enumerable.RexToLixTranslator; +import org.apache.calcite.linq4j.tree.Expression; +import org.apache.calcite.rex.RexCall; +import org.apache.calcite.sql.type.FamilyOperandTypeChecker; +import org.apache.calcite.sql.type.OperandTypes; +import org.apache.calcite.sql.type.ReturnTypes; +import org.apache.calcite.sql.type.SqlReturnTypeInference; +import org.apache.calcite.sql.type.SqlTypeName; +import org.opensearch.sql.expression.function.ImplementorUDF; +import org.opensearch.sql.expression.function.UDFOperandMetadata; + +/** + * Placeholder UDF that wraps a VARCHAR literal cast to VARBINARY for ip/binary fields. + */ +public class BinaryFunction extends ImplementorUDF { + + private static final SqlReturnTypeInference VARBINARY_FORCE_NULLABLE = + ReturnTypes.explicit( + TYPE_FACTORY.createTypeWithNullability( + TYPE_FACTORY.createSqlType(SqlTypeName.VARBINARY), true)); + + public BinaryFunction() { + super(new PassThroughImplementor(), NullPolicy.STRICT); + } + + @Override + public SqlReturnTypeInference getReturnTypeInference() { + return VARBINARY_FORCE_NULLABLE; + } + + @Override + public UDFOperandMetadata getOperandMetadata() { + return UDFOperandMetadata.wrap((FamilyOperandTypeChecker) OperandTypes.CHARACTER); + } + + public static class PassThroughImplementor implements NotNullImplementor { + @Override + public Expression implement( + RexToLixTranslator translator, RexCall call, List translatedOperands) { + return translatedOperands.get(0); + } + } +} diff --git a/core/src/test/java/org/opensearch/sql/calcite/ExtendedRexBuilderTest.java b/core/src/test/java/org/opensearch/sql/calcite/ExtendedRexBuilderTest.java new file mode 100644 index 00000000000..39fe6e1ede0 --- /dev/null +++ b/core/src/test/java/org/opensearch/sql/calcite/ExtendedRexBuilderTest.java @@ -0,0 +1,89 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.sql.calcite; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.opensearch.sql.calcite.utils.OpenSearchTypeFactory.TYPE_FACTORY; + +import org.apache.calcite.rel.type.RelDataType; +import org.apache.calcite.rex.RexBuilder; +import org.apache.calcite.rex.RexCall; +import org.apache.calcite.rex.RexNode; +import org.apache.calcite.sql.type.SqlTypeName; +import org.junit.jupiter.api.Test; +import org.opensearch.sql.expression.function.PPLBuiltinOperators; + +class ExtendedRexBuilderTest { + + private static final RexBuilder REX_BUILDER = new ExtendedRexBuilder(new RexBuilder(TYPE_FACTORY)); + + /** + * VARCHAR → VARBINARY casts must be rewritten as a {@code BINARY(varchar)} placeholder + * {@code RexCall}. + */ + @Test + void castVarcharToVarbinaryEmitsBinaryPlaceholder() { + // Use makeInputRef to construct a VARCHAR-typed RexNode reliably. makeLiteral(String) folds + // to CHAR, which would make this test pass-through default cast instead of our placeholder. + RelDataType varchar = TYPE_FACTORY.createSqlType(SqlTypeName.VARCHAR); + RexNode varcharRef = REX_BUILDER.makeInputRef(varchar, 0); + RelDataType varbinary = TYPE_FACTORY.createSqlType(SqlTypeName.VARBINARY); + + RexNode result = REX_BUILDER.makeCast(varbinary, varcharRef); + + assertInstanceOf(RexCall.class, result); + RexCall call = (RexCall) result; + assertEquals(PPLBuiltinOperators.BINARY, call.getOperator()); + assertEquals("BINARY", call.getOperator().getName()); + assertEquals(SqlTypeName.VARBINARY, call.getType().getSqlTypeName()); + assertEquals(1, call.getOperands().size()); + assertEquals(SqlTypeName.VARCHAR, call.getOperands().get(0).getType().getSqlTypeName()); + } + + /** + * Casts targeting a SqlTypeName other than VARBINARY must NOT trigger the BINARY rewrite — + * they fall through to Calcite's default cast handling. + */ + @Test + void castVarcharToIntegerDoesNotEmitBinaryPlaceholder() { + RexNode varcharLiteral = REX_BUILDER.makeLiteral("42"); + RelDataType integer = TYPE_FACTORY.createSqlType(SqlTypeName.INTEGER); + + RexNode result = REX_BUILDER.makeCast(integer, varcharLiteral); + + assertNotNull(result); + if (result instanceof RexCall call) { + assertEquals( + "BINARY".equals(call.getOperator().getName()), + false, + "VARCHAR → INTEGER must not emit BINARY placeholder"); + } + } + + /** + * Casts whose source is not VARCHAR must also fall through. The placeholder is only meant for + * the (VARCHAR → VARBINARY) case where a string IP / base64 literal is being compared against a + * VARBINARY column — non-string sources have well-defined Calcite cast semantics that should + * not be hijacked. + */ + @Test + void castIntegerToVarbinaryDoesNotEmitBinaryPlaceholder() { + RexNode intLiteral = REX_BUILDER.makeExactLiteral(java.math.BigDecimal.ONE); + RelDataType varbinary = TYPE_FACTORY.createSqlType(SqlTypeName.VARBINARY); + + RexNode result = REX_BUILDER.makeCast(varbinary, intLiteral); + + assertNotNull(result); + if (result instanceof RexCall call) { + assertEquals( + "BINARY".equals(call.getOperator().getName()), + false, + "non-VARCHAR → VARBINARY must not emit BINARY placeholder"); + } + } +} diff --git a/core/src/test/java/org/opensearch/sql/calcite/utils/OpenSearchTypeFactoryTest.java b/core/src/test/java/org/opensearch/sql/calcite/utils/OpenSearchTypeFactoryTest.java index 583d1aea6cb..b8fca157820 100644 --- a/core/src/test/java/org/opensearch/sql/calcite/utils/OpenSearchTypeFactoryTest.java +++ b/core/src/test/java/org/opensearch/sql/calcite/utils/OpenSearchTypeFactoryTest.java @@ -17,6 +17,7 @@ import org.junit.jupiter.api.Test; import org.opensearch.sql.calcite.type.AbstractExprRelDataType; import org.opensearch.sql.calcite.utils.OpenSearchTypeFactory.ExprUDT; +import org.opensearch.sql.data.type.ExprCoreType; public class OpenSearchTypeFactoryTest { @@ -126,4 +127,39 @@ public void testLeastRestrictiveDelegatesToSuperForPlainTypes() { assertNotNull(result); assertEquals(SqlTypeName.INTEGER, result.getSqlTypeName()); } + + @Test + public void testConvertSqlTypeNameVarbinaryToBinaryExprType() { + assertEquals( + ExprCoreType.BINARY, OpenSearchTypeFactory.convertSqlTypeNameToExprType(SqlTypeName.VARBINARY)); + } + + @Test + public void testConvertSqlTypeNameBinaryToBinaryExprType() { + assertEquals( + ExprCoreType.BINARY, OpenSearchTypeFactory.convertSqlTypeNameToExprType(SqlTypeName.BINARY)); + } + + @Test + public void testConvertRelDataTypeVarbinaryToBinaryExprType() { + RelDataType varbinary = TYPE_FACTORY.createSqlType(SqlTypeName.VARBINARY); + assertEquals( + ExprCoreType.BINARY, OpenSearchTypeFactory.convertRelDataTypeToExprType(varbinary)); + } + + @Test + public void testConvertExprTypeBinaryToVarbinaryRelDataType() { + RelDataType result = OpenSearchTypeFactory.convertExprTypeToRelDataType(ExprCoreType.BINARY); + assertNotNull(result); + assertEquals(SqlTypeName.VARBINARY, result.getSqlTypeName()); + } + + @Test + public void testConvertExprTypeBinaryToNullableVarbinary() { + RelDataType result = + OpenSearchTypeFactory.convertExprTypeToRelDataType(ExprCoreType.BINARY, true); + assertNotNull(result); + assertEquals(SqlTypeName.VARBINARY, result.getSqlTypeName()); + assertTrue(result.isNullable()); + } } diff --git a/core/src/test/java/org/opensearch/sql/expression/function/CoercionUtilsTest.java b/core/src/test/java/org/opensearch/sql/expression/function/CoercionUtilsTest.java index 30d827f1ecc..373881e8543 100644 --- a/core/src/test/java/org/opensearch/sql/expression/function/CoercionUtilsTest.java +++ b/core/src/test/java/org/opensearch/sql/expression/function/CoercionUtilsTest.java @@ -6,6 +6,7 @@ package org.opensearch.sql.expression.function; import static org.junit.jupiter.api.Assertions.*; +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.DOUBLE; import static org.opensearch.sql.data.type.ExprCoreType.INTEGER; @@ -37,7 +38,9 @@ private static Stream commonWidestTypeArguments() { Arguments.of(STRING, INTEGER, DOUBLE), Arguments.of(INTEGER, STRING, DOUBLE), Arguments.of(STRING, DOUBLE, DOUBLE), - Arguments.of(INTEGER, BOOLEAN, null)); + Arguments.of(INTEGER, BOOLEAN, null), + Arguments.of(BINARY, STRING, BINARY), + Arguments.of(STRING, BINARY, BINARY)); } @ParameterizedTest From a03cf6edf1a9fbb738c832ea4df14cbfaf74c0b0 Mon Sep 17 00:00:00 2001 From: Vinay Krishna Pudyodu Date: Fri, 15 May 2026 02:09:30 +0000 Subject: [PATCH 2/4] Added CIDR, IN, BETWEEN support for IP Signed-off-by: Vinay Krishna Pudyodu --- .../calcite/utils/OpenSearchTypeFactory.java | 24 +++++++++ .../expression/function/PPLFuncImpTable.java | 50 +++++++++++++++++++ 2 files changed, 74 insertions(+) diff --git a/core/src/main/java/org/opensearch/sql/calcite/utils/OpenSearchTypeFactory.java b/core/src/main/java/org/opensearch/sql/calcite/utils/OpenSearchTypeFactory.java index 9480cc717c9..e796d346449 100644 --- a/core/src/main/java/org/opensearch/sql/calcite/utils/OpenSearchTypeFactory.java +++ b/core/src/main/java/org/opensearch/sql/calcite/utils/OpenSearchTypeFactory.java @@ -414,10 +414,34 @@ public static boolean isNumericType(RelDataType fieldType) { } return first; } + // When the list has a VARBINARY column plus VARCHAR literals, treat VARBINARY + // as the common type so IN / BETWEEN can insert casts. + RelDataType varbinaryResult = leastRestrictiveVarbinaryVarchar(types); + if (varbinaryResult != null) { + return varbinaryResult; + } } return super.leastRestrictive(types); } + private @Nullable RelDataType leastRestrictiveVarbinaryVarchar(List types) { + boolean hasVarbinary = false; + boolean anyNullable = false; + for (RelDataType t : types) { + SqlTypeName name = t.getSqlTypeName(); + if (name == SqlTypeName.VARBINARY) { + hasVarbinary = true; + } else if (name != SqlTypeName.VARCHAR && name != SqlTypeName.CHAR) { + return null; + } + anyNullable |= t.isNullable(); + } + if (!hasVarbinary) { + return null; + } + return createTypeWithNullability(createSqlType(SqlTypeName.VARBINARY), anyNullable); + } + /** * Checks if the RelDataType represents a time-based field (timestamp, date, or time). Supports * both standard SQL time types (including TIMESTAMP, TIMESTAMP_WITH_LOCAL_TIME_ZONE, DATE, TIME, diff --git a/core/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.java b/core/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.java index 6f0b37af828..277025afa59 100644 --- a/core/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.java +++ b/core/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.java @@ -268,6 +268,7 @@ import static org.opensearch.sql.expression.function.BuiltinFunctionName.YEARWEEK; import com.google.common.collect.ImmutableMap; +import inet.ipaddr.IPAddress; import java.math.BigDecimal; import java.util.ArrayList; import java.util.Arrays; @@ -282,6 +283,7 @@ import java.util.stream.Collectors; import java.util.stream.Stream; import javax.annotation.Nullable; +import org.apache.calcite.avatica.util.ByteString; import org.apache.calcite.rel.type.RelDataType; import org.apache.calcite.rex.RexBuilder; import org.apache.calcite.rex.RexLambda; @@ -311,6 +313,8 @@ import org.opensearch.sql.calcite.utils.PlanUtils; import org.opensearch.sql.calcite.utils.UserDefinedFunctionUtils; import org.opensearch.sql.exception.ExpressionEvaluationException; +import org.opensearch.sql.exception.SemanticCheckException; +import org.opensearch.sql.utils.IPUtils; import org.opensearch.sql.executor.QueryType; import org.opensearch.sql.expression.function.CollectionUDF.MVIndexFunctionImp; @@ -908,6 +912,34 @@ void populate() { registerDivideFunction(DIVIDEFUNCTION); registerOperator(SHA2, PPLBuiltinOperators.SHA2); registerOperator(CIDRMATCH, PPLBuiltinOperators.CIDRMATCH); + // (VARBINARY, VARCHAR) overload for ip / binary columns. The lambda parses the cidr + // literal at plan time and emits AND(col >= low, col <= high) directly. + // Only literal cidrs are expanded. A non-literal cidr (e.g. column-on-right) falls + // through to PPLBuiltinOperators.CIDRMATCH; that UDF only accepts (IP, STRING) / + // (STRING, STRING), so it will fail downstream for VARBINARY columns. Supporting + // non-literal cidrs would require a separate runtime function and is out of scope. + register( + CIDRMATCH, + (FunctionImp2) + (builder, col, cidr) -> { + if (cidr instanceof RexLiteral lit + && col.getType().getSqlTypeName() == SqlTypeName.VARBINARY) { + byte[][] range = parseCidrToIpv6Range(lit.getValueAs(String.class)); + RelDataType varbinary = + builder.getTypeFactory().createSqlType(SqlTypeName.VARBINARY); + RexNode low = + builder.makeLiteral(new ByteString(range[0]), varbinary, false); + RexNode high = + builder.makeLiteral(new ByteString(range[1]), varbinary, false); + // makeCall(AND, ...) auto-flattens at construction, so no Filter.isFlat issue. + return builder.makeCall( + SqlStdOperatorTable.AND, + builder.makeCall(SqlStdOperatorTable.GREATER_THAN_OR_EQUAL, col, low), + builder.makeCall(SqlStdOperatorTable.LESS_THAN_OR_EQUAL, col, high)); + } + return builder.makeCall(PPLBuiltinOperators.CIDRMATCH, col, cidr); + }, + PPLTypeChecker.family(SqlTypeFamily.BINARY, SqlTypeFamily.STRING)); registerOperator(INTERNAL_GROK, PPLBuiltinOperators.GROK); registerOperator(INTERNAL_PARSE, PPLBuiltinOperators.PARSE); registerOperator(MATCH, PPLBuiltinOperators.MATCH); @@ -1589,4 +1621,22 @@ private static SqlOperandTypeChecker extractTypeCheckerFromUDF(SqlOperator opera } return typeChecker; } + + /** + * Parses a CIDR string and returns its lower and upper bounds in canonical 16-byte + * IPv6-mapped form. Used by the (BINARY, STRING) {@code cidrmatch} overload to expand into + * a byte-range conjunction at plan time. + * + *

Delegates to {@link IPUtils#toRange(String)} for parsing; converts both bounds to + * IPv6 to guarantee 16-byte output regardless of whether the input cidr is IPv4 or IPv6. + */ + private static byte[][] parseCidrToIpv6Range(String cidr) { + if (cidr == null) { + throw new SemanticCheckException("cidrmatch range argument is null"); + } + IPAddress range = IPUtils.toRange(cidr); + byte[] low = range.getLower().toIPv6().getBytes(); + byte[] high = range.getUpper().toIPv6().getBytes(); + return new byte[][] {low, high}; + } } From d3157becfe6b156834db626bf37b3a586b4187e0 Mon Sep 17 00:00:00 2001 From: Vinay Krishna Pudyodu Date: Fri, 15 May 2026 16:45:09 +0000 Subject: [PATCH 3/4] Fixed spotless checks Signed-off-by: Vinay Krishna Pudyodu --- .../expression/function/PPLFuncImpTable.java | 23 ++++++++----------- .../udf/conversion/BinaryFunction.java | 4 +--- .../sql/calcite/ExtendedRexBuilderTest.java | 19 +++++++-------- .../utils/OpenSearchTypeFactoryTest.java | 6 +++-- 4 files changed, 24 insertions(+), 28 deletions(-) diff --git a/core/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.java b/core/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.java index 277025afa59..d34216ad0ac 100644 --- a/core/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.java +++ b/core/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.java @@ -314,9 +314,9 @@ import org.opensearch.sql.calcite.utils.UserDefinedFunctionUtils; import org.opensearch.sql.exception.ExpressionEvaluationException; import org.opensearch.sql.exception.SemanticCheckException; -import org.opensearch.sql.utils.IPUtils; import org.opensearch.sql.executor.QueryType; import org.opensearch.sql.expression.function.CollectionUDF.MVIndexFunctionImp; +import org.opensearch.sql.utils.IPUtils; public class PPLFuncImpTable { private static final Logger logger = LogManager.getLogger(PPLFuncImpTable.class); @@ -914,10 +914,7 @@ void populate() { registerOperator(CIDRMATCH, PPLBuiltinOperators.CIDRMATCH); // (VARBINARY, VARCHAR) overload for ip / binary columns. The lambda parses the cidr // literal at plan time and emits AND(col >= low, col <= high) directly. - // Only literal cidrs are expanded. A non-literal cidr (e.g. column-on-right) falls - // through to PPLBuiltinOperators.CIDRMATCH; that UDF only accepts (IP, STRING) / - // (STRING, STRING), so it will fail downstream for VARBINARY columns. Supporting - // non-literal cidrs would require a separate runtime function and is out of scope. + // Only literal cidrs are expanded. register( CIDRMATCH, (FunctionImp2) @@ -927,10 +924,8 @@ void populate() { byte[][] range = parseCidrToIpv6Range(lit.getValueAs(String.class)); RelDataType varbinary = builder.getTypeFactory().createSqlType(SqlTypeName.VARBINARY); - RexNode low = - builder.makeLiteral(new ByteString(range[0]), varbinary, false); - RexNode high = - builder.makeLiteral(new ByteString(range[1]), varbinary, false); + RexNode low = builder.makeLiteral(new ByteString(range[0]), varbinary, false); + RexNode high = builder.makeLiteral(new ByteString(range[1]), varbinary, false); // makeCall(AND, ...) auto-flattens at construction, so no Filter.isFlat issue. return builder.makeCall( SqlStdOperatorTable.AND, @@ -1623,12 +1618,12 @@ private static SqlOperandTypeChecker extractTypeCheckerFromUDF(SqlOperator opera } /** - * Parses a CIDR string and returns its lower and upper bounds in canonical 16-byte - * IPv6-mapped form. Used by the (BINARY, STRING) {@code cidrmatch} overload to expand into - * a byte-range conjunction at plan time. + * Parses a CIDR string and returns its lower and upper bounds in canonical 16-byte IPv6-mapped + * form. Used by the (BINARY, STRING) {@code cidrmatch} overload to expand into a byte-range + * conjunction at plan time. * - *

Delegates to {@link IPUtils#toRange(String)} for parsing; converts both bounds to - * IPv6 to guarantee 16-byte output regardless of whether the input cidr is IPv4 or IPv6. + *

Delegates to {@link IPUtils#toRange(String)} for parsing; converts both bounds to IPv6 to + * guarantee 16-byte output regardless of whether the input cidr is IPv4 or IPv6. */ private static byte[][] parseCidrToIpv6Range(String cidr) { if (cidr == null) { diff --git a/core/src/main/java/org/opensearch/sql/expression/function/udf/conversion/BinaryFunction.java b/core/src/main/java/org/opensearch/sql/expression/function/udf/conversion/BinaryFunction.java index b2a39f627be..038a27ea99b 100644 --- a/core/src/main/java/org/opensearch/sql/expression/function/udf/conversion/BinaryFunction.java +++ b/core/src/main/java/org/opensearch/sql/expression/function/udf/conversion/BinaryFunction.java @@ -21,9 +21,7 @@ import org.opensearch.sql.expression.function.ImplementorUDF; import org.opensearch.sql.expression.function.UDFOperandMetadata; -/** - * Placeholder UDF that wraps a VARCHAR literal cast to VARBINARY for ip/binary fields. - */ +/** Placeholder UDF that wraps a VARCHAR literal cast to VARBINARY for ip/binary fields. */ public class BinaryFunction extends ImplementorUDF { private static final SqlReturnTypeInference VARBINARY_FORCE_NULLABLE = diff --git a/core/src/test/java/org/opensearch/sql/calcite/ExtendedRexBuilderTest.java b/core/src/test/java/org/opensearch/sql/calcite/ExtendedRexBuilderTest.java index 39fe6e1ede0..9fbcbdbb71e 100644 --- a/core/src/test/java/org/opensearch/sql/calcite/ExtendedRexBuilderTest.java +++ b/core/src/test/java/org/opensearch/sql/calcite/ExtendedRexBuilderTest.java @@ -20,11 +20,12 @@ class ExtendedRexBuilderTest { - private static final RexBuilder REX_BUILDER = new ExtendedRexBuilder(new RexBuilder(TYPE_FACTORY)); + private static final RexBuilder REX_BUILDER = + new ExtendedRexBuilder(new RexBuilder(TYPE_FACTORY)); /** - * VARCHAR → VARBINARY casts must be rewritten as a {@code BINARY(varchar)} placeholder - * {@code RexCall}. + * VARCHAR → VARBINARY casts must be rewritten as a {@code BINARY(varchar)} placeholder {@code + * RexCall}. */ @Test void castVarcharToVarbinaryEmitsBinaryPlaceholder() { @@ -46,8 +47,8 @@ void castVarcharToVarbinaryEmitsBinaryPlaceholder() { } /** - * Casts targeting a SqlTypeName other than VARBINARY must NOT trigger the BINARY rewrite — - * they fall through to Calcite's default cast handling. + * Casts targeting a SqlTypeName other than VARBINARY must NOT trigger the BINARY rewrite — they + * fall through to Calcite's default cast handling. */ @Test void castVarcharToIntegerDoesNotEmitBinaryPlaceholder() { @@ -66,10 +67,10 @@ void castVarcharToIntegerDoesNotEmitBinaryPlaceholder() { } /** - * Casts whose source is not VARCHAR must also fall through. The placeholder is only meant for - * the (VARCHAR → VARBINARY) case where a string IP / base64 literal is being compared against a - * VARBINARY column — non-string sources have well-defined Calcite cast semantics that should - * not be hijacked. + * Casts whose source is not VARCHAR must also fall through. The placeholder is only meant for the + * (VARCHAR → VARBINARY) case where a string IP / base64 literal is being compared against a + * VARBINARY column — non-string sources have well-defined Calcite cast semantics that should not + * be hijacked. */ @Test void castIntegerToVarbinaryDoesNotEmitBinaryPlaceholder() { diff --git a/core/src/test/java/org/opensearch/sql/calcite/utils/OpenSearchTypeFactoryTest.java b/core/src/test/java/org/opensearch/sql/calcite/utils/OpenSearchTypeFactoryTest.java index b8fca157820..f7d9e544c24 100644 --- a/core/src/test/java/org/opensearch/sql/calcite/utils/OpenSearchTypeFactoryTest.java +++ b/core/src/test/java/org/opensearch/sql/calcite/utils/OpenSearchTypeFactoryTest.java @@ -131,13 +131,15 @@ public void testLeastRestrictiveDelegatesToSuperForPlainTypes() { @Test public void testConvertSqlTypeNameVarbinaryToBinaryExprType() { assertEquals( - ExprCoreType.BINARY, OpenSearchTypeFactory.convertSqlTypeNameToExprType(SqlTypeName.VARBINARY)); + ExprCoreType.BINARY, + OpenSearchTypeFactory.convertSqlTypeNameToExprType(SqlTypeName.VARBINARY)); } @Test public void testConvertSqlTypeNameBinaryToBinaryExprType() { assertEquals( - ExprCoreType.BINARY, OpenSearchTypeFactory.convertSqlTypeNameToExprType(SqlTypeName.BINARY)); + ExprCoreType.BINARY, + OpenSearchTypeFactory.convertSqlTypeNameToExprType(SqlTypeName.BINARY)); } @Test From 0c4b263cd6f6d42fcddbb53e0e91b649156d294c Mon Sep 17 00:00:00 2001 From: Vinay Krishna Pudyodu Date: Fri, 15 May 2026 19:32:57 +0000 Subject: [PATCH 4/4] Added UT for leastRestrictiveVarbinaryVarchar Signed-off-by: Vinay Krishna Pudyodu --- .../utils/OpenSearchTypeFactoryTest.java | 118 ++++++++++++++++++ 1 file changed, 118 insertions(+) diff --git a/core/src/test/java/org/opensearch/sql/calcite/utils/OpenSearchTypeFactoryTest.java b/core/src/test/java/org/opensearch/sql/calcite/utils/OpenSearchTypeFactoryTest.java index f7d9e544c24..69ded62ed5c 100644 --- a/core/src/test/java/org/opensearch/sql/calcite/utils/OpenSearchTypeFactoryTest.java +++ b/core/src/test/java/org/opensearch/sql/calcite/utils/OpenSearchTypeFactoryTest.java @@ -6,6 +6,7 @@ package org.opensearch.sql.calcite.utils; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertInstanceOf; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -128,6 +129,123 @@ public void testLeastRestrictiveDelegatesToSuperForPlainTypes() { assertEquals(SqlTypeName.INTEGER, result.getSqlTypeName()); } + @Test + public void testLeastRestrictiveVarbinaryAndVarcharReturnsVarbinary() { + RelDataType varbinary = TYPE_FACTORY.createSqlType(SqlTypeName.VARBINARY); + RelDataType varchar = TYPE_FACTORY.createSqlType(SqlTypeName.VARCHAR); + + RelDataType result = TYPE_FACTORY.leastRestrictive(List.of(varbinary, varchar)); + + assertNotNull(result); + assertEquals(SqlTypeName.VARBINARY, result.getSqlTypeName()); + assertFalse(result.isNullable()); + } + + @Test + public void testLeastRestrictiveVarcharAndVarbinaryReturnsVarbinary() { + RelDataType varchar = TYPE_FACTORY.createSqlType(SqlTypeName.VARCHAR); + RelDataType varbinary = TYPE_FACTORY.createSqlType(SqlTypeName.VARBINARY); + + RelDataType result = TYPE_FACTORY.leastRestrictive(List.of(varchar, varbinary)); + + assertNotNull(result); + assertEquals(SqlTypeName.VARBINARY, result.getSqlTypeName()); + } + + @Test + public void testLeastRestrictiveVarbinaryAndCharReturnsVarbinary() { + RelDataType varbinary = TYPE_FACTORY.createSqlType(SqlTypeName.VARBINARY); + RelDataType ch = TYPE_FACTORY.createSqlType(SqlTypeName.CHAR); + + RelDataType result = TYPE_FACTORY.leastRestrictive(List.of(varbinary, ch)); + + assertNotNull(result); + assertEquals(SqlTypeName.VARBINARY, result.getSqlTypeName()); + } + + @Test + public void testLeastRestrictiveVarbinaryAndMultipleVarcharLiteralsReturnsVarbinary() { + RelDataType varbinary = TYPE_FACTORY.createSqlType(SqlTypeName.VARBINARY); + RelDataType v1 = TYPE_FACTORY.createSqlType(SqlTypeName.VARCHAR); + RelDataType v2 = TYPE_FACTORY.createSqlType(SqlTypeName.VARCHAR); + + RelDataType result = TYPE_FACTORY.leastRestrictive(List.of(varbinary, v1, v2)); + + assertNotNull(result); + assertEquals(SqlTypeName.VARBINARY, result.getSqlTypeName()); + } + + @Test + public void testLeastRestrictiveVarbinaryAndNullableVarcharReturnsNullableVarbinary() { + RelDataType varbinary = TYPE_FACTORY.createSqlType(SqlTypeName.VARBINARY); + RelDataType nullableVarchar = + TYPE_FACTORY.createTypeWithNullability( + TYPE_FACTORY.createSqlType(SqlTypeName.VARCHAR), true); + + RelDataType result = TYPE_FACTORY.leastRestrictive(List.of(varbinary, nullableVarchar)); + + assertNotNull(result); + assertEquals(SqlTypeName.VARBINARY, result.getSqlTypeName()); + assertTrue(result.isNullable()); + } + + @Test + public void testLeastRestrictiveNullableVarbinaryAndVarcharReturnsNullableVarbinary() { + RelDataType nullableVarbinary = + TYPE_FACTORY.createTypeWithNullability( + TYPE_FACTORY.createSqlType(SqlTypeName.VARBINARY), true); + RelDataType varchar = TYPE_FACTORY.createSqlType(SqlTypeName.VARCHAR); + + RelDataType result = TYPE_FACTORY.leastRestrictive(List.of(nullableVarbinary, varchar)); + + assertNotNull(result); + assertEquals(SqlTypeName.VARBINARY, result.getSqlTypeName()); + assertTrue(result.isNullable()); + } + + @Test + public void testLeastRestrictiveTwoVarbinariesReturnsVarbinary() { + RelDataType v1 = TYPE_FACTORY.createSqlType(SqlTypeName.VARBINARY); + RelDataType v2 = TYPE_FACTORY.createSqlType(SqlTypeName.VARBINARY); + + RelDataType result = TYPE_FACTORY.leastRestrictive(List.of(v1, v2)); + + assertNotNull(result); + assertEquals(SqlTypeName.VARBINARY, result.getSqlTypeName()); + } + + @Test + public void testLeastRestrictiveVarbinaryAndIntegerFallsBackToSuper() { + RelDataType varbinary = TYPE_FACTORY.createSqlType(SqlTypeName.VARBINARY); + RelDataType integer = TYPE_FACTORY.createSqlType(SqlTypeName.INTEGER); + + // Mixing VARBINARY with a non-string type — varbinary/varchar coercion does not apply, + // so this falls back to super.leastRestrictive (which returns null for incompatible types). + RelDataType result = TYPE_FACTORY.leastRestrictive(List.of(varbinary, integer)); + + // super.leastRestrictive cannot find a common type for VARBINARY + INTEGER + // The exact behavior depends on Calcite's type system, but the key check is that + // we did NOT return VARBINARY from leastRestrictiveVarbinaryVarchar. + if (result != null) { + assertFalse( + result.getSqlTypeName() == SqlTypeName.VARBINARY, + "VARBINARY + INTEGER should not coerce to VARBINARY via the varbinary/varchar path"); + } + } + + @Test + public void testLeastRestrictiveOnlyVarcharsReturnsVarchar() { + RelDataType v1 = TYPE_FACTORY.createSqlType(SqlTypeName.VARCHAR); + RelDataType v2 = TYPE_FACTORY.createSqlType(SqlTypeName.VARCHAR); + + // No VARBINARY in list — leastRestrictiveVarbinaryVarchar returns null, + // so super.leastRestrictive resolves to VARCHAR. + RelDataType result = TYPE_FACTORY.leastRestrictive(List.of(v1, v2)); + + assertNotNull(result); + assertEquals(SqlTypeName.VARCHAR, result.getSqlTypeName()); + } + @Test public void testConvertSqlTypeNameVarbinaryToBinaryExprType() { assertEquals(