Enable PPL eval string concat on the analytics-engine route via DataFusion CONCAT/CAST - #21498
Merged
Conversation
Contributor
PR Reviewer Guide 🔍(Review updated until commit eb2121e)Here are some key observations to aid the review process:
|
Contributor
PR Code Suggestions ✨Latest suggestions up to eb2121e Explore these optional code suggestions:
Previous suggestionsSuggestions up to commit e88711f
Suggestions up to commit e56f60d
Suggestions up to commit d6641af
Suggestions up to commit 5a011c9
Suggestions up to commit 701d627
|
Contributor
|
❌ Gradle check result for ac52d59: Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
expani
reviewed
May 5, 2026
Contributor
|
Persistent review updated to latest commit 4b17c0a |
ahkcs
added a commit
to ahkcs/OpenSearch
that referenced
this pull request
May 5, 2026
…ch-project#21498 Three feedback items in one commit: 1. Co-locate symbolic operator name with the enum constant. The static SYMBOLIC_OPERATOR_NAMES map duplicated a property that belongs on the enum itself. Moved to a nullable `symbolicOperatorName` field on each ScalarFunction constant — currently set only on CONCAT ("||"). The reverse-index map is now built from the enum at class-init time, so adding a new symbolic operator is a single-site edit on the constant rather than a separate map entry. 2. Inline the OR/IS_NULL fold in ConcatFunctionAdapter. Drop the temporary List<RexNode> nullChecks and accumulate the OR-of-IS_NULLs directly in the loop body. Same generated tree, fewer allocations, less to read. 3. Note the Map.of single-line constraint on scalarFunctionAdapters. Per-pair formatting is rejected by spotless; left a comment pointing future contributors at alphabetical ordering instead, and reordered the entries (CONCAT before TIMESTAMP) to make the convention concrete. No behavioral change. CalciteEvalCommandIT 4/4 still passes against the analytics-engine route; sandbox per-module check (excluding the unrelated commons-text dependencyLicenses task) remains green. Signed-off-by: Kai Huang <ahkcs@amazon.com>
Contributor
|
Persistent review updated to latest commit 701d627 |
Contributor
|
Persistent review updated to latest commit 5a011c9 |
Contributor
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #21498 +/- ##
============================================
+ Coverage 73.35% 73.41% +0.05%
- Complexity 74365 74400 +35
============================================
Files 5970 5970
Lines 338261 338261
Branches 48752 48752
============================================
+ Hits 248133 248325 +192
+ Misses 70310 70147 -163
+ Partials 19818 19789 -29 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
expani
approved these changes
May 5, 2026
5 tasks
ahkcs
added a commit
to ahkcs/OpenSearch
that referenced
this pull request
May 5, 2026
…earch-project#21498 Two feedback items in one commit: 1. Drop the redundant Map.copyOf on BY_REFERENCE_OPERATOR. The HashMap built in the static initializer is private static final and is only read via the resolver's get() — never returned, never iterated. The immutability wrapper added an allocation without conferring any external safety guarantee. Comment explains the reasoning so future readers don't reintroduce the wrap. 2. Add ConcatFunctionAdapterTests with seven structural assertions on the CASE rewrite contract: - testAdaptBinaryConcatProducesCaseWrapper: rewritten root is a three-operand CASE (condition, then, else). - testAdaptedCaseElseBranchIsOriginalConcat: else branch is the original RexCall by reference (assertSame, not assertEquals) — downstream substrait conversion expects the same object the resolver annotated. - testAdaptedCaseThenBranchIsNullLiteralOfMatchingSqlType: then branch is a NULL literal whose SQL type name matches the original CONCAT's. Comment explains why we compare type name rather than full RelDataType (RexBuilder.makeNullLiteral promotes nullability, so the full types differ harmlessly). - testAdaptedCaseConditionIsOrOfIsNullChecks: condition is OR with each disjunct an IS_NULL wrapping the corresponding original operand at matching index — null-propagation contract is per operand. - testAdaptPreservesReturnType: full RelDataType identity between adapted CASE and original CONCAT — locks the type-preserving argument of rexBuilder.makeCall(originalType, CASE, ...). - testAdaptNaryConcatChainsIsNullChecksLeftAssociative: builds a ternary CONCAT via SqlLibraryOperators.CONCAT_FUNCTION and verifies the left-fold structure OR(OR(IS_NULL(a), IS_NULL(b)), IS_NULL(c)) — the binary `||` only ever appears with arity 2 in production, but the loop's correctness for arbitrary N is now a test invariant. - testAdaptSingleOperandConcatPassesThroughUnchanged: 1-operand call returns input by reference; documents the early-out branch. Each test pins one structural property in isolation, so a regression that drops any one piece of the contract surfaces with a focused failure rather than at IT-level row-mismatch noise. Signed-off-by: Kai Huang <ahkcs@amazon.com>
Contributor
|
Persistent review updated to latest commit d6641af |
Contributor
|
❕ Gradle check result for d6641af: UNSTABLE Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure. |
Contributor
|
Persistent review updated to latest commit e56f60d |
Contributor
|
❕ Gradle check result for e56f60d: UNSTABLE Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure. |
mch2
approved these changes
May 6, 2026
Member
|
@ahkcs can u pls rebase to pick up sandbox check fix. |
ahkcs
added a commit
to ahkcs/OpenSearch
that referenced
this pull request
May 6, 2026
…ch-project#21498 Three feedback items in one commit: 1. Co-locate symbolic operator name with the enum constant. The static SYMBOLIC_OPERATOR_NAMES map duplicated a property that belongs on the enum itself. Moved to a nullable `symbolicOperatorName` field on each ScalarFunction constant — currently set only on CONCAT ("||"). The reverse-index map is now built from the enum at class-init time, so adding a new symbolic operator is a single-site edit on the constant rather than a separate map entry. 2. Inline the OR/IS_NULL fold in ConcatFunctionAdapter. Drop the temporary List<RexNode> nullChecks and accumulate the OR-of-IS_NULLs directly in the loop body. Same generated tree, fewer allocations, less to read. 3. Note the Map.of single-line constraint on scalarFunctionAdapters. Per-pair formatting is rejected by spotless; left a comment pointing future contributors at alphabetical ordering instead, and reordered the entries (CONCAT before TIMESTAMP) to make the convention concrete. No behavioral change. CalciteEvalCommandIT 4/4 still passes against the analytics-engine route; sandbox per-module check (excluding the unrelated commons-text dependencyLicenses task) remains green. Signed-off-by: Kai Huang <ahkcs@amazon.com>
ahkcs
added a commit
to ahkcs/OpenSearch
that referenced
this pull request
May 6, 2026
…earch-project#21498 Two feedback items in one commit: 1. Drop the redundant Map.copyOf on BY_REFERENCE_OPERATOR. The HashMap built in the static initializer is private static final and is only read via the resolver's get() — never returned, never iterated. The immutability wrapper added an allocation without conferring any external safety guarantee. Comment explains the reasoning so future readers don't reintroduce the wrap. 2. Add ConcatFunctionAdapterTests with seven structural assertions on the CASE rewrite contract: - testAdaptBinaryConcatProducesCaseWrapper: rewritten root is a three-operand CASE (condition, then, else). - testAdaptedCaseElseBranchIsOriginalConcat: else branch is the original RexCall by reference (assertSame, not assertEquals) — downstream substrait conversion expects the same object the resolver annotated. - testAdaptedCaseThenBranchIsNullLiteralOfMatchingSqlType: then branch is a NULL literal whose SQL type name matches the original CONCAT's. Comment explains why we compare type name rather than full RelDataType (RexBuilder.makeNullLiteral promotes nullability, so the full types differ harmlessly). - testAdaptedCaseConditionIsOrOfIsNullChecks: condition is OR with each disjunct an IS_NULL wrapping the corresponding original operand at matching index — null-propagation contract is per operand. - testAdaptPreservesReturnType: full RelDataType identity between adapted CASE and original CONCAT — locks the type-preserving argument of rexBuilder.makeCall(originalType, CASE, ...). - testAdaptNaryConcatChainsIsNullChecksLeftAssociative: builds a ternary CONCAT via SqlLibraryOperators.CONCAT_FUNCTION and verifies the left-fold structure OR(OR(IS_NULL(a), IS_NULL(b)), IS_NULL(c)) — the binary `||` only ever appears with arity 2 in production, but the loop's correctness for arbitrary N is now a test invariant. - testAdaptSingleOperandConcatPassesThroughUnchanged: 1-operand call returns input by reference; documents the early-out branch. Each test pins one structural property in isolation, so a regression that drops any one piece of the contract surfaces with a focused failure rather than at IT-level row-mismatch noise. Signed-off-by: Kai Huang <ahkcs@amazon.com>
Contributor
|
Persistent review updated to latest commit e88711f |
Contributor
Author
Rebased |
Contributor
|
❌ Gradle check result for e88711f: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Contributor
|
❌ Gradle check result for e88711f: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Calcite emits SqlBinaryOperators (e.g. `||`, the lowering target of PPL string `+`) with SqlKind.OTHER and a non-identifier name. The existing ScalarFunction.fromSqlKind / fromSqlFunction pair fails to resolve these: fromSqlKind misses (OTHER is shared), fromSqlFunction throws because `||` is not a SqlFunction (it's a SqlBinaryOperator). The planner-side fallout is "No backend supports scalar function [null] among [datafusion]" with no useful name in the error. Introduce ScalarFunction.fromSqlOperator(SqlOperator) — the unified entry point used by OpenSearchProjectRule, OpenSearchFilterRule, and BackendPlanAdapter in subsequent commits. Resolution order: 1. SqlKind via fromSqlKind (covers PLUS, CAST, COALESCE, etc.) 2. Symbolic-name lookup (handles `||` -> CONCAT) 3. Identifier-name valueOf fallback (covers UPPER, LOWER, etc.) The symbolic-name table currently has one entry (`||` -> CONCAT) but is the documented extension point for future SqlBinaryOperators with non- identifier names. Also adds SAFE_CAST as a sibling enum constant to CAST. PPL emits explicit `CAST(... AS ...)` lowered to Calcite's SqlKind.SAFE_CAST when the source value may be NULL or the conversion may fail. SAFE_CAST and CAST share the same backend semantics (DataFusion's native cast already returns NULL on conversion failure) but resolve through distinct SqlKinds, so they need distinct enum entries. Unit test pins all three resolution branches plus the unknown-operator return-null contract — a regression that drops a branch surfaces here rather than as an opaque "[null]" IT failure. Signed-off-by: Kai Huang <ahkcs@amazon.com>
Three call sites resolved a RexCall's operator using the same two-step pattern (SqlKind first, SqlFunction-cast second) and all three failed identically on `||` (a SqlBinaryOperator with SqlKind.OTHER): - OpenSearchProjectRule.resolveScalarViableBackends - OpenSearchFilterRule (predicate operator resolution) - BackendPlanAdapter.resolveFunction (per-function adapter dispatch) Migrate all three to ScalarFunction.fromSqlOperator, the unified resolver added in the previous commit. Behavior for previously-resolved operators is unchanged — fromSqlOperator delegates to fromSqlKind first, so anything that resolved through SqlKind continues to. New behavior: `||` now resolves to CONCAT, and unrecognized operators return null (catching the IllegalArgumentException that fromSqlFunction's valueOf threw before; the call sites already handled null and now produce a better-formed error message that includes the operator name). Also drop the unused SqlFunction import in OpenSearchFilterRule and BackendPlanAdapter, and tighten the OpenSearchProjectRule error message to fall back to operator.getName() when the resolver returns null — "[null]" was unactionable for triage; "[||]" or "[<unknown_name>]" points directly at the missing capability. Signed-off-by: Kai Huang <ahkcs@amazon.com>
…null adapter
Three new ScalarFunctions in STANDARD_PROJECT_OPS:
- CONCAT — lowering target of PPL `eval`'s `+` for strings (Calcite
emits `||`, resolved to CONCAT through the symbolic-name
branch of ScalarFunction.fromSqlOperator)
- CAST — covers PPL's explicit `CAST(... AS ...)` over non-null
source types (Calcite emits SqlKind.CAST)
- SAFE_CAST — same surface, but emitted by Calcite when the source
value is nullable (SqlKind.SAFE_CAST)
CONCAT additionally needs a ScalarFunctionAdapter to preserve null
semantics. Calcite's `||` follows the SQL standard: if any operand is
NULL, the result is NULL. Substrait's default `concat` extension is
documented with the same semantics, but DataFusion's substrait reader
maps it to the DataFusion `concat()` function — which deviates from the
standard and treats NULL operands as empty strings. PPL queries like
`'Age: ' + CAST(null AS STRING)` expect NULL, not 'Age: '.
ConcatFunctionAdapter rewrites `||(a, b, ...)` into
CASE WHEN a IS NULL OR b IS NULL OR ... THEN NULL ELSE ||(a, b, ...) END
The inner `||` survives unchanged and serializes through the same
Substrait conversion path; the surrounding CASE/IS_NULL short-circuits
the DataFusion `concat()` call whenever any operand is NULL, restoring
SQL-standard null propagation without a custom DataFusion UDF.
Trade-off: the rewrite double-evaluates each operand (once in IS_NULL,
once in the inner `||`). For RexInputRef and RexLiteral operands —
the only shapes PPL emits today for string concat — this is free; for
nested calls the cost is proportional to operand count, not operand
depth, since each `||` adapter wraps one CASE around its direct call.
A custom null-propagating concat UDF (Bucket-3 work in
sandbox/plugins/analytics-backend-datafusion/rust) is the alternative
but disproportionate for a Bucket-1 surface.
Signed-off-by: Kai Huang <ahkcs@amazon.com>
Self-contained integration test for PPL `eval` on the analytics-engine
route. Mirrors CalciteEvalCommandIT in opensearch-project/sql so the
analytics-engine path can be verified inside core without cross-plugin
dependencies on the SQL plugin. Each test sends a PPL query through
POST /_analytics/ppl (exposed by test-ppl-frontend) which runs the
same UnifiedQueryPlanner -> CalciteRelNodeVisitor -> Substrait ->
DataFusion pipeline as the SQL plugin's force-routed analytics path.
Four tests on the calcs dataset cover the eval surface this PR enables:
- testEvalStringConcatLiteralPlusField — `'literal' + str_field`
exercises the symbolic-name resolution for `||` and the CONCAT
capability; null str field rows assert null propagation through
the CASE adapter.
- testEvalStringConcatWithCastIntField — `'literal' + CAST(int AS STRING)`
exercises both CAST/SAFE_CAST and CONCAT in the same projection;
null int rows confirm CAST(NULL) -> NULL propagates through the
surrounding concat.
- testEvalStringConcatMultipleLiteralsAndFields — chained four-arg
concat exercises the recursive AnnotatedProjectExpression strip
for nested project calls.
- testEvalStringConcatTwoFields — pure field-to-field concat with
no literal operands; planner takes the hasFieldRef=true path in
resolveScalarViableBackends.
Reuses the existing calcs dataset (no new fixtures). Once this lands,
the SQL-plugin's CalciteEvalCommandIT is verification-only — this QA
IT is the source of truth for the analytics-engine path.
Signed-off-by: Kai Huang <ahkcs@amazon.com>
…llback Per @expani's PR feedback: the method walks three resolution paths (SqlKind, symbolic-name table, identifier-name valueOf) before returning null, so the name should advertise the fallback behavior at the call site rather than only in the javadoc. Mechanical rename across all callers — `ScalarFunction.fromSqlOperator` -> `ScalarFunction.fromSqlOperatorWithFallback` in: - the resolver itself plus its 7 unit tests - OpenSearchProjectRule (2 call sites) - OpenSearchFilterRule (1 call site) - BackendPlanAdapter.resolveFunction (1 call site) - EvalCommandIT javadoc cross-reference No behavioral change. Signed-off-by: Kai Huang <ahkcs@amazon.com>
…ch-project#21498 Three feedback items in one commit: 1. Co-locate symbolic operator name with the enum constant. The static SYMBOLIC_OPERATOR_NAMES map duplicated a property that belongs on the enum itself. Moved to a nullable `symbolicOperatorName` field on each ScalarFunction constant — currently set only on CONCAT ("||"). The reverse-index map is now built from the enum at class-init time, so adding a new symbolic operator is a single-site edit on the constant rather than a separate map entry. 2. Inline the OR/IS_NULL fold in ConcatFunctionAdapter. Drop the temporary List<RexNode> nullChecks and accumulate the OR-of-IS_NULLs directly in the loop body. Same generated tree, fewer allocations, less to read. 3. Note the Map.of single-line constraint on scalarFunctionAdapters. Per-pair formatting is rejected by spotless; left a comment pointing future contributors at alphabetical ordering instead, and reordered the entries (CONCAT before TIMESTAMP) to make the convention concrete. No behavioral change. CalciteEvalCommandIT 4/4 still passes against the analytics-engine route; sandbox per-module check (excluding the unrelated commons-text dependencyLicenses task) remains green. Signed-off-by: Kai Huang <ahkcs@amazon.com>
…reference Per @expani's PR follow-up: the symbolic-name string ("||") was a runtime-coupled identifier that could silently drift if Calcite renamed the operator. Replace it with a direct reference to the Calcite operator constant (SqlStdOperatorTable.CONCAT), so the link is enforced at compile time and a Calcite-side rename surfaces as a build failure here. - String symbolicOperatorName -> SqlOperator referenceOperator on the enum constructor. - CONCAT now points at SqlStdOperatorTable.CONCAT instead of "||". - Reverse index switches from Map<String, ScalarFunction> keyed by operator name to Map<SqlOperator, ScalarFunction> keyed by operator identity. Calcite's standard operators are singletons, so identity lookup is exact. - Unit test renamed (testFromSqlOperatorResolvesPipeConcatViaReferenceOperator) and its comment updated; the assertions on `getName()` / `getKind()` are kept as documentation of WHY this branch is needed at all. No behavioral change in the resolution logic — same three-step chain (SqlKind, then this branch, then identifier-name valueOf), with the middle branch now identity-comparing rather than name-comparing. CalciteEvalCommandIT 4/4 still passes; ScalarFunctionTests 7/7. Signed-off-by: Kai Huang <ahkcs@amazon.com>
…earch-project#21498 Two feedback items in one commit: 1. Drop the redundant Map.copyOf on BY_REFERENCE_OPERATOR. The HashMap built in the static initializer is private static final and is only read via the resolver's get() — never returned, never iterated. The immutability wrapper added an allocation without conferring any external safety guarantee. Comment explains the reasoning so future readers don't reintroduce the wrap. 2. Add ConcatFunctionAdapterTests with seven structural assertions on the CASE rewrite contract: - testAdaptBinaryConcatProducesCaseWrapper: rewritten root is a three-operand CASE (condition, then, else). - testAdaptedCaseElseBranchIsOriginalConcat: else branch is the original RexCall by reference (assertSame, not assertEquals) — downstream substrait conversion expects the same object the resolver annotated. - testAdaptedCaseThenBranchIsNullLiteralOfMatchingSqlType: then branch is a NULL literal whose SQL type name matches the original CONCAT's. Comment explains why we compare type name rather than full RelDataType (RexBuilder.makeNullLiteral promotes nullability, so the full types differ harmlessly). - testAdaptedCaseConditionIsOrOfIsNullChecks: condition is OR with each disjunct an IS_NULL wrapping the corresponding original operand at matching index — null-propagation contract is per operand. - testAdaptPreservesReturnType: full RelDataType identity between adapted CASE and original CONCAT — locks the type-preserving argument of rexBuilder.makeCall(originalType, CASE, ...). - testAdaptNaryConcatChainsIsNullChecksLeftAssociative: builds a ternary CONCAT via SqlLibraryOperators.CONCAT_FUNCTION and verifies the left-fold structure OR(OR(IS_NULL(a), IS_NULL(b)), IS_NULL(c)) — the binary `||` only ever appears with arity 2 in production, but the loop's correctness for arbitrary N is now a test invariant. - testAdaptSingleOperandConcatPassesThroughUnchanged: 1-operand call returns input by reference; documents the early-out branch. Each test pins one structural property in isolation, so a regression that drops any one piece of the contract surfaces with a focused failure rather than at IT-level row-mismatch noise. Signed-off-by: Kai Huang <ahkcs@amazon.com>
Contributor
|
Persistent review updated to latest commit eb2121e |
ahkcs
added a commit
to ahkcs/OpenSearch
that referenced
this pull request
May 6, 2026
PR opensearch-project#21498 ("Enable PPL eval string concat on the analytics-engine route via DataFusion CONCAT/CAST") lands the eval-side capability surface this PR also touched, with a more general fix: * Adds {@code SAFE_CAST} as a {@code SqlKind.SAFE_CAST}-keyed enum constant (not {@code SqlKind.OTHER_FUNCTION} like this PR did — theirs is semantically correct). * Wires {@code CAST}, {@code CONCAT}, {@code SAFE_CAST} into {@code STANDARD_PROJECT_OPS}. * Adds a {@code ConcatFunctionAdapter} that wraps {@code ||} with a CASE/IS_NULL null-propagation guard (the standard-SQL semantic this PR flagged as a follow-up). * Migrates {@code OpenSearchProjectRule}, {@code OpenSearchFilterRule}, and {@code BackendPlanAdapter} to a new {@code ScalarFunction.fromSqlOperatorWithFallback(SqlOperator)} resolver that handles every symbolic operator (not just {@code ||}) and gracefully swallows the {@code IllegalArgumentException} from unknown names. This commit removes the now-redundant pieces from this PR: * Drop the {@code SAFE_CAST} enum constant. * Drop {@code CAST}, {@code SAFE_CAST}, {@code CONCAT} from {@code STANDARD_PROJECT_OPS}. * Revert the {@code OpenSearchProjectRule} CONCAT special-case + IAE swallow — opensearch-project#21498's {@code fromSqlOperatorWithFallback} migration is the right fix. What stays in this PR (not covered by opensearch-project#21498): * {@code AND} / {@code OR} / {@code NOT} enum constants and their {@code STANDARD_PROJECT_OPS} entries — boolean operators inside CASE predicates of {@code count(eval(a > 1 and b < 2))}-style stats. * {@code IS_NULL} / {@code IS_NOT_NULL} / {@code CASE} / {@code NULLIF} in {@code STANDARD_PROJECT_OPS} — eval/sort-pushdown sub-expressions. * {@code UPPER} / {@code LOWER} / {@code TRIM} / {@code SUBSTRING} / {@code CHAR_LENGTH} / {@code FLOOR} / {@code ABS} in {@code STANDARD_PROJECT_OPS} — sort-pushdown and eval string ops. * {@code STDDEV_POP} / {@code STDDEV_SAMP} / {@code VAR_POP} / {@code VAR_SAMP} in {@code AGG_FUNCTIONS}, plus the {@code aggregateCapabilities()} switch dispatch on {@code AggregateFunction.Type} so a single mixed-category list doesn't trip the {@code AggregateCapability.simple()} assertion. Also bundles {@code commons-text:1.11.0} in {@code analytics-engine}'s zip — Calcite's {@code SqlFunctions.<clinit>} eagerly references {@code org.apache.commons.text.similarity.LevenshteinDistance} (SOUNDEX/JARO_WINKLER), and any agg query that touches {@code SqlFunctions} crashes the cluster with {@code NoClassDefFoundError} otherwise. The {@code resolutionStrategy.force} pin in this build.gradle pins the version but doesn't bundle the jar. Signed-off-by: Kai Huang <ahkcs@amazon.com>
This was referenced May 6, 2026
ahkcs
added a commit
to ahkcs/OpenSearch
that referenced
this pull request
May 6, 2026
…r surface Bucket-1 capability-registry expansion for the analytics-engine route. Pairs with opensearch-project#21498 (eval string concat / CAST / SAFE_CAST / `||`-resolver / ConcatFunctionAdapter) — independent surfaces, no overlap. After this PR, `CalciteFieldsCommandIT` 39/39, `CalciteRenameCommandIT` 2/2, `CalciteHeadCommandIT` 4/4 are 100% green under `tests.analytics.force_routing=true`, and the Sort suites pick up the bulk of their cast / abs / substring push-down gains. All changes are Bucket-1 in the routing-doc taxonomy: the DataFusion runtime already implements every operator listed; this PR just declares the capability so the Layer-2 planner stops rejecting the calls. Three additive layers: 1. ScalarFunction — add AND, OR, NOT enum constants. The filter rule structurally recurses into AND/OR/NOT and never looks them up, but the project rule does — these must appear in the enum + STANDARD_PROJECT_OPS for eval predicates like `count(eval(balance > 20000 and age < 35))` where AND is a sub-expression of CASE. 2. DataFusionAnalyticsBackendPlugin — - STANDARD_PROJECT_OPS additions: * FLOOR, ABS — sort-by `abs(balance)` push-down (SortCommandIT.testPushdownSortExpressionContainsNull). * IS_NULL, IS_NOT_NULL — sort-by `isnull(...)` and eval guards. * AND, OR, NOT — boolean ops in CASE predicates. * CASE, NULLIF — conditional projections in eval. * UPPER, LOWER, TRIM, SUBSTRING, CHAR_LENGTH, CONCAT — sort-by `substring(...)` push-down (CalcitePPLSortIT.testPushdownSortStringExpression), eval string transforms. - AGG_FUNCTIONS additions: STDDEV_POP, STDDEV_SAMP, VAR_POP, VAR_SAMP for `stats stddev_samp(...)` / `stats var_samp(...)`. - aggregateCapabilities() now dispatches on AggregateFunction.Type so a single mixed-category list works — the previous unconditional AggregateCapability.simple(...) asserts on non-SIMPLE inputs and crashes plugin init when STDDEV/VAR are added. 3. analytics-engine/build.gradle — bundle commons-text:1.11.0. Calcite's SqlFunctions.<clinit> eagerly references org.apache.commons.text.similarity.LevenshteinDistance (SOUNDEX/JARO_WINKLER); without bundling the jar, the first agg query that touches SqlFunctions kills the cluster with NoClassDefFoundError. The existing resolutionStrategy.force pin pins the version but doesn't bundle. Test plan: * `./gradlew :sandbox:libs:analytics-framework:check :sandbox:plugins:analytics-backend-datafusion:check :sandbox:plugins:analytics-engine:check -Dsandbox.enabled=true` green. * SQL-plugin ITs against this branch (cluster) + companion SQL plugin opensearch-project/sql#5413, with `-Dtests.analytics.force_routing=true -Dtests.analytics.parquet_indices=true`. Routing verified: 654 analytics-engine PlannerImpl entries, 0 v2 PPLService entries. Out of scope (separate follow-ups, surface mostly orthogonal): * `Unable to find binding for call AVG($N)` — Substrait isthmus' default AggregateFunctionConverter rejects Calcite's AVG/STDDEV_SAMP/VAR_SAMP signatures. Needs an AggregateSig-style additional-mappings hook registered in DataFusionFragmentConvertor. Once unblocked, the new STDDEV/VAR entries here will start contributing real test wins. * Window functions — `dedup` lowers to ROW_NUMBER OVER. RexOver reaches the project rule but isn't recognized by ScalarFunction.fromSqlKind. Blocks CalciteDedupCommandIT and CalcitePPLDedupIT. * Advanced aggregates / PPL functions — first, last, take, arg_max, percentile_approx, distinct_count_approx, PPL `span` need new enum constants + DataFusion adapters or YAML extensions. Signed-off-by: Kai Huang <ahkcs@amazon.com>
Member
|
going ahead all sandbox code and sandbox check passes: gc again: |
ahkcs
added a commit
to ahkcs/OpenSearch
that referenced
this pull request
May 6, 2026
…elds/rename/head/sort Bucket-1 capability-registry expansion for the analytics-engine route — narrow scope: only the two scalar functions PPL sort push-down materialises into a projection (ABS, SUBSTRING). Fields / rename / head don't add scalar surface; they're covered here purely by new QA ITs that lock in the routing-and-shape behavior end-to-end through `POST /_analytics/ppl`. After this PR (with the eval-side surface from opensearch-project#21498 already on main): * `CalciteFieldsCommandIT`, `CalciteRenameCommandIT`, `CalciteHeadCommandIT` 100% green on the analytics path under `tests.analytics.force_routing=true`. * `CalciteSortCommandIT` and `CalcitePPLSortIT` pick up the cast / abs / substring push-down failures (CAST is already in `STANDARD_PROJECT_OPS` upstream from opensearch-project#21476; ABS and SUBSTRING are this PR's contribution). ## Changes **1. `DataFusionAnalyticsBackendPlugin` — `STANDARD_PROJECT_OPS` += ABS, SUBSTRING.** PPL sort push-down lifts an expression like `abs(num0)` or `substring(str0, 1, 3)` into a `LogicalProject` child of the sort, which is what the project rule's capability check sees. DataFusion has both natively; isthmus' default extension catalog already binds them. Without this, the analytics planner rejects the projection with `No backend supports scalar function [ABS] among [datafusion]`. **2. QA ITs in `sandbox/qa/analytics-engine-rest`** — one per command, each self-contained and provisioning the existing `calcs` parquet-backed dataset via `DatasetProvisioner`. Tests fire through `POST /_analytics/ppl` so the core build can validate the analytics-engine path without the SQL plugin. Mirror the failing surface in `CalciteFieldsCommandIT` / `CalciteRenameCommandIT` / `CalciteHeadCommandIT` / `CalciteSortCommandIT` one query at a time: * `FieldsCommandIT` (5 tests) — basic projection, single-column, explicit order, suffix-wildcard `*0` (set-equality, since wildcard expansion order isn't part of the contract), and `fields - num*` exclusion. * `RenameCommandIT` (4 tests) — single rename, multi-rename, post-rename reference fails with "not found", backtick-quoted target names. * `HeadCommandIT` (5 tests) — default-10 cap, explicit count, count > total rows, `head N from M` offset, and value-equality on the first 5 rows (parquet preserves insertion order, so this is deterministic). * `SortCommandIT` (5 tests) — plain ASC/DESC by integer (with calcs' 6 null int0 entries placed at the head/tail per Calcite's nulls-first/last defaults), `eval n = abs(num0) | sort n` covering the 9 null + 8 non-null abs values, and `eval s = substring(str2, 1, 3) | sort s` validating the SUBSTRING capability end-to-end against the 17-row calcs dataset. ## Test plan * `./gradlew :sandbox:qa:analytics-engine-rest:integTest -Dsandbox.enabled=true --tests "*FieldsCommandIT" --tests "*RenameCommandIT" --tests "*HeadCommandIT" --tests "*SortCommandIT"` — 19/19 green. * `./gradlew check -p sandbox -Dsandbox.enabled=true` — green (the unrelated `ScalarDateTimeFunctionIT.testConvertTz` flake from a stale local `libopensearch_native.dylib` resolved by rebuilding the Rust crate; not caused by this PR). * SQL-plugin Calcite ITs against this branch + companion opensearch-project/sql#5413, with `-Dtests.analytics.force_routing=true -Dtests.analytics.parquet_indices=true`: `CalciteFieldsCommandIT` 39/39, `CalciteRenameCommandIT` 2/2, `CalciteHeadCommandIT` 4/4, plus +5 sort-push-down wins in `CalciteSortCommandIT` and +1 in `CalcitePPLSortIT` from the ABS / SUBSTRING capability additions. ## Out of scope (separate follow-ups) * `Unable to find binding for call AVG($N)` Substrait-isthmus issue — needs an `AggregateSig`-style additional-mappings hook in `DataFusionFragmentConvertor`. * Window functions (`dedup` lowers to `ROW_NUMBER OVER`). * Advanced aggregates (`first`, `last`, `take`, `arg_max`, `percentile_approx`, `distinct_count_approx`) and PPL `span`. * The eval-predicate surface (`AND`/`OR`/`NOT` in CASE projections, `IS_NULL`/`IS_NOT_NULL`, broader string/conditional ops) and STDDEV/VAR aggregates — kept out of this PR to keep the scope focused on the four commands the QA ITs cover. Signed-off-by: Kai Huang <ahkcs@amazon.com>
mch2
pushed a commit
that referenced
this pull request
May 6, 2026
…elds/rename/head/sort (#21521) Bucket-1 capability-registry expansion for the analytics-engine route — narrow scope: only the two scalar functions PPL sort push-down materialises into a projection (ABS, SUBSTRING). Fields / rename / head don't add scalar surface; they're covered here purely by new QA ITs that lock in the routing-and-shape behavior end-to-end through `POST /_analytics/ppl`. After this PR (with the eval-side surface from #21498 already on main): * `CalciteFieldsCommandIT`, `CalciteRenameCommandIT`, `CalciteHeadCommandIT` 100% green on the analytics path under `tests.analytics.force_routing=true`. * `CalciteSortCommandIT` and `CalcitePPLSortIT` pick up the cast / abs / substring push-down failures (CAST is already in `STANDARD_PROJECT_OPS` upstream from #21476; ABS and SUBSTRING are this PR's contribution). ## Changes **1. `DataFusionAnalyticsBackendPlugin` — `STANDARD_PROJECT_OPS` += ABS, SUBSTRING.** PPL sort push-down lifts an expression like `abs(num0)` or `substring(str0, 1, 3)` into a `LogicalProject` child of the sort, which is what the project rule's capability check sees. DataFusion has both natively; isthmus' default extension catalog already binds them. Without this, the analytics planner rejects the projection with `No backend supports scalar function [ABS] among [datafusion]`. **2. QA ITs in `sandbox/qa/analytics-engine-rest`** — one per command, each self-contained and provisioning the existing `calcs` parquet-backed dataset via `DatasetProvisioner`. Tests fire through `POST /_analytics/ppl` so the core build can validate the analytics-engine path without the SQL plugin. Mirror the failing surface in `CalciteFieldsCommandIT` / `CalciteRenameCommandIT` / `CalciteHeadCommandIT` / `CalciteSortCommandIT` one query at a time: * `FieldsCommandIT` (5 tests) — basic projection, single-column, explicit order, suffix-wildcard `*0` (set-equality, since wildcard expansion order isn't part of the contract), and `fields - num*` exclusion. * `RenameCommandIT` (4 tests) — single rename, multi-rename, post-rename reference fails with "not found", backtick-quoted target names. * `HeadCommandIT` (5 tests) — default-10 cap, explicit count, count > total rows, `head N from M` offset, and value-equality on the first 5 rows (parquet preserves insertion order, so this is deterministic). * `SortCommandIT` (5 tests) — plain ASC/DESC by integer (with calcs' 6 null int0 entries placed at the head/tail per Calcite's nulls-first/last defaults), `eval n = abs(num0) | sort n` covering the 9 null + 8 non-null abs values, and `eval s = substring(str2, 1, 3) | sort s` validating the SUBSTRING capability end-to-end against the 17-row calcs dataset. ## Test plan * `./gradlew :sandbox:qa:analytics-engine-rest:integTest -Dsandbox.enabled=true --tests "*FieldsCommandIT" --tests "*RenameCommandIT" --tests "*HeadCommandIT" --tests "*SortCommandIT"` — 19/19 green. * `./gradlew check -p sandbox -Dsandbox.enabled=true` — green (the unrelated `ScalarDateTimeFunctionIT.testConvertTz` flake from a stale local `libopensearch_native.dylib` resolved by rebuilding the Rust crate; not caused by this PR). * SQL-plugin Calcite ITs against this branch + companion opensearch-project/sql#5413, with `-Dtests.analytics.force_routing=true -Dtests.analytics.parquet_indices=true`: `CalciteFieldsCommandIT` 39/39, `CalciteRenameCommandIT` 2/2, `CalciteHeadCommandIT` 4/4, plus +5 sort-push-down wins in `CalciteSortCommandIT` and +1 in `CalcitePPLSortIT` from the ABS / SUBSTRING capability additions. ## Out of scope (separate follow-ups) * `Unable to find binding for call AVG($N)` Substrait-isthmus issue — needs an `AggregateSig`-style additional-mappings hook in `DataFusionFragmentConvertor`. * Window functions (`dedup` lowers to `ROW_NUMBER OVER`). * Advanced aggregates (`first`, `last`, `take`, `arg_max`, `percentile_approx`, `distinct_count_approx`) and PPL `span`. * The eval-predicate surface (`AND`/`OR`/`NOT` in CASE projections, `IS_NULL`/`IS_NOT_NULL`, broader string/conditional ops) and STDDEV/VAR aggregates — kept out of this PR to keep the scope focused on the four commands the QA ITs cover. Signed-off-by: Kai Huang <ahkcs@amazon.com>
2 tasks
mch2
pushed a commit
that referenced
this pull request
May 7, 2026
) PPL `fieldformat` is a Calcite-only command that lowers to a plain Eval node (see SQL plugin's `AstBuilder.visitFieldformatCommand`). Its expressions go through Calcite's || (CONCAT) operator and CAST, both already wired in the DataFusion backend's STANDARD_PROJECT_OPS via #21498. **No code changes required for the analytics route — this PR is QA-only.** The unique surface vs plain `eval` is the prefix-{`.`} and suffix-{`.`} string-concat sugar emitted by `AstExpressionBuilder.visitFieldFormatEvalClause` for the StringDotlogicalExpression / LogicalExpressionDotString rules: fieldformat x = "prefix".CAST(y AS STRING)." suffix" expands to a chain of CONCAT calls. Both forms route through the existing CONCAT capability — no extension lookup or adapter needed since isthmus' default catalog binds the || operator natively. Four tests against the in-process QA cluster, exercising the analytics path end-to-end via the test-ppl-frontend plugin: | Test | Shape | |---|---| | `testFieldformatPlusConcat` | `'Hello ' + str0` — basic +-concat. | | `testFieldformatPrefixDotCast` | `'Code: '.CAST(int0 AS STRING)` — StringDotlogicalExpression branch. | | `testFieldformatCastDotSuffix` | `CAST(int0 AS STRING).' pts'` — LogicalExpressionDotString branch. | | `testFieldformatPrefixDotCastDotSuffix` | `'Code: '.CAST(int0 AS STRING).' pts'` — combined. | Tests filter `where isnotnull(int0)` before sorting/limiting so the deterministic-row assertions don't flap on the calcs dataset's six null int0 rows (Calcite's default ascending sort puts nulls first). Out of scope: the v2-side `testFieldFormatStringConcatenationWithNullFieldToString` uses `tostring(age, "commas")` — a multi-mode UDF (binary / hex / commas / duration) with substantial Java logic in `ToStringFunction`. Adding it to the analytics path would need either Calcite-level rewrites or a DataFusion Rust UDF; tracked separately. Validates: 4/4 FieldFormatCommandIT pass; full :sandbox:qa:analytics-engine-rest:integTest suite green (**132 tests across 17 ITs**, no regressions). Signed-off-by: Kai Huang <ahkcs@amazon.com>
imRishN
pushed a commit
to imRishN/OpenSearch
that referenced
this pull request
May 8, 2026
…usion CONCAT/CAST (opensearch-project#21498) * [Analytics Framework] Resolve symbolic operators and add SAFE_CAST Calcite emits SqlBinaryOperators (e.g. `||`, the lowering target of PPL string `+`) with SqlKind.OTHER and a non-identifier name. The existing ScalarFunction.fromSqlKind / fromSqlFunction pair fails to resolve these: fromSqlKind misses (OTHER is shared), fromSqlFunction throws because `||` is not a SqlFunction (it's a SqlBinaryOperator). The planner-side fallout is "No backend supports scalar function [null] among [datafusion]" with no useful name in the error. Introduce ScalarFunction.fromSqlOperator(SqlOperator) — the unified entry point used by OpenSearchProjectRule, OpenSearchFilterRule, and BackendPlanAdapter in subsequent commits. Resolution order: 1. SqlKind via fromSqlKind (covers PLUS, CAST, COALESCE, etc.) 2. Symbolic-name lookup (handles `||` -> CONCAT) 3. Identifier-name valueOf fallback (covers UPPER, LOWER, etc.) The symbolic-name table currently has one entry (`||` -> CONCAT) but is the documented extension point for future SqlBinaryOperators with non- identifier names. Also adds SAFE_CAST as a sibling enum constant to CAST. PPL emits explicit `CAST(... AS ...)` lowered to Calcite's SqlKind.SAFE_CAST when the source value may be NULL or the conversion may fail. SAFE_CAST and CAST share the same backend semantics (DataFusion's native cast already returns NULL on conversion failure) but resolve through distinct SqlKinds, so they need distinct enum entries. Unit test pins all three resolution branches plus the unknown-operator return-null contract — a regression that drops a branch surfaces here rather than as an opaque "[null]" IT failure. Signed-off-by: Kai Huang <ahkcs@amazon.com> * [Analytics Engine] Migrate rules and adapter dispatch to fromSqlOperator Three call sites resolved a RexCall's operator using the same two-step pattern (SqlKind first, SqlFunction-cast second) and all three failed identically on `||` (a SqlBinaryOperator with SqlKind.OTHER): - OpenSearchProjectRule.resolveScalarViableBackends - OpenSearchFilterRule (predicate operator resolution) - BackendPlanAdapter.resolveFunction (per-function adapter dispatch) Migrate all three to ScalarFunction.fromSqlOperator, the unified resolver added in the previous commit. Behavior for previously-resolved operators is unchanged — fromSqlOperator delegates to fromSqlKind first, so anything that resolved through SqlKind continues to. New behavior: `||` now resolves to CONCAT, and unrecognized operators return null (catching the IllegalArgumentException that fromSqlFunction's valueOf threw before; the call sites already handled null and now produce a better-formed error message that includes the operator name). Also drop the unused SqlFunction import in OpenSearchFilterRule and BackendPlanAdapter, and tighten the OpenSearchProjectRule error message to fall back to operator.getName() when the resolver returns null — "[null]" was unactionable for triage; "[||]" or "[<unknown_name>]" points directly at the missing capability. Signed-off-by: Kai Huang <ahkcs@amazon.com> * [Analytics Backend / DataFusion] Wire CONCAT/CAST/SAFE_CAST + concat null adapter Three new ScalarFunctions in STANDARD_PROJECT_OPS: - CONCAT — lowering target of PPL `eval`'s `+` for strings (Calcite emits `||`, resolved to CONCAT through the symbolic-name branch of ScalarFunction.fromSqlOperator) - CAST — covers PPL's explicit `CAST(... AS ...)` over non-null source types (Calcite emits SqlKind.CAST) - SAFE_CAST — same surface, but emitted by Calcite when the source value is nullable (SqlKind.SAFE_CAST) CONCAT additionally needs a ScalarFunctionAdapter to preserve null semantics. Calcite's `||` follows the SQL standard: if any operand is NULL, the result is NULL. Substrait's default `concat` extension is documented with the same semantics, but DataFusion's substrait reader maps it to the DataFusion `concat()` function — which deviates from the standard and treats NULL operands as empty strings. PPL queries like `'Age: ' + CAST(null AS STRING)` expect NULL, not 'Age: '. ConcatFunctionAdapter rewrites `||(a, b, ...)` into CASE WHEN a IS NULL OR b IS NULL OR ... THEN NULL ELSE ||(a, b, ...) END The inner `||` survives unchanged and serializes through the same Substrait conversion path; the surrounding CASE/IS_NULL short-circuits the DataFusion `concat()` call whenever any operand is NULL, restoring SQL-standard null propagation without a custom DataFusion UDF. Trade-off: the rewrite double-evaluates each operand (once in IS_NULL, once in the inner `||`). For RexInputRef and RexLiteral operands — the only shapes PPL emits today for string concat — this is free; for nested calls the cost is proportional to operand count, not operand depth, since each `||` adapter wraps one CASE around its direct call. A custom null-propagating concat UDF (Bucket-3 work in sandbox/plugins/analytics-backend-datafusion/rust) is the alternative but disproportionate for a Bucket-1 surface. Signed-off-by: Kai Huang <ahkcs@amazon.com> * [QA] Add EvalCommandIT for the analytics-engine REST path Self-contained integration test for PPL `eval` on the analytics-engine route. Mirrors CalciteEvalCommandIT in opensearch-project/sql so the analytics-engine path can be verified inside core without cross-plugin dependencies on the SQL plugin. Each test sends a PPL query through POST /_analytics/ppl (exposed by test-ppl-frontend) which runs the same UnifiedQueryPlanner -> CalciteRelNodeVisitor -> Substrait -> DataFusion pipeline as the SQL plugin's force-routed analytics path. Four tests on the calcs dataset cover the eval surface this PR enables: - testEvalStringConcatLiteralPlusField — `'literal' + str_field` exercises the symbolic-name resolution for `||` and the CONCAT capability; null str field rows assert null propagation through the CASE adapter. - testEvalStringConcatWithCastIntField — `'literal' + CAST(int AS STRING)` exercises both CAST/SAFE_CAST and CONCAT in the same projection; null int rows confirm CAST(NULL) -> NULL propagates through the surrounding concat. - testEvalStringConcatMultipleLiteralsAndFields — chained four-arg concat exercises the recursive AnnotatedProjectExpression strip for nested project calls. - testEvalStringConcatTwoFields — pure field-to-field concat with no literal operands; planner takes the hasFieldRef=true path in resolveScalarViableBackends. Reuses the existing calcs dataset (no new fixtures). Once this lands, the SQL-plugin's CalciteEvalCommandIT is verification-only — this QA IT is the source of truth for the analytics-engine path. Signed-off-by: Kai Huang <ahkcs@amazon.com> * [Analytics Framework] Rename fromSqlOperator to fromSqlOperatorWithFallback Per @expani's PR feedback: the method walks three resolution paths (SqlKind, symbolic-name table, identifier-name valueOf) before returning null, so the name should advertise the fallback behavior at the call site rather than only in the javadoc. Mechanical rename across all callers — `ScalarFunction.fromSqlOperator` -> `ScalarFunction.fromSqlOperatorWithFallback` in: - the resolver itself plus its 7 unit tests - OpenSearchProjectRule (2 call sites) - OpenSearchFilterRule (1 call site) - BackendPlanAdapter.resolveFunction (1 call site) - EvalCommandIT javadoc cross-reference No behavioral change. Signed-off-by: Kai Huang <ahkcs@amazon.com> * [Analytics Framework + Backend] Address @expani review on PR opensearch-project#21498 Three feedback items in one commit: 1. Co-locate symbolic operator name with the enum constant. The static SYMBOLIC_OPERATOR_NAMES map duplicated a property that belongs on the enum itself. Moved to a nullable `symbolicOperatorName` field on each ScalarFunction constant — currently set only on CONCAT ("||"). The reverse-index map is now built from the enum at class-init time, so adding a new symbolic operator is a single-site edit on the constant rather than a separate map entry. 2. Inline the OR/IS_NULL fold in ConcatFunctionAdapter. Drop the temporary List<RexNode> nullChecks and accumulate the OR-of-IS_NULLs directly in the loop body. Same generated tree, fewer allocations, less to read. 3. Note the Map.of single-line constraint on scalarFunctionAdapters. Per-pair formatting is rejected by spotless; left a comment pointing future contributors at alphabetical ordering instead, and reordered the entries (CONCAT before TIMESTAMP) to make the convention concrete. No behavioral change. CalciteEvalCommandIT 4/4 still passes against the analytics-engine route; sandbox per-module check (excluding the unrelated commons-text dependencyLicenses task) remains green. Signed-off-by: Kai Huang <ahkcs@amazon.com> * [Analytics Framework] Resolve symbolic operators by Calcite-operator reference Per @expani's PR follow-up: the symbolic-name string ("||") was a runtime-coupled identifier that could silently drift if Calcite renamed the operator. Replace it with a direct reference to the Calcite operator constant (SqlStdOperatorTable.CONCAT), so the link is enforced at compile time and a Calcite-side rename surfaces as a build failure here. - String symbolicOperatorName -> SqlOperator referenceOperator on the enum constructor. - CONCAT now points at SqlStdOperatorTable.CONCAT instead of "||". - Reverse index switches from Map<String, ScalarFunction> keyed by operator name to Map<SqlOperator, ScalarFunction> keyed by operator identity. Calcite's standard operators are singletons, so identity lookup is exact. - Unit test renamed (testFromSqlOperatorResolvesPipeConcatViaReferenceOperator) and its comment updated; the assertions on `getName()` / `getKind()` are kept as documentation of WHY this branch is needed at all. No behavioral change in the resolution logic — same three-step chain (SqlKind, then this branch, then identifier-name valueOf), with the middle branch now identity-comparing rather than name-comparing. CalciteEvalCommandIT 4/4 still passes; ScalarFunctionTests 7/7. Signed-off-by: Kai Huang <ahkcs@amazon.com> * [Analytics Framework + Backend] Address @expani follow-up on PR opensearch-project#21498 Two feedback items in one commit: 1. Drop the redundant Map.copyOf on BY_REFERENCE_OPERATOR. The HashMap built in the static initializer is private static final and is only read via the resolver's get() — never returned, never iterated. The immutability wrapper added an allocation without conferring any external safety guarantee. Comment explains the reasoning so future readers don't reintroduce the wrap. 2. Add ConcatFunctionAdapterTests with seven structural assertions on the CASE rewrite contract: - testAdaptBinaryConcatProducesCaseWrapper: rewritten root is a three-operand CASE (condition, then, else). - testAdaptedCaseElseBranchIsOriginalConcat: else branch is the original RexCall by reference (assertSame, not assertEquals) — downstream substrait conversion expects the same object the resolver annotated. - testAdaptedCaseThenBranchIsNullLiteralOfMatchingSqlType: then branch is a NULL literal whose SQL type name matches the original CONCAT's. Comment explains why we compare type name rather than full RelDataType (RexBuilder.makeNullLiteral promotes nullability, so the full types differ harmlessly). - testAdaptedCaseConditionIsOrOfIsNullChecks: condition is OR with each disjunct an IS_NULL wrapping the corresponding original operand at matching index — null-propagation contract is per operand. - testAdaptPreservesReturnType: full RelDataType identity between adapted CASE and original CONCAT — locks the type-preserving argument of rexBuilder.makeCall(originalType, CASE, ...). - testAdaptNaryConcatChainsIsNullChecksLeftAssociative: builds a ternary CONCAT via SqlLibraryOperators.CONCAT_FUNCTION and verifies the left-fold structure OR(OR(IS_NULL(a), IS_NULL(b)), IS_NULL(c)) — the binary `||` only ever appears with arity 2 in production, but the loop's correctness for arbitrary N is now a test invariant. - testAdaptSingleOperandConcatPassesThroughUnchanged: 1-operand call returns input by reference; documents the early-out branch. Each test pins one structural property in isolation, so a regression that drops any one piece of the contract surfaces with a focused failure rather than at IT-level row-mismatch noise. Signed-off-by: Kai Huang <ahkcs@amazon.com> --------- Signed-off-by: Kai Huang <ahkcs@amazon.com>
imRishN
pushed a commit
to imRishN/OpenSearch
that referenced
this pull request
May 8, 2026
…elds/rename/head/sort (opensearch-project#21521) Bucket-1 capability-registry expansion for the analytics-engine route — narrow scope: only the two scalar functions PPL sort push-down materialises into a projection (ABS, SUBSTRING). Fields / rename / head don't add scalar surface; they're covered here purely by new QA ITs that lock in the routing-and-shape behavior end-to-end through `POST /_analytics/ppl`. After this PR (with the eval-side surface from opensearch-project#21498 already on main): * `CalciteFieldsCommandIT`, `CalciteRenameCommandIT`, `CalciteHeadCommandIT` 100% green on the analytics path under `tests.analytics.force_routing=true`. * `CalciteSortCommandIT` and `CalcitePPLSortIT` pick up the cast / abs / substring push-down failures (CAST is already in `STANDARD_PROJECT_OPS` upstream from opensearch-project#21476; ABS and SUBSTRING are this PR's contribution). ## Changes **1. `DataFusionAnalyticsBackendPlugin` — `STANDARD_PROJECT_OPS` += ABS, SUBSTRING.** PPL sort push-down lifts an expression like `abs(num0)` or `substring(str0, 1, 3)` into a `LogicalProject` child of the sort, which is what the project rule's capability check sees. DataFusion has both natively; isthmus' default extension catalog already binds them. Without this, the analytics planner rejects the projection with `No backend supports scalar function [ABS] among [datafusion]`. **2. QA ITs in `sandbox/qa/analytics-engine-rest`** — one per command, each self-contained and provisioning the existing `calcs` parquet-backed dataset via `DatasetProvisioner`. Tests fire through `POST /_analytics/ppl` so the core build can validate the analytics-engine path without the SQL plugin. Mirror the failing surface in `CalciteFieldsCommandIT` / `CalciteRenameCommandIT` / `CalciteHeadCommandIT` / `CalciteSortCommandIT` one query at a time: * `FieldsCommandIT` (5 tests) — basic projection, single-column, explicit order, suffix-wildcard `*0` (set-equality, since wildcard expansion order isn't part of the contract), and `fields - num*` exclusion. * `RenameCommandIT` (4 tests) — single rename, multi-rename, post-rename reference fails with "not found", backtick-quoted target names. * `HeadCommandIT` (5 tests) — default-10 cap, explicit count, count > total rows, `head N from M` offset, and value-equality on the first 5 rows (parquet preserves insertion order, so this is deterministic). * `SortCommandIT` (5 tests) — plain ASC/DESC by integer (with calcs' 6 null int0 entries placed at the head/tail per Calcite's nulls-first/last defaults), `eval n = abs(num0) | sort n` covering the 9 null + 8 non-null abs values, and `eval s = substring(str2, 1, 3) | sort s` validating the SUBSTRING capability end-to-end against the 17-row calcs dataset. ## Test plan * `./gradlew :sandbox:qa:analytics-engine-rest:integTest -Dsandbox.enabled=true --tests "*FieldsCommandIT" --tests "*RenameCommandIT" --tests "*HeadCommandIT" --tests "*SortCommandIT"` — 19/19 green. * `./gradlew check -p sandbox -Dsandbox.enabled=true` — green (the unrelated `ScalarDateTimeFunctionIT.testConvertTz` flake from a stale local `libopensearch_native.dylib` resolved by rebuilding the Rust crate; not caused by this PR). * SQL-plugin Calcite ITs against this branch + companion opensearch-project/sql#5413, with `-Dtests.analytics.force_routing=true -Dtests.analytics.parquet_indices=true`: `CalciteFieldsCommandIT` 39/39, `CalciteRenameCommandIT` 2/2, `CalciteHeadCommandIT` 4/4, plus +5 sort-push-down wins in `CalciteSortCommandIT` and +1 in `CalcitePPLSortIT` from the ABS / SUBSTRING capability additions. ## Out of scope (separate follow-ups) * `Unable to find binding for call AVG($N)` Substrait-isthmus issue — needs an `AggregateSig`-style additional-mappings hook in `DataFusionFragmentConvertor`. * Window functions (`dedup` lowers to `ROW_NUMBER OVER`). * Advanced aggregates (`first`, `last`, `take`, `arg_max`, `percentile_approx`, `distinct_count_approx`) and PPL `span`. * The eval-predicate surface (`AND`/`OR`/`NOT` in CASE projections, `IS_NULL`/`IS_NOT_NULL`, broader string/conditional ops) and STDDEV/VAR aggregates — kept out of this PR to keep the scope focused on the four commands the QA ITs cover. Signed-off-by: Kai Huang <ahkcs@amazon.com>
imRishN
pushed a commit
to imRishN/OpenSearch
that referenced
this pull request
May 8, 2026
…nsearch-project#21544) PPL `fieldformat` is a Calcite-only command that lowers to a plain Eval node (see SQL plugin's `AstBuilder.visitFieldformatCommand`). Its expressions go through Calcite's || (CONCAT) operator and CAST, both already wired in the DataFusion backend's STANDARD_PROJECT_OPS via opensearch-project#21498. **No code changes required for the analytics route — this PR is QA-only.** The unique surface vs plain `eval` is the prefix-{`.`} and suffix-{`.`} string-concat sugar emitted by `AstExpressionBuilder.visitFieldFormatEvalClause` for the StringDotlogicalExpression / LogicalExpressionDotString rules: fieldformat x = "prefix".CAST(y AS STRING)." suffix" expands to a chain of CONCAT calls. Both forms route through the existing CONCAT capability — no extension lookup or adapter needed since isthmus' default catalog binds the || operator natively. Four tests against the in-process QA cluster, exercising the analytics path end-to-end via the test-ppl-frontend plugin: | Test | Shape | |---|---| | `testFieldformatPlusConcat` | `'Hello ' + str0` — basic +-concat. | | `testFieldformatPrefixDotCast` | `'Code: '.CAST(int0 AS STRING)` — StringDotlogicalExpression branch. | | `testFieldformatCastDotSuffix` | `CAST(int0 AS STRING).' pts'` — LogicalExpressionDotString branch. | | `testFieldformatPrefixDotCastDotSuffix` | `'Code: '.CAST(int0 AS STRING).' pts'` — combined. | Tests filter `where isnotnull(int0)` before sorting/limiting so the deterministic-row assertions don't flap on the calcs dataset's six null int0 rows (Calcite's default ascending sort puts nulls first). Out of scope: the v2-side `testFieldFormatStringConcatenationWithNullFieldToString` uses `tostring(age, "commas")` — a multi-mode UDF (binary / hex / commas / duration) with substantial Java logic in `ToStringFunction`. Adding it to the analytics path would need either Calcite-level rewrites or a DataFusion Rust UDF; tracked separately. Validates: 4/4 FieldFormatCommandIT pass; full :sandbox:qa:analytics-engine-rest:integTest suite green (**132 tests across 17 ITs**, no regressions). Signed-off-by: Kai Huang <ahkcs@amazon.com>
vishwasgarg18
pushed a commit
to vishwasgarg18/OpenSearch
that referenced
this pull request
May 8, 2026
…usion CONCAT/CAST (opensearch-project#21498) * [Analytics Framework] Resolve symbolic operators and add SAFE_CAST Calcite emits SqlBinaryOperators (e.g. `||`, the lowering target of PPL string `+`) with SqlKind.OTHER and a non-identifier name. The existing ScalarFunction.fromSqlKind / fromSqlFunction pair fails to resolve these: fromSqlKind misses (OTHER is shared), fromSqlFunction throws because `||` is not a SqlFunction (it's a SqlBinaryOperator). The planner-side fallout is "No backend supports scalar function [null] among [datafusion]" with no useful name in the error. Introduce ScalarFunction.fromSqlOperator(SqlOperator) — the unified entry point used by OpenSearchProjectRule, OpenSearchFilterRule, and BackendPlanAdapter in subsequent commits. Resolution order: 1. SqlKind via fromSqlKind (covers PLUS, CAST, COALESCE, etc.) 2. Symbolic-name lookup (handles `||` -> CONCAT) 3. Identifier-name valueOf fallback (covers UPPER, LOWER, etc.) The symbolic-name table currently has one entry (`||` -> CONCAT) but is the documented extension point for future SqlBinaryOperators with non- identifier names. Also adds SAFE_CAST as a sibling enum constant to CAST. PPL emits explicit `CAST(... AS ...)` lowered to Calcite's SqlKind.SAFE_CAST when the source value may be NULL or the conversion may fail. SAFE_CAST and CAST share the same backend semantics (DataFusion's native cast already returns NULL on conversion failure) but resolve through distinct SqlKinds, so they need distinct enum entries. Unit test pins all three resolution branches plus the unknown-operator return-null contract — a regression that drops a branch surfaces here rather than as an opaque "[null]" IT failure. Signed-off-by: Kai Huang <ahkcs@amazon.com> * [Analytics Engine] Migrate rules and adapter dispatch to fromSqlOperator Three call sites resolved a RexCall's operator using the same two-step pattern (SqlKind first, SqlFunction-cast second) and all three failed identically on `||` (a SqlBinaryOperator with SqlKind.OTHER): - OpenSearchProjectRule.resolveScalarViableBackends - OpenSearchFilterRule (predicate operator resolution) - BackendPlanAdapter.resolveFunction (per-function adapter dispatch) Migrate all three to ScalarFunction.fromSqlOperator, the unified resolver added in the previous commit. Behavior for previously-resolved operators is unchanged — fromSqlOperator delegates to fromSqlKind first, so anything that resolved through SqlKind continues to. New behavior: `||` now resolves to CONCAT, and unrecognized operators return null (catching the IllegalArgumentException that fromSqlFunction's valueOf threw before; the call sites already handled null and now produce a better-formed error message that includes the operator name). Also drop the unused SqlFunction import in OpenSearchFilterRule and BackendPlanAdapter, and tighten the OpenSearchProjectRule error message to fall back to operator.getName() when the resolver returns null — "[null]" was unactionable for triage; "[||]" or "[<unknown_name>]" points directly at the missing capability. Signed-off-by: Kai Huang <ahkcs@amazon.com> * [Analytics Backend / DataFusion] Wire CONCAT/CAST/SAFE_CAST + concat null adapter Three new ScalarFunctions in STANDARD_PROJECT_OPS: - CONCAT — lowering target of PPL `eval`'s `+` for strings (Calcite emits `||`, resolved to CONCAT through the symbolic-name branch of ScalarFunction.fromSqlOperator) - CAST — covers PPL's explicit `CAST(... AS ...)` over non-null source types (Calcite emits SqlKind.CAST) - SAFE_CAST — same surface, but emitted by Calcite when the source value is nullable (SqlKind.SAFE_CAST) CONCAT additionally needs a ScalarFunctionAdapter to preserve null semantics. Calcite's `||` follows the SQL standard: if any operand is NULL, the result is NULL. Substrait's default `concat` extension is documented with the same semantics, but DataFusion's substrait reader maps it to the DataFusion `concat()` function — which deviates from the standard and treats NULL operands as empty strings. PPL queries like `'Age: ' + CAST(null AS STRING)` expect NULL, not 'Age: '. ConcatFunctionAdapter rewrites `||(a, b, ...)` into CASE WHEN a IS NULL OR b IS NULL OR ... THEN NULL ELSE ||(a, b, ...) END The inner `||` survives unchanged and serializes through the same Substrait conversion path; the surrounding CASE/IS_NULL short-circuits the DataFusion `concat()` call whenever any operand is NULL, restoring SQL-standard null propagation without a custom DataFusion UDF. Trade-off: the rewrite double-evaluates each operand (once in IS_NULL, once in the inner `||`). For RexInputRef and RexLiteral operands — the only shapes PPL emits today for string concat — this is free; for nested calls the cost is proportional to operand count, not operand depth, since each `||` adapter wraps one CASE around its direct call. A custom null-propagating concat UDF (Bucket-3 work in sandbox/plugins/analytics-backend-datafusion/rust) is the alternative but disproportionate for a Bucket-1 surface. Signed-off-by: Kai Huang <ahkcs@amazon.com> * [QA] Add EvalCommandIT for the analytics-engine REST path Self-contained integration test for PPL `eval` on the analytics-engine route. Mirrors CalciteEvalCommandIT in opensearch-project/sql so the analytics-engine path can be verified inside core without cross-plugin dependencies on the SQL plugin. Each test sends a PPL query through POST /_analytics/ppl (exposed by test-ppl-frontend) which runs the same UnifiedQueryPlanner -> CalciteRelNodeVisitor -> Substrait -> DataFusion pipeline as the SQL plugin's force-routed analytics path. Four tests on the calcs dataset cover the eval surface this PR enables: - testEvalStringConcatLiteralPlusField — `'literal' + str_field` exercises the symbolic-name resolution for `||` and the CONCAT capability; null str field rows assert null propagation through the CASE adapter. - testEvalStringConcatWithCastIntField — `'literal' + CAST(int AS STRING)` exercises both CAST/SAFE_CAST and CONCAT in the same projection; null int rows confirm CAST(NULL) -> NULL propagates through the surrounding concat. - testEvalStringConcatMultipleLiteralsAndFields — chained four-arg concat exercises the recursive AnnotatedProjectExpression strip for nested project calls. - testEvalStringConcatTwoFields — pure field-to-field concat with no literal operands; planner takes the hasFieldRef=true path in resolveScalarViableBackends. Reuses the existing calcs dataset (no new fixtures). Once this lands, the SQL-plugin's CalciteEvalCommandIT is verification-only — this QA IT is the source of truth for the analytics-engine path. Signed-off-by: Kai Huang <ahkcs@amazon.com> * [Analytics Framework] Rename fromSqlOperator to fromSqlOperatorWithFallback Per @expani's PR feedback: the method walks three resolution paths (SqlKind, symbolic-name table, identifier-name valueOf) before returning null, so the name should advertise the fallback behavior at the call site rather than only in the javadoc. Mechanical rename across all callers — `ScalarFunction.fromSqlOperator` -> `ScalarFunction.fromSqlOperatorWithFallback` in: - the resolver itself plus its 7 unit tests - OpenSearchProjectRule (2 call sites) - OpenSearchFilterRule (1 call site) - BackendPlanAdapter.resolveFunction (1 call site) - EvalCommandIT javadoc cross-reference No behavioral change. Signed-off-by: Kai Huang <ahkcs@amazon.com> * [Analytics Framework + Backend] Address @expani review on PR opensearch-project#21498 Three feedback items in one commit: 1. Co-locate symbolic operator name with the enum constant. The static SYMBOLIC_OPERATOR_NAMES map duplicated a property that belongs on the enum itself. Moved to a nullable `symbolicOperatorName` field on each ScalarFunction constant — currently set only on CONCAT ("||"). The reverse-index map is now built from the enum at class-init time, so adding a new symbolic operator is a single-site edit on the constant rather than a separate map entry. 2. Inline the OR/IS_NULL fold in ConcatFunctionAdapter. Drop the temporary List<RexNode> nullChecks and accumulate the OR-of-IS_NULLs directly in the loop body. Same generated tree, fewer allocations, less to read. 3. Note the Map.of single-line constraint on scalarFunctionAdapters. Per-pair formatting is rejected by spotless; left a comment pointing future contributors at alphabetical ordering instead, and reordered the entries (CONCAT before TIMESTAMP) to make the convention concrete. No behavioral change. CalciteEvalCommandIT 4/4 still passes against the analytics-engine route; sandbox per-module check (excluding the unrelated commons-text dependencyLicenses task) remains green. Signed-off-by: Kai Huang <ahkcs@amazon.com> * [Analytics Framework] Resolve symbolic operators by Calcite-operator reference Per @expani's PR follow-up: the symbolic-name string ("||") was a runtime-coupled identifier that could silently drift if Calcite renamed the operator. Replace it with a direct reference to the Calcite operator constant (SqlStdOperatorTable.CONCAT), so the link is enforced at compile time and a Calcite-side rename surfaces as a build failure here. - String symbolicOperatorName -> SqlOperator referenceOperator on the enum constructor. - CONCAT now points at SqlStdOperatorTable.CONCAT instead of "||". - Reverse index switches from Map<String, ScalarFunction> keyed by operator name to Map<SqlOperator, ScalarFunction> keyed by operator identity. Calcite's standard operators are singletons, so identity lookup is exact. - Unit test renamed (testFromSqlOperatorResolvesPipeConcatViaReferenceOperator) and its comment updated; the assertions on `getName()` / `getKind()` are kept as documentation of WHY this branch is needed at all. No behavioral change in the resolution logic — same three-step chain (SqlKind, then this branch, then identifier-name valueOf), with the middle branch now identity-comparing rather than name-comparing. CalciteEvalCommandIT 4/4 still passes; ScalarFunctionTests 7/7. Signed-off-by: Kai Huang <ahkcs@amazon.com> * [Analytics Framework + Backend] Address @expani follow-up on PR opensearch-project#21498 Two feedback items in one commit: 1. Drop the redundant Map.copyOf on BY_REFERENCE_OPERATOR. The HashMap built in the static initializer is private static final and is only read via the resolver's get() — never returned, never iterated. The immutability wrapper added an allocation without conferring any external safety guarantee. Comment explains the reasoning so future readers don't reintroduce the wrap. 2. Add ConcatFunctionAdapterTests with seven structural assertions on the CASE rewrite contract: - testAdaptBinaryConcatProducesCaseWrapper: rewritten root is a three-operand CASE (condition, then, else). - testAdaptedCaseElseBranchIsOriginalConcat: else branch is the original RexCall by reference (assertSame, not assertEquals) — downstream substrait conversion expects the same object the resolver annotated. - testAdaptedCaseThenBranchIsNullLiteralOfMatchingSqlType: then branch is a NULL literal whose SQL type name matches the original CONCAT's. Comment explains why we compare type name rather than full RelDataType (RexBuilder.makeNullLiteral promotes nullability, so the full types differ harmlessly). - testAdaptedCaseConditionIsOrOfIsNullChecks: condition is OR with each disjunct an IS_NULL wrapping the corresponding original operand at matching index — null-propagation contract is per operand. - testAdaptPreservesReturnType: full RelDataType identity between adapted CASE and original CONCAT — locks the type-preserving argument of rexBuilder.makeCall(originalType, CASE, ...). - testAdaptNaryConcatChainsIsNullChecksLeftAssociative: builds a ternary CONCAT via SqlLibraryOperators.CONCAT_FUNCTION and verifies the left-fold structure OR(OR(IS_NULL(a), IS_NULL(b)), IS_NULL(c)) — the binary `||` only ever appears with arity 2 in production, but the loop's correctness for arbitrary N is now a test invariant. - testAdaptSingleOperandConcatPassesThroughUnchanged: 1-operand call returns input by reference; documents the early-out branch. Each test pins one structural property in isolation, so a regression that drops any one piece of the contract surfaces with a focused failure rather than at IT-level row-mismatch noise. Signed-off-by: Kai Huang <ahkcs@amazon.com> --------- Signed-off-by: Kai Huang <ahkcs@amazon.com>
vishwasgarg18
pushed a commit
to vishwasgarg18/OpenSearch
that referenced
this pull request
May 8, 2026
…elds/rename/head/sort (opensearch-project#21521) Bucket-1 capability-registry expansion for the analytics-engine route — narrow scope: only the two scalar functions PPL sort push-down materialises into a projection (ABS, SUBSTRING). Fields / rename / head don't add scalar surface; they're covered here purely by new QA ITs that lock in the routing-and-shape behavior end-to-end through `POST /_analytics/ppl`. After this PR (with the eval-side surface from opensearch-project#21498 already on main): * `CalciteFieldsCommandIT`, `CalciteRenameCommandIT`, `CalciteHeadCommandIT` 100% green on the analytics path under `tests.analytics.force_routing=true`. * `CalciteSortCommandIT` and `CalcitePPLSortIT` pick up the cast / abs / substring push-down failures (CAST is already in `STANDARD_PROJECT_OPS` upstream from opensearch-project#21476; ABS and SUBSTRING are this PR's contribution). ## Changes **1. `DataFusionAnalyticsBackendPlugin` — `STANDARD_PROJECT_OPS` += ABS, SUBSTRING.** PPL sort push-down lifts an expression like `abs(num0)` or `substring(str0, 1, 3)` into a `LogicalProject` child of the sort, which is what the project rule's capability check sees. DataFusion has both natively; isthmus' default extension catalog already binds them. Without this, the analytics planner rejects the projection with `No backend supports scalar function [ABS] among [datafusion]`. **2. QA ITs in `sandbox/qa/analytics-engine-rest`** — one per command, each self-contained and provisioning the existing `calcs` parquet-backed dataset via `DatasetProvisioner`. Tests fire through `POST /_analytics/ppl` so the core build can validate the analytics-engine path without the SQL plugin. Mirror the failing surface in `CalciteFieldsCommandIT` / `CalciteRenameCommandIT` / `CalciteHeadCommandIT` / `CalciteSortCommandIT` one query at a time: * `FieldsCommandIT` (5 tests) — basic projection, single-column, explicit order, suffix-wildcard `*0` (set-equality, since wildcard expansion order isn't part of the contract), and `fields - num*` exclusion. * `RenameCommandIT` (4 tests) — single rename, multi-rename, post-rename reference fails with "not found", backtick-quoted target names. * `HeadCommandIT` (5 tests) — default-10 cap, explicit count, count > total rows, `head N from M` offset, and value-equality on the first 5 rows (parquet preserves insertion order, so this is deterministic). * `SortCommandIT` (5 tests) — plain ASC/DESC by integer (with calcs' 6 null int0 entries placed at the head/tail per Calcite's nulls-first/last defaults), `eval n = abs(num0) | sort n` covering the 9 null + 8 non-null abs values, and `eval s = substring(str2, 1, 3) | sort s` validating the SUBSTRING capability end-to-end against the 17-row calcs dataset. ## Test plan * `./gradlew :sandbox:qa:analytics-engine-rest:integTest -Dsandbox.enabled=true --tests "*FieldsCommandIT" --tests "*RenameCommandIT" --tests "*HeadCommandIT" --tests "*SortCommandIT"` — 19/19 green. * `./gradlew check -p sandbox -Dsandbox.enabled=true` — green (the unrelated `ScalarDateTimeFunctionIT.testConvertTz` flake from a stale local `libopensearch_native.dylib` resolved by rebuilding the Rust crate; not caused by this PR). * SQL-plugin Calcite ITs against this branch + companion opensearch-project/sql#5413, with `-Dtests.analytics.force_routing=true -Dtests.analytics.parquet_indices=true`: `CalciteFieldsCommandIT` 39/39, `CalciteRenameCommandIT` 2/2, `CalciteHeadCommandIT` 4/4, plus +5 sort-push-down wins in `CalciteSortCommandIT` and +1 in `CalcitePPLSortIT` from the ABS / SUBSTRING capability additions. ## Out of scope (separate follow-ups) * `Unable to find binding for call AVG($N)` Substrait-isthmus issue — needs an `AggregateSig`-style additional-mappings hook in `DataFusionFragmentConvertor`. * Window functions (`dedup` lowers to `ROW_NUMBER OVER`). * Advanced aggregates (`first`, `last`, `take`, `arg_max`, `percentile_approx`, `distinct_count_approx`) and PPL `span`. * The eval-predicate surface (`AND`/`OR`/`NOT` in CASE projections, `IS_NULL`/`IS_NOT_NULL`, broader string/conditional ops) and STDDEV/VAR aggregates — kept out of this PR to keep the scope focused on the four commands the QA ITs cover. Signed-off-by: Kai Huang <ahkcs@amazon.com>
Bukhtawar
pushed a commit
to Bukhtawar/OpenSearch
that referenced
this pull request
May 10, 2026
…nsearch-project#21544) PPL `fieldformat` is a Calcite-only command that lowers to a plain Eval node (see SQL plugin's `AstBuilder.visitFieldformatCommand`). Its expressions go through Calcite's || (CONCAT) operator and CAST, both already wired in the DataFusion backend's STANDARD_PROJECT_OPS via opensearch-project#21498. **No code changes required for the analytics route — this PR is QA-only.** The unique surface vs plain `eval` is the prefix-{`.`} and suffix-{`.`} string-concat sugar emitted by `AstExpressionBuilder.visitFieldFormatEvalClause` for the StringDotlogicalExpression / LogicalExpressionDotString rules: fieldformat x = "prefix".CAST(y AS STRING)." suffix" expands to a chain of CONCAT calls. Both forms route through the existing CONCAT capability — no extension lookup or adapter needed since isthmus' default catalog binds the || operator natively. Four tests against the in-process QA cluster, exercising the analytics path end-to-end via the test-ppl-frontend plugin: | Test | Shape | |---|---| | `testFieldformatPlusConcat` | `'Hello ' + str0` — basic +-concat. | | `testFieldformatPrefixDotCast` | `'Code: '.CAST(int0 AS STRING)` — StringDotlogicalExpression branch. | | `testFieldformatCastDotSuffix` | `CAST(int0 AS STRING).' pts'` — LogicalExpressionDotString branch. | | `testFieldformatPrefixDotCastDotSuffix` | `'Code: '.CAST(int0 AS STRING).' pts'` — combined. | Tests filter `where isnotnull(int0)` before sorting/limiting so the deterministic-row assertions don't flap on the calcs dataset's six null int0 rows (Calcite's default ascending sort puts nulls first). Out of scope: the v2-side `testFieldFormatStringConcatenationWithNullFieldToString` uses `tostring(age, "commas")` — a multi-mode UDF (binary / hex / commas / duration) with substantial Java logic in `ToStringFunction`. Adding it to the analytics path would need either Calcite-level rewrites or a DataFusion Rust UDF; tracked separately. Validates: 4/4 FieldFormatCommandIT pass; full :sandbox:qa:analytics-engine-rest:integTest suite green (**132 tests across 17 ITs**, no regressions). Signed-off-by: Kai Huang <ahkcs@amazon.com>
This was referenced Jun 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Drives the analytics-engine route to parity for PPL
evalwith string concatenation andCAST(... AS STRING). Pairs with the routing playbook (developed alongside the fillnull PR #21472) — same pattern, one new resolver helper, three new project capabilities, one Bucket-2 adapter, one QA IT.Bucket classification
'lit' + str_field,str + strSTANDARD_PROJECT_OPS+=CONCATCAST(x AS STRING)(non-null source)STANDARD_PROJECT_OPS+=CASTCAST(x AS STRING)(nullable source)STANDARD_PROJECT_OPS+=SAFE_CAST||ConcatFunctionAdapterrewrites||(a,b)→CASE WHEN IS_NULL(a) OR IS_NULL(b) THEN NULL ELSE ||(a,b) END||)ScalarFunction.fromSqlOperator(SqlOperator)Failure modes addressed
PPL string
+lowers to Calcite'sSqlStdOperatorTable.CONCAT— aSqlBinaryOperatornamed\|\|withSqlKind.OTHER. Before this PR, bothOpenSearchProjectRuleandOpenSearchFilterRulefailed to resolve it because:ScalarFunction.fromSqlKind(OTHER)returns null (OTHER is shared by many ops).instanceof SqlFunctionis false forSqlBinaryOperator, so the SqlFunction-cast branch was skipped.After wiring
CONCAT/CAST/SAFE_CASTintoSTANDARD_PROJECT_OPS, the next failure was DataFusion-side null semantics:'Age: ' + CAST(null AS STRING)returned'Age: 'instead ofnull. Calcite's\|\|follows SQL standard (any NULL → NULL); Substrait's default extension catalog documents the same; but DataFusion's substrait reader maps to its nativeconcat()function which treats NULL as empty string.ConcatFunctionAdaptershort-circuits via a CASE/IS_NULL wrapper.Test results
CalciteEvalCommandIT(SQL-plugin v2-side, analytics-engine route)EvalCommandIT(this PR, QA-side,POST /_analytics/ppl)FillNullCommandIT(regression check)AppendCommandIT(regression check)ScalarFunctionTests(new unit coverage)./gradlew check -p sandboxDesign trade-off worth flagging
ConcatFunctionAdapteruses a CASE/IS_NULL plan rewrite rather than a custom DataFusion null-propagating concat UDF. The rewrite is surgical, reuses the existing Substrait conversion path (CASE serializes toif_then, no new extension), and avoids cross-language work. The cost is a per-\|\|-call CASE wrapper that double-references each operand in IS_NULL checks — fine forRexInputRef/RexLiteral(the only shapes PPL emits for string concat today), proportional in nested concats ('A=' + str0 + ', B=' + str2becomes three CASE-wrapped\|\|calls — each evaluating its direct operands twice, not the whole expression). A Bucket-3 UDF is the alternative if reviewers prefer; happy to move the adapter out if so.Commit shape
[Analytics Framework] Resolve symbolic operators and add SAFE_CASTScalarFunction.java,ScalarFunctionTests.java[Analytics Engine] Migrate rules and adapter dispatch to fromSqlOperatorOpenSearchProjectRule,OpenSearchFilterRule,BackendPlanAdapter[Analytics Backend / DataFusion] Wire CONCAT/CAST/SAFE_CAST + concat null adapterDataFusionAnalyticsBackendPlugin.java,ConcatFunctionAdapter.java[QA] Add EvalCommandIT for the analytics-engine REST pathEvalCommandIT.javasandbox/qa/analytics-engine-rest, reusescalcsdataset.Forward pattern
After this lands, future Bucket-1 PPL
evalfunctions for which DataFusion has the substrait mapping are one-line additions toSTANDARD_PROJECT_OPSplus an enum entry inScalarFunctionif missing — no resolver work needed.By submitting this pull request