[Analytics Engine] Error on BIGINT arithmetic overflow instead of wrapping

[ahkcs]

Description

BIGINT (long / Int64) arithmetic (+, -, *) on the analytics-engine DataFusion backend silently wraps on 64-bit overflow instead of erroring. For example eval x = long_col * long_col where the product exceeds 9,223,372,036,854,775,807 returns a wrapped negative value with HTTP 200, rather than a clear client error. DataFusion lowers integer Plus/Minus/Multiply to arrow's wrapping kernels (add_wrapping/sub_wrapping/mul_wrapping) by default.

This makes BIGINT overflow an explicit HTTP 400 on the DataFusion route.

Mechanism

A logical-plan rewrite (checked_arith_rewrite) runs between from_substrait_plan and create_physical_plan on every executor. It walks the plan with LogicalPlan::map_expressions + Expr::transform_up and rewrites every BinaryExpr { op: Plus | Minus | Multiply } whose both operands resolve to Int64 into a call to an overflow-checked scalar UDF (checked_add_i64 / checked_sub_i64 / checked_mul_i64). The UDFs invoke arrow's checked numeric kernels (add/sub/mul), which return an error on overflow. The error carries a stable, self-authored phrase (BIGINT arithmetic overflow) that NativeErrorConverter maps to an IllegalArgumentException → HTTP 400.

Why a logical rewrite (rather than flipping DataFusion's physical BinaryExpr::fail_on_overflow): walking the logical plan reaches arithmetic in every node uniformly — projection exprs, filter predicates, aggregate group/argument exprs (sum(a*b)), window args, sort keys, join conditions — because those are all first-class top-level Exprs at the logical level. The physical-plan approach would depend on per-node expression accessors that do not uniformly surface aggregate/window argument expressions, leaving silent overflow gaps.

Type gate (what is and isn't affected)

Only Int64 op Int64 is rewritten. Everything else passes through unchanged:

operands	behavior
Int64 {+,-,*} Int64	checked → 400 on overflow
Float32/Float64	unchanged (wrap to ±Infinity, PPL/Calcite parity)
Decimal*	unchanged (own overflow path)
narrower ints (Int8/16/32), UInt64	unchanged
/, %	unchanged (not +/-/*)

The analytics scan boundary already narrows the only UInt64 source (unsigned_long) to Int64 before planning (schema_coerce), and the SQL-plugin widening (opensearch-project/sql#5603) widens all sub-BIGINT integer arithmetic to Int64 before the engine fork — so Int64 op Int64 is the only integer arithmetic that reaches DataFusion and can still overflow. The gate is therefore both correct and sufficient.

Type resolution failures (e.g. a correlated column absent from the merged input schema) fall through to "leave unchanged" rather than propagate, so the rewrite can never turn a previously-working query into a planning error.

The UDFs are Volatility::Immutable, and their return_type is Int64 — identical to the BinaryExpr they replace — so plan schemas are byte-identical (no recompute_schema, no relabel_exec churn), and predicate pushdown / CSE still apply. Const-folding an overflowing literal pair does not lose the error: DataFusion's ConstEvaluator preserves the original expr on a UDF runtime error (it only fails-fast for CAST), so overflow still surfaces at execution with the stable message; a non-overflowing constant simply folds to its exact value. Each rewritten expression's output name is preserved (NamePreserver) so a later recompute_schema can't rename an aggregate measure like sum(a * b) and break a parent node that references it by the derived name.

Distributed / aggregate path (transport)

Flight transport does not serialize the Java exception type — only a StreamErrorCode + message survive. A client-error IllegalArgumentException with no HTTP status would cross as INTERNAL and be redacted to a generic 500 on the coordinator (this is what stats sum(a*b) overflow originally did). AnalyticsTransportErrors now tags the overflow as StreamErrorCode.INVALID_ARGUMENT on the data-node send side and rebuilds it as a BAD_REQUEST (400) OpenSearchStatusException on the coordinator receive side — mirroring the existing breaker→429 and cancel→CANCELLED mappings — so the 400 survives the distributed reduce.

Indexed path

On the data-node indexed path the rewrite is applied to both the executed plan and the plan used for filter extraction, so arithmetic inside a WHERE predicate that seeds the parquet pushdown predicate is checked too (otherwise where long1 * long2 > k would be scan-filtered with wrapping arithmetic and drop overflowing rows silently). Checked arithmetic only ever appears nested inside a comparison operand — never as a top-level AND/OR or delegation marker — so predicate delegation classification is unaffected.

Scope / divergence

This is the DataFusion-backend fix for the residual long × long overflow that operand-widening cannot address (there is no wider primitive than long). The Calcite/v2 engine still wraps long overflow; unifying the two engines' long-overflow behavior is tracked separately (opensearch-project/sql#5604). The AE-errors-while-V2-wraps divergence is intentional and scoped here to the analytics route.

Resolves the reported analytics-engine long-arithmetic overflow (see opensearch-project/sql#5164).

Testing

test	before	after
checked_arith Rust UDF unit tests (overflow errors, null-propagation, exact values, batch-fail)	—	7/7 pass
checked_arith_rewrite Rust unit tests (Int64 +/-/* rewritten; float/mixed/divide untouched; nested bottom-up; Filter + Sort node coverage)	—	8/8 pass
full analytics-backend-datafusion Rust lib suite (regression)	1284 pass	1284 pass
NativeErrorConverterTests (raw + controlled BIGINT arithmetic overflow → IllegalArgumentException)	—	19/19 pass
AnalyticsTransportErrorsTests (BIGINT overflow → INVALID_ARGUMENT → 400 round-trip)	—	20/20 pass
BigintOverflowIT QA (column *, literal+column, where-arith, stats sum(arith) → 400; non-overflow → 200; double → 200 no-error)	overflow → 200 wrapped	overflow → 400, non-overflow → 200

Check List

• New functionality includes testing.
• New functionality has been documented (module docs + PR).
• Commits are signed per the DCO using --signoff or -s.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit b39f770)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 Multiple PR themes Sub-PR theme: Stabilize NativeArenaPurger threshold test Relevant files: sandbox/libs/dataformat-native/src/test/java/org/opensearch/nativebridge/spi/NativeArenaPurgerTests.java Sub-PR theme: BIGINT overflow checked arithmetic (Rust rewrite + UDFs + local error mapping) Relevant files: sandbox/plugins/analytics-backend-datafusion/rust/src/checked_arith_rewrite.rs sandbox/plugins/analytics-backend-datafusion/rust/src/udf/checked_arith.rs sandbox/plugins/analytics-backend-datafusion/rust/src/udf/mod.rs sandbox/plugins/analytics-backend-datafusion/rust/src/lib.rs sandbox/plugins/analytics-backend-datafusion/rust/src/indexed_executor.rs sandbox/plugins/analytics-backend-datafusion/rust/src/local_executor.rs sandbox/plugins/analytics-backend-datafusion/rust/src/query_executor.rs sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/NativeErrorConverter.java sandbox/plugins/analytics-backend-datafusion/src/test/java/org/opensearch/be/datafusion/NativeErrorConverterTests.java sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/BigintOverflowIT.java Sub-PR theme: Transport error wire-mapping for INVALID_ARGUMENT (400) round-trip Relevant files: sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/exec/AnalyticsTransportErrors.java sandbox/plugins/analytics-engine/src/test/java/org/opensearch/analytics/exec/AnalyticsTransportErrorsTests.java

⚡ Recommended focus areas for review Ignored merge error In merged_input_schema, merged.merge(child.schema()) returns a Result that is discarded (comment says "ignore duplicate-column errors"), but the compiler will warn about the unused Result and any non-duplicate error (e.g. metadata conflict) will be silently swallowed. Consider explicitly binding with let _ = merged.merge(...); and confirm that no non-duplicate errors can arise here — otherwise a schema-merge failure could cause the pass to silently skip rewrites on plans with joins/unions, leaving overflow behavior wrapping. let mut merged = DFSchema::empty(); for child in inputs { // merge() is order-preserving and dedups by qualified name; ignore duplicate-column errors. merged.merge(child.schema()); } Arc::new(merged) Nullable-field type resolution left.get_type(schema) returns the DataType without nullability info; that's fine here, but note that merged_input_schema uses DFSchema::empty() then merge, which may not carry qualifier/relation info from the child. For expressions referencing qualified columns (e.g. t.a after a Join), get_type may fail to resolve and the rewrite will silently skip via the Ok/Ok guard, leaving BIGINT arithmetic in joined plans wrapping instead of erroring. Worth verifying with a Join integration test. // Resolve operand types. If either operand's type cannot be resolved against this schema // (an edge case such as a correlated column not present in the merged input schema), leave // the expression unchanged rather than failing the whole query — a missed rewrite falls back // to today's wrapping behavior, whereas propagating the error would break a working query. let (Ok(lt), Ok(rt)) = (left.get_type(schema), right.get_type(schema)) else { return Ok(Transformed::no(expr)); }; if lt != DataType::Int64 || rt != DataType::Int64 { return Ok(Transformed::no(expr)); }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to b39f770

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Tighten wire-error match to typed exception String-substring matching on exception messages is brittle and can be exploited/tripped by arbitrary user data (e.g. a field value or query literal containing the phrase "BIGINT arithmetic overflow" that ends up embedded in a different error message). Prefer matching on exception type (e.g. OpenSearchStatusException with RestStatus.BAD_REQUEST produced by NativeErrorConverter) to avoid false positives that could misclassify unrelated errors as 400s. sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/exec/AnalyticsTransportErrors.java [74-80] Throwable badRequest = ExceptionsHelper.unwrapCausesAndSuppressed( e, - t -> t.getMessage() != null && t.getMessage().contains(BIGINT_OVERFLOW_PHRASE) + t -> t instanceof OpenSearchException ose + && ose.status() == RestStatus.BAD_REQUEST + && t.getMessage() != null + && t.getMessage().contains(BIGINT_OVERFLOW_PHRASE) ).orElse(null); if (badRequest != null) { return new StreamException(StreamErrorCode.INVALID_ARGUMENT, badRequest.getMessage(), e); } Suggestion importance[1-10]: 5 __ Why: Tightening the match to a typed exception reduces the risk of false positives from user data containing the phrase, which is a reasonable robustness improvement, though the phrase is specific enough that collisions are unlikely.	Low
	Use JSON builder instead of string concatenation Building JSON via string concatenation with a manual escapeJson helper is fragile — any PPL query containing characters not handled by the helper (backslash, control chars, unicode) would produce malformed JSON and a misleading test failure. Prefer a JSON builder (e.g. XContentBuilder/Strings.toString) already available in the test framework to guarantee correct escaping. sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/BigintOverflowIT.java [145-147] private void assertOverflow400(String ppl) throws IOException { Request request = new Request("POST", "/_analytics/ppl"); - request.setJsonEntity("{\"query\": \"" + escapeJson(ppl) + "\"}"); + request.setJsonEntity(org.opensearch.common.Strings.toString( + org.opensearch.common.xcontent.XContentFactory.jsonBuilder().startObject().field("query", ppl).endObject())); Suggestion importance[1-10]: 4 __ Why: Using a proper JSON builder is a reasonable robustness improvement for test code, though the test inputs are controlled and the escapeJson helper likely suffices. Minor impact.	Low
	Avoid silently discarding schema merge results DFSchema::merge returns () in current DataFusion versions, but if it returns a Result the return value is being silently dropped here. More importantly, the comment claims duplicate errors are ignored — verify the actual signature; if merge can fail for reasons other than duplicates, silently discarding could lead to incorrect type resolution and missed rewrites. Consider logging or explicitly matching on the outcome to avoid masking legitimate schema issues. sandbox/plugins/analytics-backend-datafusion/rust/src/checked_arith_rewrite.rs [77-82] let mut merged = DFSchema::empty(); for child in inputs { - // merge() is order-preserving and dedups by qualified name; ignore duplicate-column errors. - merged.merge(child.schema()); + // merge() is order-preserving and dedups by qualified name; duplicate-column errors are + // expected and ignored, but log unexpected merge failures rather than silently swallow them. + let _ = merged.merge(child.schema()); } Arc::new(merged) Suggestion importance[1-10]: 3 __ Why: The suggestion only adds let _ = to explicitly discard the result, which is a minor stylistic change and does not change behavior. The comment about logging is not implemented.	Low
Possible issue	Guard against null message before indexOf match.message() could be null if the underlying exception has no message (the pattern-matcher may have matched on a cause's message rather than the top-level one). Calling indexOf on a null msg would throw NPE inside the converter, defeating the purpose of the graceful mapping. Guard against a null message before dereferencing. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/NativeErrorConverter.java [193-196] String msg = match.message(); +if (msg == null) { + return new OpenSearchStatusException(BIGINT_OVERFLOW_MSG, RestStatus.BAD_REQUEST); +} int start = msg.indexOf(BIGINT_OVERFLOW_MSG); String clean = start >= 0 ? msg.substring(start) : BIGINT_OVERFLOW_MSG; return new OpenSearchStatusException(clean, RestStatus.BAD_REQUEST); Suggestion importance[1-10]: 5 __ Why: Guarding against a null message is a defensive improvement that prevents a potential NPE, though the pattern matching likely only matches non-null messages that contain the keyphrase, making this an edge case.	Low

---

Previous suggestions

Suggestions up to commit ed05653

Category	Suggestion	Impact
General	Strip native "underlying" detail from message The comment claims "trailing cause detail" is stripped, but the code only strips the leading prefix — the entire suffix (including parenthesized underlying: ... native detail from arrow) is surfaced verbatim to the client. This leaks native internals in the client-facing message. Consider trimming the trailing " (underlying: ...)" portion to keep the response free of implementation detail. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/NativeErrorConverter.java [193-196] String msg = match.message(); int start = msg.indexOf(BIGINT_OVERFLOW_MSG); String clean = start >= 0 ? msg.substring(start) : BIGINT_OVERFLOW_MSG; +int underlyingIdx = clean.indexOf(" (underlying:"); +if (underlyingIdx >= 0) { + clean = clean.substring(0, underlyingIdx); +} return new OpenSearchStatusException(clean, RestStatus.BAD_REQUEST); Suggestion importance[1-10]: 6 __ Why: A valid observation: the comment mentions stripping trailing cause detail but the code only strips the prefix, so native underlying: detail leaks to clients. This is a reasonable improvement for message hygiene, though not critical.	Low
	Avoid silently discarding merge errors DFSchema::merge returns () (not Result) in current DataFusion versions, but if it returns a Result the .merge(...) call result is silently dropped which the comment suggests. If it does return Result, ignoring it may hide real merge failures beyond duplicate columns; consider explicitly matching on the error and only ignoring the duplicate-column case, or documenting why silent-drop is safe. sandbox/plugins/analytics-backend-datafusion/rust/src/checked_arith_rewrite.rs [77-82] let mut merged = DFSchema::empty(); for child in inputs { - // merge() is order-preserving and dedups by qualified name; ignore duplicate-column errors. - merged.merge(child.schema()); + // merge() is order-preserving and dedups by qualified name; duplicate-column errors are + // expected here (e.g. joins with overlapping names) and can be safely ignored. + let _ = merged.merge(child.schema()); } Arc::new(merged) Suggestion importance[1-10]: 3 __ Why: In current DataFusion DFSchema::merge returns Result, and the existing code already ignores it via an unused expression. Adding let _ = is a minor stylistic improvement with low impact.	Low
	Build JSON safely instead of concatenating The JSON body is constructed via naive string concatenation with an escapeJson helper. If escapeJson does not fully escape control characters, backslashes, or quotes, the request body could become invalid JSON. Prefer building the JSON with a proper JSON builder (e.g. XContentBuilder/Strings.toString) to guarantee correctness across all inputs. sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/BigintOverflowIT.java [145-146] private void assertOverflow400(String ppl) throws IOException { Request request = new Request("POST", "/_analytics/ppl"); - request.setJsonEntity("{\"query\": \"" + escapeJson(ppl) + "\"}"); + request.setJsonEntity(org.opensearch.common.xcontent.XContentFactory.jsonBuilder() + .startObject().field("query", ppl).endObject().toString()); Suggestion importance[1-10]: 3 __ Why: Using a JSON builder is more robust than string concatenation, but the test queries are hardcoded and simple, so the practical risk is low.	Low

Suggestions up to commit ea95017

Category	Suggestion	Impact
General	Explicitly discard merge result value DFSchema::merge returns Result in some DataFusion versions but here its return value is discarded via a bare statement. If the signature returns Result, this will emit an unused-must-use warning or fail to compile; if it returns (), the comment is misleading. Verify the signature and explicitly handle/ignore the result (e.g., let _ = merged.merge(...)) to make the intent unambiguous. sandbox/plugins/analytics-backend-datafusion/rust/src/checked_arith_rewrite.rs [77-82] let mut merged = DFSchema::empty(); for child in inputs { // merge() is order-preserving and dedups by qualified name; ignore duplicate-column errors. - merged.merge(child.schema()); + let _ = merged.merge(child.schema()); } Arc::new(merged) Suggestion importance[1-10]: 5 __ Why: DFSchema::merge returns Result, so discarding it silently will trigger a must_use warning. Adding let _ = clarifies intent, though it's a minor code-quality issue.	Low
	Guard against null message in converter match.message() could be null if upstream ever passes a null-message exception through the pattern matcher, causing a NullPointerException on indexOf. Guard against a null message to remain consistent with the null-safe checks used elsewhere (e.g. the t.getMessage() != null guard in AnalyticsTransportErrors). sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/NativeErrorConverter.java [193-196] String msg = match.message(); -int start = msg.indexOf(BIGINT_OVERFLOW_MSG); -String clean = start >= 0 ? msg.substring(start) : BIGINT_OVERFLOW_MSG; +int start = msg == null ? -1 : msg.indexOf(BIGINT_OVERFLOW_MSG); +String clean = (msg != null && start >= 0) ? msg.substring(start) : BIGINT_OVERFLOW_MSG; return new OpenSearchStatusException(clean, RestStatus.BAD_REQUEST); Suggestion importance[1-10]: 4 __ Why: The pattern matching relies on getMessage().contains(...), which typically ensures message is non-null before reaching this converter. Adding null-guarding is a minor defensive improvement.	Low
	Use a JSON builder for request body The PPL string is inserted into JSON via escapeJson, but the queries here contain no characters that would need escaping. If escapeJson is a helper that only handles a subset (e.g. quotes) and the PPL later contains backslashes, control chars, or newlines, the request body will be malformed. Prefer constructing the JSON with a proper JSON builder/library to avoid subtle escape bugs. sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/BigintOverflowIT.java [145-146] private void assertOverflow400(String ppl) throws IOException { Request request = new Request("POST", "/_analytics/ppl"); - request.setJsonEntity("{\"query\": \"" + escapeJson(ppl) + "\"}"); + request.setJsonEntity(org.opensearch.common.xcontent.XContentFactory.jsonBuilder() + .startObject().field("query", ppl).endObject().toString()); Suggestion importance[1-10]: 3 __ Why: The current queries have no special characters requiring complex escaping, so this is a defensive suggestion with limited practical impact for the test cases in question.	Low

Suggestions up to commit 9e28428

Category	Suggestion	Impact
General	Match typed 400 exception, not message substring Matching on message substring across the whole cause chain is fragile: any wrapper exception whose getMessage() includes the child's message (a common pattern, e.g. RuntimeException(cause) which returns cause.toString()) will match first at the outer level, causing the outer wrapper's (possibly longer/mangled) message to be sent on the wire instead of the original overflow message. Prefer matching a typed exception (e.g. OpenSearchStatusException with BAD_REQUEST status) that NativeErrorConverter now emits, or find the innermost matching cause. sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/exec/AnalyticsTransportErrors.java [74-80] Throwable badRequest = ExceptionsHelper.unwrapCausesAndSuppressed( e, - t -> t.getMessage() != null && t.getMessage().contains(BIGINT_OVERFLOW_PHRASE) + t -> t instanceof OpenSearchException ose + && ose.status() == RestStatus.BAD_REQUEST + && t.getMessage() != null + && t.getMessage().contains(BIGINT_OVERFLOW_PHRASE) ).orElse(null); if (badRequest != null) { return new StreamException(StreamErrorCode.INVALID_ARGUMENT, badRequest.getMessage(), e); } Suggestion importance[1-10]: 6 __ Why: Valid concern: matching by message substring across the cause chain can pick up outer wrappers whose message includes the child's. Since NativeErrorConverter now emits OpenSearchStatusException with 400, matching by typed exception is more robust.	Low
	Fix schema merge deref and single-input path DFSchema::merge takes &DFSchema but child.schema() returns &DFSchemaRef (i.e. &Arc); relying on auto-deref plus ignoring its return silently discards potential errors. Also, merge returns () in current DataFusion versions but the comment implies error handling — verify the API. More importantly, for a single input this needlessly clones; short-circuit that case to avoid rebuilding the schema. sandbox/plugins/analytics-backend-datafusion/rust/src/checked_arith_rewrite.rs [77-82] +if inputs.len() == 1 { + return Arc::new(inputs[0].schema().as_ref().clone()); +} let mut merged = DFSchema::empty(); for child in inputs { - // merge() is order-preserving and dedups by qualified name; ignore duplicate-column errors. - merged.merge(child.schema()); + merged.merge(child.schema().as_ref()); } Arc::new(merged) Suggestion importance[1-10]: 3 __ Why: The suggestion about auto-deref is minor stylistic; merge in DataFusion does return Result in some versions, but the existing code ignores it (which the suggestion also does). The single-input short-circuit is a minor optimization with marginal impact.	Low
	Use expectThrows for clearer failure handling fail() throws an AssertionError which is not caught by the catch (ResponseException e) block, but if performRequest returns a non-error 2xx there is no issue — however if it throws a different exception type (e.g. IOException due to a 5xx), the test fails with an unclear error. Additionally, fail() inside the try means a successful response results in AssertionError, but you should also close/consume the response entity to avoid resource leaks. Consider capturing the response and asserting on the status explicitly, or letting fail run outside the try. sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/BigintOverflowIT.java [144-156] private void assertOverflow400(String ppl) throws IOException { Request request = new Request("POST", "/_analytics/ppl"); request.setJsonEntity("{\"query\": \"" + escapeJson(ppl) + "\"}"); - try { - client().performRequest(request); - fail("Expected 400 BIGINT overflow error for query: " + ppl); - } catch (ResponseException e) { + ResponseException re = expectThrows(ResponseException.class, () -> client().performRequest(request)); + int status = re.getResponse().getStatusLine().getStatusCode(); + String body = new String(re.getResponse().getEntity().getContent().readAllBytes(), StandardCharsets.UTF_8); + assertEquals("Expected 400 but got " + status + ": " + body, 400, status); + assertTrue("Error should mention BIGINT overflow, got: " + body, body.contains("BIGINT arithmetic overflow")); +} Suggestion importance[1-10]: 2 __ Why: The current try/catch pattern is standard and works correctly; fail() throwing AssertionError inside try is fine since the catch only handles ResponseException. This is a minor style suggestion with low impact.	Low

Suggestions up to commit 59b9d22

Category	Suggestion	Impact
General	Tighten bad-request detection predicate A substring match on any Throwable's message is broad: any wrapper exception that includes the phrase (e.g., a log-line-style RuntimeException wrapping the original) will match first and its message will be sent over the wire, potentially leaking outer context. Prefer matching only on the typed exception produced by NativeErrorConverter (e.g., OpenSearchStatusException with BAD_REQUEST status and the phrase) to avoid false-positive matches on higher-level wrappers. sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/exec/AnalyticsTransportErrors.java [74-80] Throwable badRequest = ExceptionsHelper.unwrapCausesAndSuppressed( e, - t -> t.getMessage() != null && t.getMessage().contains(BIGINT_OVERFLOW_PHRASE) + t -> t instanceof OpenSearchException ose + && ose.status() == RestStatus.BAD_REQUEST + && t.getMessage() != null + && t.getMessage().contains(BIGINT_OVERFLOW_PHRASE) ).orElse(null); if (badRequest != null) { return new StreamException(StreamErrorCode.INVALID_ARGUMENT, badRequest.getMessage(), e); } Suggestion importance[1-10]: 5 __ Why: Valid concern about broad substring matching; however NativeErrorConverter produces OpenSearchStatusException on the coordinator but on the data node the wire path may see a different exception type (the phrase-based match is intentionally type-agnostic across wrappers), so tightening the predicate may miss cases and requires care.	Low
	Fast-path single-input schema resolution DFSchema::merge takes &DFSchema and returns () (it doesn't return a Result), but more importantly if any input schema is itself an Arc, cloning via as_ref().clone() is fine — however for a single-input node (the vast majority: Projection, Filter, Sort, Aggregate) you allocate an empty schema and merge one child unnecessarily. Fast-path the single-input case by returning inputs[0].schema().clone() directly to avoid the extra allocation and merge work on every node. sandbox/plugins/analytics-backend-datafusion/rust/src/checked_arith_rewrite.rs [77-82] +if inputs.len() == 1 { + return inputs[0].schema().clone(); +} let mut merged = DFSchema::empty(); for child in inputs { // merge() is order-preserving and dedups by qualified name; ignore duplicate-column errors. merged.merge(child.schema()); } Arc::new(merged) Suggestion importance[1-10]: 4 __ Why: Minor optimization to avoid an extra allocation/merge for the common single-input node case; correct but low-impact and not a correctness issue.	Low
	Build request body via JSON builder The ppl argument is concatenated into a JSON string via escapeJson, but if escapeJson doesn't handle all JSON special characters (quotes, backslashes, control chars) the request body will be malformed and produce a misleading 400 unrelated to overflow. Consider building the JSON via a proper builder (e.g., XContentBuilder or a Map serialized to JSON) to ensure the request is well-formed and the 400 assertion truly reflects the overflow error. sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/BigintOverflowIT.java [145-146] private void assertOverflow400(String ppl) throws IOException { Request request = new Request("POST", "/_analytics/ppl"); - request.setJsonEntity("{\"query\": \"" + escapeJson(ppl) + "\"}"); + XContentBuilder body = XContentFactory.jsonBuilder().startObject().field("query", ppl).endObject(); + request.setJsonEntity(Strings.toString(body)); Suggestion importance[1-10]: 3 __ Why: Reasonable test-hygiene improvement, but the PPL queries here contain only simple characters so the risk of malformed JSON is low; also relies on an unshown escapeJson helper.	Low

Suggestions up to commit 1e1d666

Category	Suggestion	Impact
General	Explicitly ignore merge() result DFSchema::merge returns a Result, and calling it as a bare statement will produce an unused-must-use warning or fail to compile depending on the DataFusion version. Explicitly discard or handle the result to make the intent clear and avoid build breakage. sandbox/plugins/analytics-backend-datafusion/rust/src/checked_arith_rewrite.rs [77-82] let mut merged = DFSchema::empty(); for child in inputs { // merge() is order-preserving and dedups by qualified name; ignore duplicate-column errors. - merged.merge(child.schema()); + let _ = merged.merge(child.schema()); } Arc::new(merged) Suggestion importance[1-10]: 5 __ Why: DFSchema::merge returns a Result marked #[must_use], so ignoring it would produce a compiler warning. Adding let _ = is a minor but valid correctness/cleanliness improvement.	Low
	Narrow overflow match to expected types This predicate matches any exception in the cause chain whose message merely contains the phrase, which can misclassify unrelated wrappers or already-mapped OpenSearchStatusException(400) instances. Consider restricting the match to the expected exception types produced by NativeErrorConverter (e.g. IllegalArgumentException or OpenSearchStatusException with status 400) to avoid false positives that could downgrade a legitimate 500 to a 400. sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/exec/AnalyticsTransportErrors.java [74-80] Throwable badRequest = ExceptionsHelper.unwrapCausesAndSuppressed( e, - t -> t.getMessage() != null && t.getMessage().contains(BIGINT_OVERFLOW_PHRASE) + t -> (t instanceof IllegalArgumentException + || (t instanceof OpenSearchException ose && ose.status() == RestStatus.BAD_REQUEST)) + && t.getMessage() != null + && t.getMessage().contains(BIGINT_OVERFLOW_PHRASE) ).orElse(null); if (badRequest != null) { return new StreamException(StreamErrorCode.INVALID_ARGUMENT, badRequest.getMessage(), e); } Suggestion importance[1-10]: 4 __ Why: Narrowing the predicate to specific expected types reduces the risk of misclassifying unrelated exceptions carrying the phrase, though the phrase is quite specific making false positives unlikely in practice.	Low
	Use safe JSON serialization for request body Building a JSON body via string concatenation with a helper escapeJson is fragile; if the helper does not escape all required characters (backslashes, control chars, quotes) the request body will be malformed and the assertion could fail with a JSON parse error rather than the intended 400. Prefer using a JSON builder (e.g., XContentBuilder/XContentFactory.jsonBuilder()) to safely serialize the query string. sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/BigintOverflowIT.java [145-146] private void assertOverflow400(String ppl) throws IOException { Request request = new Request("POST", "/_analytics/ppl"); - request.setJsonEntity("{\"query\": \"" + escapeJson(ppl) + "\"}"); + XContentBuilder body = XContentFactory.jsonBuilder().startObject().field("query", ppl).endObject(); + request.setJsonEntity(body.toString()); Suggestion importance[1-10]: 3 __ Why: Using a JSON builder is safer than hand-rolled escaping, but the test queries here are static known strings, so risk of malformed JSON is low.	Low
	Avoid redundant Arc allocations at registration **Each call to checked_*_udf() allocates a new Arc; you then clone the inner ScalarUDF value and drop the original Arc, which is wasteful and slightly obscures intent. Dereference once via (arc).clone() or, preferably, expose a function returning ScalarUDF directly for registration, or use ctx.register_udf((checked_add_udf()).clone()) to avoid the redundant Arc roundtrip. sandbox/plugins/analytics-backend-datafusion/rust/src/udf/checked_arith.rs [165-169] pub fn register_all(ctx: &SessionContext) { - ctx.register_udf(ScalarUDF::clone(&checked_add_udf())); - ctx.register_udf(ScalarUDF::clone(&checked_sub_udf())); - ctx.register_udf(ScalarUDF::clone(&checked_mul_udf())); + ctx.register_udf((*checked_add_udf()).clone()); + ctx.register_udf((*checked_sub_udf()).clone()); + ctx.register_udf((*checked_mul_udf()).clone()); } Suggestion importance[1-10]: 2 __ Why: A minor micro-optimization on a one-time registration path; impact is negligible and does not affect correctness.	Low

[github-actions]

✅ Gradle check result for 1e1d666: SUCCESS

[codecov]

Codecov Report

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

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #22378      +/-   ##
============================================
+ Coverage     73.45%   73.46%   +0.01%     
- Complexity    76424    76440      +16     
============================================
  Files          6101     6101              
  Lines        346486   346486              
  Branches      49870    49870              
============================================
+ Hits         254496   254548      +52     
+ Misses        71740    71696      -44     
+ Partials      20250    20242       -8     

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

[github-actions]

Persistent review updated to latest commit 59b9d22

[github-actions]

✅ Gradle check result for 59b9d22: SUCCESS

[github-actions]

Persistent review updated to latest commit 9e28428

[github-actions]

✅ Gradle check result for 9e28428: SUCCESS

[github-actions]

Persistent review updated to latest commit ea95017

[github-actions]

✅ Gradle check result for ea95017: SUCCESS

[github-actions]

Persistent review updated to latest commit ed05653

[github-actions]

❌ Gradle check result for ed05653: 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 b39f770

[github-actions]

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