[analytics-engine]Add PPL bucketing scalars (span_bucket / width_bucket / minspan_bucket / range_bucket) to analytics-engine route

[sandeshkr419]

Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com
Signed-off-by: Marc Handalian marc.handalian@gmail.com
Signed-off-by: Sandesh Kumar sandeshkr419@gmail.com

Summary

Wires PPL's four bucketing label functions end-to-end on the analytics-engine route, exercised by the bin <field> {span,bins,minspan,start..end}=N command surfaces. Each returns a VARCHAR bucket label (e.g. "0-100") via the OpenSearch nice-number algorithm — these are the PPL-specific variants, not ISO-SQL width_bucket (which returns an integer index).

User-facing surface delivered:

• bin <f> span=N → span_bucket(f, N)
• bin <f> bins=N → width_bucket(f, N, MAX(f) OVER (), …)
• bin <f> minspan=N → minspan_bucket(f, N, MAX(f) OVER (), …)
• bin <f> start=X end=Y → range_bucket(f, MIN(f) OVER (), MAX(f) OVER (), X, Y)

The span() scalar (used by stats … by span(…)) was wired separately in #21584 and is reused here.

Implementation

• 4 Rust UDFs under analytics-backend-datafusion/rust/src/udf/ ({span,width,minspan,range}_bucket.rs) ported from sql/core's Java reference implementations. Per-UDF unit tests cover algorithm correctness, null propagation, arity, and type coercion.
• 4 Java adapters (rename adapters extending AbstractNameMappingAdapter) bind PPL's SqlUserDefinedFunction operators to locally-declared SqlFunctions whose FunctionMappings.Sig resolves to the Rust UDFs.
• ScalarFunction enum entries + STANDARD_PROJECT_OPS capability registration + scalarFunctionAdapters() map wiring.
• YAML signatures in opensearch_scalar_functions.yaml with distinct any1 … anyN per slot so substrait can bind mixed-type calls.
• OpenSearchProject.liftNestedRexOver: pre-substrait pass that hoists nested RexOver out of project expressions into a childLogicalProject. Required because bin bins=N / minspan=N / start=… end=… lower to scalar calls whose operands embedMAX/MIN OVER (). DataFusion's substrait consumer auto-lifts top-level WindowFunction project expressions into a LogicalWindow but doesn't touch nested RexOvers — without this lift the data-node fragment errors with "Physical plan does not support logicalexpression WindowFunction(…)". Builds on the window support added by analytics-engine: add window function support #21668.

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

Check List

• Functionality includes testing.
• API changes companion pull request created, if applicable.
• Public documentation issue/PR created, if applicable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 9e5f9f0)

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 liftNestedRexOver method collects unique RexOver expressions using toString() as the key. If two semantically identical RexOver expressions produce different string representations (e.g., due to whitespace or formatting differences in Calcite's toString implementation), they will be hoisted separately and consume distinct column slots. This wastes columns and may cause the outer Project to reference the wrong slot if the rewriter's map lookup fails to match the digest. private Project liftNestedRexOver(RelNode input, List<RexNode> outerExprs) { // Collect unique RexOvers from the expression trees. LinkedHashMap by digest so // the same RexOver from multiple expressions (e.g. MAX(f) OVER () appearing as // both data_range operand and max_value operand of width_bucket) is hoisted once // and shares a single column slot. LinkedHashMap<String, RexOver> uniqueOvers = new LinkedHashMap<>(); RexShuttle collector = new RexShuttle() { @Override public RexNode visitOver(RexOver over) { uniqueOvers.putIfAbsent(over.toString(), over); return over; } }; for (RexNode expr : outerExprs) { expr.accept(collector); Possible Issue In the liftNestedRexOver rewriter shuttle (line 264), if overIndex.get(over.toString()) returns null (which the code acknowledges as "Should not happen"), the method falls back to super.visitOver(over), which recursively descends into the RexOver's operands. This can produce a malformed RexNode tree if the RexOver contains nested expressions that are not valid outside a window context. A safer fallback would be to throw an exception or return the original over unchanged. public RexNode visitOver(RexOver over) { Integer slot = overIndex.get(over.toString()); if (slot == null) { // Should not happen — collector found every RexOver. return super.visitOver(over); } Possible Issue The fromSqlFunction method now catches IllegalArgumentException and returns null when valueOf fails. However, the method does not validate that function.getName() is non-null before calling toUpperCase. If a SqlFunction implementation returns null from getName(), this will throw a NullPointerException instead of the intended IllegalArgumentException, bypassing the catch block and breaking the null-return contract. try { return ScalarFunction.valueOf(function.getName().toUpperCase(Locale.ROOT)); } catch (IllegalArgumentException e) { // Callers (e.g. OpenSearchProjectRule) short-circuit on null — routing the // function through the non-ScalarFunction path (opaque or YAML-alias based // name lookup) rather than aborting Hep rule matching with an exception. return null; } }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 9e5f9f0

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Validate value is finite Verify that value is finite before computing bin_start. A non-finite value (NaN or infinite) will produce a non-finite bin_start and bin_end, resulting in malformed bucket labels like "inf-inf" or "nan-nan". sandbox/plugins/analytics-backend-datafusion/rust/src/udf/minspan_bucket.rs [152-169] fn calculate(value: f64, min_span: f64, range: f64, _max_value: f64) -> Option<String> { + if !value.is_finite() { + return None; + } if !min_span.is_finite() || min_span <= 0.0 { return None; } if !range.is_finite() || range <= 0.0 { return None; } let minspan_width = 10f64.powf(min_span.log10().ceil()); let default_width = 10f64.powf(range.log10().floor()); let width = if default_width >= min_span { default_width } else { minspan_width }; Suggestion importance[1-10]: 4 __ Why: Checking that value is finite before computing bin_start prevents malformed labels like "inf-inf" or "nan-nan". While the formatter may handle these gracefully, explicitly rejecting non-finite values aligns with the function's contract and improves robustness. This is a small but meaningful improvement.	Low
	Validate finite bounds early Validate that data_min and data_max are finite before computing effective_range. If either is NaN or infinite, the subtraction produces a non-finite result that passes through to magnitude_based_width, potentially causing incorrect bucket calculations or panics. sandbox/plugins/analytics-backend-datafusion/rust/src/udf/range_bucket.rs [169-189] fn calculate( value: f64, data_min: f64, data_max: f64, start_param: Option<f64>, end_param: Option<f64>, ) -> Option<String> { + if !data_min.is_finite() || !data_max.is_finite() { + return None; + } let effective_min = match start_param { Some(s) => s.min(data_min), None => data_min, }; let effective_max = match end_param { Some(e) => e.max(data_max), None => data_max, }; let effective_range = effective_max - effective_min; if !effective_range.is_finite() || effective_range <= 0.0 { return None; } Suggestion importance[1-10]: 3 __ Why: Adding explicit is_finite() checks for data_min and data_max before computing effective_range improves code clarity and guards against edge cases. However, the existing !effective_range.is_finite() check already catches non-finite results from the subtraction, so this is a minor defensive improvement rather than a critical bug fix.	Low
	Guard against non-finite inputs Check that data_range and max_value are finite before performing arithmetic operations. Non-finite inputs can produce NaN or infinite intermediate values in target_width.log10() or the modulo operation, leading to incorrect width calculations. sandbox/plugins/analytics-backend-datafusion/rust/src/udf/width_bucket.rs [177-187] fn optimal_width(data_range: f64, max_value: f64, requested_bins: i64) -> f64 { - if data_range <= 0.0 || requested_bins <= 0 { + if !data_range.is_finite() || data_range <= 0.0 || requested_bins <= 0 { + return 1.0; + } + if !max_value.is_finite() { return 1.0; } let target_width = data_range / requested_bins as f64; let exponent = target_width.log10().ceil(); let mut width = 10f64.powf(exponent); let mut actual_bins = (data_range / width).ceil(); if max_value % width == 0.0 { actual_bins += 1.0; } Suggestion importance[1-10]: 3 __ Why: Adding is_finite() checks for data_range and max_value in optimal_width is a defensive improvement. The function already has a data_range <= 0.0 guard, but explicitly checking is_finite() makes the contract clearer and prevents potential NaN propagation in log10() or modulo operations. This is a minor robustness enhancement.	Low
General	Correct return type to string The return type should be explicitly set to string or utf8 instead of any1 to match the actual Rust UDF return type (Utf8). While the comment mentions the Rust side is authoritative, having a mismatched signature in the YAML could cause type resolution issues or confusion. sandbox/plugins/analytics-backend-datafusion/src/main/resources/opensearch_scalar_functions.yaml [821-827] - name: "span_bucket" description: "Fixed-width numeric bucket label (PPL). See sql/core SpanBucketFunction for semantics." impls: - args: - { value: "any1", name: "value" } - { value: "any1", name: "span" } - return: any1 + return: string Suggestion importance[1-10]: 3 __ Why: While the suggestion correctly identifies that the actual return type is Utf8/string, the PR explicitly states that any1 is used "to stay consistent with surrounding signatures" and that "the UDF's actual return type (Utf8) is authoritative on the Rust side." This appears to be an intentional design choice for consistency, making the suggestion's impact minimal.	Low

---

Previous suggestions

Suggestions up to commit 60712c2

Category	Suggestion	Impact
Possible issue	Ensure recursive traversal of nested expressions The RexShuttle collector does not recursively visit nested expressions within the RexOver node. If a RexOver contains nested RexOver expressions in its operands, they won't be collected. Call super.visitOver(over) to ensure the shuttle traverses the entire expression tree. sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/planner/rel/OpenSearchProject.java [226-232] LinkedHashMap<String, RexOver> uniqueOvers = new LinkedHashMap<>(); RexShuttle collector = new RexShuttle() { @Override public RexNode visitOver(RexOver over) { uniqueOvers.putIfAbsent(over.toString(), over); - return over; + return super.visitOver(over); } }; Suggestion importance[1-10]: 8 __ Why: The RexShuttle collector may miss nested RexOver expressions within operands of other RexOver nodes. Calling super.visitOver(over) ensures complete traversal of the expression tree, which is critical for correctly hoisting all window functions.	Medium
	Validate bounds are finite before range computation The function doesn't validate that effective_min and effective_max are finite before computing effective_range. If either bound is NaN or infinite, the range check may pass unexpectedly. Add explicit finite checks for both bounds before computing the range. sandbox/plugins/analytics-backend-datafusion/rust/src/udf/range_bucket.rs [174-189] fn calculate( value: f64, data_min: f64, data_max: f64, start_param: Option<f64>, end_param: Option<f64>, ) -> Option<String> { ... + if !effective_min.is_finite() || !effective_max.is_finite() { + return None; + } let effective_range = effective_max - effective_min; - if !effective_range.is_finite() || effective_range <= 0.0 { + if effective_range <= 0.0 { return None; } let width = magnitude_based_width(effective_range); Suggestion importance[1-10]: 7 __ Why: While effective_range.is_finite() catches some cases, explicitly checking effective_min and effective_max before computing the range provides clearer error handling and prevents subtle bugs when bounds are NaN or infinite.	Medium
General	Use tolerance for floating-point modulo comparison The modulo operation max_value % width can produce unexpected results when width is very small or when floating-point precision issues occur. Consider adding a tolerance-based comparison instead of exact equality to zero to handle floating-point rounding errors. sandbox/plugins/analytics-backend-datafusion/rust/src/udf/width_bucket.rs [176-190] fn optimal_width(data_range: f64, max_value: f64, requested_bins: i64) -> f64 { if data_range <= 0.0 || requested_bins <= 0 { return 1.0; } let target_width = data_range / requested_bins as f64; let exponent = target_width.log10().ceil(); let mut width = 10f64.powf(exponent); let mut actual_bins = (data_range / width).ceil(); - if max_value % width == 0.0 { + if (max_value % width).abs() < 1e-10 { actual_bins += 1.0; } Suggestion importance[1-10]: 6 __ Why: Floating-point modulo operations can suffer from precision issues. Using a tolerance-based comparison (e.g., abs() < 1e-10) instead of exact equality improves robustness, though the current code may work in most practical cases.	Low
	Add null safety check for function parameter The method silently catches IllegalArgumentException and returns null, but it should also handle potential NullPointerException if function or function.getName() is null. Add a null check before calling getName() to prevent unexpected NPE. sandbox/libs/analytics-framework/src/main/java/org/opensearch/analytics/spi/ScalarFunction.java [363-374] public static ScalarFunction fromSqlFunction(SqlFunction function) { + if (function == null || function.getName() == null) { + return null; + } try { return ScalarFunction.valueOf(function.getName().toUpperCase(Locale.ROOT)); } catch (IllegalArgumentException e) { return null; } } Suggestion importance[1-10]: 5 __ Why: Adding explicit null checks for function and function.getName() improves defensive programming and prevents potential NullPointerException. However, the calling context may already guarantee non-null inputs, making this a minor improvement.	Low
	Correct return type to string The return type should be explicitly set to string or utf8 instead of any1. While the comment states the actual return type (Utf8) is authoritative on the Rust side, the YAML signature should accurately reflect the function's return type to avoid confusion and potential type resolution issues. sandbox/plugins/analytics-backend-datafusion/src/main/resources/opensearch_scalar_functions.yaml [821-827] - name: "span_bucket" description: "Fixed-width numeric bucket label (PPL). See sql/core SpanBucketFunction for semantics." impls: - args: - { value: "any1", name: "value" } - { value: "any1", name: "span" } - return: any1 + return: string Suggestion importance[1-10]: 3 __ Why: While the suggestion correctly identifies that span_bucket returns a string/UTF-8 value, the PR's comment explicitly states that using any1 is intentional "to stay consistent with surrounding signatures" and that "the UDF's actual return type (Utf8) is authoritative on the Rust side." The suggestion contradicts this deliberate design choice without addressing why consistency should be broken.	Low

Suggestions up to commit b4884d9

Category	Suggestion	Impact
Possible issue	Correct return type to string The return type should be explicitly set to string or utf8 instead of any1. The comment states the actual return type is Utf8, but the signature declares any1, which creates a type mismatch that could cause runtime errors when the substrait consumer expects a numeric type but receives a string. sandbox/plugins/analytics-backend-datafusion/src/main/resources/opensearch_scalar_functions.yaml [821-827] - name: "span_bucket" description: "Fixed-width numeric bucket label (PPL). See sql/core SpanBucketFunction for semantics." impls: - args: - { value: "any1", name: "value" } - { value: "any1", name: "span" } - return: any1 + return: string Suggestion importance[1-10]: 8 __ Why: The comment at line 820 explicitly states the UDF's actual return type is Utf8, but the signature declares any1. This type mismatch could cause runtime errors when consumers expect the declared type but receive a string, making this a significant correctness issue.	Medium
	Fix return type to string The return type should be string instead of any1. The comment explicitly states this returns a VARCHAR label via the OpenSearch algorithm, not a numeric value. Using any1 as the return type contradicts the documented behavior and could cause type resolution failures. sandbox/plugins/analytics-backend-datafusion/src/main/resources/opensearch_scalar_functions.yaml [842-850] - name: "width_bucket" description: "Histogram bucket label (PPL). See sql/core WidthBucketFunction for semantics." impls: - args: - { value: "any1", name: "value" } - { value: "any2", name: "num_bins" } - { value: "any3", name: "data_range" } - { value: "any4", name: "max_value" } - return: any1 + return: string Suggestion importance[1-10]: 8 __ Why: The comment at line 830-831 clearly states this returns a VARCHAR label, not a numeric value. Using any1 as the return type contradicts the documented behavior and could cause type resolution failures, representing a significant correctness issue.	Medium
General	Remove extra blank line Remove the extra blank line between the test function and the comment section. The code has two consecutive blank lines where only one is needed, which is inconsistent with the rest of the file's formatting style. sandbox/plugins/analytics-backend-datafusion/rust/src/udf/mod.rs [286-297] #[test] fn int64_accepts_decimal_types() { // PPL emits Decimal128(p,s) literals (e.g. `span=2.5` becomes // Decimal128(2, 1)). The Int64 coerce-mode must accept and canonicalize. for observed in [DataType::Decimal128(2, 1), DataType::Decimal256(10, 3)] { let result = coerce_slot("i", 0, &observed, CoerceMode::Int64).unwrap(); assert_eq!(result, DataType::Int64); } } - // ── Float64 ──────────────────────────────────────────────────────────── Suggestion importance[1-10]: 3 __ Why: The suggestion correctly identifies an extra blank line (line 296) between the test function and the comment. While this is a valid style improvement for consistency, it has minimal impact on code functionality or readability. The existing code is already readable and the extra blank line doesn't cause any issues.	Low

[github-actions]

✅ Gradle check result for b4884d9: SUCCESS

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.50%. Comparing base (c39c7c3) to head (b4884d9).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #21621      +/-   ##
============================================
- Coverage     73.60%   73.50%   -0.10%     
+ Complexity    74763    74753      -10     
============================================
  Files          5985     5985              
  Lines        339176   339176              
  Branches      48900    48900              
============================================
- Hits         249637   249320     -317     
- Misses        69643    69991     +348     
+ Partials      19896    19865      -31     

☔ 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.

[github-actions]

Persistent review updated to latest commit 9e5f9f0

[github-actions]

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