diff --git a/sandbox/libs/analytics-framework/src/main/java/org/opensearch/analytics/spi/ScalarFunction.java b/sandbox/libs/analytics-framework/src/main/java/org/opensearch/analytics/spi/ScalarFunction.java index 13cbc837a8056..2bc65b658f3d9 100644 --- a/sandbox/libs/analytics-framework/src/main/java/org/opensearch/analytics/spi/ScalarFunction.java +++ b/sandbox/libs/analytics-framework/src/main/java/org/opensearch/analytics/spi/ScalarFunction.java @@ -10,8 +10,12 @@ import org.apache.calcite.sql.SqlFunction; import org.apache.calcite.sql.SqlKind; +import org.apache.calcite.sql.SqlOperator; +import org.apache.calcite.sql.fun.SqlStdOperatorTable; +import java.util.HashMap; import java.util.Locale; +import java.util.Map; /** * All scalar functions a backend may support — comparisons, full-text search, @@ -52,7 +56,15 @@ public enum ScalarFunction { LOWER(Category.STRING, SqlKind.OTHER_FUNCTION), TRIM(Category.STRING, SqlKind.TRIM), SUBSTRING(Category.STRING, SqlKind.OTHER_FUNCTION), - CONCAT(Category.STRING, SqlKind.OTHER_FUNCTION), + /** + * String concatenation. Calcite's {@code SqlStdOperatorTable.CONCAT} is a + * {@link org.apache.calcite.sql.SqlBinaryOperator} named {@code "||"} (not {@code "CONCAT"}) + * with {@link SqlKind#OTHER}, so neither {@link #fromSqlKind(SqlKind)} nor identifier-name + * {@link #valueOf(String)} resolves it. The {@code referenceOperator} hook below pins the + * concrete Calcite operator constant so resolution is a singleton-identity match — a Calcite + * rename surfaces as a compile error rather than as a silent string mismatch at runtime. + */ + CONCAT(Category.STRING, SqlKind.OTHER_FUNCTION, SqlStdOperatorTable.CONCAT), CHAR_LENGTH(Category.STRING, SqlKind.OTHER_FUNCTION), // ── Math ───────────────────────────────────────────────────────── @@ -68,6 +80,14 @@ public enum ScalarFunction { // ── Cast / type ────────────────────────────────────────────────── CAST(Category.SCALAR, SqlKind.CAST), + /** + * Calcite's {@code SAFE_CAST} — emitted by PPL's explicit {@code CAST(... AS ...)} when the + * source value may be NULL or the conversion may fail; returns NULL on failure rather than + * throwing. Resolves through {@link SqlKind#SAFE_CAST}, distinct from {@link #CAST} which + * uses {@link SqlKind#CAST}. DataFusion's native cast already returns NULL on conversion + * failure, so SAFE_CAST and CAST share the same backend semantics. + */ + SAFE_CAST(Category.SCALAR, SqlKind.SAFE_CAST), // ── Conditional ────────────────────────────────────────────────── CASE(Category.SCALAR, SqlKind.CASE), @@ -98,10 +118,24 @@ public enum Category { private final Category category; private final SqlKind sqlKind; + /** + * Optional Calcite operator that this constant maps to when the operator cannot be resolved + * via {@link SqlKind} or via identifier-name {@link #valueOf(String)} — typically operators + * whose {@code getName()} returns a non-identifier token (e.g. {@code SqlStdOperatorTable.CONCAT} + * is named {@code "||"}). Null for the common case where SqlKind or name resolution suffices. + * Stored as a reference (not a string) so a Calcite-side rename of the operator surfaces as a + * compile error here. + */ + private final SqlOperator referenceOperator; ScalarFunction(Category category, SqlKind sqlKind) { + this(category, sqlKind, null); + } + + ScalarFunction(Category category, SqlKind sqlKind, SqlOperator referenceOperator) { this.category = category; this.sqlKind = sqlKind; + this.referenceOperator = referenceOperator; } public Category getCategory() { @@ -134,4 +168,52 @@ public static ScalarFunction fromSqlFunction(SqlFunction function) { // valueOf(toUpperCase). This couples enum constant naming to SQL function naming convention. return ScalarFunction.valueOf(function.getName().toUpperCase(Locale.ROOT)); } + + /** + * Reverse index from {@link #referenceOperator} to enum constant. Built from the enum itself + * at class init — adding a new symbolic operator is a single-site change on the enum constant, + * no separate map to maintain. Lookup is identity-keyed because Calcite's standard operators + * are singletons (e.g. {@code SqlStdOperatorTable.CONCAT}). Empty in the common case (most + * constants resolve by SqlKind or identifier-name valueOf). + */ + private static final Map BY_REFERENCE_OPERATOR; + + static { + Map byOperator = new HashMap<>(); + for (ScalarFunction func : values()) { + if (func.referenceOperator != null) { + byOperator.put(func.referenceOperator, func); + } + } + // The HashMap is private static final and never exposed beyond the get() in the resolver + // below — wrapping it in Map.copyOf adds an allocation without any external safety guarantee. + BY_REFERENCE_OPERATOR = byOperator; + } + + /** + * Maps any Calcite {@link SqlOperator} to a {@link ScalarFunction}, or returns null if + * unrecognized. Resolution order: {@link SqlKind} match, then {@link #referenceOperator} + * identity match (handles {@code SqlStdOperatorTable.CONCAT} a.k.a. {@code ||}), then + * identifier-name {@link #valueOf(String)} match. + * + *

Prefer this entry point over {@link #fromSqlKind(SqlKind)} / + * {@link #fromSqlFunction(SqlFunction)} when resolving an arbitrary {@code RexCall}'s + * operator: a {@code RexCall} may be backed by a {@code SqlBinaryOperator} (e.g. {@code ||}) + * which is neither covered by {@code OTHER} {@code SqlKind} nor by {@code SqlFunction}. + */ + public static ScalarFunction fromSqlOperatorWithFallback(SqlOperator operator) { + ScalarFunction byKind = fromSqlKind(operator.getKind()); + if (byKind != null) { + return byKind; + } + ScalarFunction byReference = BY_REFERENCE_OPERATOR.get(operator); + if (byReference != null) { + return byReference; + } + try { + return ScalarFunction.valueOf(operator.getName().toUpperCase(Locale.ROOT)); + } catch (IllegalArgumentException ignored) { + return null; + } + } } diff --git a/sandbox/libs/analytics-framework/src/test/java/org/opensearch/analytics/spi/ScalarFunctionTests.java b/sandbox/libs/analytics-framework/src/test/java/org/opensearch/analytics/spi/ScalarFunctionTests.java index 077c52a3103cc..e0db1cd82ab8e 100644 --- a/sandbox/libs/analytics-framework/src/test/java/org/opensearch/analytics/spi/ScalarFunctionTests.java +++ b/sandbox/libs/analytics-framework/src/test/java/org/opensearch/analytics/spi/ScalarFunctionTests.java @@ -9,13 +9,42 @@ package org.opensearch.analytics.spi; import org.apache.calcite.sql.SqlKind; +import org.apache.calcite.sql.fun.SqlStdOperatorTable; import org.opensearch.test.OpenSearchTestCase; import java.util.EnumMap; import java.util.Map; +/** + * Unit coverage for {@link ScalarFunction}'s three resolution paths used by the analytics-engine + * planner ({@code OpenSearchProjectRule}, {@code OpenSearchFilterRule}, {@code BackendPlanAdapter}). + * + *

Each test pins one of the resolver's branches so a regression that drops a branch surfaces + * here rather than in IT-level "No backend supports scalar function [null]" errors. + */ public class ScalarFunctionTests extends OpenSearchTestCase { + // ── fromSqlKind ───────────────────────────────────────────────────────────── + + public void testFromSqlKindResolvesDedicatedKind() { + assertEquals(ScalarFunction.EQUALS, ScalarFunction.fromSqlKind(SqlKind.EQUALS)); + assertEquals(ScalarFunction.PLUS, ScalarFunction.fromSqlKind(SqlKind.PLUS)); + assertEquals(ScalarFunction.CAST, ScalarFunction.fromSqlKind(SqlKind.CAST)); + assertEquals(ScalarFunction.SAFE_CAST, ScalarFunction.fromSqlKind(SqlKind.SAFE_CAST)); + assertEquals(ScalarFunction.COALESCE, ScalarFunction.fromSqlKind(SqlKind.COALESCE)); + } + + public void testFromSqlKindReturnsNullForOtherKind() { + // SqlKind.OTHER is shared by many SqlBinaryOperators — must NOT resolve via SqlKind. + assertNull(ScalarFunction.fromSqlKind(SqlKind.OTHER)); + } + + public void testFromSqlKindReturnsNullForOtherFunctionKind() { + // SqlKind.OTHER_FUNCTION is shared by many name-distinguished SqlFunctions — must NOT + // resolve via SqlKind even though several enum entries declare it. + assertNull(ScalarFunction.fromSqlKind(SqlKind.OTHER_FUNCTION)); + } + /** Non-OTHER_FUNCTION SqlKinds must be unique: fromSqlKind picks the first match and would shadow later entries. */ public void testNoDuplicateSqlKindBindings() { Map claimedBy = new EnumMap<>(SqlKind.class); @@ -34,4 +63,42 @@ public void testNoDuplicateSqlKindBindings() { public void testSargPredicateIsBoundToSqlKindSearch() { assertSame(ScalarFunction.SARG_PREDICATE, ScalarFunction.fromSqlKind(SqlKind.SEARCH)); } + + // ── fromSqlOperatorWithFallback: SqlKind branch ──────────────────────────────────────── + + public void testFromSqlOperatorResolvesViaSqlKind() { + // Calcite's CAST has a dedicated SqlKind.CAST — short-circuit before name lookup. + assertEquals(ScalarFunction.CAST, ScalarFunction.fromSqlOperatorWithFallback(SqlStdOperatorTable.CAST)); + assertEquals(ScalarFunction.PLUS, ScalarFunction.fromSqlOperatorWithFallback(SqlStdOperatorTable.PLUS)); + assertEquals(ScalarFunction.GREATER_THAN, ScalarFunction.fromSqlOperatorWithFallback(SqlStdOperatorTable.GREATER_THAN)); + assertEquals(ScalarFunction.COALESCE, ScalarFunction.fromSqlOperatorWithFallback(SqlStdOperatorTable.COALESCE)); + } + + // ── fromSqlOperatorWithFallback: reference-operator branch ───────────────────────────── + + public void testFromSqlOperatorResolvesPipeConcatViaReferenceOperator() { + // The original "no backend supports scalar function [null]" symptom for PPL string `+`. + // SqlStdOperatorTable.CONCAT is a SqlBinaryOperator named "||" with SqlKind.OTHER — + // neither fromSqlKind nor fromSqlFunction(SqlFunction) resolves it. CONCAT's + // referenceOperator field points at the singleton, so the resolver matches by identity. + assertEquals("||", SqlStdOperatorTable.CONCAT.getName()); + assertEquals(SqlKind.OTHER, SqlStdOperatorTable.CONCAT.getKind()); + assertEquals(ScalarFunction.CONCAT, ScalarFunction.fromSqlOperatorWithFallback(SqlStdOperatorTable.CONCAT)); + } + + // ── fromSqlOperatorWithFallback: identifier-name branch ──────────────────────────────── + + public void testFromSqlOperatorResolvesViaIdentifierName() { + // SqlStdOperatorTable.UPPER is a SqlFunction named "UPPER" with SqlKind.OTHER_FUNCTION; + // resolves through the valueOf(name.toUpperCase()) fallback after SqlKind misses. + assertEquals(ScalarFunction.UPPER, ScalarFunction.fromSqlOperatorWithFallback(SqlStdOperatorTable.UPPER)); + assertEquals(ScalarFunction.LOWER, ScalarFunction.fromSqlOperatorWithFallback(SqlStdOperatorTable.LOWER)); + assertEquals(ScalarFunction.ABS, ScalarFunction.fromSqlOperatorWithFallback(SqlStdOperatorTable.ABS)); + } + + public void testFromSqlOperatorReturnsNullForUnknownFunction() { + // UNARY_MINUS has SqlKind.MINUS_PREFIX (no enum) and name "-" (not a valid valueOf input); + // both resolution paths miss and the resolver returns null instead of throwing. + assertNull(ScalarFunction.fromSqlOperatorWithFallback(SqlStdOperatorTable.UNARY_MINUS)); + } } diff --git a/sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/ConcatFunctionAdapter.java b/sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/ConcatFunctionAdapter.java new file mode 100644 index 0000000000000..04887359d884f --- /dev/null +++ b/sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/ConcatFunctionAdapter.java @@ -0,0 +1,69 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.be.datafusion; + +import org.apache.calcite.plan.RelOptCluster; +import org.apache.calcite.rex.RexBuilder; +import org.apache.calcite.rex.RexCall; +import org.apache.calcite.rex.RexNode; +import org.apache.calcite.sql.fun.SqlStdOperatorTable; +import org.opensearch.analytics.spi.FieldStorageInfo; +import org.opensearch.analytics.spi.ScalarFunctionAdapter; + +import java.util.List; + +/** + * Adapts {@code ||(a, b, ...)} (Calcite {@code SqlStdOperatorTable.CONCAT}) into a + * null-propagating form for the DataFusion backend. + * + *

Calcite's {@code ||} operator follows the SQL standard: if any operand is NULL, the result + * is NULL. Substrait's default {@code concat} extension is documented with the same semantics, + * but DataFusion's substrait reader maps it to the DataFusion {@code concat()} function — which + * deviates from the standard and treats NULL operands as empty strings. To preserve Calcite's + * semantics on the analytics-engine path, this adapter rewrites + * + * 

{@code
+ *   ||(a, b)
+ *     →
+ *   CASE WHEN a IS NULL OR b IS NULL THEN NULL ELSE ||(a, b) END
+ * }
+ * + * The inner {@code ||} is left intact and serializes through the same Substrait conversion path, + * but with the surrounding CASE/IS_NULL the DataFusion {@code concat()} call is short-circuited + * for any input that contains a NULL — restoring SQL-standard null-propagation without requiring + * a custom DataFusion UDF. + * + *

Single-operand calls fall through unchanged (the result equals the operand, so no + * null-handling rewrite is needed). + */ +class ConcatFunctionAdapter implements ScalarFunctionAdapter { + + @Override + public RexNode adapt(RexCall original, List fieldStorage, RelOptCluster cluster) { + List operands = original.getOperands(); + if (operands.size() < 2) { + return original; + } + RexBuilder rexBuilder = cluster.getRexBuilder(); + // Fold operands into a single OR(IS_NULL(o0), IS_NULL(o1), ...) predicate. IS_NULL on a + // non-null literal reduces to constant-false, so the OR collapses cleanly through the + // optimizer for cases where some operands are statically non-null. + RexNode anyNull = rexBuilder.makeCall(SqlStdOperatorTable.IS_NULL, operands.get(0)); + for (int i = 1; i < operands.size(); i++) { + anyNull = rexBuilder.makeCall( + SqlStdOperatorTable.OR, + anyNull, + rexBuilder.makeCall(SqlStdOperatorTable.IS_NULL, operands.get(i)) + ); + } + // Result type stays the same as the original CONCAT — nullable VARCHAR. + RexNode nullLiteral = rexBuilder.makeNullLiteral(original.getType()); + return rexBuilder.makeCall(original.getType(), SqlStdOperatorTable.CASE, List.of(anyNull, nullLiteral, original)); + } +} diff --git a/sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DataFusionAnalyticsBackendPlugin.java b/sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DataFusionAnalyticsBackendPlugin.java index 08913c55615a3..9e34579447db2 100644 --- a/sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DataFusionAnalyticsBackendPlugin.java +++ b/sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DataFusionAnalyticsBackendPlugin.java @@ -85,12 +85,17 @@ public class DataFusionAnalyticsBackendPlugin implements AnalyticsSearchBackendP // path). COALESCE is the lowering target of PPL `fillnull`. CAST is required because // ReduceExpressionsRule.ProjectReduceExpressionsRule (in PlannerImpl) constant-folds field // references through equality filters into typed literals — e.g. after `where str0 = 'FURNITURE'`, - // the projection `fields str0` is rewritten to `CAST('FURNITURE' AS VARCHAR)`. The remaining - // comparison / arithmetic / logical operators are project-capable for eval-style projections. + // the projection `fields str0` is rewritten to `CAST('FURNITURE' AS VARCHAR)`. CONCAT is the + // lowering target of PPL `eval`'s `+` for strings (Calcite emits `||`, resolved to CONCAT in + // ScalarFunction); SAFE_CAST covers PPL `eval`'s explicit nullable `CAST(... AS ...)` + // expressions. The remaining comparison / arithmetic / logical operators are project-capable + // for eval-style projections. private static final Set STANDARD_PROJECT_OPS = Set.of( ScalarFunction.COALESCE, ScalarFunction.CEIL, ScalarFunction.CAST, + ScalarFunction.CONCAT, + ScalarFunction.SAFE_CAST, ScalarFunction.SARG_PREDICATE, ScalarFunction.EQUALS, ScalarFunction.NOT_EQUALS, @@ -180,15 +185,19 @@ public Set aggregateCapabilities() { @Override public Map scalarFunctionAdapters() { + // Add new (ScalarFunction, ScalarFunctionAdapter) pairs in alphabetical order for + // readability — the Map.ofEntries form keeps spotless happy past the 5-pair point + // where Map.of becomes single-line and unreadable. return Map.ofEntries( - Map.entry(ScalarFunction.TIMESTAMP, new TimestampFunctionAdapter()), - Map.entry(ScalarFunction.SARG_PREDICATE, new SargAdapter()), + Map.entry(ScalarFunction.CONCAT, new ConcatFunctionAdapter()), + Map.entry(ScalarFunction.CONVERT_TZ, new ConvertTzAdapter()), Map.entry(ScalarFunction.DIVIDE, new StdOperatorRewriteAdapter("DIVIDE", SqlStdOperatorTable.DIVIDE)), - Map.entry(ScalarFunction.MOD, new StdOperatorRewriteAdapter("MOD", SqlStdOperatorTable.MOD)), Map.entry(ScalarFunction.LIKE, new LikeAdapter()), - Map.entry(ScalarFunction.YEAR, new YearAdapter()), - Map.entry(ScalarFunction.CONVERT_TZ, new ConvertTzAdapter()), - Map.entry(ScalarFunction.UNIX_TIMESTAMP, new UnixTimestampAdapter()) + Map.entry(ScalarFunction.MOD, new StdOperatorRewriteAdapter("MOD", SqlStdOperatorTable.MOD)), + Map.entry(ScalarFunction.SARG_PREDICATE, new SargAdapter()), + Map.entry(ScalarFunction.TIMESTAMP, new TimestampFunctionAdapter()), + Map.entry(ScalarFunction.UNIX_TIMESTAMP, new UnixTimestampAdapter()), + Map.entry(ScalarFunction.YEAR, new YearAdapter()) ); } }; diff --git a/sandbox/plugins/analytics-backend-datafusion/src/test/java/org/opensearch/be/datafusion/ConcatFunctionAdapterTests.java b/sandbox/plugins/analytics-backend-datafusion/src/test/java/org/opensearch/be/datafusion/ConcatFunctionAdapterTests.java new file mode 100644 index 0000000000000..e8123a3446c14 --- /dev/null +++ b/sandbox/plugins/analytics-backend-datafusion/src/test/java/org/opensearch/be/datafusion/ConcatFunctionAdapterTests.java @@ -0,0 +1,187 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.be.datafusion; + +import org.apache.calcite.jdbc.JavaTypeFactoryImpl; +import org.apache.calcite.plan.RelOptCluster; +import org.apache.calcite.plan.hep.HepPlanner; +import org.apache.calcite.plan.hep.HepProgramBuilder; +import org.apache.calcite.rel.type.RelDataType; +import org.apache.calcite.rel.type.RelDataTypeFactory; +import org.apache.calcite.rex.RexBuilder; +import org.apache.calcite.rex.RexCall; +import org.apache.calcite.rex.RexLiteral; +import org.apache.calcite.rex.RexNode; +import org.apache.calcite.sql.SqlKind; +import org.apache.calcite.sql.fun.SqlLibraryOperators; +import org.apache.calcite.sql.fun.SqlStdOperatorTable; +import org.apache.calcite.sql.type.SqlTypeName; +import org.opensearch.test.OpenSearchTestCase; + +import java.util.List; + +/** + * Unit tests for {@link ConcatFunctionAdapter}. The adapter rewrites Calcite's binary + * {@code ||(a, b)} (a.k.a. {@code SqlStdOperatorTable.CONCAT}) into a null-propagating + * {@code CASE WHEN IS_NULL(a) OR IS_NULL(b) THEN NULL ELSE ||(a, b) END}, restoring + * SQL-standard null semantics that DataFusion's substrait-mapped {@code concat()} + * function deviates from. + * + *

Each test pins one structural invariant of the rewrite — a regression that drops + * the CASE wrapper, mis-orders the IS_NULL operands, or swaps the THEN/ELSE branches + * surfaces here rather than at IT-level row-mismatch failures. + */ +public class ConcatFunctionAdapterTests extends OpenSearchTestCase { + + private RelDataTypeFactory typeFactory; + private RexBuilder rexBuilder; + private RelOptCluster cluster; + private RelDataType varcharType; + + private final ConcatFunctionAdapter adapter = new ConcatFunctionAdapter(); + + @Override + public void setUp() throws Exception { + super.setUp(); + typeFactory = new JavaTypeFactoryImpl(); + rexBuilder = new RexBuilder(typeFactory); + HepPlanner planner = new HepPlanner(new HepProgramBuilder().build()); + cluster = RelOptCluster.create(planner, rexBuilder); + varcharType = typeFactory.createSqlType(SqlTypeName.VARCHAR); + } + + /** Builds {@code ||(field0, field1)} — Calcite's binary string concat operator. */ + private RexCall buildBinaryConcat() { + RexNode field0 = rexBuilder.makeInputRef(varcharType, 0); + RexNode field1 = rexBuilder.makeInputRef(varcharType, 1); + return (RexCall) rexBuilder.makeCall(SqlStdOperatorTable.CONCAT, field0, field1); + } + + /** + * Builds an n-ary {@code CONCAT(field0, field1, field2)} via {@code SqlLibraryOperators.CONCAT_FUNCTION} + * to exercise the multi-operand IS_NULL chain path. The binary {@code ||} only ever appears with + * arity 2 in production, but the adapter's loop handles N — this test guards that path. + */ + private RexCall buildTernaryConcat() { + RexNode field0 = rexBuilder.makeInputRef(varcharType, 0); + RexNode field1 = rexBuilder.makeInputRef(varcharType, 1); + RexNode field2 = rexBuilder.makeInputRef(varcharType, 2); + return (RexCall) rexBuilder.makeCall(SqlLibraryOperators.CONCAT_FUNCTION, field0, field1, field2); + } + + // ── core rewrite shape ────────────────────────────────────────────────── + + public void testAdaptBinaryConcatProducesCaseWrapper() { + RexCall concat = buildBinaryConcat(); + RexNode adapted = adapter.adapt(concat, List.of(), cluster); + + assertTrue("expected RexCall, got " + adapted.getClass().getSimpleName(), adapted instanceof RexCall); + RexCall caseCall = (RexCall) adapted; + assertEquals("rewritten root must be CASE", SqlKind.CASE, caseCall.getKind()); + assertEquals("CASE must have exactly three operands [condition, then, else]", 3, caseCall.getOperands().size()); + } + + public void testAdaptedCaseElseBranchIsOriginalConcat() { + RexCall concat = buildBinaryConcat(); + RexCall caseCall = (RexCall) adapter.adapt(concat, List.of(), cluster); + + // Else branch must be the original RexCall, untouched — by reference, not just equal. + // Substrait conversion downstream relies on seeing the same object the resolver annotated. + assertSame("else branch must be the original CONCAT call", concat, caseCall.getOperands().get(2)); + } + + public void testAdaptedCaseThenBranchIsNullLiteralOfMatchingSqlType() { + RexCall concat = buildBinaryConcat(); + RexCall caseCall = (RexCall) adapter.adapt(concat, List.of(), cluster); + + RexNode thenBranch = caseCall.getOperands().get(1); + assertTrue("then branch must be a literal", thenBranch instanceof RexLiteral); + RexLiteral literal = (RexLiteral) thenBranch; + assertNull("then branch literal must be NULL-valued", literal.getValue()); + // RexBuilder.makeNullLiteral promotes nullability on the literal's type even when the + // original isn't nullable, so the full RelDataType objects differ. The SQL type name + // (VARCHAR vs INTEGER vs ...) is the load-bearing invariant — overall CASE return type + // identity to the original is asserted in testAdaptPreservesReturnType. + assertEquals( + "NULL literal SQL type must match the original CONCAT's SQL type", + concat.getType().getSqlTypeName(), + literal.getType().getSqlTypeName() + ); + } + + public void testAdaptedCaseConditionIsOrOfIsNullChecks() { + RexCall concat = buildBinaryConcat(); + RexCall caseCall = (RexCall) adapter.adapt(concat, List.of(), cluster); + + RexNode condition = caseCall.getOperands().get(0); + assertEquals("condition must be OR(IS_NULL(a), IS_NULL(b))", SqlKind.OR, condition.getKind()); + + RexCall orCall = (RexCall) condition; + assertEquals(2, orCall.getOperands().size()); + for (int i = 0; i < orCall.getOperands().size(); i++) { + RexNode disjunct = orCall.getOperands().get(i); + assertEquals("OR operand " + i + " must be IS_NULL", SqlKind.IS_NULL, disjunct.getKind()); + // Each IS_NULL must wrap the corresponding original operand — order matters for the + // null-propagation contract. + assertSame( + "IS_NULL operand " + i + " must reference the original CONCAT operand " + i, + concat.getOperands().get(i), + ((RexCall) disjunct).getOperands().get(0) + ); + } + } + + public void testAdaptPreservesReturnType() { + RexCall concat = buildBinaryConcat(); + RexNode adapted = adapter.adapt(concat, List.of(), cluster); + + assertEquals("CASE return type must equal the original CONCAT return type", concat.getType(), adapted.getType()); + } + + // ── n-ary path ────────────────────────────────────────────────────────── + + public void testAdaptNaryConcatChainsIsNullChecksLeftAssociative() { + RexCall concat = buildTernaryConcat(); + RexCall caseCall = (RexCall) adapter.adapt(concat, List.of(), cluster); + + // Condition shape: OR(OR(IS_NULL(a), IS_NULL(b)), IS_NULL(c)) — left-fold. + RexNode condition = caseCall.getOperands().get(0); + assertEquals(SqlKind.OR, condition.getKind()); + + // Right child is IS_NULL(c) — the most recently appended operand in the fold. + RexCall outerOr = (RexCall) condition; + assertEquals(2, outerOr.getOperands().size()); + RexNode rightChild = outerOr.getOperands().get(1); + assertEquals(SqlKind.IS_NULL, rightChild.getKind()); + assertSame(concat.getOperands().get(2), ((RexCall) rightChild).getOperands().get(0)); + + // Left child is OR(IS_NULL(a), IS_NULL(b)) — the previously folded prefix. + RexNode leftChild = outerOr.getOperands().get(0); + assertEquals(SqlKind.OR, leftChild.getKind()); + RexCall innerOr = (RexCall) leftChild; + assertEquals(SqlKind.IS_NULL, innerOr.getOperands().get(0).getKind()); + assertEquals(SqlKind.IS_NULL, innerOr.getOperands().get(1).getKind()); + assertSame(concat.getOperands().get(0), ((RexCall) innerOr.getOperands().get(0)).getOperands().get(0)); + assertSame(concat.getOperands().get(1), ((RexCall) innerOr.getOperands().get(1)).getOperands().get(0)); + } + + // ── pass-through guard ───────────────────────────────────────────────── + + public void testAdaptSingleOperandConcatPassesThroughUnchanged() { + // Built via the variadic CONCAT_FUNCTION since SqlStdOperatorTable.CONCAT is binary and + // can't represent a single-operand call. The adapter's contract is that a 1-operand call + // is a no-op — concat with one input equals that input, no null handling needed. + RexNode field0 = rexBuilder.makeInputRef(varcharType, 0); + RexCall singleOperand = (RexCall) rexBuilder.makeCall(SqlLibraryOperators.CONCAT_FUNCTION, field0); + + RexNode adapted = adapter.adapt(singleOperand, List.of(), cluster); + + assertSame("single-operand call must pass through unmodified", singleOperand, adapted); + } +} diff --git a/sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/planner/dag/BackendPlanAdapter.java b/sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/planner/dag/BackendPlanAdapter.java index a3bd02fa811dd..e65cff3b8b686 100644 --- a/sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/planner/dag/BackendPlanAdapter.java +++ b/sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/planner/dag/BackendPlanAdapter.java @@ -13,7 +13,6 @@ import org.apache.calcite.rel.RelNode; import org.apache.calcite.rex.RexCall; import org.apache.calcite.rex.RexNode; -import org.apache.calcite.sql.SqlFunction; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.opensearch.analytics.planner.CapabilityRegistry; @@ -196,13 +195,6 @@ private static RexNode adaptRex( } private static ScalarFunction resolveFunction(RexCall call) { - if (call.getOperator() instanceof SqlFunction sqlFunction) { - try { - return ScalarFunction.fromSqlFunction(sqlFunction); - } catch (IllegalArgumentException ignored) { - // Not in our enum — fall through to SqlKind resolution - } - } - return ScalarFunction.fromSqlKind(call.getKind()); + return ScalarFunction.fromSqlOperatorWithFallback(call.getOperator()); } } diff --git a/sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/planner/rules/OpenSearchFilterRule.java b/sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/planner/rules/OpenSearchFilterRule.java index c50e643282821..379240c44ee81 100644 --- a/sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/planner/rules/OpenSearchFilterRule.java +++ b/sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/planner/rules/OpenSearchFilterRule.java @@ -15,7 +15,6 @@ import org.apache.calcite.rex.RexCall; import org.apache.calcite.rex.RexInputRef; import org.apache.calcite.rex.RexNode; -import org.apache.calcite.sql.SqlFunction; import org.apache.calcite.sql.SqlKind; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -153,15 +152,11 @@ private List resolveViableBackends( ); } - ScalarFunction function = null; - if (predicate.getOperator() instanceof SqlFunction sqlFunction) { - function = ScalarFunction.fromSqlFunction(sqlFunction); - } - if (function == null) { - function = ScalarFunction.fromSqlKind(predicate.getKind()); - } + ScalarFunction function = ScalarFunction.fromSqlOperatorWithFallback(predicate.getOperator()); if (function == null) { - throw new IllegalStateException("Unrecognized filter operator [" + predicate.getKind() + "]"); + throw new IllegalStateException( + "Unrecognized filter operator [" + predicate.getOperator().getName() + " / " + predicate.getKind() + "]" + ); } Set viableSet = new HashSet<>(registry.filterCapableBackends()); diff --git a/sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/planner/rules/OpenSearchProjectRule.java b/sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/planner/rules/OpenSearchProjectRule.java index 8d98096e21557..711bb3a5c8e1b 100644 --- a/sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/planner/rules/OpenSearchProjectRule.java +++ b/sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/planner/rules/OpenSearchProjectRule.java @@ -119,9 +119,9 @@ private RexNode annotateExpr(RexNode expr, List childViableBackends) { // Standard scalar function List scalarViable = resolveScalarViableBackends(rexCall, childViableBackends); if (scalarViable.isEmpty()) { - throw new IllegalStateException( - "No backend supports scalar function [" + ScalarFunction.fromSqlKind(rexCall.getKind()) + "] among " + childViableBackends - ); + ScalarFunction resolved = ScalarFunction.fromSqlOperatorWithFallback(rexCall.getOperator()); + String label = resolved != null ? resolved.name() : rexCall.getOperator().getName(); + throw new IllegalStateException("No backend supports scalar function [" + label + "] among " + childViableBackends); } // Recurse into operands @@ -158,10 +158,7 @@ private List resolveOpaqueViableBackends(String funcName, List c } private List resolveScalarViableBackends(RexCall rexCall, List childViableBackends) { - ScalarFunction scalarFunc = ScalarFunction.fromSqlKind(rexCall.getKind()); - if (scalarFunc == null && rexCall.getOperator() instanceof SqlFunction sqlFunction) { - scalarFunc = ScalarFunction.fromSqlFunction(sqlFunction); - } + ScalarFunction scalarFunc = ScalarFunction.fromSqlOperatorWithFallback(rexCall.getOperator()); if (scalarFunc == null) { return List.of(); } diff --git a/sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/EvalCommandIT.java b/sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/EvalCommandIT.java new file mode 100644 index 0000000000000..285f3a771df89 --- /dev/null +++ b/sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/EvalCommandIT.java @@ -0,0 +1,224 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.analytics.qa; + +import org.opensearch.client.Request; +import org.opensearch.client.Response; + +import java.io.IOException; +import java.util.Arrays; +import java.util.List; +import java.util.Map; + +/** + * Self-contained integration test for PPL {@code eval} on the analytics-engine route. + * + *

Mirrors {@code CalciteEvalCommandIT} from the {@code opensearch-project/sql} + * repository so that the analytics-engine path can be verified inside core without + * cross-plugin dependencies on the SQL plugin. Each test sends a PPL query through + * {@code POST /_analytics/ppl} (exposed by the {@code test-ppl-frontend} plugin), + * which runs the same {@code UnifiedQueryPlanner} → {@code CalciteRelNodeVisitor} → + * Substrait → DataFusion pipeline as the SQL plugin's force-routed analytics path. + * + *

The eval surface this test exercises is string concatenation via PPL's {@code +} + * operator (lowered to Calcite's {@code SqlStdOperatorTable.CONCAT}, i.e. the {@code ||} + * binary operator) and {@code CAST(... AS STRING)}, both routed through the + * {@link org.opensearch.analytics.spi.ScalarFunction#CONCAT} and + * {@link org.opensearch.analytics.spi.ScalarFunction#CAST} entries in the DataFusion + * backend's {@code STANDARD_PROJECT_OPS}. {@code ||} resolves through the symbolic-name + * branch of {@link org.opensearch.analytics.spi.ScalarFunction#fromSqlOperatorWithFallback} since it + * is a {@code SqlBinaryOperator} (not a {@code SqlFunction}) with {@code SqlKind.OTHER}. + * + *

Provisions the {@code calcs} dataset (parquet-backed) once per class via + * {@link DatasetProvisioner}; {@link AnalyticsRestTestCase#preserveIndicesUponCompletion()} + * keeps it across test methods. + */ +public class EvalCommandIT extends AnalyticsRestTestCase { + + private static final Dataset DATASET = new Dataset("calcs", "calcs"); + + private static boolean dataProvisioned = false; + + /** + * Lazily provision the calcs dataset on first invocation. Must be called inside a test + * method (not {@code setUp()}) — {@link org.opensearch.test.rest.OpenSearchRestTestCase}'s + * static {@code client()} is not initialized until after {@code @BeforeClass}, but is + * reliably available inside test bodies. + */ + private void ensureDataProvisioned() throws IOException { + if (dataProvisioned == false) { + DatasetProvisioner.provision(client(), DATASET); + dataProvisioned = true; + } + } + + // ── string concat: 'literal' + str_field ────────────────────────────────── + + public void testEvalStringConcatLiteralPlusField() throws IOException { + // 'Hello ' + str2 — Calcite emits || (CONCAT). Null str2 propagates through CONCAT, + // producing a null greeting (e.g. row index 3 has str2 = null → greeting = null). + assertRows( + "source=" + DATASET.indexName + " | fields str2 | eval greeting = 'Hello ' + str2", + row("one", "Hello one"), + row("two", "Hello two"), + row("three", "Hello three"), + row(null, null), + row("five", "Hello five"), + row("six", "Hello six"), + row(null, null), + row("eight", "Hello eight"), + row("nine", "Hello nine"), + row("ten", "Hello ten"), + row("eleven", "Hello eleven"), + row("twelve", "Hello twelve"), + row(null, null), + row("fourteen", "Hello fourteen"), + row("fifteen", "Hello fifteen"), + row("sixteen", "Hello sixteen"), + row(null, null) + ); + } + + // ── CAST + concat: 'literal' + CAST(int AS STRING) ──────────────────────── + + public void testEvalStringConcatWithCastIntField() throws IOException { + // CAST(null AS STRING) is null; concat with null propagates → label is null. + // int0 has nulls at rows 1, 2, 3, 7, 8, 12 (per FillNullCommandIT row data). + assertRows( + "source=" + DATASET.indexName + " | eval label = 'Int: ' + CAST(int0 AS STRING) | fields str2, int0, label", + row("one", 1, "Int: 1"), + row("two", null, null), + row("three", null, null), + row(null, null, null), + row("five", 7, "Int: 7"), + row("six", 3, "Int: 3"), + row(null, 8, "Int: 8"), + row("eight", null, null), + row("nine", null, null), + row("ten", 8, "Int: 8"), + row("eleven", 4, "Int: 4"), + row("twelve", 10, "Int: 10"), + row(null, null, null), + row("fourteen", 4, "Int: 4"), + row("fifteen", 11, "Int: 11"), + row("sixteen", 4, "Int: 4"), + row(null, 8, "Int: 8") + ); + } + + // ── chained concat: 'a' + str + 'b' + str' ──────────────────────────────── + + public void testEvalStringConcatMultipleLiteralsAndFields() throws IOException { + // Chains four CONCAT calls — exercises the recursive AnnotatedProjectExpression strip + // for nested project calls (same pattern that fillnull surfaced for ceil(num1)). + // str0 ("FURNITURE"-style) is non-null in calcs; str2 has nulls — null str2 + // propagates through the chain to make the whole row's full_label null. + assertRows( + "source=" + DATASET.indexName + " | eval full_label = 'A=' + str0 + ', B=' + str2 | fields str0, str2, full_label", + row("FURNITURE", "one", "A=FURNITURE, B=one"), + row("FURNITURE", "two", "A=FURNITURE, B=two"), + row("OFFICE SUPPLIES", "three", "A=OFFICE SUPPLIES, B=three"), + row("OFFICE SUPPLIES", null, null), + row("OFFICE SUPPLIES", "five", "A=OFFICE SUPPLIES, B=five"), + row("OFFICE SUPPLIES", "six", "A=OFFICE SUPPLIES, B=six"), + row("OFFICE SUPPLIES", null, null), + row("OFFICE SUPPLIES", "eight", "A=OFFICE SUPPLIES, B=eight"), + row("TECHNOLOGY", "nine", "A=TECHNOLOGY, B=nine"), + row("TECHNOLOGY", "ten", "A=TECHNOLOGY, B=ten"), + row("TECHNOLOGY", "eleven", "A=TECHNOLOGY, B=eleven"), + row("TECHNOLOGY", "twelve", "A=TECHNOLOGY, B=twelve"), + row("TECHNOLOGY", null, null), + row("TECHNOLOGY", "fourteen", "A=TECHNOLOGY, B=fourteen"), + row("TECHNOLOGY", "fifteen", "A=TECHNOLOGY, B=fifteen"), + row("TECHNOLOGY", "sixteen", "A=TECHNOLOGY, B=sixteen"), + row("TECHNOLOGY", null, null) + ); + } + + // ── concat between two field references ─────────────────────────────────── + + public void testEvalStringConcatTwoFields() throws IOException { + // Pure field-to-field concat through two || calls (str0 + ' ' + str2). + // No literal-only operands — the planner must accept CONCAT with both + // RexInputRef inputs (hasFieldRef=true path in resolveScalarViableBackends). + assertRows( + "source=" + DATASET.indexName + " | eval combo = str0 + ' ' + str2 | fields str0, str2, combo", + row("FURNITURE", "one", "FURNITURE one"), + row("FURNITURE", "two", "FURNITURE two"), + row("OFFICE SUPPLIES", "three", "OFFICE SUPPLIES three"), + row("OFFICE SUPPLIES", null, null), + row("OFFICE SUPPLIES", "five", "OFFICE SUPPLIES five"), + row("OFFICE SUPPLIES", "six", "OFFICE SUPPLIES six"), + row("OFFICE SUPPLIES", null, null), + row("OFFICE SUPPLIES", "eight", "OFFICE SUPPLIES eight"), + row("TECHNOLOGY", "nine", "TECHNOLOGY nine"), + row("TECHNOLOGY", "ten", "TECHNOLOGY ten"), + row("TECHNOLOGY", "eleven", "TECHNOLOGY eleven"), + row("TECHNOLOGY", "twelve", "TECHNOLOGY twelve"), + row("TECHNOLOGY", null, null), + row("TECHNOLOGY", "fourteen", "TECHNOLOGY fourteen"), + row("TECHNOLOGY", "fifteen", "TECHNOLOGY fifteen"), + row("TECHNOLOGY", "sixteen", "TECHNOLOGY sixteen"), + row("TECHNOLOGY", null, null) + ); + } + + // ── helpers ───────────────────────────────────────────────────────────────── + + private static List row(Object... values) { + return Arrays.asList(values); + } + + @SafeVarargs + @SuppressWarnings("varargs") + private final void assertRows(String ppl, List... expected) throws IOException { + Map response = executePpl(ppl); + @SuppressWarnings("unchecked") + List> actualRows = (List>) response.get("rows"); + assertNotNull("Response missing 'rows' field for query: " + ppl, actualRows); + assertEquals("Row count mismatch for query: " + ppl, expected.length, actualRows.size()); + for (int i = 0; i < expected.length; i++) { + List want = expected[i]; + List got = actualRows.get(i); + assertEquals("Column count mismatch at row " + i + " for query: " + ppl, want.size(), got.size()); + for (int j = 0; j < want.size(); j++) { + assertCellEquals("Cell mismatch at row " + i + ", col " + j + " for query: " + ppl, want.get(j), got.get(j)); + } + } + } + + private Map executePpl(String ppl) throws IOException { + ensureDataProvisioned(); + Request request = new Request("POST", "/_analytics/ppl"); + request.setJsonEntity("{\"query\": \"" + escapeJson(ppl) + "\"}"); + Response response = client().performRequest(request); + return assertOkAndParse(response, "PPL: " + ppl); + } + + /** + * Numeric-tolerant cell comparison — JSON parsing returns {@code Integer}/{@code Long}/{@code Double} + * interchangeably. PPL doesn't preserve the distinction at the API surface, so cross-type numeric + * equality must be measured by {@code double} values rather than {@link Object#equals(Object)}. + */ + private static void assertCellEquals(String message, Object expected, Object actual) { + if (expected == null || actual == null) { + assertEquals(message, expected, actual); + return; + } + if (expected instanceof Number && actual instanceof Number) { + double e = ((Number) expected).doubleValue(); + double a = ((Number) actual).doubleValue(); + if (Double.compare(e, a) != 0) { + fail(message + ": expected <" + expected + "> but was <" + actual + ">"); + } + return; + } + assertEquals(message, expected, actual); + } +}