[Analytics Backend] Timechart route end-to-end closure (1/30 → 30/30)

[ahkcs]

Summary

End-to-end coverage closure for PPL timechart on the analytics-engine route. Five coupled commits in analytics-backend-datafusion take CalciteTimechart{Command,PerFunction}IT from 1/30 → 30/30 ✅ on upstream/main. Each commit corresponds to a distinct Substrait-binding gap surfaced by progressive IT runs against a :run cluster with -Dtests.analytics.force_routing=true -Dtests.analytics.parquet_indices=true.

Commits

1. [Analytics] Wire TIMESTAMPDIFF/TIMESTAMPADD + skip non-prefix-groupSet split

Two coupled changes:

• Register TIMESTAMPDIFF and TIMESTAMPADD as scalar functions (enum constants in ScalarFunction, added to STANDARD_PROJECT_OPS). Clears the capability-layer rejection at OpenSearchProjectRule.annotateExpr; isthmus-side substrait conversion is still gated by per-call adapters (commit 2).
• OpenSearchAggregateSplitRule.onMatch() skips the PARTIAL/FINAL alternative when it would emit a row type that fails Volcano's typeMatchesInferred. Two unsafe shapes today:
  • percentile_approx (2-arg aggregate whose FINAL phase needs (tdigest_state, percent_literal))
  • Cross-family non-prefix groupSet — when a non-prefix group-key index lands on PARTIAL's agg-output slot whose SqlTypeFamily differs from the ORIGINAL input column's family (e.g. PPL timechart's no-by form: groupSet={2} on a [@timestamp:TIMESTAMP, cpu_usage:DOUBLE, SPAN(@timestamp):TIMESTAMP] input where the SUM aggregate's DOUBLE return at position 2 clashes with TIMESTAMP at the same input position).
    Same-family non-prefix cases (e.g. group={1} over two INTEGER columns + a NUMERIC agg) still pass through the split unchanged — PlanShapeTests.testJoinWithDifferentGroupKeys_multiShard verifies this. The skip lives in onMatch (not matches) so the SINGLE+SINGLETON alternative is still registered, giving the planner a coordinator-gather path for the skipped shapes.

2. [Analytics Backend] Add TimestampDiffAdapter peephole for timechart per_*

Peephole that constant-folds TIMESTAMPDIFF(out_unit, t, TIMESTAMPADD(in_unit, n, t)) to a BIGINT literal when both units are fixed-length (MICROSECOND…WEEK). Eliminates both PPL UDF references in one step — neither has a Substrait extension binding, so the unfolded calls would surface as Unable to convert call TIMESTAMPADD(...). OperatorAnnotation wrappers introduced by OpenSearchProjectRule are peeled at each operand before structural comparison.

3. [Analytics Backend] UInt64 → Int64 bridge at partition-stream boundary

Analytics-engine reduce-stage fragments crashed with Substrait error: Field '<name>' in Substrait schema has a different type (Int64) than the corresponding field in the table schema (UInt64) when the upstream PARTIAL fragment emitted an Arrow type Substrait/Calcite has no native equivalent for. Canonical case: DataFusion's row_number() physical op (output: UInt64) feeding a consumer plan that types ROW_NUMBER OVER as BIGINT/Int64.

Extends the existing schema_coerce::coerce_inferred_schema bridge — currently applied only at the parquet-scan boundary — to the partition-stream registration boundary used by every inter-stage exchange:

• api::register_partition_stream runs coerce_inferred_schema on the producer-derived schema before session.register_partition and the IPC return path.
• api::sender_send adds coerce_batch_to_sender_schema — casts incoming RecordBatch columns to match the sender's coerced schema via arrow::compute::cast. Without this, DataFusion's HashJoin / RepartitionExec strict-downcasts to Int64Type and panics with "primitive array".
• DatafusionReduceSink.typesMatch relaxed to treat same-width Int (any signedness) as compatible, mirroring the existing Timestamp precision tolerance.

4. [Analytics Backend] SpanAdapter — multi-unit fixed-length time bucket

Pre-existing SpanAdapter handled time spans only when interval==1. Multi-unit spans (span=2m, span=12h, span=5d, ...) fell through unchanged. Rewrite for fixed-length units (s, m, h, d, w):

SPAN(t, N, '<unit>')
  → CAST(from_unixtime(CAST((to_unixtime(t) / B) * B AS DOUBLE)) AS <result>)
  where B = N * unit_seconds

Three component choices were each forced by a binding-time substrait constraint:

1. LOCAL_TO_UNIXTIME_OP / LOCAL_FROM_UNIXTIME_OP (package-local) instead of Calcite stdlib UNIX_SECONDS / TIMESTAMP_SECONDS. The latter bind to BigQuery-named substrait functions DF's consumer has no entries for.
2. No FLOOR between divide and multiply. Integer ÷ integer already truncates (correct for non-negative epoch seconds); FLOOR(i64) doesn't bind in substrait (DF's floor is floating-point only).
3. CAST(... AS DOUBLE) before from_unixtime. yaml signature is (fp64) → precision_timestamp<6>.

Variable-length units (M, q, y) intentionally fall through — bucketing by calendar month isn't N × fixed_seconds. Commit 5 handles those.

5. [Analytics Backend] TimestampDiffAdapter — variable-month runtime rewrite

Extends commit 2's peephole to handle variable-length inner units (MONTH/QUARTER/YEAR) when the out-unit is fixed-length. Folding isn't safe (Feb 2025 = 28 days, Oct 2025 = 31 days — value depends on calendar month). Atomic runtime rewrite:

TIMESTAMPDIFF(out_unit, t, TIMESTAMPADD(MONTH|QUARTER|YEAR, n, t))
  →  (to_unixtime(DATETIME_PLUS(t, INTERVAL n*m MONTH)) - to_unixtime(t)) * out_factor

where m converts QUARTER→3 / YEAR→12, INTERVAL_YEAR_MONTH is built via SqlIntervalQualifier(MONTH) (same idiom as EarliestLatestAdapter#makeIntervalAdd, which has prior wiring proof that DATETIME_PLUS(t, INTERVAL_MONTH) binds end-to-end via Substrait's standard add(timestamp, interval_year_month) into DataFusion's native interval add).

PPL's Chart#transformPerFunction always emits MILLISECOND out-unit, so the multiplier path (×1000) is the hot case; the divisor path (MINUTE→÷60 etc.) covers consumer code shapes that compute coarser durations.

Pass-rate impact

Verified locally against upstream/main + this PR's full stack:

Test class	Baseline	+ c1 (TIMESTAMPDIFF wire)	+ c2 (peephole)	+ c3 (UInt64 bridge)	+ c4 (SpanAdapter multi-unit)	+ c5 (variable-month rewrite)
CalciteTimechartCommandIT	1/18	1/18	5/18	18/18 ✅	18/18 ✅	18/18 ✅
CalciteTimechartPerFunctionIT	0/12	0/12	1/12	1/12	10/12	12/12 ✅
Combined	1/30	1/30	6/30	19/30	28/30	30/30 ✅

PlanShapeTests.testJoinWithDifferentGroupKeys_multiShard (the multi-shard join-with-non-prefix-groupSet plan-shape regression test) and the 12 other PlanShapeTests cases also pass — no regression to the existing split rule's coverage.

Sandbox check status

./gradlew check -p sandbox -Dsandbox.enabled=true results on this branch:

• ✅ All analytics-backend-datafusion / analytics-engine / analytics-framework / parquet-data-format / composite-engine unit tests pass (-Werror compile, spotless, checkstyle, forbiddenApis, licenseHeaders, dependencyLicenses, thirdPartyAudit all clean).
• ⚠️ Pre-existing failures in :sandbox:qa:analytics-engine-coordinator:internalClusterTest and :sandbox:plugins:composite-engine:internalClusterTest (9 + 2 tests) all surface the same Allocator[ROOT] closed with outstanding child allocators teardown leak. Identical failures reproduce on plain upstream/main without any of this PR's commits — confirmed in a separate run. Unrelated to the code paths this PR touches (SpanAdapter, TimestampDiffAdapter, OpenSearchAggregateSplitRule, register_partition_stream, sender_send, typesMatch).

Test plan

• CalciteTimechartCommandIT — 18/18 on analytics-engine route
• CalciteTimechartPerFunctionIT — 12/12 on analytics-engine route
• PlanShapeTests (analytics-engine) — 13/13, no regression to non-prefix-groupSet handling
• ./gradlew check -p sandbox -Dsandbox.enabled=true — unit-test surface clean; pre-existing flakes in unrelated internalClusterTest modules

[github-actions]

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 46e797d.

Path	Line	Severity	Description
sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DatafusionReduceSink.java	256	low	The typesMatch validation is relaxed to allow any same-bit-width integer mismatch (signed/unsigned) to pass silently. If this function acts as a correctness gate elsewhere, the blanket bypass could allow unintended data to flow through without type error. The rationale (UInt64 row_number vs Int64 BIGINT) is narrow and specific, but the predicate accepts all same-width Int pairs, which is broader than documented. No evidence of malicious intent; flagged as a minor logic anomaly worth a second review.

The table above displays the top 10 most important findings.

Total: 1 | Critical: 0 | High: 0 | Medium: 0 | Low: 1

---

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

---

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit cc0b9ab)

Here are some key observations to aid the review process:

🧪 No relevant tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Possible Issue The bucketSecondsIfPositiveInteger method returns null when n <= 0, but the caller at line 119 checks bucketSeconds != null && bucketSeconds > 0L. This creates a redundant check: if bucketSeconds is non-null, it is already guaranteed to be positive. However, the real issue is that when n == 0, the method returns null instead of 0L, which might be a valid bucket size in some contexts. If n == 0 is intentionally invalid, the check at line 119 is correct but redundant. If n == 0 should be allowed, the method logic is incorrect. if (bucketSeconds != null && bucketSeconds > 0L) { return rewriteFixedLengthTimeBucket(rexBuilder, field, bucketSeconds, original.getType()); } } Possible Issue The constantFold method checks outMs == 0L at line 241, but UNIT_TO_MILLIS maps MICROSECOND to 0L (line 74). This means any TIMESTAMPDIFF with MICROSECOND as the out-unit will return null from constantFold, even though the comment at line 244 suggests microseconds are "treated separately below" — but no separate handling exists. This causes valid microsecond-based diffs to fall through to the runtime rewrite path or be rejected, when they could be constant-folded. Long inMs = UNIT_TO_MILLIS.get(inUnit.toUpperCase(Locale.ROOT)); Long outMs = UNIT_TO_MILLIS.get(outUnit.toUpperCase(Locale.ROOT)); if (inMs == null || outMs == null || outMs == 0L) { return null; Arithmetic Overflow The constantFold method uses Math.multiplyExact(inValue, inMs) at line 247, which throws ArithmeticException on overflow. However, this exception is not caught, so it will propagate up and likely cause query failure. The caller at line 158 does not handle exceptions from constantFold. If inValue is large (e.g., Long.MAX_VALUE with unit WEEK), the multiplication overflows and crashes the adapter instead of gracefully falling back to the runtime rewrite. long totalMs = Math.multiplyExact(inValue, inMs); Possible Issue The shouldSkipPartialFinalSplit method checks aggIdx >= aggCalls.size() || k >= inputFields.size() at line 114 and returns true (skip split) when out of bounds. However, this condition should never occur if the aggregate is well-formed — groupSet indices should always be valid input column indices, and aggIdx should always be within aggCalls. If this condition fires, it indicates a malformed plan, yet the code silently skips the split instead of failing fast. This could mask bugs in plan construction. if (aggIdx >= aggCalls.size() || k >= inputFields.size()) { return true; // out of bounds → split would be structurally invalid

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to cc0b9ab

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Handle BigDecimal overflow exception The longValueExact() call can throw ArithmeticException when the BigDecimal value is out of range for long. Wrap this call in a try-catch to handle overflow gracefully and return null instead of propagating the exception. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/SpanAdapter.java [138-152] long n; if (value instanceof BigDecimal bd) { if (bd.scale() > 0 && bd.stripTrailingZeros().scale() > 0) { return null; // non-integer interval not supported } - n = bd.longValueExact(); + try { + n = bd.longValueExact(); + } catch (ArithmeticException unused) { + return null; + } } else if (value instanceof Number num) { double d = num.doubleValue(); if (d != Math.floor(d)) { return null; } n = (long) d; } else { return null; } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that longValueExact() can throw ArithmeticException for out-of-range values. Adding exception handling improves robustness by gracefully returning null instead of propagating the exception, which aligns with the method's contract of returning null for invalid inputs.	Medium
	Remove redundant positive check The condition bucketSeconds > 0L is redundant because bucketSecondsIfPositiveInteger already returns null when n <= 0. Remove the redundant check to simplify the logic. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/SpanAdapter.java [118-121] Long bucketSeconds = bucketSecondsIfPositiveInteger(interval, unitSeconds); -if (bucketSeconds != null && bucketSeconds > 0L) { +if (bucketSeconds != null) { return rewriteFixedLengthTimeBucket(rexBuilder, field, bucketSeconds, original.getType()); } Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that bucketSecondsIfPositiveInteger already validates n <= 0 and returns null in that case (line 153-155), making the bucketSeconds > 0L check redundant. However, the redundant check doesn't cause incorrect behavior and the improvement is minor.	Low
	Remove unreachable zero check The check outMs == 0L is unreachable because UNIT_TO_MILLIS only contains positive values (1L minimum for MILLISECOND). Remove this dead code condition to improve clarity. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/TimestampDiffAdapter.java [241-243] -if (inMs == null || outMs == null || outMs == 0L) { +if (inMs == null || outMs == null) { return null; } Suggestion importance[1-10]: 5 __ Why: The suggestion is correct that UNIT_TO_MILLIS only contains positive values (minimum 1L for MILLISECOND), making outMs == 0L unreachable. However, the check for MICROSECOND maps to 0L (line 74), so the condition is actually reachable and the suggestion is incorrect about it being dead code.	Low

---

Previous suggestions

Suggestions up to commit c75f6ad

Category	Suggestion	Impact
General	Remove redundant positive value check The condition bucketSeconds > 0L is redundant because bucketSecondsIfPositiveInteger already returns null for non-positive values (see the n <= 0 check). This redundant check can be removed to simplify the logic. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/SpanAdapter.java [118-121] Long bucketSeconds = bucketSecondsIfPositiveInteger(interval, unitSeconds); -if (bucketSeconds != null && bucketSeconds > 0L) { +if (bucketSeconds != null) { return rewriteFixedLengthTimeBucket(rexBuilder, field, bucketSeconds, original.getType()); } Suggestion importance[1-10]: 5 __ Why: The bucketSeconds > 0L check is indeed redundant since bucketSecondsIfPositiveInteger already ensures non-null implies positive. Removing it simplifies the code slightly without changing behavior.	Low

Suggestions up to commit e9b314c

Category	Suggestion	Impact
Possible issue	Validate schema field count before iteration If target_schema has fewer fields than the batch, accessing batch.column(i) will panic when i >= batch.num_columns(). Verify that both schemas have the same field count before iterating to prevent index-out-of-bounds panics. sandbox/plugins/analytics-backend-datafusion/rust/src/api.rs [969-985] +if batch.num_columns() != target_schema.fields().len() { + return Err(DataFusionError::Execution(format!( + "Schema field count mismatch: batch has {} columns, target schema has {} fields", + batch.num_columns(), + target_schema.fields().len() + ))); +} let mut columns: Vec<arrow::array::ArrayRef> = Vec::with_capacity(batch.num_columns()); for (i, target_field) in target_schema.fields().iter().enumerate() { let src = batch.column(i); if src.data_type() == target_field.data_type() { columns.push(src.clone()); } else { columns.push(arrow::compute::cast(src, target_field.data_type()).map_err(|e| { ... })?); } } Suggestion importance[1-10]: 8 __ Why: This is a valid safety concern. If target_schema has more fields than the batch, accessing batch.column(i) will panic. Adding an upfront validation prevents potential runtime panics and provides a clearer error message.	Medium
General	Prevent unnecessary overflow in unit conversion The multiplication inValue * inMs can overflow when both values are large, but the subsequent division by outMs might bring the result back into range. Consider performing the division before multiplication when inMs is divisible by outMs to avoid unnecessary overflow rejections. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/TimestampDiffAdapter.java [240-244] +if (inMs % outMs == 0) { + long factor = inMs / outMs; + return Math.multiplyExact(inValue, factor); +} long totalMs = Math.multiplyExact(inValue, inMs); if (totalMs % outMs != 0) { return null; } return totalMs / outMs; Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that when inMs is divisible by outMs, performing division before multiplication can prevent overflow while maintaining correctness. This optimization is valid and improves robustness for large values.	Medium
	Remove redundant positive value check The check bucketSeconds > 0L is redundant because bucketSecondsIfPositiveInteger already returns null when n <= 0. The null-check alone is sufficient, simplifying the condition. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/SpanAdapter.java [118-121] Long bucketSeconds = bucketSecondsIfPositiveInteger(interval, unitSeconds); -if (bucketSeconds != null && bucketSeconds > 0L) { +if (bucketSeconds != null) { return rewriteFixedLengthTimeBucket(rexBuilder, field, bucketSeconds, original.getType()); } Suggestion importance[1-10]: 5 __ Why: The suggestion is correct that bucketSecondsIfPositiveInteger already returns null for non-positive values (line 153-155), making the > 0L check redundant. This is a minor code cleanup that improves readability.	Low

Suggestions up to commit 2817c6e

Category	Suggestion	Impact
Possible issue	Prevent silent overflow in multiplication The multiplication n * unitSeconds can silently overflow when both values are large positive longs, producing incorrect negative or wrapped results. Check for overflow before returning, or use Math.multiplyExact() to throw on overflow and handle it appropriately. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/SpanAdapter.java [153-156] if (n <= 0) { return null; } -return n * unitSeconds; +try { + return Math.multiplyExact(n, unitSeconds); +} catch (ArithmeticException e) { + return null; +} Suggestion importance[1-10]: 8 __ Why: This is a valid concern as multiplying two large positive long values can silently overflow, producing incorrect results. Using Math.multiplyExact() with exception handling prevents silent data corruption in time bucket calculations, which could lead to incorrect query results.	Medium
	Validate column count before indexing If batch.num_columns() is less than target_schema.fields().len(), the loop will panic when calling batch.column(i) with an out-of-bounds index. Validate that the column counts match before iterating, or use batch.columns().iter().zip(target_schema.fields()) to safely pair columns with fields. sandbox/plugins/analytics-backend-datafusion/rust/src/api.rs [969-985] +if batch.num_columns() != target_schema.fields().len() { + return Err(DataFusionError::Execution(format!( + "Column count mismatch: batch has {} columns but target schema has {} fields", + batch.num_columns(), + target_schema.fields().len() + ))); +} let mut columns: Vec<arrow::array::ArrayRef> = Vec::with_capacity(batch.num_columns()); for (i, target_field) in target_schema.fields().iter().enumerate() { let src = batch.column(i); if src.data_type() == target_field.data_type() { columns.push(src.clone()); } else { columns.push(arrow::compute::cast(src, target_field.data_type()).map_err(|e| { - DataFusionError::Execution(format!( + DatafusionError::Execution(format!( "Failed to cast partition stream column '{}' from {:?} to {:?}: {}", target_field.name(), src.data_type(), target_field.data_type(), e )) })?); } } Suggestion importance[1-10]: 8 __ Why: The suggestion identifies a potential panic when batch.num_columns() is less than target_schema.fields().len(). Adding an upfront validation prevents out-of-bounds access and provides a clear error message, improving robustness of the schema coercion logic.	Medium
	Handle ArithmeticException from longValueExact The longValueExact() call can throw ArithmeticException if the BigDecimal has a non-zero fractional part or exceeds long range. Wrap this in a try-catch block to return null gracefully instead of propagating an exception, maintaining consistency with the other validation paths. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/SpanAdapter.java [138-152] long n; if (value instanceof BigDecimal bd) { if (bd.scale() > 0 && bd.stripTrailingZeros().scale() > 0) { return null; // non-integer interval not supported } - n = bd.longValueExact(); + try { + n = bd.longValueExact(); + } catch (ArithmeticException e) { + return null; + } } else if (value instanceof Number num) { double d = num.doubleValue(); if (d != Math.floor(d)) { return null; } n = (long) d; } else { return null; } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that longValueExact() can throw ArithmeticException when the BigDecimal exceeds long range. However, the existing code already checks for non-integer values via scale(), making overflow the primary concern. Adding the try-catch improves robustness and maintains consistency with the validation pattern.	Medium

Suggestions up to commit 2817c6e

Category	Suggestion	Impact
Possible issue	Validate column count before iteration If target_schema has fewer fields than batch.num_columns(), accessing batch.column(i) will panic when i exceeds the batch's column count. Verify that both schemas have the same number of columns before iterating. sandbox/plugins/analytics-backend-datafusion/rust/src/api.rs [969-971] +if batch.num_columns() != target_schema.fields().len() { + return Err(DataFusionError::Execution(format!( + "Schema mismatch: batch has {} columns but target schema has {} fields", + batch.num_columns(), + target_schema.fields().len() + ))); +} let mut columns: Vec<arrow::array::ArrayRef> = Vec::with_capacity(batch.num_columns()); for (i, target_field) in target_schema.fields().iter().enumerate() { let src = batch.column(i); Suggestion importance[1-10]: 8 __ Why: Important safety check to prevent potential panic when target_schema and batch have mismatched column counts. The iteration uses target_schema.fields().len() but accesses batch.column(i), which could panic if schemas diverge in size.	Medium
General	Handle potential ArithmeticException from longValueExact Calling longValueExact() can throw ArithmeticException if the BigDecimal has a non-zero fractional part or exceeds Long range. Wrap this call in a try-catch block to handle potential exceptions gracefully. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/SpanAdapter.java [140-143] if (bd.scale() > 0 && bd.stripTrailingZeros().scale() > 0) { return null; // non-integer interval not supported } -n = bd.longValueExact(); +try { + n = bd.longValueExact(); +} catch (ArithmeticException e) { + return null; +} Suggestion importance[1-10]: 7 __ Why: Valid concern about longValueExact() potentially throwing ArithmeticException if the value exceeds Long range. The try-catch addition provides defensive error handling, though the preceding scale check already filters out non-integers.	Medium
	Remove redundant positive value check The check bucketSeconds > 0L is redundant because bucketSecondsIfPositiveInteger already returns null when n <= 0. Remove the redundant check to simplify the condition. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/SpanAdapter.java [118-121] Long bucketSeconds = bucketSecondsIfPositiveInteger(interval, unitSeconds); -if (bucketSeconds != null && bucketSeconds > 0L) { +if (bucketSeconds != null) { return rewriteFixedLengthTimeBucket(rexBuilder, field, bucketSeconds, original.getType()); } Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that bucketSecondsIfPositiveInteger already validates n <= 0 and returns null in that case, making the bucketSeconds > 0L check redundant. This simplifies the code without changing behavior.	Low

[github-actions]

Persistent review updated to latest commit 2817c6e

[github-actions]

Persistent review updated to latest commit e9b314c

[github-actions]

❌ Gradle check result for e9b314c: 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 c75f6ad

[github-actions]

❌ Gradle check result for c75f6ad: 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 cc0b9ab

[github-actions]

✅ Gradle check result for cc0b9ab: SUCCESS

[codecov]

Codecov Report

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

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #21747      +/-   ##
============================================
- Coverage     73.48%   73.44%   -0.04%     
+ Complexity    75078    75068      -10     
============================================
  Files          6012     6016       +4     
  Lines        340940   341072     +132     
  Branches      49076    49091      +15     
============================================
- Hits         250543   250505      -38     
- Misses        70409    70559     +150     
- Partials      19988    20008      +20     

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

Thanks for making the changes.