[Analytics Backend / DataFusion] Wire SPAN scalar through analytics-engine route

[ahkcs]

Summary

Wires the PPL span(...) scalar through the analytics-engine route to DataFusion. Repurposed per @sandeshkr419's review — the AVG / STDDEV / VAR custom-Sig collection, percentile_approx UDAF onboarding, and defensive try/catch wrappers have all been dropped. AVG / STDDEV_POP / STDDEV_SAMP / VAR_POP / VAR_SAMP now flow through Calcite's OpenSearchAggregateReduceRule decomposition (landed in #21645) — no hand-holding needed.

The PR now contains two commits:

1. [Analytics Backend / DataFusion] Wire SPAN scalar for stats — adds the bucketing primitive used by stats … by span(...). Numeric span lowers to FLOOR(field/interval)*interval; time span with interval=1 lowers to DATE_TRUNC(<unit>, field). Multi-unit time spans (12h, …) fall through unchanged — separate follow-up (DataFusion date_bin).
2. [QA] Add self-contained StatsCommandIT mirror — 16 self-contained tests under sandbox/qa/analytics-engine-rest exercising the stats operators reachable from this PR (SPAN, plus the broader stats family that already flows through the analytics route via [analytics-engine] Collapse AggregateDecompositionResolver into BackendPlanAdapter's adaptNode chain #21645's decomposition).

The previous [Rebase tax] Add missing FieldType import commit is dropped — #21663 fixed it upstream.

What changed since the last review

@sandeshkr419 comment	Disposition
DataFusionFragmentConvertor.java:479 — "We are decomposing the methods via Calcite decomposition. I think all these cases/functions here are covered."	Done. collectCustomAggregateSigs + substraitNameForCustomAgg removed.
SubstraitPlanRewriter.java:94 — "These changes should go away as well as they seem related to support the above decomposed methods."	Done. The whole percentile_approx UDAF onboarding commit was dropped (including the SubstraitPlanRewriter Aggregate visitor literal-lift).
DataFusionAnalyticsBackendPlugin.java:316 — "This can go away as well."	Done. STDDEV/VAR AGG_FUNCTIONS additions + AggregateCapability type-dispatch removed.

Additionally dropped (related cleanup): PlannerImpl / FragmentConversionDriver defensive AssertionError try/catch wrappers; AggregateFunction.fromNameOrError case-insensitive lookup; OpenSearchAggregateSplitRule percentile_approx skip guard.

Files

File	Change
…/analytics-framework/.../spi/ScalarFunction.java	ScalarFunction.SPAN enum entry
…/analytics-backend-datafusion/.../be/datafusion/SpanAdapter.java	New. Rewrites SPAN → FLOOR/MULTIPLY (numeric) or DATE_TRUNC (time)
…/analytics-backend-datafusion/.../be/datafusion/DataFusionAnalyticsBackendPlugin.java	Registers SpanAdapter + ScalarFunction.SPAN in PROJECT_SCALAR_FUNCTIONS
…/analytics-backend-datafusion/.../be/datafusion/DataFusionFragmentConvertor.java	+1: SqlLibraryOperators.DATE_TRUNC → "date_trunc" substrait mapping
…/analytics-backend-datafusion/src/main/resources/opensearch_scalar_functions.yaml	Declares date_trunc so the time-span rewrite resolves through isthmus binding
…/qa/analytics-engine-rest/.../analytics/qa/StatsCommandIT.java	New. 16 self-contained PPL stats tests over the parquet-backed calcs dataset

Verification

CalciteStatsCommandIT against the analytics-engine route

Configuration: this PR + sql#5435 (deterministic tiebreaker queries) + sql#5436 (tests.analytics.parquet_indices toggle). Force-routed via -Dtests.analytics.parquet_indices=true. Rebased onto current upstream/main (post-#21650 to_char rewrite, post-#21663 FieldType import fix, post-#21645 AggregateDecompositionResolver collapse).

Result	Count	Bucket
PASS	51 / 63	—
FAIL	8 / 63	percentile_approx UDAF — intentionally dropped per Sandesh's "repurpose for span explicitly" guidance. Tests: testStatsPercentile, testStatsPercentileByNullValue, testStatsPercentileByNullValueNonNullBucket, testStatsPercentileBySpan, testStatsPercentileWhere, testStatsPercentileWithCompression, testStatsPercentileWithMin, testStatsPercentileWithNull.
FAIL	1 / 63	Schema-type mismatch on span(birthdate, 1y) (now returns string instead of timestamp after #21650's to_char rewrite — separate concern). Test: testStatsTimeSpan.
FAIL	1 / 63	Multi-unit time-span unsupported (span(@timestamp, 12h) falls through SpanAdapter — only interval=1 is rewritten to DATE_TRUNC). Test: testStatsBySpanTimeWithNullBucket.
FAIL	1 / 63	head 2 from 1 returns size != 2 on the analytics path (pagination quirk to investigate). Test: testStatsWithLimit.
SKIP	1 / 63	testDisableLegacyPreferred — semantic divergence between v2 default-engine preference and the analytics route's default-on.

Delta from previous review: 50 → 51 (+1 PASS) thanks to #21650 which fixed testStatsSpanSortOnMeasure's datetime cast format divergence (sql#5420).

QA mirror

./gradlew :sandbox:qa:analytics-engine-rest:integTest -Dsandbox.enabled=true --tests "*StatsCommandIT" — 16 / 16 pass (single-value aggregates · filter + aggregate · group-by · STDDEV_POP/SAMP · VAR_POP/SAMP · numeric SPAN).

Sandbox build

./gradlew :sandbox:plugins:analytics-backend-datafusion:assemble :sandbox:plugins:analytics-engine:assemble -Dsandbox.enabled=true — passes on current upstream/main.

Reproduction

# Terminal A — boot the analytics-engine cluster (JDK 25, sandbox plugin set)
cd /path/to/OpenSearch
JAVA_HOME=/path/to/temurin-25 ./gradlew :run -Dsandbox.enabled=true \
  -PinstalledPlugins="['opensearch-job-scheduler:3.7.0.0-SNAPSHOT', \
    'arrow-flight-rpc', 'analytics-engine', 'parquet-data-format', \
    'analytics-backend-datafusion', 'analytics-backend-lucene', \
    'composite-engine', 'opensearch-sql-plugin:3.7.0.0-SNAPSHOT']"

# Terminal B — run the IT through the analytics path
cd /path/to/sql  # branch with sql#5435 + sql#5436 applied
./gradlew :integ-test:integTestRemote \
  -Dtests.rest.cluster=localhost:9200 -Dtests.cluster=localhost:9300 \
  -Dtests.clustername=runTask \
  -Dtests.analytics.parquet_indices=true \
  --tests "org.opensearch.sql.calcite.remote.CalciteStatsCommandIT"

Test plan

• :sandbox:plugins:analytics-backend-datafusion:assemble + :sandbox:plugins:analytics-engine:assemble pass on current upstream/main.
• :sandbox:qa:analytics-engine-rest:integTest --tests "*StatsCommandIT" — 16 / 16 pass.
• CalciteStatsCommandIT against the rebased analytics-engine cluster — 51 / 63 pass + 1 skip + 11 fail (breakdown above).
• CI on this PR.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 92e6cb1)

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 isUnitInterval method returns true for any Number with doubleValue() == 1.0, but this loses precision for large integers or exact decimal comparisons. For example, BigDecimal("1.0000000001") would be treated as 1.0 due to double rounding. Since the method is checking whether an interval is exactly 1 (to decide if date_trunc is applicable), this could incorrectly rewrite time spans with intervals like 1.0000001 into date_trunc calls. private static boolean isUnitInterval(RexNode interval) { if (!(interval instanceof RexLiteral lit)) { return false; } Object value = lit.getValue(); if (value instanceof BigDecimal bd) { return bd.compareTo(BigDecimal.ONE) == 0; } if (value instanceof Number n) { return n.doubleValue() == 1.0; } return false; } Incomplete Handling When unit is a non-null RexLiteral but getValueAs(String.class) returns null (e.g., if the literal holds a non-string type that cannot be coerced), the code silently falls through to return the original call. This means a malformed SPAN call with a non-string third argument will pass through unchanged and likely fail later in DataFusion with a less clear error. Consider an explicit check or comment acknowledging this scenario. if (unit instanceof RexLiteral lit && lit.getValue() != null) { String unitText = lit.getValueAs(String.class); String dateTruncUnit = unitText == null ? null : UNIT_TO_DATE_TRUNC.get(unitText); if (dateTruncUnit != null && isUnitInterval(interval)) { RexNode unitArg = rexBuilder.makeLiteral(dateTruncUnit); return rexBuilder.makeCall(original.getType(), SqlLibraryOperators.DATE_TRUNC, List.of(unitArg, field)); } } // Anything else falls through unchanged. return original;

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 92e6cb1

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Use tolerance for floating-point comparison The doubleValue() comparison using == for floating-point equality can produce incorrect results due to precision issues. For numeric literals that should equal 1, consider using a tolerance-based comparison or checking if the value is exactly 1 for integer types before converting to double. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/SpanAdapter.java [119-131] private static boolean isUnitInterval(RexNode interval) { if (!(interval instanceof RexLiteral lit)) { return false; } Object value = lit.getValue(); if (value instanceof BigDecimal bd) { return bd.compareTo(BigDecimal.ONE) == 0; } if (value instanceof Number n) { - return n.doubleValue() == 1.0; + double d = n.doubleValue(); + return Math.abs(d - 1.0) < 1e-9; } return false; } Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies a potential floating-point precision issue when comparing doubleValue() with ==. Using tolerance-based comparison (Math.abs(d - 1.0) < 1e-9) is a better practice for floating-point equality checks, though the impact is moderate since RexLiteral values are typically exact.	Low
	Simplify null-checking logic order The condition checks both SqlTypeName.NULL and lit.isNull() separately, but the second check already covers the first case when unit is a RexLiteral. Consider simplifying the logic by checking lit.isNull() first, which handles both typed-NULL literals and actual null values more directly. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/SpanAdapter.java [83-85] -if (unit.getType().getSqlTypeName() == SqlTypeName.NULL || (unit instanceof RexLiteral lit && lit.isNull())) { +if ((unit instanceof RexLiteral lit && lit.isNull()) || unit.getType().getSqlTypeName() == SqlTypeName.NULL) { return rewriteNumericSpan(rexBuilder, field, interval, original.getType()); } Suggestion importance[1-10]: 3 __ Why: The suggestion proposes reordering the null checks, but this is a minor stylistic change that doesn't impact correctness or performance. The current logic is already correct and clear, checking both SqlTypeName.NULL type and lit.isNull() covers all null cases appropriately.	Low

---

Previous suggestions

Suggestions up to commit 7adbc56

Category	Suggestion	Impact
General	Remove redundant null check The condition checks both SqlTypeName.NULL and lit.isNull() redundantly. When unit is a RexLiteral with isNull() true, its type is already SqlTypeName.NULL. Simplify by checking only the type name, which covers both typed-NULL literals and NULL-typed non-literal nodes. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/SpanAdapter.java [83-85] -if (unit.getType().getSqlTypeName() == SqlTypeName.NULL || (unit instanceof RexLiteral lit && lit.isNull())) { +if (unit.getType().getSqlTypeName() == SqlTypeName.NULL) { return rewriteNumericSpan(rexBuilder, field, interval, original.getType()); } Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies redundant null checking logic. When unit is a RexLiteral with isNull() true, its type is SqlTypeName.NULL, making the second check unnecessary. However, this is a minor code style improvement that doesn't affect correctness or performance significantly.	Low
	Simplify redundant null check The lit.getValue() != null check is redundant because getValueAs(String.class) already returns null when the literal's value is null. The subsequent unitText == null check handles this case. Remove the outer null guard to simplify the logic. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/SpanAdapter.java [88-95] -if (unit instanceof RexLiteral lit && lit.getValue() != null) { +if (unit instanceof RexLiteral lit) { String unitText = lit.getValueAs(String.class); String dateTruncUnit = unitText == null ? null : UNIT_TO_DATE_TRUNC.get(unitText); if (dateTruncUnit != null && isUnitInterval(interval)) { RexNode unitArg = rexBuilder.makeLiteral(dateTruncUnit); return rexBuilder.makeCall(original.getType(), SqlLibraryOperators.DATE_TRUNC, List.of(unitArg, field)); } } Suggestion importance[1-10]: 4 __ Why: The suggestion identifies a redundant null check since getValueAs(String.class) returns null when the literal's value is null, and the subsequent unitText == null check handles this. While correct, this is a minor simplification that marginally improves code readability without significant impact on functionality.	Low

Suggestions up to commit dc70c68

Category	Suggestion	Impact
General	Avoid exact floating-point equality check The floating-point comparison n.doubleValue() == 1.0 is unsafe for non-BigDecimal numeric types due to precision loss. Use Math.abs(n.doubleValue() - 1.0) < 1e-9 or convert to BigDecimal before comparing to avoid false negatives when the interval is logically 1 but represented as a float/double. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/SpanAdapter.java [119-131] private static boolean isUnitInterval(RexNode interval) { if (!(interval instanceof RexLiteral lit)) { return false; } Object value = lit.getValue(); if (value instanceof BigDecimal bd) { return bd.compareTo(BigDecimal.ONE) == 0; } if (value instanceof Number n) { - return n.doubleValue() == 1.0; + return Math.abs(n.doubleValue() - 1.0) < 1e-9; } return false; } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential precision issue with exact floating-point comparison (n.doubleValue() == 1.0). Using an epsilon-based comparison is a best practice for floating-point equality checks and prevents false negatives due to representation errors.	Medium
	Guard against field-name list size mismatch Accessing oldNames.get(i) without verifying i < oldNames.size() risks IndexOutOfBoundsException if the Project's field-name list is shorter than its expression list. Add a bounds check or use a safe accessor to prevent runtime failures when the schema is malformed. sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/planner/PlannerImpl.java [275-291] for (int i = 0; i < projectFieldCount; i++) { if (drop[i]) continue; RexNode expr = projectExprs.get(i); if (usedAsPercentColumn[i] && !usedByOther[i]) { // Rescale percent: CAST(percent AS DOUBLE) / 100.0 — produces a literal fp64 // value at planning time after constant fold. RexNode asDouble = rexBuilder.makeCast(fp64NotNull, expr, false); expr = rexBuilder.makeCall( SqlStdOperatorTable.DIVIDE, asDouble, rexBuilder.makeApproxLiteral(java.math.BigDecimal.valueOf(100.0)) ); } remap.put(i, newExprs.size()); newExprs.add(expr); - newNames.add(oldNames.get(i)); + String fieldName = (i < oldNames.size()) ? oldNames.get(i) : ("field_" + i); + newNames.add(fieldName); } Suggestion importance[1-10]: 6 __ Why: The suggestion identifies a potential IndexOutOfBoundsException if oldNames.size() is less than projectFieldCount. While the code assumes schema consistency, adding a bounds check improves robustness and prevents runtime failures in edge cases where the schema might be malformed.	Low
	Remove redundant negative index check The bounds check fieldIdx < 0 is redundant because simpleStructFieldIndex returns null for invalid references and never returns negative integers. Remove the fieldIdx < 0 condition to simplify the guard clause. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/SubstraitPlanRewriter.java [120-130] private Optional<AggregateFunctionInvocation> liftPercentileLiteral( AggregateFunctionInvocation func, List<Expression> projectExprs ) { List<FunctionArg> args = func.arguments(); // Expect exactly (field_ref, percent_arg) — see PlannerImpl.rewritePercentileApprox. if (args.size() != 2) return Optional.empty(); FunctionArg percentArg = args.get(1); if (!(percentArg instanceof FieldReference fieldRef)) return Optional.empty(); Integer fieldIdx = simpleStructFieldIndex(fieldRef); - if (fieldIdx == null || fieldIdx < 0 || fieldIdx >= projectExprs.size()) return Optional.empty(); + if (fieldIdx == null || fieldIdx >= projectExprs.size()) return Optional.empty(); ... } Suggestion importance[1-10]: 3 __ Why: The suggestion is technically correct that simpleStructFieldIndex returns null for invalid references rather than negative integers. However, the redundant check fieldIdx < 0 is defensive programming and doesn't harm correctness or performance, making this a minor code cleanup.	Low

Suggestions up to commit 77d2cc1

Category	Suggestion	Impact
General	Avoid fragile floating-point equality check The floating-point comparison n.doubleValue() == 1.0 is fragile and may fail for values like 1.0f stored as Float due to precision loss during conversion. Use BigDecimal comparison or an epsilon-based check to handle numeric types safely. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/SpanAdapter.java [119-131] private static boolean isUnitInterval(RexNode interval) { if (!(interval instanceof RexLiteral lit)) { return false; } Object value = lit.getValue(); if (value instanceof BigDecimal bd) { return bd.compareTo(BigDecimal.ONE) == 0; } if (value instanceof Number n) { - return n.doubleValue() == 1.0; + BigDecimal bd = new BigDecimal(n.toString()); + return bd.compareTo(BigDecimal.ONE) == 0; } return false; } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential precision issue with n.doubleValue() == 1.0 for floating-point comparisons. Converting to BigDecimal via toString() provides more robust equality checking across different numeric types, though the impact is moderate since interval values are typically well-defined.	Medium

Suggestions up to commit f10a09b

Category	Suggestion	Impact
Possible issue	Prevent division by zero Add validation to prevent division by zero when interval is a literal zero. If interval evaluates to zero, the division operation will fail at runtime. Check if interval is a zero literal and either throw an exception or return the original call unchanged. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/SpanAdapter.java [101-116] private static RexNode rewriteNumericSpan(RexBuilder rexBuilder, RexNode field, RexNode interval, RelDataType resultType) { + if (interval instanceof RexLiteral lit && lit.getValue() != null) { + Object value = lit.getValue(); + if ((value instanceof BigDecimal bd && bd.compareTo(BigDecimal.ZERO) == 0) || + (value instanceof Number n && n.doubleValue() == 0.0)) { + throw new IllegalArgumentException("SPAN interval cannot be zero"); + } + } SqlTypeName resultTypeName = resultType.getSqlTypeName(); boolean integerResult = resultTypeName == SqlTypeName.INTEGER || resultTypeName == SqlTypeName.BIGINT || resultTypeName == SqlTypeName.SMALLINT || resultTypeName == SqlTypeName.TINYINT; RexNode quotient = rexBuilder.makeCall(SqlStdOperatorTable.DIVIDE, field, interval); RexNode bucket = integerResult ? quotient : rexBuilder.makeCall(SqlStdOperatorTable.FLOOR, quotient); RexNode product = rexBuilder.makeCall(SqlStdOperatorTable.MULTIPLY, bucket, interval); ... } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential division-by-zero issue in the numeric span rewrite logic. Adding validation for zero interval values would prevent runtime errors. However, this is defensive error handling rather than fixing a critical bug, as invalid intervals should ideally be caught earlier in query validation.	Medium
General	Validate percentile range bounds Validate that the percent column value is within the valid range [0, 100] before rescaling. If the value is outside this range, the rescaled fraction could produce invalid percentile arguments (negative or >1.0) that may cause runtime errors in DataFusion's percentile function. sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/planner/PlannerImpl.java [261-278] for (int i = 0; i < projectFieldCount; i++) { if (drop[i]) continue; RexNode expr = projectExprs.get(i); if (usedAsPercentColumn[i] && !usedByOther[i]) { + if (expr instanceof RexLiteral lit && lit.getValue() != null) { + double val = ((Number) lit.getValue()).doubleValue(); + if (val < 0.0 || val > 100.0) { + throw new IllegalArgumentException("Percentile value must be between 0 and 100, got: " + val); + } + } RexNode asDouble = rexBuilder.makeCast(fp64NotNull, expr, false); expr = rexBuilder.makeCall( SqlStdOperatorTable.DIVIDE, asDouble, rexBuilder.makeApproxLiteral(java.math.BigDecimal.valueOf(100.0)) ); } remap.put(i, newExprs.size()); newExprs.add(expr); newNames.add(oldNames.get(i)); } Suggestion importance[1-10]: 6 __ Why: The suggestion adds validation to ensure percentile values are within [0, 100] before rescaling. While this is good defensive programming, the validation only applies when the percent column is a literal, and invalid values should typically be caught during query parsing or validation phases rather than during plan rewriting.	Low
	Validate field offset bounds Add bounds validation for the field offset returned by sf.offset(). If the offset is negative or exceeds the project expression list size, it could cause an IndexOutOfBoundsException in liftPercentileLiteral when accessing projectExprs.get(fieldIdx). sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/SubstraitPlanRewriter.java [139-150] private static Integer simpleStructFieldIndex(FieldReference ref) { if (!ref.isSimpleRootReference() || ref.segments().size() != 1) return null; FieldReference.ReferenceSegment seg = ref.segments().get(0); if (seg instanceof FieldReference.StructField sf) { - return sf.offset(); + int offset = sf.offset(); + return offset >= 0 ? offset : null; } return null; } Suggestion importance[1-10]: 5 __ Why: The suggestion adds a check for negative offsets in simpleStructFieldIndex. While the bounds check for negative values is reasonable, the upper bound check is already performed in the calling method liftPercentileLiteral at line 130, making this a minor defensive improvement rather than fixing an actual bug.	Low

Suggestions up to commit a2a5a92

Category	Suggestion	Impact
General	Fix precision loss in numeric comparison The floating-point comparison n.doubleValue() == 1.0 can produce false negatives for numeric types that lose precision when converted to double (e.g., large Long values). Use exact comparison methods appropriate for each numeric type instead of converting to double. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/SpanAdapter.java [119-131] private static boolean isUnitInterval(RexNode interval) { if (!(interval instanceof RexLiteral lit)) { return false; } Object value = lit.getValue(); if (value instanceof BigDecimal bd) { return bd.compareTo(BigDecimal.ONE) == 0; } + if (value instanceof Long l) { + return l == 1L; + } + if (value instanceof Integer i) { + return i == 1; + } if (value instanceof Number n) { return n.doubleValue() == 1.0; } return false; } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential precision issue when comparing numeric types via doubleValue(). Adding explicit checks for Long and Integer before falling back to double comparison improves correctness for exact numeric types.	Medium
	Simplify null check logic flow The getValueAs(String.class) call can return null even when lit.getValue() != null, creating redundant null checks. Consolidate the null check after getValueAs to simplify the logic and avoid potential NPE if the map lookup is performed on a null key. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/SpanAdapter.java [88-95] if (unit instanceof RexLiteral lit && lit.getValue() != null) { String unitText = lit.getValueAs(String.class); - String dateTruncUnit = unitText == null ? null : UNIT_TO_DATE_TRUNC.get(unitText); - if (dateTruncUnit != null && isUnitInterval(interval)) { - RexNode unitArg = rexBuilder.makeLiteral(dateTruncUnit); - return rexBuilder.makeCall(original.getType(), SqlLibraryOperators.DATE_TRUNC, List.of(unitArg, field)); + if (unitText != null) { + String dateTruncUnit = UNIT_TO_DATE_TRUNC.get(unitText); + if (dateTruncUnit != null && isUnitInterval(interval)) { + RexNode unitArg = rexBuilder.makeLiteral(dateTruncUnit); + return rexBuilder.makeCall(original.getType(), SqlLibraryOperators.DATE_TRUNC, List.of(unitArg, field)); + } } } Suggestion importance[1-10]: 5 __ Why: The suggestion improves code readability by restructuring the null checks more clearly. However, the functional behavior remains identical, so the impact is limited to maintainability and style rather than correctness.	Low
	Improve operator instance comparison robustness The reference equality check op == SqlStdOperatorTable.AVG may fail if the operator instance was deserialized or created differently. Consider using a more robust comparison method such as checking the operator's name or kind consistently. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DataFusionFragmentConvertor.java [411-421] private static String substraitNameForCustomAgg(SqlAggFunction op) { SqlKind kind = op.getKind(); return switch (kind) { - case AVG -> op == SqlStdOperatorTable.AVG ? null : "avg"; + case AVG -> op.getClass() == SqlStdOperatorTable.AVG.getClass() ? null : "avg"; case STDDEV_POP -> "stddev_pop"; case STDDEV_SAMP -> "stddev_samp"; case VAR_POP -> "var_pop"; case VAR_SAMP -> "var_samp"; default -> null; }; } Suggestion importance[1-10]: 3 __ Why: While the concern about reference equality is valid, the suggested fix using getClass() comparison is not necessarily more robust and may introduce false positives if subclasses exist. The current reference equality check is intentional per the code comments explaining custom operator instances.	Low

[github-actions]

✅ Gradle check result for a2a5a92: SUCCESS

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.51%. Comparing base (40a1461) to head (92e6cb1).
⚠️ Report is 5 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##               main   #21584   +/-   ##
=========================================
  Coverage     73.50%   73.51%           
+ Complexity    74753    74711   -42     
=========================================
  Files          5984     5985    +1     
  Lines        339118   339137   +19     
  Branches      48895    48894    -1     
=========================================
+ Hits         249273   249303   +30     
+ Misses        69993    69973   -20     
- Partials      19852    19861    +9     

☔ View full report in Codecov by Sentry.
📢 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]

Hi @ahkcs - Can we please hold this change (move to draft ideally to avoid accidental merge) until #21457 gets merged. Most of these agg functions should be easy enum declarations then, also will give a practical testing scenario with partial-final chanegs merged in.
Thanks!

[ahkcs]

Hi @ahkcs - Can we please hold this change (move to draft ideally to avoid accidental merge) until #21457 gets merged. Most of these agg functions should be easy enum declarations then, also will give a practical testing scenario with partial-final chanegs merged in. Thanks!

Moved to draft now

[github-actions]

❌ Gradle check result for f10a09b: null

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?

[ahkcs]

Rebased onto post-#21457 main. Adjusted to the new framework:

• Capability declarations now use the 3-arg AggregateCapability constructor (no buildAggregateCapability dispatcher) so the resolver falls back to AggregateFunction.intermediateFields.
• OpenSearchAggregateFunctionConverter (introduced in [analytics-backend-datafusion] Implement partial/final aggregation mode for distributed execution #21457) is now the converter — dynamic per-tree sigs for PPL's UDAF-family operators (custom AVG / STDDEV / VAR / percentile_approx) are appended onto ADDITIONAL_AGGREGATE_SIGS.
• Percentile-approx rewrite runs at the top of markAndOptimize (before phase 1a) so the SYMBOL sqlflag column is dropped before OpenSearchAggregateReduceRule or any HEP marking rule sees the plan.
• PERCENTILE_APPROX and APPROX_COUNT_DISTINCT coexist in AGG_FUNCTIONS; the new enum's hasDecomposition() / intermediateFields path is unchanged.

Local compile + spotless clean on the rebased branch. Unit tests pending in CI.

[github-actions]

Persistent review updated to latest commit 77d2cc1

[github-actions]

❌ Gradle check result for 77d2cc1: 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?

[github-actions]

Persistent review updated to latest commit dc70c68

[github-actions]

❌ Gradle check result for dc70c68: 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?

[sandeshkr419]

Hi,
Functions: avg/stddev/related are already pushed in with my changes and handled by Calcite decomposition directly, I think you can repurpose this PR for span explicitly.

[github-actions]

Persistent review updated to latest commit 7adbc56

[github-actions]

✅ Gradle check result for 7adbc56: SUCCESS

[github-actions]

Persistent review updated to latest commit 92e6cb1

[github-actions]

✅ Gradle check result for 92e6cb1: SUCCESS