Support different shapes of TIMESTAMP function

[vinaykpud]

Description

Enhances the DatetimeAdapter on the analytics-backend-datafusion route to handle the full set of call shapes that PPL's TIMESTAMP(...) function supports, so legacy PPL parity holds without invoking DataFusion's to_timestamp(to_date(...)) chain.

The adapter recognizes seven call shapes and routes each to the right backend path:

Shape	Example	Path
A	TIMESTAMP('2020-01-01 00:00:00')	plan-time fold (varchar literal)
B	TIMESTAMP(DATE('2020-08-26'))	plan-time fold → date.atStartOfDay(UTC)
C	TIMESTAMP(TIME('10:20:30'))	plan-time fold → today's UTC date + time
D	TIMESTAMP(TIMESTAMP('lit'))	inner shape A folds; outer falls through to to_timestamp (identity at runtime)
E	TIMESTAMP('2020-01-01 10:00:00', '01:30:00')	2-arg fold, adds time-of-day
F	TIMESTAMP(<column>)	falls through to DateTimeAdapters.DatetimeAdapter (to_timestamp builtin)
G	TIMESTAMP('<bad string>')	rejected up front with IllegalArgumentException carrying the raw input → HTTP 400

[github-actions]

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 25b2c3e.

Path	Line	Severity	Description
sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/TimestampFunctionAdapter.java	303	low	Raw user-controlled input is passed directly as the IllegalArgumentException message (`throw new IllegalArgumentException(input)`). The integration test in TimestampFunctionIT.java explicitly pins this contract, confirming the raw input is reflected back in HTTP 400 responses. This is intentional by design (documented in comments and tested), but it means arbitrary user-supplied strings appear verbatim in API error responses. No injection risk in the current JSON context, but worth noting as a deliberate reflection of unsanitized input.

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 25b2c3e)

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 In tryFoldTwoArg, when LocalDateTime.parse(tsStr.toString().replace(' ', 'T')) throws DateTimeParseException, the method returns null and the caller falls through to DATETIME_ADAPTER. However, the first operand was already validated by parseTimestamp(tsValue) at line 197, which throws IllegalArgumentException for unparseable input. This creates inconsistent error handling: a bad first arg in 2-arg calls triggers HTTP 400 via parseTimestamp, but if tsStr.toString() somehow produces a string that LocalDateTime.parse rejects (e.g., due to precision loss or formatting edge cases in TimestampString), the query silently falls through instead of rejecting. The realistic trigger is if TimestampString.toString() ever emits a format that LocalDateTime.parse cannot consume after the space-to-T replacement. LocalDateTime base; try { base = LocalDateTime.parse(tsStr.toString().replace(' ', 'T')); } catch (DateTimeParseException e) { return null; } Possible Issue parseTimeOfDay at line 216 throws DateTimeParseException with message "Unsupported time format" when no formatter matches, but the caller tryFoldTwoArg at line 199 catches DateTimeParseException and returns null, causing fall-through to DATETIME_ADAPTER instead of rejecting with HTTP 400. This is inconsistent with Shape A/E first-arg handling where parseTimestamp throws IllegalArgumentException that propagates to HTTP 400. A user passing TIMESTAMP('2020-01-01 10:00:00', 'bad-time') gets a silent fall-through (likely a DataFusion error at runtime) instead of an immediate HTTP 400 with the bad input. The trigger is any unparseable second argument in a 2-arg call. try { addTime = parseTimeOfDay(timeValue); } catch (DateTimeParseException e) { return null; }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 25b2c3e

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Use IllegalArgumentException for consistency The DateTimeParseException constructor requires a valid error index, but passing 0 for unparseable input may be misleading. Consider using IllegalArgumentException with just the input string to match the pattern used in parseTimestamp, ensuring consistent error handling across the adapter. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/TimestampFunctionAdapter.java [216-223] private static LocalTime parseTimeOfDay(String input) { for (java.time.format.DateTimeFormatter formatter : TIME_OF_DAY_FORMATS) { try { return formatter == null ? LocalTime.parse(input) : LocalTime.parse(input, formatter); } catch (DateTimeParseException ignored) {} } - throw new DateTimeParseException("Unsupported time format", input, 0); + throw new IllegalArgumentException(input); } Suggestion importance[1-10]: 6 __ Why: Valid suggestion for consistency. The parseTimestamp method throws IllegalArgumentException with the raw input (line 298), while parseTimeOfDay throws DateTimeParseException. Using IllegalArgumentException would provide consistent error handling across the adapter, though the current code is functionally correct.	Low

---

Previous suggestions

Suggestions up to commit b132c96

Category	Suggestion	Impact
General	Validate timestamp after time addition The plusNanos operation can overflow when addTime.toNanoOfDay() is large (e.g., 23:59:59.999999999 adds ~86.4 billion nanoseconds). While LocalDateTime handles this internally, the subsequent toTimestampString call may produce a timestamp beyond the i64-ns ceiling without triggering rejectIfOutsideI64NsRange. Validate the sum before converting. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/TimestampFunctionAdapter.java [212-213] private static RexNode tryFoldTwoArg(RexCall original, int precision, RexBuilder rexBuilder) { ... LocalDateTime sum = base.plusNanos(addTime.toNanoOfDay()); + rejectIfOutsideI64NsRange(sum); return rexBuilder.makeTimestampLiteral(toTimestampString(sum), precision); } Suggestion importance[1-10]: 7 __ Why: Valid concern about overflow past the i64-ns ceiling after plusNanos. The toTimestampString method calls rejectIfOutsideI64NsRange internally (line 302), but adding an explicit check before conversion improves clarity and ensures the validation happens before any potential issues.	Medium
	Include input in error message The exception message "Unsupported time format" doesn't include the actual input value, making debugging difficult when users provide invalid time strings. Include the input in the message to match the pattern used in parseTimestamp where the raw input is the exception message. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/TimestampFunctionAdapter.java [216-223] private static LocalTime parseTimeOfDay(String input) { for (java.time.format.DateTimeFormatter formatter : TIME_OF_DAY_FORMATS) { try { return formatter == null ? LocalTime.parse(input) : LocalTime.parse(input, formatter); } catch (DateTimeParseException ignored) {} } - throw new DateTimeParseException("Unsupported time format", input, 0); + throw new IllegalArgumentException(input); } Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies inconsistency with parseTimestamp which throws IllegalArgumentException(input). However, changing to IllegalArgumentException alters the exception type from DateTimeParseException, which may affect error handling. The improvement to include the input value is valid but the exception type change needs consideration.	Low
	Validate maximum operand count Add validation to reject operand counts greater than 2 before processing. The current code silently returns null for 3+ operands, which may mask configuration errors or unexpected call shapes. Explicitly rejecting invalid arities improves debuggability. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/TimestampFunctionAdapter.java [110-123] private static RexNode tryFoldLiteral(RexCall original, List<FieldStorageInfo> fieldStorage, RelOptCluster cluster) { + if (original.getOperands().size() > 2) { + throw new IllegalArgumentException("TIMESTAMP function accepts at most 2 arguments, got " + original.getOperands().size()); + } int precision = resolvePrecision(fieldStorage); RexBuilder rexBuilder = cluster.getRexBuilder(); if (original.getOperands().size() == 2) { return tryFoldTwoArg(original, precision, rexBuilder); } if (original.getOperands().size() != 1) { return null; } ... Suggestion importance[1-10]: 3 __ Why: The suggestion proposes throwing an exception for operand counts > 2, but the current code already handles this by returning null and falling through to DATETIME_ADAPTER. The silent fallback is intentional design, not a bug. The suggestion would change behavior without clear benefit.	Low

Suggestions up to commit 415608e

Category	Suggestion	Impact
Possible issue	Add lower bound validation The range validation only checks the upper bound but ignores negative timestamps. Values before 1970-01-01 (negative epoch seconds) can underflow when converted to i64 nanoseconds. Add a lower bound check using ldt.isBefore(I64_NS_MIN) where I64_NS_MIN represents the minimum representable timestamp. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/TimestampFunctionAdapter.java [335-339] private static void rejectIfOutsideI64NsRange(LocalDateTime ldt) { - if (ldt.isAfter(I64_NS_MAX)) { + if (ldt.isBefore(I64_NS_MIN) || ldt.isAfter(I64_NS_MAX)) { throw new IllegalArgumentException("timestamp " + ldt + " is outside the supported range"); } } Suggestion importance[1-10]: 7 __ Why: Valid concern about negative timestamps before 1970 potentially underflowing i64 nanoseconds. Adding lower bound validation would prevent runtime errors, though the PR focuses on upper bound issues that were actually observed.	Medium
General	Prevent potential arithmetic overflow Adding toNanoOfDay() (which returns a long representing nanoseconds within a 24-hour period) directly to a LocalDateTime via plusNanos can cause arithmetic overflow for large nano values. Consider using base.plus(Duration.ofNanos(addTime.toNanoOfDay())) or validate that toNanoOfDay() fits within safe bounds before addition. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/TimestampFunctionAdapter.java [212] -LocalDateTime sum = base.plusNanos(addTime.toNanoOfDay()); +LocalDateTime sum = base.plus(java.time.Duration.ofNanos(addTime.toNanoOfDay())); Suggestion importance[1-10]: 2 __ Why: toNanoOfDay() returns a value bounded by 24 hours (max ~86 trillion nanoseconds), which fits safely in a long. plusNanos handles this correctly without overflow risk. Using Duration.ofNanos adds unnecessary complexity without benefit.	Low

Suggestions up to commit 84ab7b8

Category	Suggestion	Impact
General	Add range validation for 2-arg fold The 2-arg fold path doesn't validate the result against the i64-ns range after adding the time offset. If base is near the ceiling and addTime pushes it over, the overflow check in toTimestampString won't catch it until after the arithmetic. Call rejectIfOutsideI64NsRange(sum) before returning. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/TimestampFunctionAdapter.java [212-213] private static RexNode tryFoldTwoArg(RexCall original, int precision, RexBuilder rexBuilder) { ... LocalDateTime sum = base.plusNanos(addTime.toNanoOfDay()); + rejectIfOutsideI64NsRange(sum); return rexBuilder.makeTimestampLiteral(toTimestampString(sum), precision); } Suggestion importance[1-10]: 7 __ Why: Valid suggestion to add rejectIfOutsideI64NsRange(sum) before creating the literal in tryFoldTwoArg. The range check in toTimestampString would catch overflow, but explicit validation before the literal creation provides clearer error handling for the 2-arg case.	Medium
	Preserve original parse error index The DateTimeParseException constructor requires a valid error index, but passing 0 for all unparseable inputs is misleading when the actual parse failure might occur at a different position. Consider using -1 to indicate "unknown position" or catch and rethrow the original exception's index. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/TimestampFunctionAdapter.java [216-223] private static LocalTime parseTimeOfDay(String input) { + DateTimeParseException lastException = null; for (java.time.format.DateTimeFormatter formatter : TIME_OF_DAY_FORMATS) { try { return formatter == null ? LocalTime.parse(input) : LocalTime.parse(input, formatter); - } catch (DateTimeParseException ignored) {} + } catch (DateTimeParseException e) { + lastException = e; + } } - throw new DateTimeParseException("Unsupported time format", input, 0); + throw lastException != null ? lastException : new DateTimeParseException("Unsupported time format", input, 0); } Suggestion importance[1-10]: 4 __ Why: Minor improvement to preserve the original DateTimeParseException details. While the error index accuracy is helpful for debugging, the current implementation with index 0 is acceptable for user-facing errors. The suggestion adds complexity for marginal benefit.	Low

[github-actions]

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

[github-actions]

Persistent review updated to latest commit 415608e

[github-actions]

❌ Gradle check result for 415608e: 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 b132c96

[github-actions]

❌ Gradle check result for b132c96: 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 25b2c3e

[github-actions]

✅ Gradle check result for 25b2c3e: SUCCESS

[codecov]

Codecov Report

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

Additional details and impacted files

@@            Coverage Diff             @@
##               main   #21751    +/-   ##
==========================================
  Coverage     73.48%   73.48%            
- Complexity    75078    75129    +51     
==========================================
  Files          6012     6016     +4     
  Lines        340940   341072   +132     
  Branches      49076    49091    +15     
==========================================
+ Hits         250543   250645   +102     
- Misses        70409    70476    +67     
+ Partials      19988    19951    -37     

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