Skip to content

Fix TIMESTAMP/DATE arithmetic and DATE→TIMESTAMP cast - #21978

Merged
sandeshkr419 merged 2 commits into
opensearch-project:mainfrom
vinaykpud:fix/timestamp-date-arith-cast
Jun 4, 2026
Merged

Fix TIMESTAMP/DATE arithmetic and DATE→TIMESTAMP cast#21978
sandeshkr419 merged 2 commits into
opensearch-project:mainfrom
vinaykpud:fix/timestamp-date-arith-cast

Conversation

@vinaykpud

@vinaykpud vinaykpud commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

Description

Two small bugs on the analytics-engine PPL path that crashed any
query touching DATE-typed expressions.

1. TIMESTAMP - TIMESTAMP returns HTTP 400.
PPL lowers t1 - t2 to MINUS(precision_timestamp, precision_timestamp),
but Substrait's default catalog only declares numeric subtract.
Isthmus rejects with:

Unable to convert call -(precision_timestamp<N>?, precision_timestamp<N>?)

Fix: new MinusAdapter rewrites the call as
from_unixtime(to_unixtime(t1) - to_unixtime(t2)) (a TIMESTAMP-typed
result so Calcite's Project type-check holds). Numeric operands pass
through.

2. TIMESTAMP(<date column>) returns HTTP 500.
The DATE column lowers to to_timestamp(date_col), but DataFusion's
to_timestamp UDF doesn't accept Date32:

Unsupported data type Date32 for function to_timestamp

This also blew up TIME = DATE comparisons (the coercion lifts
the DATE side through TIMESTAMP(...)).

Fix: in TimestampFunctionAdapter, when the operand's SQL type
is DATE, emit a native CAST(Date32 → TIMESTAMP) (arrow's
cast_kernel) instead of falling through to the to_timestamp UDF.
Matches Shape B's literal-fold semantics (midnight UTC of the day).

Changes

  • MinusAdapter (new) — registered on ScalarFunction.MINUS.
  • TimestampFunctionAdapter#tryLiftDateOperand — DATE-column lift.
  • MinusAdapterTests (new, 5 tests).
  • TimestampFunctionAdapterTests — Shape F split into
    TIMESTAMP-column and DATE-column cases.
  • IT coverage in DateTimeScalarFunctionsIT and TimestampFunctionIT.

Check List

  • Functionality includes testing.

@github-actions

github-actions Bot commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ Recommended focus areas for review

Possible Issue

The adapter always casts the result back to the original call type (line 54), but for DATE-DATE subtraction, PPL semantics may expect an integer day-count (as shown in the IT test at DateTimeScalarFunctionsIT.java:197), not a TIMESTAMP. If the original call type is TIMESTAMP when operands are DATE, the cast will produce a TIMESTAMP instead of the expected integer, breaking the day-count semantics.

// PPL's MINUS(DATETIME, DATETIME) infers TIMESTAMP at the call site; lift the
// BIGINT seconds back through from_unixtime so Project.isValid type-matches.
RelDataType fp64 = rexBuilder.getTypeFactory().createSqlType(SqlTypeName.DOUBLE);
RexNode diffDouble = rexBuilder.makeCast(fp64, diffSeconds, true);
RexNode asTimestamp = rexBuilder.makeCall(RustUdfDateTimeAdapters.LOCAL_FROM_UNIXTIME_OP, diffDouble);
return rexBuilder.makeCast(original.getType(), asTimestamp, true);

@github-actions

github-actions Bot commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Add null safety for operand types

Add null safety checks for operand types before calling getType(). If either operand
is null or has a null type, the adapter could throw a NullPointerException. Return
the original call early if types are unavailable.

sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/MinusAdapter.java [38-42]

 RexNode left = original.getOperands().get(0);
 RexNode right = original.getOperands().get(1);
+if (left.getType() == null || right.getType() == null) {
+    return original;
+}
 if (!isDateOrTimestamp(left.getType()) || !isDateOrTimestamp(right.getType())) {
     return original;
 }
Suggestion importance[1-10]: 7

__

Why: Adding null checks for left.getType() and right.getType() prevents potential NullPointerException when operands have null types. This is a defensive programming practice that improves robustness.

Medium
Add null check for operand type

Add a null check for operand.getType() before calling getSqlTypeName(). If the
operand type is null after stripping annotations, this will cause a
NullPointerException. Return null early if the type is unavailable.

sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/TimestampFunctionAdapter.java [116-119]

 RexNode operand = stripOperatorAnnotation(original.getOperands().get(0));
-if (operand.getType().getSqlTypeName() != SqlTypeName.DATE) {
+if (operand.getType() == null || operand.getType().getSqlTypeName() != SqlTypeName.DATE) {
     return null;
 }
Suggestion importance[1-10]: 7

__

Why: Adding a null check for operand.getType() before calling getSqlTypeName() prevents potential NullPointerException. This defensive check improves code robustness when handling edge cases.

Medium
Clarify unary minus handling

The adapter should handle the unary minus operator (negation) explicitly by checking
the operand count first. Currently, unary minus passes through correctly, but the
logic could be clearer by separating the operator check from the operand count check
to avoid confusion.

sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/MinusAdapter.java [35-37]

-if (original.getOperator() != SqlStdOperatorTable.MINUS || original.getOperands().size() != 2) {
+if (original.getOperands().size() != 2) {
+    return original;
+}
+if (original.getOperator() != SqlStdOperatorTable.MINUS) {
     return original;
 }
Suggestion importance[1-10]: 3

__

Why: While separating the checks might improve readability slightly, the current implementation is correct and the test testUnaryMinusPassesThrough confirms it works. The improvement is marginal and mostly stylistic.

Low

@vinaykpud
vinaykpud marked this pull request as ready for review June 3, 2026 23:02
@vinaykpud
vinaykpud requested a review from a team as a code owner June 3, 2026 23:02
@vinaykpud vinaykpud added skip-changelog skip-diff-analyzer Maintainer to skip code-diff-analyzer check, after reviewing issues in AI analysis. skip-diff-reviewer Maintainer to skip code-diff-reviewer check, after reviewing issues in AI analysis. labels Jun 3, 2026
vinaykpud added 2 commits June 4, 2026 03:12
- Add MinusAdapter to rewrite TIMESTAMP/DATE subtract via to_unixtime
  (Substrait default catalog has no subtract(precision_timestamp, ...) binding)
- TimestampFunctionAdapter: lift DATE column operands via native CAST instead
  of to_timestamp(Date32), which DataFusion's UDF rejects
- Register MinusAdapter on ScalarFunction.MINUS
- Add MinusAdapterTests; split Shape F unit test into TIMESTAMP/DATE column cases
- Add IT coverage in DateTimeScalarFunctionsIT and TimestampFunctionIT

Signed-off-by: Vinay Krishna Pudyodu <vinkrish.neo@gmail.com>
Signed-off-by: Vinay Krishna Pudyodu <vinkrish.neo@gmail.com>
@vinaykpud
vinaykpud force-pushed the fix/timestamp-date-arith-cast branch from 8cf486b to 23167b7 Compare June 4, 2026 03:26
@github-actions

github-actions Bot commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

✅ Gradle check result for 23167b7: SUCCESS

@codecov

codecov Bot commented Jun 4, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.44%. Comparing base (0755be0) to head (23167b7).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main   #21978      +/-   ##
============================================
+ Coverage     73.43%   73.44%   +0.01%     
- Complexity    75515    75555      +40     
============================================
  Files          6037     6037              
  Lines        342786   342786              
  Branches      49311    49311              
============================================
+ Hits         251723   251759      +36     
- Misses        71046    71050       +4     
+ Partials      20017    19977      -40     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@sandeshkr419
sandeshkr419 merged commit c430d53 into opensearch-project:main Jun 4, 2026
16 checks passed
mengweieric added a commit to mengweieric/OpenSearch that referenced this pull request Jun 4, 2026
…tream MinusAdapter (opensearch-project#21978)

Upstream opensearch-project#21978 added MinusAdapter, the proper named ScalarFunctionAdapter for
timestamp/date subtraction (t1 - t2 -> from_unixtime(to_unixtime(t1) - to_unixtime(t2))).
It also coordinates with WidthBucketAdapter by deliberately leaving the
MINUS(MAX OVER(), MIN OVER()) binning shape untouched. Our pre-Substrait
TimestampSubtractRewriter rewrote every MINUS(timestamp, timestamp) including that
binning shape, so keeping both risked clobbering the binning path. Remove the
rewriter (and its test); MinusAdapter fully covers the case. The shared
preprocessForSubstrait helper is retained for the remaining rewriters.

Signed-off-by: Eric Wei <mengwei.eric@gmail.com>
mengweieric added a commit to mengweieric/OpenSearch that referenced this pull request Jun 8, 2026
…tream MinusAdapter (opensearch-project#21978)

Upstream opensearch-project#21978 added MinusAdapter, the proper named ScalarFunctionAdapter for
timestamp/date subtraction (t1 - t2 -> from_unixtime(to_unixtime(t1) - to_unixtime(t2))).
It also coordinates with WidthBucketAdapter by deliberately leaving the
MINUS(MAX OVER(), MIN OVER()) binning shape untouched. Our pre-Substrait
TimestampSubtractRewriter rewrote every MINUS(timestamp, timestamp) including that
binning shape, so keeping both risked clobbering the binning path. Remove the
rewriter (and its test); MinusAdapter fully covers the case. The shared
preprocessForSubstrait helper is retained for the remaining rewriters.

Signed-off-by: Eric Wei <mengwei.eric@gmail.com>
mengweieric added a commit to mengweieric/OpenSearch that referenced this pull request Jun 10, 2026
…tream MinusAdapter (opensearch-project#21978)

Upstream opensearch-project#21978 added MinusAdapter, the proper named ScalarFunctionAdapter for
timestamp/date subtraction (t1 - t2 -> from_unixtime(to_unixtime(t1) - to_unixtime(t2))).
It also coordinates with WidthBucketAdapter by deliberately leaving the
MINUS(MAX OVER(), MIN OVER()) binning shape untouched. Our pre-Substrait
TimestampSubtractRewriter rewrote every MINUS(timestamp, timestamp) including that
binning shape, so keeping both risked clobbering the binning path. Remove the
rewriter (and its test); MinusAdapter fully covers the case. The shared
preprocessForSubstrait helper is retained for the remaining rewriters.

Signed-off-by: Eric Wei <mengwei.eric@gmail.com>
mengweieric added a commit to mengweieric/OpenSearch that referenced this pull request Jun 11, 2026
…tream MinusAdapter (opensearch-project#21978)

Upstream opensearch-project#21978 added MinusAdapter, the proper named ScalarFunctionAdapter for
timestamp/date subtraction (t1 - t2 -> from_unixtime(to_unixtime(t1) - to_unixtime(t2))).
It also coordinates with WidthBucketAdapter by deliberately leaving the
MINUS(MAX OVER(), MIN OVER()) binning shape untouched. Our pre-Substrait
TimestampSubtractRewriter rewrote every MINUS(timestamp, timestamp) including that
binning shape, so keeping both risked clobbering the binning path. Remove the
rewriter (and its test); MinusAdapter fully covers the case. The shared
preprocessForSubstrait helper is retained for the remaining rewriters.

Signed-off-by: Eric Wei <mengwei.eric@gmail.com>
mengweieric added a commit to mengweieric/OpenSearch that referenced this pull request Jun 12, 2026
…tream MinusAdapter (opensearch-project#21978)

Upstream opensearch-project#21978 added MinusAdapter, the proper named ScalarFunctionAdapter for
timestamp/date subtraction (t1 - t2 -> from_unixtime(to_unixtime(t1) - to_unixtime(t2))).
It also coordinates with WidthBucketAdapter by deliberately leaving the
MINUS(MAX OVER(), MIN OVER()) binning shape untouched. Our pre-Substrait
TimestampSubtractRewriter rewrote every MINUS(timestamp, timestamp) including that
binning shape, so keeping both risked clobbering the binning path. Remove the
rewriter (and its test); MinusAdapter fully covers the case. The shared
preprocessForSubstrait helper is retained for the remaining rewriters.

Signed-off-by: Eric Wei <mengwei.eric@gmail.com>
mengweieric added a commit to mengweieric/OpenSearch that referenced this pull request Jun 12, 2026
…tream MinusAdapter (opensearch-project#21978)

Upstream opensearch-project#21978 added MinusAdapter, the proper named ScalarFunctionAdapter for
timestamp/date subtraction (t1 - t2 -> from_unixtime(to_unixtime(t1) - to_unixtime(t2))).
It also coordinates with WidthBucketAdapter by deliberately leaving the
MINUS(MAX OVER(), MIN OVER()) binning shape untouched. Our pre-Substrait
TimestampSubtractRewriter rewrote every MINUS(timestamp, timestamp) including that
binning shape, so keeping both risked clobbering the binning path. Remove the
rewriter (and its test); MinusAdapter fully covers the case. The shared
preprocessForSubstrait helper is retained for the remaining rewriters.

Signed-off-by: Eric Wei <mengwei.eric@gmail.com>
mengweieric added a commit to mengweieric/OpenSearch that referenced this pull request Jun 16, 2026
…tream MinusAdapter (opensearch-project#21978)

Upstream opensearch-project#21978 added MinusAdapter, the proper named ScalarFunctionAdapter for
timestamp/date subtraction (t1 - t2 -> from_unixtime(to_unixtime(t1) - to_unixtime(t2))).
It also coordinates with WidthBucketAdapter by deliberately leaving the
MINUS(MAX OVER(), MIN OVER()) binning shape untouched. Our pre-Substrait
TimestampSubtractRewriter rewrote every MINUS(timestamp, timestamp) including that
binning shape, so keeping both risked clobbering the binning path. Remove the
rewriter (and its test); MinusAdapter fully covers the case. The shared
preprocessForSubstrait helper is retained for the remaining rewriters.

Signed-off-by: Eric Wei <mengwei.eric@gmail.com>
mengweieric added a commit to mengweieric/OpenSearch that referenced this pull request Jun 16, 2026
…tream MinusAdapter (opensearch-project#21978)

Upstream opensearch-project#21978 added MinusAdapter, the proper named ScalarFunctionAdapter for
timestamp/date subtraction (t1 - t2 -> from_unixtime(to_unixtime(t1) - to_unixtime(t2))).
It also coordinates with WidthBucketAdapter by deliberately leaving the
MINUS(MAX OVER(), MIN OVER()) binning shape untouched. Our pre-Substrait
TimestampSubtractRewriter rewrote every MINUS(timestamp, timestamp) including that
binning shape, so keeping both risked clobbering the binning path. Remove the
rewriter (and its test); MinusAdapter fully covers the case. The shared
preprocessForSubstrait helper is retained for the remaining rewriters.

Signed-off-by: Eric Wei <mengwei.eric@gmail.com>
mengweieric added a commit to mengweieric/OpenSearch that referenced this pull request Jun 16, 2026
…tream MinusAdapter (opensearch-project#21978)

Upstream opensearch-project#21978 added MinusAdapter, the proper named ScalarFunctionAdapter for
timestamp/date subtraction (t1 - t2 -> from_unixtime(to_unixtime(t1) - to_unixtime(t2))).
It also coordinates with WidthBucketAdapter by deliberately leaving the
MINUS(MAX OVER(), MIN OVER()) binning shape untouched. Our pre-Substrait
TimestampSubtractRewriter rewrote every MINUS(timestamp, timestamp) including that
binning shape, so keeping both risked clobbering the binning path. Remove the
rewriter (and its test); MinusAdapter fully covers the case. The shared
preprocessForSubstrait helper is retained for the remaining rewriters.

Signed-off-by: Eric Wei <mengwei.eric@gmail.com>
sandeshkr419 pushed a commit that referenced this pull request Jun 18, 2026
…lytics-engine route + review fixes (#21975)

* [analytics-engine] Fix now()-family + enable now(fsp)/rand(seed)/sha2/span-month/ts-subtract/dc-approx on the analytics-engine route

Closes several PPL scalar/aggregate gaps on the analytics-engine (DataFusion)
route. Each was either an "Unable to convert call ..." Substrait failure or a
result-rendering gap; all are verified end-to-end via the CalcitePPL*IT remote
ITs on a parquet/composite (analytics-engine-routed) cluster.

now()-family (CalciteNowLikeFunctionIT 12/12):
- ArrowValues now converts Arrow Date32/Date64 -> LocalDate and
  Time{Sec,Milli,Micro,Nano} -> LocalTime so DATE/TIME results render as
  "uuuu-MM-dd" / "HH:mm:ss" via ExprValueUtils.fromObjectValue's existing
  temporal branch (mirrors how TIMESTAMP already arrives as LocalDateTime).
  Fixes current_date/curdate/current_time/curtime returning raw epoch-day /
  units-of-day integers.
- UTC_TIMESTAMP/UTC_DATE/UTC_TIME enum constants + adapter registrations route
  to the same DataFusion builtins as their CURRENT_* equivalents (cluster runs
  in UTC), reusing the existing now/currentDate/currentTime adapters.
- NowFspAdapter drops the optional fractional-seconds-precision arg so
  now(fsp)/current_timestamp(fsp)/sysdate(fsp) map to DataFusion's niladic
  now() instead of failing as "Unable to convert call now(i32)".

Other scalar/aggregate fixes:
- RandSeedAdapter drops the optional rand(seed) operand -> DataFusion random().
- Sha2FunctionAdapter raises a clear "Unsupported SHA2 algorithm [N]" for a
  concrete unsupported literal bit length, matching the SQL-plugin reference
  (CryptographicFunction) instead of surfacing a cryptic Substrait error.
- SpanAdapter supports variable-length month/quarter/year buckets.
- TimestampSubtractRewriter rewrites MINUS(timestamp, timestamp) to an
  epoch-second difference (to_unixtime), which is Substrait-convertible.
- PplAggregateCallRewriter + AggregateFunction map DISTINCT_COUNT_APPROX to
  APPROX_COUNT_DISTINCT (dc/distinct_count approx form).

Adds unit tests for the new adapters/rewriter and ArrowValues DATE/TIME
conversion; updates the SHA2 adapter test to assert the clear-error behavior.

Signed-off-by: Eric Wei <mengwei.eric@gmail.com>

* [analytics-engine] Address review: RAND seed fail-clear, shared Substrait preprocess, adapter arity bounds, span hardening, SHA2 message

Review-driven hardening on top of the initial PR:

- RandSeedAdapter: stop silently dropping the rand(seed) operand (which turned a
  deterministic seeded call into a non-deterministic one). Niladic rand() still
  maps to DataFusion random(); seeded rand(seed) now fails with a clear
  unsupported-shape error until a seeded random is available on the backend.
- DataFusionFragmentConvertor: extract preprocessForSubstrait() and call it from
  both convertToSubstrait and convertStandalone so TimestampSubtractRewriter
  (and any future rewriter) runs on the wrapper/partial-aggregate path too, not
  only the top-level fragment path.
- NowFspAdapter / RandSeedAdapter: only normalize the valid 0-arg / 1-arg shapes;
  leave unexpected arities untouched instead of inventing a valid call.
- SpanAdapter: guard the month and second interval multiplications with
  Math.multiplyExact (clear "interval is too large" error on overflow), and use
  floored modulo/division for the month-index bucketing so pre-1970 (negative
  month index) timestamps snap down to the correct bucket start.
- Sha2FunctionAdapter: list the supported bit lengths (224, 256, 384, 512) in the
  unsupported-algorithm error.

Tests: add NowFsp unexpected-arity test, RAND seeded-reject test, span
month-bucket + overflow tests; update SHA2 test to assert the supported-values
message.

Signed-off-by: Eric Wei <mengwei.eric@gmail.com>

* [analytics-engine] Fix grouped distinct_count_approx nullability + make TimestampSubtractRewriter identity-preserving

Two correctness fixes found while verifying the review round:

- PplAggregateCallRewriter (DISTINCT_COUNT_APPROX): pin the remapped aggregate's
  explicit return type to NOT NULL BIGINT. APPROX_COUNT_DISTINCT is a count, so its
  return-type inference is BIGINT NOT NULL; reusing the PPL call's nullable BIGINT
  left the declared and inferred types disagreeing and tripped Calcite's validity
  assertion ("aggCall type BIGINT vs inferred BIGINT NOT NULL") on the grouped
  two-phase path (e.g. `stats distinct_count_approx(x) by g`). This was a latent bug
  in the un-grouped-only test coverage; the grouped form is now verified green.

- TimestampSubtractRewriter: make it a true identity no-op for plans without a
  MINUS(timestamp, timestamp). Previously it walked every RelNode and called
  RelNode.accept(RexShuttle) unconditionally, which re-derives expression / aggCall
  types and can flip a cached nullable BIGINT to BIGINT NOT NULL — breaking unrelated
  shapes once the shared preprocessing pipeline runs it on the two-phase aggregate
  path. It now detects the target shape first (read-only visitors) and only applies
  the rewriting RexShuttle to nodes that actually contain it, returning the original
  object otherwise. Adds identity (assertSame) regression tests.

Signed-off-by: Eric Wei <mengwei.eric@gmail.com>

* [analytics-engine] Drop TimestampSubtractRewriter — superseded by upstream MinusAdapter (#21978)

Upstream #21978 added MinusAdapter, the proper named ScalarFunctionAdapter for
timestamp/date subtraction (t1 - t2 -> from_unixtime(to_unixtime(t1) - to_unixtime(t2))).
It also coordinates with WidthBucketAdapter by deliberately leaving the
MINUS(MAX OVER(), MIN OVER()) binning shape untouched. Our pre-Substrait
TimestampSubtractRewriter rewrote every MINUS(timestamp, timestamp) including that
binning shape, so keeping both risked clobbering the binning path. Remove the
rewriter (and its test); MinusAdapter fully covers the case. The shared
preprocessForSubstrait helper is retained for the remaining rewriters.

Signed-off-by: Eric Wei <mengwei.eric@gmail.com>

* [analytics-engine] Bind distinct_count_approx after SQL-plugin emits APPROX_COUNT_DISTINCT (#5525)

sql#5525 changed PPLBuiltinOperators.DISTINCT_COUNT_APPROX to emit a user-defined
SqlAggFunction NAMED "APPROX_COUNT_DISTINCT" (DistinctCountApproxLogicalAggFunction).
isthmus binds aggregate sigs by operator identity, not name, so this custom op has no
substrait sig and grouped/ungrouped `stats distinct_count_approx(x)` fails with
"Unable to find binding for call APPROX_COUNT_DISTINCT". (The `dc`->COUNT(DISTINCT)
path works via OpenSearchDistinctCountRule/#22013; the named UDAF path does not.)

Re-key the PplAggregateCallRewriter case from the old name DISTINCT_COUNT_APPROX to
APPROX_COUNT_DISTINCT and remap the PPL marker to stock SqlStdOperatorTable.APPROX_COUNT_DISTINCT
(which ADDITIONAL_AGGREGATE_SIGS binds to DataFusion approx_distinct), guarding against
the stock operator itself. Remove the now-dead DISTINCT_COUNT_APPROX alias in
AggregateFunction.fromNameOrError — the enum constant is APPROX_COUNT_DISTINCT, which
valueOf resolves directly (addresses review comment).

Verified: CalcitePPLAggregationIT.testCountDistinctApprox + testCountDistinctApproxWithAlias
pass on a force-routed analytics-engine cluster.

Signed-off-by: Eric Wei <mengwei.eric@gmail.com>

* [analytics-engine] Don't push filters below non-deterministic projects (fixes rand() predicates)

`eval r = rand() | where r > 0` failed with "Comparison performance-delegation requires
(RexInputRef, RexLiteral); got RAND()". Calcite's stock FILTER_PROJECT_TRANSPOSE only guards
against window functions (!containsOver()), so it pushed the filter below the rand() project,
inlining RAND() into the predicate — turning a delegatable ($ref > literal) comparison into
RAND() > literal on the scan, which Lucene performance-delegation cannot serialize (and which
must stay on the single in-memory engine, since each backend would draw different values).

Replace the stock rule with FILTER_PROJECT_TRANSPOSE_DETERMINISTIC: a FilterProjectTransposeRule
configured to refuse the transpose when the Project computes any non-deterministic expression
(RexUtil.isDeterministic). Keeping the Filter above the Project preserves the clean
($ref > literal) shape evaluated in memory.

Verified: CalcitePPLBuiltinFunctionIT.testRand passes on a force-routed analytics-engine cluster.
Signed-off-by: Eric Wei <mengwei.eric@gmail.com>

* [analytics-engine] Restore distinct_count_approx binding on the analytics-engine route

Validation requested in PR review (sandeshkr419) found that sql#5525 alone does
NOT make `stats distinct_count_approx(x)` work on the analytics-engine route, so
the engine-side handling removed in the prior commit is still required. On a
fresh, force-routed cluster with sql#5525 deployed, CalcitePPLAggregationIT
testCountDistinctApprox + testCountDistinctApproxWithAlias fail in two stages:

  1. Planner stage: OpenSearchAggregateRule.resolveViableBackendsForCall reads
     aggCall.getAggregation().getName(), which is still "DISTINCT_COUNT_APPROX"
     (the DistinctCountApproxLogicalAggFunction marker). fromNameOrError then
     throws "No enum constant ...AggregateFunction.DISTINCT_COUNT_APPROX".
  2. Substrait stage: the PplAggregateCallRewriter case keyed only on
     "APPROX_COUNT_DISTINCT" never matched the marker, so the unbound op reached
     isthmus -> "Unable to find binding for call DISTINCT_COUNT_APPROX($1)".

sql#5525 only sets the substrait-emission name to APPROX_COUNT_DISTINCT, which is
a later stage than both of the above, so the marker's runtime name still reaches
them. Fixes:

  - Restore the DISTINCT_COUNT_APPROX -> APPROX_COUNT_DISTINCT alias in
    AggregateFunction.fromNameOrError (covers the planner path).
  - Match both "DISTINCT_COUNT_APPROX" and "APPROX_COUNT_DISTINCT" in the
    PplAggregateCallRewriter case (covers the substrait-binding path); keep the
    guard that skips the already-bound stock operator.
  - Add a fromNameOrError alias unit test.

Verified on a force-routed analytics-engine cluster (parquet-backed indices,
AE-routed): both tests pass; the surrounding 4-class IT sweep is unchanged at
6 pre-existing failures (date_format/strftime/percentile/nested-field), i.e. no
regressions.

Signed-off-by: Eric Wei <mengwei.eric@gmail.com>

* [analytics-engine] Add qa-module ITs for distinct_count_approx + min/max(boolean)

Engine-side end-to-end regression coverage for the two analytics-engine binding
fixes in this PR, run against the production /_plugins/_ppl surface on the
qa-module's analytics-routed cluster (mirrors the SQL-repo CalcitePPLAggregationIT
cases that proved the fixes):

  - DistinctCountApproxAggregationIT: grouped / aliased / dc-shorthand / ungrouped
    distinct_count_approx (guards the planner enum alias + rewriter rebind).
  - MinMaxBooleanAggregationIT: ungrouped / grouped / mixed boolean+string
    min/max (guards the boolean min/max substrait overload).

Each IT creates its own parquet-backed (composite/parquet + lucene secondary)
index, bulk-ingests a small fixed dataset, and asserts oracle values. All 7
tests pass (4 + 3).

Signed-off-by: Eric Wei <mengwei.eric@gmail.com>

* [analytics-engine] Fix date_format %c month padding; correct stale RAND comments

Two small follow-ups surfaced while verifying the analytics-engine route:

  - date_format/strftime %c now emits the zero-padded month (Jan -> "01"),
    matching the PPL reference (SQL-plugin DateTimeFormatterUtil maps %c -> "MM"
    in date mode), which differs from stock MySQL's no-leading-zero %c. Time mode
    keeps the reference's single-"0" literal. Fixes the %c portion of
    CalciteDateTimeFunctionIT.testDateFormat and updates the Rust token-set unit
    test expectation accordingly. (The test's %U/%u/%V/%v week-number padding is a
    separate, unrelated date-format issue left for a follow-up.)

  - Correct two stale comments that described the old "drop the seed" RAND
    behavior; the adapter now rejects RAND(seed) with a clear error (addresses
    review feedback). No behavior change — comment/javadoc only.

Verified on a force-routed analytics-engine cluster: date_format(ts,'%c') returns
"01"; the now/sha2/dc/min-max ITs remain green (no regressions).

Signed-off-by: Eric Wei <mengwei.eric@gmail.com>

* [analytics-engine] Drop DISTINCT_COUNT_APPROX enum alias per review

Remove the DISTINCT_COUNT_APPROX -> APPROX_COUNT_DISTINCT alias in
AggregateFunction.fromNameOrError (and its unit test), and key the
PplAggregateCallRewriter case on "APPROX_COUNT_DISTINCT" only, per
@sandeshkr419's review. The SQL plugin (sql#5525) emits the operator named
APPROX_COUNT_DISTINCT, which the enum resolves directly, so the legacy-name
alias is no longer needed.

Signed-off-by: Eric Wei <mengwei.eric@gmail.com>

* [analytics-engine] Address review: bulk NDJSON content-type, dc comments, error wording

Review follow-ups (sandeshkr419):

  - QA ITs: send _bulk as application/x-ndjson via the per-index endpoint with bare
    {"index": {}} action metadata, matching the existing CountFastPathIT pattern.
    setJsonEntity alone defaults to application/json, which the bulk endpoint rejects.
  - Clarify the distinct_count_approx marker naming across layers: current runtime name
    is APPROX_COUNT_DISTINCT (sql#5525), DISTINCT_COUNT_APPROX is the legacy/defensive
    spelling; OpenSearchDistinctCountRule is the primary planner rewrite and
    PplAggregateCallRewriter is a late Substrait-emission defensive fallback.
  - Document the phase-specific nullability handling: the early planner rewrite preserves
    the marker's nullable type (LogicalAggregate.copy re-validates row type); the late
    Substrait rewrite pins NOT NULL (Calcite validates against the stock op's inferred type).
  - SPAN overflow message now includes the operands.
  - Drop "yet" from the seeded-RAND error so it doesn't imply imminent support.

Signed-off-by: Eric Wei <mengwei.eric@gmail.com>

* [analytics-engine] Empty commit to retrigger CI

No code change. The prior gradle-check failed in the Jenkins-trigger step
(jq parse error polling the Jenkins API, 10 retries, empty result) — a
transient CI-infra glitch unrelated to this PR's changes. Re-running checks.

Signed-off-by: Eric Wei <mengwei.eric@gmail.com>

* [analytics-engine] Fix DatetimeCoverageIT %c assertions to match zero-padded month

The date_format %c code change (no-leading-zero -> zero-padded "MM") aligns the
analytics-engine Rust UDF with the legacy PPL contract: DateTimeFormatterUtil.DATE_HANDLERS
maps %c -> "MM" (date_format dispatches through DATE_HANDLERS), so January renders as "01".
The Rust code + unit test were updated for this, but the two DatetimeCoverageIT assertions
still expected the old unpadded "1" — the gradle-check / sandbox-check failures.

Update both assertions to "01", matching the canonical merged ITs (ppl/DateTimeFunctionIT
and sql/DateTimeFunctionIT both assert "Sat Jan 01 31st ..." for the same spec). Test-only.

Signed-off-by: Eric Wei <mengwei.eric@gmail.com>

* [analytics-engine] Enable now()/rand(seed)/sha2/span-month on the analytics-engine route + review fixes

Enables these PPL functions on the force-routed analytics-engine (DataFusion)
path and addresses review feedback:

- now()-family: NowFspAdapter handles now([fsp]) / current_timestamp /
  localtimestamp, where fsp is MySQL fractional-seconds precision (0-6).
- rand([seed]): RandSeedAdapter.
- sha2(input, bitLen): Sha2FunctionAdapter -> encode(digest(input, 'shaN'), 'hex'),
  with a backend fail-clear guard for unsupported bit lengths (TODO to move
  validation to the frontend once the handoff carries function metadata).
- span() month bucketing: SpanAdapter.
- strftime: os_strftime.rs.
- date_format %c: zero-padded month (DatetimeCoverageIT updated to match the
  merged ppl/sql DateTimeFunctionIT expectations).
- PlannerImpl: don't push filters below non-deterministic projects; rand() in a
  pushed-down predicate would be re-evaluated and draw a fresh value. Documented
  as a semantic-correctness guard.
- DataFusionFragmentConvertor: shared Substrait preprocess entry point.
- New QA ITs: MinMaxBooleanAggregationIT.

Review fixes: removed the late defensive APPROX_COUNT_DISTINCT fallback in
PplAggregateCallRewriter (fail loud instead); restored the Substrait-layer
assessment TODO; clarified fsp in NowFspAdapter; tidied AggregateFunction.

distinct_count_approx on the analytics-engine route is handled upstream by #22120
and is not part of this PR.

Signed-off-by: Eric Wei <mengwei.eric@gmail.com>

* [analytics-engine] Add 2-shard reduce coverage for boolean min/max

Addresses review feedback to cover the multi-shard case for boolean min/max.
min(flag)/max(flag) over the merge_coverage dataset's boolean field are added as
golden cases in the TwoShardReduceTestCase suite (#21951 convention), so they run
at 1 shard and 2 shards with a differential equality check plus a pinned golden,
exercising the two-phase per-shard-partial then coordinator-merge reduce path.
The merge_coverage flag field has 15 false and 15 true rows, so min=false and
max=true regardless of shard layout.

Signed-off-by: Eric Wei <mengwei.eric@gmail.com>

---------

Signed-off-by: Eric Wei <mengwei.eric@gmail.com>
OVyshnevskyi pushed a commit to OVyshnevskyi/OpenSearch that referenced this pull request Jun 22, 2026
…lytics-engine route + review fixes (opensearch-project#21975)

* [analytics-engine] Fix now()-family + enable now(fsp)/rand(seed)/sha2/span-month/ts-subtract/dc-approx on the analytics-engine route

Closes several PPL scalar/aggregate gaps on the analytics-engine (DataFusion)
route. Each was either an "Unable to convert call ..." Substrait failure or a
result-rendering gap; all are verified end-to-end via the CalcitePPL*IT remote
ITs on a parquet/composite (analytics-engine-routed) cluster.

now()-family (CalciteNowLikeFunctionIT 12/12):
- ArrowValues now converts Arrow Date32/Date64 -> LocalDate and
  Time{Sec,Milli,Micro,Nano} -> LocalTime so DATE/TIME results render as
  "uuuu-MM-dd" / "HH:mm:ss" via ExprValueUtils.fromObjectValue's existing
  temporal branch (mirrors how TIMESTAMP already arrives as LocalDateTime).
  Fixes current_date/curdate/current_time/curtime returning raw epoch-day /
  units-of-day integers.
- UTC_TIMESTAMP/UTC_DATE/UTC_TIME enum constants + adapter registrations route
  to the same DataFusion builtins as their CURRENT_* equivalents (cluster runs
  in UTC), reusing the existing now/currentDate/currentTime adapters.
- NowFspAdapter drops the optional fractional-seconds-precision arg so
  now(fsp)/current_timestamp(fsp)/sysdate(fsp) map to DataFusion's niladic
  now() instead of failing as "Unable to convert call now(i32)".

Other scalar/aggregate fixes:
- RandSeedAdapter drops the optional rand(seed) operand -> DataFusion random().
- Sha2FunctionAdapter raises a clear "Unsupported SHA2 algorithm [N]" for a
  concrete unsupported literal bit length, matching the SQL-plugin reference
  (CryptographicFunction) instead of surfacing a cryptic Substrait error.
- SpanAdapter supports variable-length month/quarter/year buckets.
- TimestampSubtractRewriter rewrites MINUS(timestamp, timestamp) to an
  epoch-second difference (to_unixtime), which is Substrait-convertible.
- PplAggregateCallRewriter + AggregateFunction map DISTINCT_COUNT_APPROX to
  APPROX_COUNT_DISTINCT (dc/distinct_count approx form).

Adds unit tests for the new adapters/rewriter and ArrowValues DATE/TIME
conversion; updates the SHA2 adapter test to assert the clear-error behavior.

Signed-off-by: Eric Wei <mengwei.eric@gmail.com>

* [analytics-engine] Address review: RAND seed fail-clear, shared Substrait preprocess, adapter arity bounds, span hardening, SHA2 message

Review-driven hardening on top of the initial PR:

- RandSeedAdapter: stop silently dropping the rand(seed) operand (which turned a
  deterministic seeded call into a non-deterministic one). Niladic rand() still
  maps to DataFusion random(); seeded rand(seed) now fails with a clear
  unsupported-shape error until a seeded random is available on the backend.
- DataFusionFragmentConvertor: extract preprocessForSubstrait() and call it from
  both convertToSubstrait and convertStandalone so TimestampSubtractRewriter
  (and any future rewriter) runs on the wrapper/partial-aggregate path too, not
  only the top-level fragment path.
- NowFspAdapter / RandSeedAdapter: only normalize the valid 0-arg / 1-arg shapes;
  leave unexpected arities untouched instead of inventing a valid call.
- SpanAdapter: guard the month and second interval multiplications with
  Math.multiplyExact (clear "interval is too large" error on overflow), and use
  floored modulo/division for the month-index bucketing so pre-1970 (negative
  month index) timestamps snap down to the correct bucket start.
- Sha2FunctionAdapter: list the supported bit lengths (224, 256, 384, 512) in the
  unsupported-algorithm error.

Tests: add NowFsp unexpected-arity test, RAND seeded-reject test, span
month-bucket + overflow tests; update SHA2 test to assert the supported-values
message.

Signed-off-by: Eric Wei <mengwei.eric@gmail.com>

* [analytics-engine] Fix grouped distinct_count_approx nullability + make TimestampSubtractRewriter identity-preserving

Two correctness fixes found while verifying the review round:

- PplAggregateCallRewriter (DISTINCT_COUNT_APPROX): pin the remapped aggregate's
  explicit return type to NOT NULL BIGINT. APPROX_COUNT_DISTINCT is a count, so its
  return-type inference is BIGINT NOT NULL; reusing the PPL call's nullable BIGINT
  left the declared and inferred types disagreeing and tripped Calcite's validity
  assertion ("aggCall type BIGINT vs inferred BIGINT NOT NULL") on the grouped
  two-phase path (e.g. `stats distinct_count_approx(x) by g`). This was a latent bug
  in the un-grouped-only test coverage; the grouped form is now verified green.

- TimestampSubtractRewriter: make it a true identity no-op for plans without a
  MINUS(timestamp, timestamp). Previously it walked every RelNode and called
  RelNode.accept(RexShuttle) unconditionally, which re-derives expression / aggCall
  types and can flip a cached nullable BIGINT to BIGINT NOT NULL — breaking unrelated
  shapes once the shared preprocessing pipeline runs it on the two-phase aggregate
  path. It now detects the target shape first (read-only visitors) and only applies
  the rewriting RexShuttle to nodes that actually contain it, returning the original
  object otherwise. Adds identity (assertSame) regression tests.

Signed-off-by: Eric Wei <mengwei.eric@gmail.com>

* [analytics-engine] Drop TimestampSubtractRewriter — superseded by upstream MinusAdapter (opensearch-project#21978)

Upstream opensearch-project#21978 added MinusAdapter, the proper named ScalarFunctionAdapter for
timestamp/date subtraction (t1 - t2 -> from_unixtime(to_unixtime(t1) - to_unixtime(t2))).
It also coordinates with WidthBucketAdapter by deliberately leaving the
MINUS(MAX OVER(), MIN OVER()) binning shape untouched. Our pre-Substrait
TimestampSubtractRewriter rewrote every MINUS(timestamp, timestamp) including that
binning shape, so keeping both risked clobbering the binning path. Remove the
rewriter (and its test); MinusAdapter fully covers the case. The shared
preprocessForSubstrait helper is retained for the remaining rewriters.

Signed-off-by: Eric Wei <mengwei.eric@gmail.com>

* [analytics-engine] Bind distinct_count_approx after SQL-plugin emits APPROX_COUNT_DISTINCT (opensearch-project#5525)

sql#5525 changed PPLBuiltinOperators.DISTINCT_COUNT_APPROX to emit a user-defined
SqlAggFunction NAMED "APPROX_COUNT_DISTINCT" (DistinctCountApproxLogicalAggFunction).
isthmus binds aggregate sigs by operator identity, not name, so this custom op has no
substrait sig and grouped/ungrouped `stats distinct_count_approx(x)` fails with
"Unable to find binding for call APPROX_COUNT_DISTINCT". (The `dc`->COUNT(DISTINCT)
path works via OpenSearchDistinctCountRule/opensearch-project#22013; the named UDAF path does not.)

Re-key the PplAggregateCallRewriter case from the old name DISTINCT_COUNT_APPROX to
APPROX_COUNT_DISTINCT and remap the PPL marker to stock SqlStdOperatorTable.APPROX_COUNT_DISTINCT
(which ADDITIONAL_AGGREGATE_SIGS binds to DataFusion approx_distinct), guarding against
the stock operator itself. Remove the now-dead DISTINCT_COUNT_APPROX alias in
AggregateFunction.fromNameOrError — the enum constant is APPROX_COUNT_DISTINCT, which
valueOf resolves directly (addresses review comment).

Verified: CalcitePPLAggregationIT.testCountDistinctApprox + testCountDistinctApproxWithAlias
pass on a force-routed analytics-engine cluster.

Signed-off-by: Eric Wei <mengwei.eric@gmail.com>

* [analytics-engine] Don't push filters below non-deterministic projects (fixes rand() predicates)

`eval r = rand() | where r > 0` failed with "Comparison performance-delegation requires
(RexInputRef, RexLiteral); got RAND()". Calcite's stock FILTER_PROJECT_TRANSPOSE only guards
against window functions (!containsOver()), so it pushed the filter below the rand() project,
inlining RAND() into the predicate — turning a delegatable ($ref > literal) comparison into
RAND() > literal on the scan, which Lucene performance-delegation cannot serialize (and which
must stay on the single in-memory engine, since each backend would draw different values).

Replace the stock rule with FILTER_PROJECT_TRANSPOSE_DETERMINISTIC: a FilterProjectTransposeRule
configured to refuse the transpose when the Project computes any non-deterministic expression
(RexUtil.isDeterministic). Keeping the Filter above the Project preserves the clean
($ref > literal) shape evaluated in memory.

Verified: CalcitePPLBuiltinFunctionIT.testRand passes on a force-routed analytics-engine cluster.
Signed-off-by: Eric Wei <mengwei.eric@gmail.com>

* [analytics-engine] Restore distinct_count_approx binding on the analytics-engine route

Validation requested in PR review (sandeshkr419) found that sql#5525 alone does
NOT make `stats distinct_count_approx(x)` work on the analytics-engine route, so
the engine-side handling removed in the prior commit is still required. On a
fresh, force-routed cluster with sql#5525 deployed, CalcitePPLAggregationIT
testCountDistinctApprox + testCountDistinctApproxWithAlias fail in two stages:

  1. Planner stage: OpenSearchAggregateRule.resolveViableBackendsForCall reads
     aggCall.getAggregation().getName(), which is still "DISTINCT_COUNT_APPROX"
     (the DistinctCountApproxLogicalAggFunction marker). fromNameOrError then
     throws "No enum constant ...AggregateFunction.DISTINCT_COUNT_APPROX".
  2. Substrait stage: the PplAggregateCallRewriter case keyed only on
     "APPROX_COUNT_DISTINCT" never matched the marker, so the unbound op reached
     isthmus -> "Unable to find binding for call DISTINCT_COUNT_APPROX($1)".

sql#5525 only sets the substrait-emission name to APPROX_COUNT_DISTINCT, which is
a later stage than both of the above, so the marker's runtime name still reaches
them. Fixes:

  - Restore the DISTINCT_COUNT_APPROX -> APPROX_COUNT_DISTINCT alias in
    AggregateFunction.fromNameOrError (covers the planner path).
  - Match both "DISTINCT_COUNT_APPROX" and "APPROX_COUNT_DISTINCT" in the
    PplAggregateCallRewriter case (covers the substrait-binding path); keep the
    guard that skips the already-bound stock operator.
  - Add a fromNameOrError alias unit test.

Verified on a force-routed analytics-engine cluster (parquet-backed indices,
AE-routed): both tests pass; the surrounding 4-class IT sweep is unchanged at
6 pre-existing failures (date_format/strftime/percentile/nested-field), i.e. no
regressions.

Signed-off-by: Eric Wei <mengwei.eric@gmail.com>

* [analytics-engine] Add qa-module ITs for distinct_count_approx + min/max(boolean)

Engine-side end-to-end regression coverage for the two analytics-engine binding
fixes in this PR, run against the production /_plugins/_ppl surface on the
qa-module's analytics-routed cluster (mirrors the SQL-repo CalcitePPLAggregationIT
cases that proved the fixes):

  - DistinctCountApproxAggregationIT: grouped / aliased / dc-shorthand / ungrouped
    distinct_count_approx (guards the planner enum alias + rewriter rebind).
  - MinMaxBooleanAggregationIT: ungrouped / grouped / mixed boolean+string
    min/max (guards the boolean min/max substrait overload).

Each IT creates its own parquet-backed (composite/parquet + lucene secondary)
index, bulk-ingests a small fixed dataset, and asserts oracle values. All 7
tests pass (4 + 3).

Signed-off-by: Eric Wei <mengwei.eric@gmail.com>

* [analytics-engine] Fix date_format %c month padding; correct stale RAND comments

Two small follow-ups surfaced while verifying the analytics-engine route:

  - date_format/strftime %c now emits the zero-padded month (Jan -> "01"),
    matching the PPL reference (SQL-plugin DateTimeFormatterUtil maps %c -> "MM"
    in date mode), which differs from stock MySQL's no-leading-zero %c. Time mode
    keeps the reference's single-"0" literal. Fixes the %c portion of
    CalciteDateTimeFunctionIT.testDateFormat and updates the Rust token-set unit
    test expectation accordingly. (The test's %U/%u/%V/%v week-number padding is a
    separate, unrelated date-format issue left for a follow-up.)

  - Correct two stale comments that described the old "drop the seed" RAND
    behavior; the adapter now rejects RAND(seed) with a clear error (addresses
    review feedback). No behavior change — comment/javadoc only.

Verified on a force-routed analytics-engine cluster: date_format(ts,'%c') returns
"01"; the now/sha2/dc/min-max ITs remain green (no regressions).

Signed-off-by: Eric Wei <mengwei.eric@gmail.com>

* [analytics-engine] Drop DISTINCT_COUNT_APPROX enum alias per review

Remove the DISTINCT_COUNT_APPROX -> APPROX_COUNT_DISTINCT alias in
AggregateFunction.fromNameOrError (and its unit test), and key the
PplAggregateCallRewriter case on "APPROX_COUNT_DISTINCT" only, per
@sandeshkr419's review. The SQL plugin (sql#5525) emits the operator named
APPROX_COUNT_DISTINCT, which the enum resolves directly, so the legacy-name
alias is no longer needed.

Signed-off-by: Eric Wei <mengwei.eric@gmail.com>

* [analytics-engine] Address review: bulk NDJSON content-type, dc comments, error wording

Review follow-ups (sandeshkr419):

  - QA ITs: send _bulk as application/x-ndjson via the per-index endpoint with bare
    {"index": {}} action metadata, matching the existing CountFastPathIT pattern.
    setJsonEntity alone defaults to application/json, which the bulk endpoint rejects.
  - Clarify the distinct_count_approx marker naming across layers: current runtime name
    is APPROX_COUNT_DISTINCT (sql#5525), DISTINCT_COUNT_APPROX is the legacy/defensive
    spelling; OpenSearchDistinctCountRule is the primary planner rewrite and
    PplAggregateCallRewriter is a late Substrait-emission defensive fallback.
  - Document the phase-specific nullability handling: the early planner rewrite preserves
    the marker's nullable type (LogicalAggregate.copy re-validates row type); the late
    Substrait rewrite pins NOT NULL (Calcite validates against the stock op's inferred type).
  - SPAN overflow message now includes the operands.
  - Drop "yet" from the seeded-RAND error so it doesn't imply imminent support.

Signed-off-by: Eric Wei <mengwei.eric@gmail.com>

* [analytics-engine] Empty commit to retrigger CI

No code change. The prior gradle-check failed in the Jenkins-trigger step
(jq parse error polling the Jenkins API, 10 retries, empty result) — a
transient CI-infra glitch unrelated to this PR's changes. Re-running checks.

Signed-off-by: Eric Wei <mengwei.eric@gmail.com>

* [analytics-engine] Fix DatetimeCoverageIT %c assertions to match zero-padded month

The date_format %c code change (no-leading-zero -> zero-padded "MM") aligns the
analytics-engine Rust UDF with the legacy PPL contract: DateTimeFormatterUtil.DATE_HANDLERS
maps %c -> "MM" (date_format dispatches through DATE_HANDLERS), so January renders as "01".
The Rust code + unit test were updated for this, but the two DatetimeCoverageIT assertions
still expected the old unpadded "1" — the gradle-check / sandbox-check failures.

Update both assertions to "01", matching the canonical merged ITs (ppl/DateTimeFunctionIT
and sql/DateTimeFunctionIT both assert "Sat Jan 01 31st ..." for the same spec). Test-only.

Signed-off-by: Eric Wei <mengwei.eric@gmail.com>

* [analytics-engine] Enable now()/rand(seed)/sha2/span-month on the analytics-engine route + review fixes

Enables these PPL functions on the force-routed analytics-engine (DataFusion)
path and addresses review feedback:

- now()-family: NowFspAdapter handles now([fsp]) / current_timestamp /
  localtimestamp, where fsp is MySQL fractional-seconds precision (0-6).
- rand([seed]): RandSeedAdapter.
- sha2(input, bitLen): Sha2FunctionAdapter -> encode(digest(input, 'shaN'), 'hex'),
  with a backend fail-clear guard for unsupported bit lengths (TODO to move
  validation to the frontend once the handoff carries function metadata).
- span() month bucketing: SpanAdapter.
- strftime: os_strftime.rs.
- date_format %c: zero-padded month (DatetimeCoverageIT updated to match the
  merged ppl/sql DateTimeFunctionIT expectations).
- PlannerImpl: don't push filters below non-deterministic projects; rand() in a
  pushed-down predicate would be re-evaluated and draw a fresh value. Documented
  as a semantic-correctness guard.
- DataFusionFragmentConvertor: shared Substrait preprocess entry point.
- New QA ITs: MinMaxBooleanAggregationIT.

Review fixes: removed the late defensive APPROX_COUNT_DISTINCT fallback in
PplAggregateCallRewriter (fail loud instead); restored the Substrait-layer
assessment TODO; clarified fsp in NowFspAdapter; tidied AggregateFunction.

distinct_count_approx on the analytics-engine route is handled upstream by opensearch-project#22120
and is not part of this PR.

Signed-off-by: Eric Wei <mengwei.eric@gmail.com>

* [analytics-engine] Add 2-shard reduce coverage for boolean min/max

Addresses review feedback to cover the multi-shard case for boolean min/max.
min(flag)/max(flag) over the merge_coverage dataset's boolean field are added as
golden cases in the TwoShardReduceTestCase suite (opensearch-project#21951 convention), so they run
at 1 shard and 2 shards with a differential equality check plus a pinned golden,
exercising the two-phase per-shard-partial then coordinator-merge reduce path.
The merge_coverage flag field has 15 false and 15 true rows, so min=false and
max=true regardless of shard layout.

Signed-off-by: Eric Wei <mengwei.eric@gmail.com>

---------

Signed-off-by: Eric Wei <mengwei.eric@gmail.com>
KhishorekumarBS pushed a commit to KhishorekumarBS/OpenSearch that referenced this pull request Jul 3, 2026
…ject#21978)

Signed-off-by: Vinay Krishna Pudyodu <vinkrish.neo@gmail.com>
KhishorekumarBS pushed a commit to KhishorekumarBS/OpenSearch that referenced this pull request Jul 3, 2026
…lytics-engine route + review fixes (opensearch-project#21975)

* [analytics-engine] Fix now()-family + enable now(fsp)/rand(seed)/sha2/span-month/ts-subtract/dc-approx on the analytics-engine route

Closes several PPL scalar/aggregate gaps on the analytics-engine (DataFusion)
route. Each was either an "Unable to convert call ..." Substrait failure or a
result-rendering gap; all are verified end-to-end via the CalcitePPL*IT remote
ITs on a parquet/composite (analytics-engine-routed) cluster.

now()-family (CalciteNowLikeFunctionIT 12/12):
- ArrowValues now converts Arrow Date32/Date64 -> LocalDate and
  Time{Sec,Milli,Micro,Nano} -> LocalTime so DATE/TIME results render as
  "uuuu-MM-dd" / "HH:mm:ss" via ExprValueUtils.fromObjectValue's existing
  temporal branch (mirrors how TIMESTAMP already arrives as LocalDateTime).
  Fixes current_date/curdate/current_time/curtime returning raw epoch-day /
  units-of-day integers.
- UTC_TIMESTAMP/UTC_DATE/UTC_TIME enum constants + adapter registrations route
  to the same DataFusion builtins as their CURRENT_* equivalents (cluster runs
  in UTC), reusing the existing now/currentDate/currentTime adapters.
- NowFspAdapter drops the optional fractional-seconds-precision arg so
  now(fsp)/current_timestamp(fsp)/sysdate(fsp) map to DataFusion's niladic
  now() instead of failing as "Unable to convert call now(i32)".

Other scalar/aggregate fixes:
- RandSeedAdapter drops the optional rand(seed) operand -> DataFusion random().
- Sha2FunctionAdapter raises a clear "Unsupported SHA2 algorithm [N]" for a
  concrete unsupported literal bit length, matching the SQL-plugin reference
  (CryptographicFunction) instead of surfacing a cryptic Substrait error.
- SpanAdapter supports variable-length month/quarter/year buckets.
- TimestampSubtractRewriter rewrites MINUS(timestamp, timestamp) to an
  epoch-second difference (to_unixtime), which is Substrait-convertible.
- PplAggregateCallRewriter + AggregateFunction map DISTINCT_COUNT_APPROX to
  APPROX_COUNT_DISTINCT (dc/distinct_count approx form).

Adds unit tests for the new adapters/rewriter and ArrowValues DATE/TIME
conversion; updates the SHA2 adapter test to assert the clear-error behavior.

Signed-off-by: Eric Wei <mengwei.eric@gmail.com>

* [analytics-engine] Address review: RAND seed fail-clear, shared Substrait preprocess, adapter arity bounds, span hardening, SHA2 message

Review-driven hardening on top of the initial PR:

- RandSeedAdapter: stop silently dropping the rand(seed) operand (which turned a
  deterministic seeded call into a non-deterministic one). Niladic rand() still
  maps to DataFusion random(); seeded rand(seed) now fails with a clear
  unsupported-shape error until a seeded random is available on the backend.
- DataFusionFragmentConvertor: extract preprocessForSubstrait() and call it from
  both convertToSubstrait and convertStandalone so TimestampSubtractRewriter
  (and any future rewriter) runs on the wrapper/partial-aggregate path too, not
  only the top-level fragment path.
- NowFspAdapter / RandSeedAdapter: only normalize the valid 0-arg / 1-arg shapes;
  leave unexpected arities untouched instead of inventing a valid call.
- SpanAdapter: guard the month and second interval multiplications with
  Math.multiplyExact (clear "interval is too large" error on overflow), and use
  floored modulo/division for the month-index bucketing so pre-1970 (negative
  month index) timestamps snap down to the correct bucket start.
- Sha2FunctionAdapter: list the supported bit lengths (224, 256, 384, 512) in the
  unsupported-algorithm error.

Tests: add NowFsp unexpected-arity test, RAND seeded-reject test, span
month-bucket + overflow tests; update SHA2 test to assert the supported-values
message.

Signed-off-by: Eric Wei <mengwei.eric@gmail.com>

* [analytics-engine] Fix grouped distinct_count_approx nullability + make TimestampSubtractRewriter identity-preserving

Two correctness fixes found while verifying the review round:

- PplAggregateCallRewriter (DISTINCT_COUNT_APPROX): pin the remapped aggregate's
  explicit return type to NOT NULL BIGINT. APPROX_COUNT_DISTINCT is a count, so its
  return-type inference is BIGINT NOT NULL; reusing the PPL call's nullable BIGINT
  left the declared and inferred types disagreeing and tripped Calcite's validity
  assertion ("aggCall type BIGINT vs inferred BIGINT NOT NULL") on the grouped
  two-phase path (e.g. `stats distinct_count_approx(x) by g`). This was a latent bug
  in the un-grouped-only test coverage; the grouped form is now verified green.

- TimestampSubtractRewriter: make it a true identity no-op for plans without a
  MINUS(timestamp, timestamp). Previously it walked every RelNode and called
  RelNode.accept(RexShuttle) unconditionally, which re-derives expression / aggCall
  types and can flip a cached nullable BIGINT to BIGINT NOT NULL — breaking unrelated
  shapes once the shared preprocessing pipeline runs it on the two-phase aggregate
  path. It now detects the target shape first (read-only visitors) and only applies
  the rewriting RexShuttle to nodes that actually contain it, returning the original
  object otherwise. Adds identity (assertSame) regression tests.

Signed-off-by: Eric Wei <mengwei.eric@gmail.com>

* [analytics-engine] Drop TimestampSubtractRewriter — superseded by upstream MinusAdapter (opensearch-project#21978)

Upstream opensearch-project#21978 added MinusAdapter, the proper named ScalarFunctionAdapter for
timestamp/date subtraction (t1 - t2 -> from_unixtime(to_unixtime(t1) - to_unixtime(t2))).
It also coordinates with WidthBucketAdapter by deliberately leaving the
MINUS(MAX OVER(), MIN OVER()) binning shape untouched. Our pre-Substrait
TimestampSubtractRewriter rewrote every MINUS(timestamp, timestamp) including that
binning shape, so keeping both risked clobbering the binning path. Remove the
rewriter (and its test); MinusAdapter fully covers the case. The shared
preprocessForSubstrait helper is retained for the remaining rewriters.

Signed-off-by: Eric Wei <mengwei.eric@gmail.com>

* [analytics-engine] Bind distinct_count_approx after SQL-plugin emits APPROX_COUNT_DISTINCT (opensearch-project#5525)

sql#5525 changed PPLBuiltinOperators.DISTINCT_COUNT_APPROX to emit a user-defined
SqlAggFunction NAMED "APPROX_COUNT_DISTINCT" (DistinctCountApproxLogicalAggFunction).
isthmus binds aggregate sigs by operator identity, not name, so this custom op has no
substrait sig and grouped/ungrouped `stats distinct_count_approx(x)` fails with
"Unable to find binding for call APPROX_COUNT_DISTINCT". (The `dc`->COUNT(DISTINCT)
path works via OpenSearchDistinctCountRule/opensearch-project#22013; the named UDAF path does not.)

Re-key the PplAggregateCallRewriter case from the old name DISTINCT_COUNT_APPROX to
APPROX_COUNT_DISTINCT and remap the PPL marker to stock SqlStdOperatorTable.APPROX_COUNT_DISTINCT
(which ADDITIONAL_AGGREGATE_SIGS binds to DataFusion approx_distinct), guarding against
the stock operator itself. Remove the now-dead DISTINCT_COUNT_APPROX alias in
AggregateFunction.fromNameOrError — the enum constant is APPROX_COUNT_DISTINCT, which
valueOf resolves directly (addresses review comment).

Verified: CalcitePPLAggregationIT.testCountDistinctApprox + testCountDistinctApproxWithAlias
pass on a force-routed analytics-engine cluster.

Signed-off-by: Eric Wei <mengwei.eric@gmail.com>

* [analytics-engine] Don't push filters below non-deterministic projects (fixes rand() predicates)

`eval r = rand() | where r > 0` failed with "Comparison performance-delegation requires
(RexInputRef, RexLiteral); got RAND()". Calcite's stock FILTER_PROJECT_TRANSPOSE only guards
against window functions (!containsOver()), so it pushed the filter below the rand() project,
inlining RAND() into the predicate — turning a delegatable ($ref > literal) comparison into
RAND() > literal on the scan, which Lucene performance-delegation cannot serialize (and which
must stay on the single in-memory engine, since each backend would draw different values).

Replace the stock rule with FILTER_PROJECT_TRANSPOSE_DETERMINISTIC: a FilterProjectTransposeRule
configured to refuse the transpose when the Project computes any non-deterministic expression
(RexUtil.isDeterministic). Keeping the Filter above the Project preserves the clean
($ref > literal) shape evaluated in memory.

Verified: CalcitePPLBuiltinFunctionIT.testRand passes on a force-routed analytics-engine cluster.
Signed-off-by: Eric Wei <mengwei.eric@gmail.com>

* [analytics-engine] Restore distinct_count_approx binding on the analytics-engine route

Validation requested in PR review (sandeshkr419) found that sql#5525 alone does
NOT make `stats distinct_count_approx(x)` work on the analytics-engine route, so
the engine-side handling removed in the prior commit is still required. On a
fresh, force-routed cluster with sql#5525 deployed, CalcitePPLAggregationIT
testCountDistinctApprox + testCountDistinctApproxWithAlias fail in two stages:

  1. Planner stage: OpenSearchAggregateRule.resolveViableBackendsForCall reads
     aggCall.getAggregation().getName(), which is still "DISTINCT_COUNT_APPROX"
     (the DistinctCountApproxLogicalAggFunction marker). fromNameOrError then
     throws "No enum constant ...AggregateFunction.DISTINCT_COUNT_APPROX".
  2. Substrait stage: the PplAggregateCallRewriter case keyed only on
     "APPROX_COUNT_DISTINCT" never matched the marker, so the unbound op reached
     isthmus -> "Unable to find binding for call DISTINCT_COUNT_APPROX($1)".

sql#5525 only sets the substrait-emission name to APPROX_COUNT_DISTINCT, which is
a later stage than both of the above, so the marker's runtime name still reaches
them. Fixes:

  - Restore the DISTINCT_COUNT_APPROX -> APPROX_COUNT_DISTINCT alias in
    AggregateFunction.fromNameOrError (covers the planner path).
  - Match both "DISTINCT_COUNT_APPROX" and "APPROX_COUNT_DISTINCT" in the
    PplAggregateCallRewriter case (covers the substrait-binding path); keep the
    guard that skips the already-bound stock operator.
  - Add a fromNameOrError alias unit test.

Verified on a force-routed analytics-engine cluster (parquet-backed indices,
AE-routed): both tests pass; the surrounding 4-class IT sweep is unchanged at
6 pre-existing failures (date_format/strftime/percentile/nested-field), i.e. no
regressions.

Signed-off-by: Eric Wei <mengwei.eric@gmail.com>

* [analytics-engine] Add qa-module ITs for distinct_count_approx + min/max(boolean)

Engine-side end-to-end regression coverage for the two analytics-engine binding
fixes in this PR, run against the production /_plugins/_ppl surface on the
qa-module's analytics-routed cluster (mirrors the SQL-repo CalcitePPLAggregationIT
cases that proved the fixes):

  - DistinctCountApproxAggregationIT: grouped / aliased / dc-shorthand / ungrouped
    distinct_count_approx (guards the planner enum alias + rewriter rebind).
  - MinMaxBooleanAggregationIT: ungrouped / grouped / mixed boolean+string
    min/max (guards the boolean min/max substrait overload).

Each IT creates its own parquet-backed (composite/parquet + lucene secondary)
index, bulk-ingests a small fixed dataset, and asserts oracle values. All 7
tests pass (4 + 3).

Signed-off-by: Eric Wei <mengwei.eric@gmail.com>

* [analytics-engine] Fix date_format %c month padding; correct stale RAND comments

Two small follow-ups surfaced while verifying the analytics-engine route:

  - date_format/strftime %c now emits the zero-padded month (Jan -> "01"),
    matching the PPL reference (SQL-plugin DateTimeFormatterUtil maps %c -> "MM"
    in date mode), which differs from stock MySQL's no-leading-zero %c. Time mode
    keeps the reference's single-"0" literal. Fixes the %c portion of
    CalciteDateTimeFunctionIT.testDateFormat and updates the Rust token-set unit
    test expectation accordingly. (The test's %U/%u/%V/%v week-number padding is a
    separate, unrelated date-format issue left for a follow-up.)

  - Correct two stale comments that described the old "drop the seed" RAND
    behavior; the adapter now rejects RAND(seed) with a clear error (addresses
    review feedback). No behavior change — comment/javadoc only.

Verified on a force-routed analytics-engine cluster: date_format(ts,'%c') returns
"01"; the now/sha2/dc/min-max ITs remain green (no regressions).

Signed-off-by: Eric Wei <mengwei.eric@gmail.com>

* [analytics-engine] Drop DISTINCT_COUNT_APPROX enum alias per review

Remove the DISTINCT_COUNT_APPROX -> APPROX_COUNT_DISTINCT alias in
AggregateFunction.fromNameOrError (and its unit test), and key the
PplAggregateCallRewriter case on "APPROX_COUNT_DISTINCT" only, per
@sandeshkr419's review. The SQL plugin (sql#5525) emits the operator named
APPROX_COUNT_DISTINCT, which the enum resolves directly, so the legacy-name
alias is no longer needed.

Signed-off-by: Eric Wei <mengwei.eric@gmail.com>

* [analytics-engine] Address review: bulk NDJSON content-type, dc comments, error wording

Review follow-ups (sandeshkr419):

  - QA ITs: send _bulk as application/x-ndjson via the per-index endpoint with bare
    {"index": {}} action metadata, matching the existing CountFastPathIT pattern.
    setJsonEntity alone defaults to application/json, which the bulk endpoint rejects.
  - Clarify the distinct_count_approx marker naming across layers: current runtime name
    is APPROX_COUNT_DISTINCT (sql#5525), DISTINCT_COUNT_APPROX is the legacy/defensive
    spelling; OpenSearchDistinctCountRule is the primary planner rewrite and
    PplAggregateCallRewriter is a late Substrait-emission defensive fallback.
  - Document the phase-specific nullability handling: the early planner rewrite preserves
    the marker's nullable type (LogicalAggregate.copy re-validates row type); the late
    Substrait rewrite pins NOT NULL (Calcite validates against the stock op's inferred type).
  - SPAN overflow message now includes the operands.
  - Drop "yet" from the seeded-RAND error so it doesn't imply imminent support.

Signed-off-by: Eric Wei <mengwei.eric@gmail.com>

* [analytics-engine] Empty commit to retrigger CI

No code change. The prior gradle-check failed in the Jenkins-trigger step
(jq parse error polling the Jenkins API, 10 retries, empty result) — a
transient CI-infra glitch unrelated to this PR's changes. Re-running checks.

Signed-off-by: Eric Wei <mengwei.eric@gmail.com>

* [analytics-engine] Fix DatetimeCoverageIT %c assertions to match zero-padded month

The date_format %c code change (no-leading-zero -> zero-padded "MM") aligns the
analytics-engine Rust UDF with the legacy PPL contract: DateTimeFormatterUtil.DATE_HANDLERS
maps %c -> "MM" (date_format dispatches through DATE_HANDLERS), so January renders as "01".
The Rust code + unit test were updated for this, but the two DatetimeCoverageIT assertions
still expected the old unpadded "1" — the gradle-check / sandbox-check failures.

Update both assertions to "01", matching the canonical merged ITs (ppl/DateTimeFunctionIT
and sql/DateTimeFunctionIT both assert "Sat Jan 01 31st ..." for the same spec). Test-only.

Signed-off-by: Eric Wei <mengwei.eric@gmail.com>

* [analytics-engine] Enable now()/rand(seed)/sha2/span-month on the analytics-engine route + review fixes

Enables these PPL functions on the force-routed analytics-engine (DataFusion)
path and addresses review feedback:

- now()-family: NowFspAdapter handles now([fsp]) / current_timestamp /
  localtimestamp, where fsp is MySQL fractional-seconds precision (0-6).
- rand([seed]): RandSeedAdapter.
- sha2(input, bitLen): Sha2FunctionAdapter -> encode(digest(input, 'shaN'), 'hex'),
  with a backend fail-clear guard for unsupported bit lengths (TODO to move
  validation to the frontend once the handoff carries function metadata).
- span() month bucketing: SpanAdapter.
- strftime: os_strftime.rs.
- date_format %c: zero-padded month (DatetimeCoverageIT updated to match the
  merged ppl/sql DateTimeFunctionIT expectations).
- PlannerImpl: don't push filters below non-deterministic projects; rand() in a
  pushed-down predicate would be re-evaluated and draw a fresh value. Documented
  as a semantic-correctness guard.
- DataFusionFragmentConvertor: shared Substrait preprocess entry point.
- New QA ITs: MinMaxBooleanAggregationIT.

Review fixes: removed the late defensive APPROX_COUNT_DISTINCT fallback in
PplAggregateCallRewriter (fail loud instead); restored the Substrait-layer
assessment TODO; clarified fsp in NowFspAdapter; tidied AggregateFunction.

distinct_count_approx on the analytics-engine route is handled upstream by opensearch-project#22120
and is not part of this PR.

Signed-off-by: Eric Wei <mengwei.eric@gmail.com>

* [analytics-engine] Add 2-shard reduce coverage for boolean min/max

Addresses review feedback to cover the multi-shard case for boolean min/max.
min(flag)/max(flag) over the merge_coverage dataset's boolean field are added as
golden cases in the TwoShardReduceTestCase suite (opensearch-project#21951 convention), so they run
at 1 shard and 2 shards with a differential equality check plus a pinned golden,
exercising the two-phase per-shard-partial then coordinator-merge reduce path.
The merge_coverage flag field has 15 false and 15 true rows, so min=false and
max=true regardless of shard layout.

Signed-off-by: Eric Wei <mengwei.eric@gmail.com>

---------

Signed-off-by: Eric Wei <mengwei.eric@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip-changelog skip-diff-analyzer Maintainer to skip code-diff-analyzer check, after reviewing issues in AI analysis. skip-diff-reviewer Maintainer to skip code-diff-reviewer check, after reviewing issues in AI analysis.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants