[analytics-engine] PPL date/time/timestamp fixes — format, overloads, span types, invalid literals

[vinaykpud]

Summary

Fixes a batch of PPL date/time/timestamp gaps on the analytics-engine (parquet-primary) path: format rendering, missing function overloads, invalid-literal error surface, span() column type preservation, and sub-second precision preservation.

Pairs with opensearch-project/sql#5521. Tests that depend on the sql siblings are gated with @AwaitsFix and listed at the bottom of this description.

Commits

datetime stringify uses space, time has no epoch prefix

• Timestamp cells render as yyyy-MM-dd HH:mm:ss (space separator instead of ISO-T).
• Time-only cells render as HH:mm:ss (no 1970-01-01 epoch prefix).
• cast(boolean AS varchar) rewrites to upper-case TRUE / FALSE to match PPL semantics.

PPL date function gap fixes: week modes, format tokens, tz NULL, cast TIME

• WEEK() / WEEK_OF_YEAR() default to mode 0 (Sunday-start, MySQL semantics) instead of ISO.
• STR_TO_DATE parses %b (Jan/Feb/.../Dec) abbreviations.
• DATE_FORMAT adds %c %U %u %V %v %P tokens.
• UNIX_TIMESTAMP accepts numeric YYYYMMDDhhmmss literals.
• CONVERT_TZ returns NULL on out-of-range tz instead of throwing.
• CAST(<datetime-string> AS TIME) strips the date prefix.
• New Rust UDFs: os_week (8-mode MySQL WEEK) and os_strftime (shared parse / render engine for DATE_FORMAT and STR_TO_DATE).

PPL invalid date literals reject with typed format-hint message

• Pre-isthmus validators surface malformed string literals as IllegalArgumentException with the typed message "<type>:<value> in unsupported format, please use '<pattern>'", replacing the opaque Arrow Parser error / 500 StreamException.
• Covers CAST(<lit> AS DATE/TIME/TIMESTAMP), TIMESTAMPADD/DIFF string args, DATE/TIME/DATE_FORMAT/TIME_FORMAT/DATE_PART operands, and silent-success cases (HOUR('99:99:99'), STR_TO_DATE out-of-range month/day).
• DATETIME(<invalid>) folds to NULL TIMESTAMP at plan time.

Add missing PPL date function overloads

Ten function overloads that previously failed isthmus lowering with "Unable to convert call ..." now bind:

• DATETIME(string, tz) — plan-time fold for literals; column input rewrites to convert_tz.
• now(fsp) / sysdate(fsp) — drops the FSP argument.
• DATE_ADD(TIME, interval) and DATE_SUB(TIME, interval) — anchors TIME to today UTC.
• TIMESTAMPADD standalone — lowers to DATETIME_PLUS + interval.
• TIMESTAMPDIFF standalone — to_unixtime delta with unit scaling.
• DATE_PART(unit, TIME) — today-UTC anchor on TIME operands.
• FROM_UNIXTIME(epoch, format) — composes date_format around 1-arg UDF.
• SPAN(timestamp, N, 'M' | 'q' | 'y') — calendar-unit date_bin rewrite.
• CONVERT_TZ invalid timestamp literal — folds to typed NULL.
• microsecond() — single CAST to TIMESTAMP(6) so the µs fraction survives date_part.

PPL span(date) and span(time) preserve column type

• Adds DateOnlyType / TimeOnlyType UDT markers in the schema builder.
• A date / date_nanos field with a date-only or time-only format mapping now surfaces with the right user-visible label.
• span(<date col>, …) returns a date-typed bucket column (not widened to timestamp); span(<time col>, …) returns a time-typed bucket.
• Substrait wire stays Timestamp(ms) — only the schema label changes.

PPL TIMESTAMP(date, time) combine and HOUR-of-TIME-literal fold

Two capabilities lifted from #22014:

• TIMESTAMP(<date col>, <time col>) 2-arg combine via from_unixtime(to_unixtime(date) + to_unixtime(time)).
• HOUR / MINUTE / SECOND / MICROSECOND on a TIME literal folds to BIGINT at plan time (avoids the precision_time substrait gap).

test(sandbox): backfill CAST(time-string AS DATE) rejection test

• Backfills a single missing unit test in CastTemporalLiteralValidatorTests.

test(sandbox): add DatetimeCoverageIT for cluster-all regression gate

• End-to-end IT — 59 methods covering every shape this PR fixes plus min/max boundary values for DATE / TIME / TIMESTAMP.
• One-to-one mapping between methods and commits — any cluster regression surfaces as a single named failure.

test(sandbox): @AwaitsFix on tests that require sql cluster A/D

• 18 tests temporarily annotated @AwaitsFix because they depend on the sql/core siblings (cluster A schema label + cluster D list-element formatter).
• To be reverted once the sql PR merges.

fix(sandbox): DateOnly/TimeOnly UDTs carry source-type precision (date→3, date_nanos→9)

#22049 added sub-second precision (date→3, date_nanos→9) on the plain-TIMESTAMP path. The DateOnlyType / TimeOnlyType UDT branch short-circuited before that switch, so a date_nanos field with a date-only or time-only format produced TIMESTAMP(0) and broke multi-shard sort. Mirror the same precision selection on the UDT factories.

Query shapes fixed

Format / output rendering

source=calcs | head 1 | eval ts = cast('2024-01-15 12:00:00' as timestamp) | fields ts
# was: "2024-01-15T12:00:00"     now: "2024-01-15 12:00:00"

source=calcs | head 5 | stats list(time1) as l
# was: ["1970-01-01T19:36:22", ...]   now: ["19:36:22", ...]

source=calcs | head 1 | eval s = cast(true as string) | fields s
# was: "true"     now: "TRUE"

source=calcs | head 1 | eval m = microsecond('2024-01-15 12:00:00.123456') | fields m
# was: 123000     now: 123456

source=calcs | head 1 | eval f = date_format('2024-01-15 12:00:00.123456', '%f') | fields f
# was: "123000"   now: "123456"

source=calcs | head 1 | eval ts = cast('2023-10-01 12:00:00.123456' as timestamp) | fields ts
# was: "2023-10-01 12:00:00.123"   now: "2023-10-01 12:00:00.123456"

Function overloads — previously failed with Unable to convert call ...

source=calcs | head 1 | eval f = DATETIME('2008-01-01 02:00:00', '+10:00') | fields f
# now: "2008-01-01 12:00:00"

source=calcs | head 1 | eval f = DATE_ADD(TIME('08:40:00'), INTERVAL 1 DAY) | fields f
source=calcs | head 1 | eval f = DATE_SUB(TIME('08:40:00'), INTERVAL 1 DAY) | fields f

source=calcs | head 1 | eval f = TIMESTAMPADD(DAY, 5, '2016-03-01 00:00:00') | fields f
# now: "2016-03-06 00:00:00"

source=calcs | head 1 | eval f = TIMESTAMPDIFF(DAY, '2020-01-01 00:00:00', '2020-01-05 00:00:00') | fields f
# now: 4

source=calcs | head 1 | eval h = HOUR(TIME('17:30:00')) | fields h
# now: 17

source=calcs | head 1 | eval f = FROM_UNIXTIME(1611657712, '%Y-%m-%d') | fields f
# now: "2021-01-26"

source=calcs | head 1 | eval f = sysdate(0) | fields f      # now: current ts (FSP arg dropped)

source=calcs | stats count() as c by span(datetime0, 3month) as bucket
# now: 200 + buckets at 3-month boundaries

source=calcs | head 1 | eval ts = TIMESTAMP(date1, time1) | fields ts   # 2-arg date+time combine

Format tokens — date_format / str_to_date

source=calcs | head 1 | eval f = date_format(timestamp('1998-01-31 13:14:15'), '%c %U %u %V %v') | fields f
# now: "01 4 4 4 4"   (was: missing tokens or wrong values)

source=calcs | head 1 | eval f = str_to_date('1-May-13', '%d-%b-%y') | fields f
# was: "2013-01-01" (silent month=1)   now: "2013-05-01"

Mode / range corrections

source=calcs | head 1 | eval w = week(date('2008-02-20')) | fields w
# was: 8 (ISO)   now: 7 (MySQL mode 0, Sunday-start)

source=calcs | head 1 | eval u = unix_timestamp(20771122143845) | fields u
# was: pass-through 20771122143845   now: parsed as YYYYMMDDhhmmss → 3404817525

source=calcs | head 1 | eval f = convert_tz('2021-05-12 11:34:50', '+09:00', '+15:00') | fields f
# was: HTTP 400 IllegalArgumentException   now: null (out-of-range tz)

source=calcs | head 1 | eval t = cast('1985-10-09 12:00:00' as TIME) | fields t
# was: HTTP 500 parse error   now: "12:00:00" (date prefix stripped)

date_nanos sub-second precision (multi-shard sort)

# date_nanos field with a date-only format on a 2-shard parquet-backed index:
source=ix | sort d | fields d
# was: HTTP 500 — "Timestamp(ms) got Timestamp(ns)" at coordinator RowConverter
# now: HTTP 200 — rows surface in ascending order

Span column type preservation

source=date_formats | stats count() as c by span(basic_date, 1day) as date_span
# was: schema date_span:timestamp, value "1984-04-12 00:00:00"
# now: schema date_span:date,      value "1984-04-12"

source=date_formats | stats count() as c by span(time1, 1minute) as time_span
# was: schema time_span:timestamp, value "1970-01-01 09:00:00"
# now: schema time_span:time,      value "09:00:00"

Invalid literal rejection

source=calcs | head 1 | eval a = DATE('2025-13-02')                                  | fields a
source=calcs | head 1 | eval a = TIME('99:99:99')                                    | fields a
source=calcs | head 1 | eval a = TIMESTAMP('2025-15-26 25:00:00')                    | fields a
source=calcs | head 1 | eval a = cast('09:07:42' as DATE)                            | fields a
source=calcs | head 1 | eval a = HOUR('99:99:99')                                    | fields a   # was: silent 200 with 0
source=calcs | head 1 | eval a = STR_TO_DATE('01,13,2013', '%d,%m,%Y')               | fields a   # was: silent month=13

# All now: HTTP 400 IllegalArgumentException
# {"details": "<type>:<value> in unsupported format, please use '<pattern>'"}
# (replacing prior HTTP 500 / opaque Arrow Parser error / silent wrong row)

source=calcs | head 1 | eval a = DATETIME('2025-13-02 10:00:00') | fields a
# now: HTTP 200, returns null (legacy SQL DATETIME(<invalid>) semantics)

Tests gated @AwaitsFix until sql PR merges

The following pass when the sql/core sibling PR is installed; without it they fail with substrait schema mismatches or unbound function-call lowerings.

• DatetimeCoverageIT — 14 methods (cluster A span tests, cluster B 2-arg DATETIME, cluster E µs preservation, cluster F format/cast/maketime, max-time / max-timestamp boundaries).
• DateTimeScalarFunctionsIT.testDateAddMillisecondIntervalOnTimestampColumn
• TimestampFunctionIT.testShapeCTimeLiteralFoldsWithTodayUtc
• TimestampFunctionIT.testTimeEqualsDateDoesNotCrash
• TwoShardCommandIT.testReduceCorrectnessAcrossTwoShards

Sites are searchable via grep -rn "pull/<TBD>" sandbox/qa/ and will be unmuted in a follow-up commit once the sql PR merges.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 28279be)

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 standalone TIMESTAMPDIFF rewrite uses 30/90/365-day approximations for MONTH/QUARTER/YEAR out-units. This is lossy and can produce incorrect results when the actual calendar months have different lengths (e.g., February vs. March). The approximation is noted as "legacy SQL plugin parity" but no validation confirms the input timestamps fall within a range where the approximation is acceptable. For queries spanning months of varying lengths, the result will be wrong. /** Standalone TIMESTAMPDIFF: {@code (to_unixtime(t2) - to_unixtime(t1))} scaled to out-unit. */ private static RexNode rewriteStandaloneDiff(RelOptCluster cluster, RexCall original, String outUnit, RexNode start, RexNode end) { RexBuilder rb = cluster.getRexBuilder(); RexNode t1 = liftToTimestamp(rb, start, original.getType()); RexNode t2 = liftToTimestamp(rb, end, original.getType()); RexNode endSeconds = rb.makeCall(UnixTimestampAdapter.LOCAL_TO_UNIXTIME_OP, t2); RexNode startSeconds = rb.makeCall(UnixTimestampAdapter.LOCAL_TO_UNIXTIME_OP, t1); RexNode diffSeconds = rb.makeCall(SqlStdOperatorTable.MINUS, endSeconds, startSeconds); String upper = outUnit.toUpperCase(Locale.ROOT); Long mult = OUT_UNIT_MULTIPLIER_FROM_SECONDS.get(upper); Long div = mult == null ? OUT_UNIT_DIVISOR_FROM_SECONDS.get(upper) : null; Long approxSeconds = mult == null && div == null ? VARIABLE_OUT_APPROX_SECONDS.get(upper) : null; RexNode scaled; if (mult != null && mult > 1L) { RexNode multLit = rb.makeBigintLiteral(BigDecimal.valueOf(mult)); scaled = rb.makeCall(SqlStdOperatorTable.MULTIPLY, diffSeconds, multLit); } else if (div != null && div > 1L) { RexNode divLit = rb.makeBigintLiteral(BigDecimal.valueOf(div)); scaled = rb.makeCall(SqlStdOperatorTable.DIVIDE, diffSeconds, divLit); } else if (approxSeconds != null) { // variable-length out unit — MONTH≈30d, QUARTER≈90d, YEAR≈365d RexNode divLit = rb.makeBigintLiteral(BigDecimal.valueOf(approxSeconds)); scaled = rb.makeCall(SqlStdOperatorTable.DIVIDE, diffSeconds, divLit); } else { scaled = diffSeconds; } return rb.makeCast(original.getType(), scaled, true); } Possible Issue The two-arg DATETIME adapter extracts the source offset via regex and defaults to UTC if no offset is found. However, if the input string contains an invalid offset that the regex does not match (e.g., a malformed offset like "+99:99"), the code silently treats it as UTC rather than rejecting it. This can lead to incorrect timestamp conversions when the user intended a specific timezone but provided a typo. if (value == null || tz == null) { return rexBuilder.makeNullLiteral(original.getType()); } // extract source offset (default UTC) String sourceTz = "+00:00"; String stripped = value; java.util.regex.Matcher m = OFFSET_SUFFIX_REGEX.matcher(value); if (m.find()) { Possible Issue The MySQL DATETIME tz bound check uses the current instant to determine the offset, but timezone offsets can vary with DST. A timezone that is within bounds at query-plan time might be out of bounds at a different time of year, or vice versa. This means the NULL-fold decision is not stable across DST transitions, potentially causing different results for the same literal depending on when the query is planned. Possible Issue The TIME base anchoring to today-UTC uses LocalDate.now(ZoneOffset.UTC) at plan time. If the query is planned in one day and executed in another (e.g., a long-running query or a cached plan), the "today" date will be stale, leading to incorrect results. The anchoring should use the query execution time, not the plan time. if (baseSqlType == SqlTypeName.TIME) { org.apache.calcite.rel.type.RelDataType varchar = rexBuilder.getTypeFactory().createSqlType(SqlTypeName.VARCHAR); org.apache.calcite.rel.type.RelDataType nullableVarchar = rexBuilder.getTypeFactory() .createTypeWithNullability(varchar, base.getType().isNullable()); RexNode timeAsVarchar = rexBuilder.makeCast(nullableVarchar, base); String prefix = java.time.LocalDate.now(java.time.ZoneOffset.UTC).toString() + " "; RexNode prefixLit = rexBuilder.makeLiteral(prefix, varchar, false); RexNode concat = rexBuilder.makeCall( nullableVarchar, org.apache.calcite.sql.fun.SqlStdOperatorTable.CONCAT, List.of(prefixLit, timeAsVarchar) ); baseTimestamp = rexBuilder.makeAbstractCast(original.getType(), concat); } else { baseTimestamp = rexBuilder.makeAbstractCast(original.getType(), base); }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 643892c

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Handle TIME operand anchoring The method liftToTimestamp casts string/date operands to TIMESTAMP but doesn't handle TIME types. TIME operands should be anchored to today's UTC date before casting, similar to how DatePartAdapters.castTimeToTimestamp handles this case. Without this, TIME values may produce incorrect results. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/TimestampDiffAdapter.java [169-180] private static RexNode liftToTimestamp(RexBuilder rb, RexNode operand, org.apache.calcite.rel.type.RelDataType resultType) { - if (operand.getType().getSqlTypeName() == SqlTypeName.TIMESTAMP) { + SqlTypeName operandType = operand.getType().getSqlTypeName(); + if (operandType == SqlTypeName.TIMESTAMP) { return operand; + } + if (operandType == SqlTypeName.TIME) { + RelDataType varchar = rb.getTypeFactory().createSqlType(SqlTypeName.VARCHAR); + RelDataType nullableVarchar = rb.getTypeFactory().createTypeWithNullability(varchar, operand.getType().isNullable()); + RexNode timeAsVarchar = rb.makeCast(nullableVarchar, operand); + String prefix = java.time.LocalDate.now(java.time.ZoneOffset.UTC).toString() + " "; + RexNode prefixLit = rb.makeLiteral(prefix, varchar, false); + RexNode concat = rb.makeCall(nullableVarchar, org.apache.calcite.sql.fun.SqlStdOperatorTable.CONCAT, List.of(prefixLit, timeAsVarchar)); + org.apache.calcite.rel.type.RelDataType tsType = rb.getTypeFactory() + .createTypeWithNullability(rb.getTypeFactory().createSqlType(SqlTypeName.TIMESTAMP), operand.getType().isNullable() || resultType.isNullable()); + return rb.makeCast(tsType, concat, true); } org.apache.calcite.rel.type.RelDataType tsType = rb.getTypeFactory() .createTypeWithNullability( rb.getTypeFactory().createSqlType(SqlTypeName.TIMESTAMP), operand.getType().isNullable() || resultType.isNullable() ); return rb.makeCast(tsType, operand, true); } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that liftToTimestamp doesn't handle TIME operands, which should be anchored to today's UTC date. However, the improved code duplicates logic already present in DatePartAdapters.castTimeToTimestamp, and the PR already handles TIME in the standalone diff path via liftToTimestamp. The suggestion is valid but not critical since TIME handling exists elsewhere.	Medium
General	Validate timezone offset boundary The test validates lenient behavior for out-of-range timezone offsets (e.g., "+15:00"), but the maximum valid UTC offset is ±14:00. Verify that ConvertTzAdapter correctly identifies "+15:00" as invalid and returns a typed NULL, as offsets beyond ±14:00 are not recognized by standard timezone libraries. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/ConvertTzAdapter.java [190-197] public void testAdaptOutOfRangeLiteralReturnsTypedNull() { + // +15:00 exceeds the valid UTC offset range (±14:00) RexCall original = buildConvertTz("UTC", "+15:00"); RexNode adapted = new ConvertTzAdapter().adapt(original, List.of(), cluster); assertTrue("out-of-range tz literal must yield a NULL literal", adapted instanceof RexLiteral); assertTrue("NULL literal must report null value", ((RexLiteral) adapted).isNull()); assertEquals("typed NULL must keep the original return type", original.getType(), adapted.getType()); } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that "+15:00" exceeds the valid UTC offset range (±14:00) and asks to verify the adapter handles this correctly. The test already validates this behavior, so the suggestion mainly adds a clarifying comment. This is a useful verification but doesn't change functionality.	Medium
	Use fixed reference for offset The method isWithinMysqlDatetimeTzBounds uses Instant.now() to get the current offset, but this can produce inconsistent results for zones with DST transitions. The offset should be evaluated at a fixed reference point (e.g., the epoch or the timestamp being converted) to ensure deterministic validation. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DateTimeAdapters.java [307-315] private static boolean isWithinMysqlDatetimeTzBounds(String canonical) { try { java.time.ZoneId zone = java.time.ZoneId.of(canonical); - int totalSeconds = zone.getRules().getOffset(java.time.Instant.now()).getTotalSeconds(); + int totalSeconds = zone.getRules().getOffset(java.time.Instant.EPOCH).getTotalSeconds(); return totalSeconds >= -50340 && totalSeconds <= 50400; } catch (java.time.DateTimeException e) { return false; } } Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that using Instant.now() can produce inconsistent results for zones with DST transitions. Using Instant.EPOCH as a fixed reference point would make the validation deterministic. However, the MySQL DATETIME tz bounds check is primarily concerned with the offset range, and DST variations within the [-13:59, +14:00] range are unlikely to cause practical issues. The improvement is valid but not critical.	Low
	Add timestamp overflow protection The formatTimestamp method assumes numeric values are always valid epoch timestamps, but doesn't handle potential overflow or underflow when converting microsecond/nanosecond units. Values outside the supported i64-ns range could cause ArithmeticException or produce incorrect results. Add bounds checking before the conversion. sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/exec/ArrowValues.java [131-149] private static String formatTimestamp(ArrowType.Timestamp type, Object value) { LocalDateTime ldt; if (value instanceof LocalDateTime t) { ldt = t; } else if (value instanceof LocalDate ld) { ldt = ld.atStartOfDay(); } else { long raw = ((Number) value).longValue(); - Instant instant = switch (type.getUnit()) { - case SECOND -> Instant.ofEpochSecond(raw); - case MILLISECOND -> Instant.ofEpochMilli(raw); - case MICROSECOND -> Instant.ofEpochSecond(Math.floorDiv(raw, 1_000_000L), Math.floorMod(raw, 1_000_000L) * 1_000L); - case NANOSECOND -> Instant.ofEpochSecond(Math.floorDiv(raw, 1_000_000_000L), Math.floorMod(raw, 1_000_000_000L)); - }; + Instant instant; + try { + instant = switch (type.getUnit()) { + case SECOND -> Instant.ofEpochSecond(raw); + case MILLISECOND -> Instant.ofEpochMilli(raw); + case MICROSECOND -> Instant.ofEpochSecond(Math.floorDiv(raw, 1_000_000L), Math.floorMod(raw, 1_000_000L) * 1_000L); + case NANOSECOND -> Instant.ofEpochSecond(Math.floorDiv(raw, 1_000_000_000L), Math.floorMod(raw, 1_000_000_000L)); + }; + } catch (ArithmeticException | java.time.DateTimeException e) { + return "INVALID_TIMESTAMP"; + } ldt = LocalDateTime.ofInstant(instant, ZoneOffset.UTC); } return ldt.getNano() == 0 ? ldt.format(TIMESTAMP_NO_NANO) : ldt.format(TIMESTAMP_WITH_NANO); } Suggestion importance[1-10]: 5 __ Why: The suggestion proposes adding bounds checking for timestamp conversion to handle potential overflow/underflow. While this is a valid concern, the PR already handles boundary values in DatetimeCoverageIT tests (e.g., testBoundary_minSupportedTimestamp, testBoundary_maxSupportedTimestampMilli). The suggested error handling returns "INVALID_TIMESTAMP" which is not a standard approach; returning null or throwing an exception would be more consistent. The improvement is marginal.	Low
	Strengthen datetime separator validation The space-to-'T' replacement assumes a single space separator, but malformed input with multiple spaces or tabs could bypass validation. Consider trimming and validating the separator pattern before replacement to ensure only well-formed datetime strings are accepted. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DatetimeLiteralValidator.java [80-87] private static boolean isParseableAsFullDatetime(String value) { try { - LocalDateTime.parse(value.replace(' ', 'T')); + String normalized = value.trim().replaceAll("\\s+", "T"); + if (normalized.chars().filter(ch -> ch == 'T').count() != 1) { + return false; + } + LocalDateTime.parse(normalized); return true; } catch (DateTimeParseException ignored) { return false; } } Suggestion importance[1-10]: 5 __ Why: The suggestion addresses a potential edge case with multiple spaces or tabs in datetime strings. However, the current implementation using replace(' ', 'T') only replaces single spaces, which is the expected format. The suggested regex replacement and 'T' count validation adds complexity for an edge case that may not be relevant given the expected input format.	Low
	Strengthen date_bin operand validation The LOCAL_DATE_BIN_OP operator uses OperandTypes.VARIADIC which is overly permissive. The date_bin function expects specific operand types (string stride, timestamp, optional origin). Consider using a more specific operand type checker to catch invalid operand combinations at plan time rather than runtime. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/SpanAdapter.java [76-83] static final SqlOperator LOCAL_DATE_BIN_OP = new SqlFunction( "date_bin", SqlKind.OTHER_FUNCTION, ReturnTypes.ARG1_NULLABLE, null, - OperandTypes.VARIADIC, + OperandTypes.or(OperandTypes.STRING_DATETIME, OperandTypes.STRING_DATETIME_DATETIME), SqlFunctionCategory.TIMEDATE ); Suggestion importance[1-10]: 4 __ Why: The suggestion proposes replacing OperandTypes.VARIADIC with more specific operand types for date_bin. While this could improve plan-time validation, the suggested OperandTypes.or(OperandTypes.STRING_DATETIME, OperandTypes.STRING_DATETIME_DATETIME) may not accurately reflect DataFusion's date_bin signature (which accepts string stride, timestamp, and optional origin). The improvement is minor and the current permissive approach is acceptable since DataFusion validates at runtime.	Low

---

Previous suggestions

Suggestions up to commit c5b4995

Category	Suggestion	Impact
Possible issue	Handle TIME operands with today-UTC anchoring The method should handle TIME operands specially by anchoring them to today's UTC date before casting to TIMESTAMP, similar to how other adapters handle TIME values. Without this, TIME values will be anchored to the epoch (1970-01-01) instead of the current date, producing incorrect results. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/TimestampDiffAdapter.java [169-180] private static RexNode liftToTimestamp(RexBuilder rb, RexNode operand, org.apache.calcite.rel.type.RelDataType resultType) { if (operand.getType().getSqlTypeName() == SqlTypeName.TIMESTAMP) { return operand; + } + if (operand.getType().getSqlTypeName() == SqlTypeName.TIME) { + RelDataType varchar = rb.getTypeFactory().createSqlType(SqlTypeName.VARCHAR); + RelDataType nullableVarchar = rb.getTypeFactory().createTypeWithNullability(varchar, operand.getType().isNullable()); + RexNode timeAsVarchar = rb.makeCast(nullableVarchar, operand); + String prefix = java.time.LocalDate.now(java.time.ZoneOffset.UTC).toString() + " "; + RexNode prefixLit = rb.makeLiteral(prefix, varchar, false); + RexNode concat = rb.makeCall(nullableVarchar, org.apache.calcite.sql.fun.SqlStdOperatorTable.CONCAT, List.of(prefixLit, timeAsVarchar)); + org.apache.calcite.rel.type.RelDataType tsType = rb.getTypeFactory() + .createTypeWithNullability(rb.getTypeFactory().createSqlType(SqlTypeName.TIMESTAMP), operand.getType().isNullable() || resultType.isNullable()); + return rb.makeCast(tsType, concat, true); } org.apache.calcite.rel.type.RelDataType tsType = rb.getTypeFactory() .createTypeWithNullability( rb.getTypeFactory().createSqlType(SqlTypeName.TIMESTAMP), operand.getType().isNullable() || resultType.isNullable() ); return rb.makeCast(tsType, operand, true); } Suggestion importance[1-10]: 8 __ Why: The liftToTimestamp method currently casts TIME operands directly to TIMESTAMP, which anchors them to the epoch (1970-01-01) instead of today's date. This is inconsistent with other adapters in the codebase (e.g., DateAddSubAdapter, TimestampAddAdapter) that explicitly anchor TIME values to today-UTC. The suggestion correctly identifies this gap and provides the proper anchoring logic.	Medium
	Prevent year underflow panic The function computes week numbers using (date - prev_week1).num_days() without validating that prev_week1 is valid. For dates near year boundaries, first_anchored_week_start(year - 1, start) could fail if year - 1 is out of NaiveDate's supported range (year 0 or earlier), causing a panic. Add bounds checking before calling first_anchored_week_start with decremented years. sandbox/plugins/analytics-backend-datafusion/rust/src/udf/os_week.rs [177-194] fn week_number_iso_fold(date: NaiveDate, start: Weekday) -> u32 { let year = date.year(); let week1_start = first_anchored_week_start(year, start); if date < week1_start { + if year <= 1 { + return 1; // Clamp to week 1 for dates before year 1 + } let prev_week1 = first_anchored_week_start(year - 1, start); let days = (date - prev_week1).num_days(); (days / 7) as u32 + 1 } else { - let next_week1 = first_anchored_week_start(year + 1, start); - if date >= next_week1 { - 1 - } else { - let days = (date - week1_start).num_days(); - (days / 7) as u32 + 1 - } + ... } } Suggestion importance[1-10]: 6 __ Why: The suggestion identifies a potential panic when year - 1 goes out of NaiveDate's supported range. While NaiveDate supports years from approximately -262,000 to +262,000, adding bounds checking for edge cases near year 0 or 1 would improve robustness. However, this is a rare edge case unlikely to occur in practice.	Low
General	Use fixed instant for timezone validation Using Instant.now() to check timezone bounds makes the validation time-dependent and can produce inconsistent results across DST transitions. Use a fixed reference instant (e.g., epoch) to ensure deterministic validation behavior. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DateTimeAdapters.java [307-315] private static boolean isWithinMysqlDatetimeTzBounds(String canonical) { try { java.time.ZoneId zone = java.time.ZoneId.of(canonical); - int totalSeconds = zone.getRules().getOffset(java.time.Instant.now()).getTotalSeconds(); + int totalSeconds = zone.getRules().getOffset(java.time.Instant.EPOCH).getTotalSeconds(); return totalSeconds >= -50340 && totalSeconds <= 50400; } catch (java.time.DateTimeException e) { return false; } } Suggestion importance[1-10]: 7 __ Why: Using Instant.now() for timezone offset validation can produce inconsistent results across DST transitions. The suggestion to use Instant.EPOCH as a fixed reference point is valid and would ensure deterministic behavior. However, the current implementation may be intentional to validate against current DST rules, so the impact depends on the intended semantics.	Medium
	Handle LocalTime values in timestamp formatting The method should handle LocalTime instances that may appear in timestamp columns, similar to how formatTime handles LocalDateTime. Without this, a LocalTime value would fall through to the numeric conversion path and likely cause a ClassCastException. sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/exec/ArrowValues.java [132-149] private static String formatTimestamp(ArrowType.Timestamp type, Object value) { LocalDateTime ldt; if (value instanceof LocalDateTime t) { ldt = t; } else if (value instanceof LocalDate ld) { ldt = ld.atStartOfDay(); + } else if (value instanceof LocalTime lt) { + ldt = LocalDateTime.of(LocalDate.ofEpochDay(0), lt); } else { long raw = ((Number) value).longValue(); Instant instant = switch (type.getUnit()) { case SECOND -> Instant.ofEpochSecond(raw); case MILLISECOND -> Instant.ofEpochMilli(raw); case MICROSECOND -> Instant.ofEpochSecond(Math.floorDiv(raw, 1_000_000L), Math.floorMod(raw, 1_000_000L) * 1_000L); case NANOSECOND -> Instant.ofEpochSecond(Math.floorDiv(raw, 1_000_000_000L), Math.floorMod(raw, 1_000_000_000L)); }; ldt = LocalDateTime.ofInstant(instant, ZoneOffset.UTC); } return ldt.getNano() == 0 ? ldt.format(TIMESTAMP_NO_NANO) : ldt.format(TIMESTAMP_WITH_NANO); } Suggestion importance[1-10]: 5 __ Why: The suggestion identifies a potential gap where LocalTime instances might not be handled in formatTimestamp. However, the likelihood of a LocalTime appearing in a TIMESTAMP-typed Arrow column is low, as Arrow's type system would typically prevent this. The suggestion is defensive programming but addresses an edge case that may not occur in practice.	Low
	Validate timezone offset bounds The test validates lenient behavior for out-of-range timezone offsets ("+15:00" exceeds ±14:00 limit), but the implementation in ConvertTzAdapter may not handle this edge case. Verify that the adapter explicitly checks offset bounds and returns a typed NULL for invalid values, rather than propagating them to runtime where they could cause exceptions. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/ConvertTzAdapter.java [190-197] /** out-of-range tz literal -> typed NULL (MySQL-compatible lenient behavior). */ public void testAdaptOutOfRangeLiteralReturnsTypedNull() { RexCall original = buildConvertTz("UTC", "+15:00"); RexNode adapted = new ConvertTzAdapter().adapt(original, List.of(), cluster); assertTrue("out-of-range tz literal must yield a NULL literal", adapted instanceof RexLiteral); assertTrue("NULL literal must report null value", ((RexLiteral) adapted).isNull()); assertEquals("typed NULL must keep the original return type", original.getType(), adapted.getType()); } +// Add companion test for lower bound: testAdaptOutOfRangeLiteralNegativeBound() with "-15:00" Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies a test for out-of-range timezone offsets but only recommends adding a companion test for the negative bound. The existing test already validates the lenient behavior, so the suggestion adds minor test coverage without addressing a critical issue.	Low
	Use space-separator format for origin literal The origin literal uses ISO-T format which will be converted to space-separator format by ArrowValues.spaceSeparator(). To maintain consistency and avoid unnecessary string transformations, use space-separator format directly in the origin literal. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/SpanAdapter.java [129-137] String calendarStrideUnit = CALENDAR_UNIT_TO_DATE_BIN_STRIDE.get(unitText); if (calendarStrideUnit != null) { Long n = extractPositiveInteger(interval); if (n != null) { RexNode stride = rexBuilder.makeLiteral(n + " " + calendarStrideUnit); - RexNode origin = rexBuilder.makeLiteral("1970-01-01T00:00:00Z"); + RexNode origin = rexBuilder.makeLiteral("1970-01-01 00:00:00"); return rexBuilder.makeCall(original.getType(), LOCAL_DATE_BIN_OP, List.of(stride, field, origin)); } } Suggestion importance[1-10]: 3 __ Why: While the suggestion is technically correct that ArrowValues.spaceSeparator() will convert ISO-T format to space-separator format, the current code is not incorrect. The transformation is handled automatically and the ISO-T format is a valid input. This is a minor style improvement rather than a functional issue.	Low

Suggestions up to commit 28279be

Category	Suggestion	Impact
Possible issue	Handle TIME operands with today-UTC anchoring The liftToTimestamp method doesn't handle TIME operands, which need today-UTC anchoring before casting to TIMESTAMP (similar to DatePartAdapters.castTimeToTimestamp). Without this, TIME values will anchor to the epoch (1970-01-01) instead of the current date, producing incorrect results for standalone TIMESTAMPDIFF calls with TIME arguments. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/TimestampDiffAdapter.java [169-180] private static RexNode liftToTimestamp(RexBuilder rb, RexNode operand, org.apache.calcite.rel.type.RelDataType resultType) { - if (operand.getType().getSqlTypeName() == SqlTypeName.TIMESTAMP) { + SqlTypeName operandType = operand.getType().getSqlTypeName(); + if (operandType == SqlTypeName.TIMESTAMP) { return operand; + } + if (operandType == SqlTypeName.TIME) { + RelDataType varchar = rb.getTypeFactory().createSqlType(SqlTypeName.VARCHAR); + RelDataType nullableVarchar = rb.getTypeFactory().createTypeWithNullability(varchar, operand.getType().isNullable()); + RexNode timeAsVarchar = rb.makeCast(nullableVarchar, operand); + String prefix = java.time.LocalDate.now(java.time.ZoneOffset.UTC).toString() + " "; + RexNode prefixLit = rb.makeLiteral(prefix, varchar, false); + RexNode concat = rb.makeCall(nullableVarchar, org.apache.calcite.sql.fun.SqlStdOperatorTable.CONCAT, List.of(prefixLit, timeAsVarchar)); + operand = concat; } org.apache.calcite.rel.type.RelDataType tsType = rb.getTypeFactory() .createTypeWithNullability( rb.getTypeFactory().createSqlType(SqlTypeName.TIMESTAMP), operand.getType().isNullable() || resultType.isNullable() ); return rb.makeCast(tsType, operand, true); } Suggestion importance[1-10]: 8 __ Why: The liftToTimestamp method lacks TIME operand handling, which would cause TIME values to anchor to epoch (1970-01-01) instead of today-UTC. This is a correctness issue that affects standalone TIMESTAMPDIFF calls with TIME arguments, and the suggested fix mirrors the pattern used in DatePartAdapters.castTimeToTimestamp.	Medium
	Validate TIME operand before combining The logic adds unix timestamps of two temporal values, which produces incorrect results when both operands are full timestamps (e.g., adding two dates with times doubles the epoch offset). Verify that at least one operand is a TIME type before applying this transformation. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/TimestampFunctionAdapter.java [110-127] private static RexNode tryCombineTimestampPlusTime(RexCall original, RelOptCluster cluster) { if (original.getOperands().size() != 2) { return null; } RexNode op0 = stripOperatorAnnotation(original.getOperands().get(0)); RexNode op1 = stripOperatorAnnotation(original.getOperands().get(1)); if (!isTemporal(op0) || !isTemporal(op1)) { return null; } + // Only combine if at least one operand is TIME type + SqlTypeName tn0 = op0.getType().getSqlTypeName(); + SqlTypeName tn1 = op1.getType().getSqlTypeName(); + if (tn0 != SqlTypeName.TIME && tn1 != SqlTypeName.TIME) { + return null; + } RexBuilder rexBuilder = cluster.getRexBuilder(); RexNode unix0 = rexBuilder.makeCall(UnixTimestampAdapter.LOCAL_TO_UNIXTIME_OP, op0); RexNode unix1 = rexBuilder.makeCall(UnixTimestampAdapter.LOCAL_TO_UNIXTIME_OP, op1); RexNode sum = rexBuilder.makeCall(org.apache.calcite.sql.fun.SqlStdOperatorTable.PLUS, unix0, unix1); ... } Suggestion importance[1-10]: 8 __ Why: The suggestion identifies a critical logic flaw where adding two full timestamps would incorrectly double the epoch offset. The method name tryCombineTimestampPlusTime and the comment indicate it's specifically for combining date and time, so validating that at least one operand is TIME type is essential.	Medium
	Guard against negative duration cast The function computes week numbers but doesn't handle potential overflow when subtracting dates near year boundaries. If date - prev_week1 produces a negative duration, the cast to u32 will wrap. Add bounds checking or use checked arithmetic to prevent incorrect week calculations. sandbox/plugins/analytics-backend-datafusion/rust/src/udf/os_strftime.rs [176-194] fn week_number_iso_fold(date: NaiveDate, start: Weekday) -> u32 { let year = date.year(); let week1_start = first_anchored_week_start(year, start); if date < week1_start { let prev_week1 = first_anchored_week_start(year - 1, start); let days = (date - prev_week1).num_days(); + if days < 0 { + return 1; + } (days / 7) as u32 + 1 } else { let next_week1 = first_anchored_week_start(year + 1, start); if date >= next_week1 { 1 } else { let days = (date - week1_start).num_days(); (days / 7) as u32 + 1 } } } Suggestion importance[1-10]: 3 __ Why: The concern about negative duration is theoretically valid, but in practice date - prev_week1 should always be non-negative by construction since prev_week1 is the start of week 1 in the prior year and date is in the current year. The check adds defensive programming but addresses an unlikely edge case.	Low
General	Use fixed instant for timezone offset validation Using Instant.now() to check timezone bounds introduces non-deterministic behavior because the offset can vary with DST transitions. For a named zone like "America/New_York", the offset changes between EST (-05:00) and EDT (-04:00) depending on the date. This could cause the same timezone to be accepted or rejected based on when the query runs. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DateTimeAdapters.java [307-315] private static boolean isWithinMysqlDatetimeTzBounds(String canonical) { try { java.time.ZoneId zone = java.time.ZoneId.of(canonical); - int totalSeconds = zone.getRules().getOffset(java.time.Instant.now()).getTotalSeconds(); + // Use a fixed instant to get a stable offset check (e.g., epoch or a known date) + int totalSeconds = zone.getRules().getOffset(java.time.Instant.EPOCH).getTotalSeconds(); return totalSeconds >= -50340 && totalSeconds <= 50400; } catch (java.time.DateTimeException e) { return false; } } Suggestion importance[1-10]: 7 __ Why: Using Instant.now() for timezone offset validation introduces non-deterministic behavior due to DST transitions. The suggested fix to use Instant.EPOCH provides stable, deterministic validation. This is a correctness issue that could cause inconsistent query behavior depending on when the query runs.	Medium
	Use genuinely invalid timezone offset The test validates that out-of-range timezone offsets return typed NULL, but the offset "+15:00" is actually within the valid range (±18:00 per IANA). Use an offset like "+19:00" to ensure the test properly exercises the out-of-range path. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/ConvertTzAdapter.java [190-197] /** out-of-range tz literal -> typed NULL (MySQL-compatible lenient behavior). */ public void testAdaptOutOfRangeLiteralReturnsTypedNull() { - RexCall original = buildConvertTz("UTC", "+15:00"); + RexCall original = buildConvertTz("UTC", "+19:00"); RexNode adapted = new ConvertTzAdapter().adapt(original, List.of(), cluster); assertTrue("out-of-range tz literal must yield a NULL literal", adapted instanceof RexLiteral); assertTrue("NULL literal must report null value", ((RexLiteral) adapted).isNull()); assertEquals("typed NULL must keep the original return type", original.getType(), adapted.getType()); } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that +15:00 is within the valid timezone offset range (±18:00), making the test potentially ineffective. Using +19:00 would properly test the out-of-range path.	Medium
	Optimize regex matching for non-temporal strings The spaceSeparator method is called on every VARCHAR cell, including non-temporal strings. The regex matching overhead could impact performance for large result sets with many string columns. Consider checking if the string looks like a timestamp before applying the regex (e.g., length check or quick character scan). sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/exec/ArrowValues.java [74-79] private static String spaceSeparator(String s) { - if (s == null) return null; + if (s == null || s.length() < 19) return s; + // Quick check: must have 'T' at position 10 for ISO timestamp + if (s.length() >= 11 && s.charAt(10) != 'T') return s; var m = ISO_TIMESTAMP_T.matcher(s); return m.matches() ? m.group(1) + " " + m.group(2) : s; } Suggestion importance[1-10]: 5 __ Why: The suggestion adds a quick pre-check before regex matching to improve performance for non-temporal strings. While the optimization is valid and could help with large result sets, the impact is moderate since regex matching on short strings is relatively fast. The length and character checks are simple and low-risk.	Low
	Verify return type inference for 3-arg date_bin The LOCAL_DATE_BIN_OP uses ReturnTypes.ARG1_NULLABLE which assumes the first argument is the timestamp. However, for calendar units (M/q/y), the call signature is date_bin(stride, field, origin) where the timestamp is the second argument. This mismatch could cause type inference issues when the origin argument is present. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/SpanAdapter.java [76-83] static final SqlOperator LOCAL_DATE_BIN_OP = new SqlFunction( "date_bin", SqlKind.OTHER_FUNCTION, ReturnTypes.ARG1_NULLABLE, null, OperandTypes.VARIADIC, SqlFunctionCategory.TIMEDATE ); +// Note: For 3-arg form date_bin(stride, ts, origin), return type should match ARG1 (ts). +// Current implementation works because origin is a string literal, but consider using +// a custom return type inference that checks operand count. + Suggestion importance[1-10]: 4 __ Why: The suggestion raises a valid concern about ReturnTypes.ARG1_NULLABLE potentially mismatching the 3-arg date_bin(stride, ts, origin) signature. However, the comment acknowledges the current implementation works, and the suggestion is more of a verification request than a concrete fix. The impact is low since the code appears functional.	Low

Suggestions up to commit 5bf9be5

Category	Suggestion	Impact
Possible issue	Fix timestamp-time combination arithmetic The TIMESTAMP(date, time) combination logic adds unix timestamps directly, which produces incorrect results. A TIME value represents seconds since midnight (0-86399), not an epoch offset. Adding to_unixtime(date) and to_unixtime(time) will double-count the epoch base. Instead, extract the time-of-day seconds from the TIME operand and add them to the date's midnight timestamp. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/TimestampFunctionAdapter.java [110-127] private static RexNode tryCombineTimestampPlusTime(RexCall original, RelOptCluster cluster) { if (original.getOperands().size() != 2) { return null; } RexNode op0 = stripOperatorAnnotation(original.getOperands().get(0)); RexNode op1 = stripOperatorAnnotation(original.getOperands().get(1)); if (!isTemporal(op0) || !isTemporal(op1)) { return null; } RexBuilder rexBuilder = cluster.getRexBuilder(); RexNode unix0 = rexBuilder.makeCall(UnixTimestampAdapter.LOCAL_TO_UNIXTIME_OP, op0); - RexNode unix1 = rexBuilder.makeCall(UnixTimestampAdapter.LOCAL_TO_UNIXTIME_OP, op1); - RexNode sum = rexBuilder.makeCall(org.apache.calcite.sql.fun.SqlStdOperatorTable.PLUS, unix0, unix1); + // For TIME operand, extract seconds-since-midnight instead of epoch offset + RexNode timeSeconds = extractTimeOfDaySeconds(op1, rexBuilder); + RexNode sum = rexBuilder.makeCall(org.apache.calcite.sql.fun.SqlStdOperatorTable.PLUS, unix0, timeSeconds); ... Suggestion importance[1-10]: 9 __ Why: The suggestion identifies a critical arithmetic error in combining date and time values. Adding to_unixtime(date) and to_unixtime(time) directly will produce incorrect results because TIME values represent seconds since midnight, not epoch offsets. This is a major correctness issue that would cause wrong timestamp calculations.	High
	Validate datetime literals before casting The standalone TIMESTAMPDIFF rewrite does not validate string literal operands before casting to TIMESTAMP. Invalid datetime literals (e.g., '2025-13-02') should be rejected at plan time with a format-hint error, matching the behavior of Cluster C validation elsewhere in the codebase. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/TimestampDiffAdapter.java [140-167] private static RexNode rewriteStandaloneDiff(RelOptCluster cluster, RexCall original, String outUnit, RexNode start, RexNode end) { RexBuilder rb = cluster.getRexBuilder(); + DatetimeLiteralValidator.validate(start, DatetimeLiteralValidator.Kind.TIMESTAMP); + DatetimeLiteralValidator.validate(end, DatetimeLiteralValidator.Kind.TIMESTAMP); RexNode t1 = liftToTimestamp(rb, start, original.getType()); RexNode t2 = liftToTimestamp(rb, end, original.getType()); RexNode endSeconds = rb.makeCall(UnixTimestampAdapter.LOCAL_TO_UNIXTIME_OP, t2); RexNode startSeconds = rb.makeCall(UnixTimestampAdapter.LOCAL_TO_UNIXTIME_OP, t1); RexNode diffSeconds = rb.makeCall(SqlStdOperatorTable.MINUS, endSeconds, startSeconds); ... Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that rewriteStandaloneDiff does not validate string literal operands before casting to TIMESTAMP. Adding validation would provide better error messages at plan time, consistent with Cluster C behavior elsewhere. However, the validation may not be strictly necessary if downstream processing handles invalid literals appropriately.	Medium
	Prevent index out of bounds exception The validation logic assumes TIMESTAMPADD has at least 3 operands and TIMESTAMPDIFF has at least 2, but doesn't verify this before accessing indices. If a malformed call has fewer operands, call.getOperands().get(i) will throw IndexOutOfBoundsException. Add a bounds check before the loop to prevent crashes on invalid input. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/CastTemporalLiteralValidator.java [57-69] private static void validateTimestampAddDiffOperands(RexCall call) { String name = call.getOperator().getName(); if (!"TIMESTAMPADD".equalsIgnoreCase(name) && !"TIMESTAMPDIFF".equalsIgnoreCase(name)) { return; } int firstArg = "TIMESTAMPADD".equalsIgnoreCase(name) ? 2 : 1; + if (call.getOperands().size() <= firstArg) { + return; + } for (int i = firstArg; i < call.getOperands().size(); i++) { RexNode operand = call.getOperands().get(i); if (operand instanceof RexLiteral lit && SqlTypeName.CHAR_TYPES.contains(lit.getType().getSqlTypeName())) { DatetimeLiteralValidator.validate(operand, DatetimeLiteralValidator.Kind.TIMESTAMP); } } } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential IndexOutOfBoundsException when accessing operands without verifying the operand count first. While this is a valid defensive programming improvement, the impact is moderate since malformed calls should ideally be caught earlier in the validation pipeline.	Medium
General	Prevent infinite recursion in unwrapping The CAST unwrapping loop does not guard against infinite recursion if a malformed expression tree contains a cycle. Add a depth limit or visited-node tracking to prevent potential stack overflow. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/ConvertTzAdapter.java [64-86] private static RexNode foldInvalidTimestampLiteralToNull( RexNode operand, org.apache.calcite.rel.type.RelDataType resultType, RexBuilder rexBuilder ) { RexNode unwrapped = operand; + int depth = 0; while (unwrapped instanceof RexCall call && (call.getKind() == org.apache.calcite.sql.SqlKind.CAST || call.getKind() == org.apache.calcite.sql.SqlKind.SAFE_CAST) - && call.getOperands().size() == 1) { + && call.getOperands().size() == 1 + && depth < 100) { unwrapped = call.getOperands().get(0); + depth++; } ... Suggestion importance[1-10]: 5 __ Why: The suggestion adds a depth limit to prevent infinite recursion when unwrapping CAST nodes. While this is a defensive programming practice, Calcite's expression trees are typically well-formed and cycles are unlikely. The added complexity may not be justified without evidence of actual risk, but the safeguard is reasonable.	Low
	Document epoch conversion constant The constant 719_163 converts Arrow's Date32 epoch (1970-01-01) to from_num_days_from_ce epoch (0001-01-01), but this magic number lacks documentation. If the epoch offset calculation is incorrect, all week numbers will be wrong. Add a comment explaining the epoch conversion formula to prevent future maintenance errors. sandbox/plugins/analytics-backend-datafusion/rust/src/udf/os_week.rs [117-122] +// Arrow Date32 counts days from 1970-01-01; from_num_days_from_ce counts from 0001-01-01. +// Offset = days from 0001-01-01 to 1970-01-01 = 719163 days. let date = NaiveDate::from_num_days_from_ce_opt(days + 719_163) .ok_or_else(|| datafusion::error::DataFusionError::Execution( format!("os_week: invalid Date32 day count {days}"), ))?; Suggestion importance[1-10]: 5 __ Why: The suggestion recommends adding documentation for the magic number 719_163. While this improves code maintainability and readability, it's a minor enhancement that doesn't affect functionality. The constant appears to be correct based on the context.	Low
	Propagate timezone validation errors immediately The two-arg DATETIME literal fold silently returns NULL when timezone canonicalization fails. This masks invalid timezone inputs at plan time. Consider throwing the exception to surface the error immediately, matching the behavior of other literal validation paths. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DateTimeAdapters.java [263-268] private static RexNode foldTwoArgLiteral(String value, String tz, RexCall original, RelOptCluster cluster) { ... - try { - canonicalFrom = ConvertTzAdapter.canonicalizeTz(sourceTz); - canonicalTo = ConvertTzAdapter.canonicalizeTz(tz); - } catch (IllegalArgumentException invalidTz) { - return rexBuilder.makeNullLiteral(original.getType()); - } + String canonicalFrom = ConvertTzAdapter.canonicalizeTz(sourceTz); + String canonicalTo = ConvertTzAdapter.canonicalizeTz(tz); ... Suggestion importance[1-10]: 4 __ Why: The suggestion proposes throwing exceptions instead of returning NULL for invalid timezones. However, the current NULL-folding behavior matches MySQL's DATETIME semantics (as documented in the PR). Changing this would alter the function's contract and may not be desirable. The suggestion misunderstands the intended behavior.	Low
	Use space separator in timestamp literal The calendar unit date_bin rewrite uses a hardcoded ISO-T format for the origin literal. This is inconsistent with the space-separator contract enforced elsewhere in the codebase (Cluster D). Use space separator for consistency. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/SpanAdapter.java [128-137] String calendarStrideUnit = CALENDAR_UNIT_TO_DATE_BIN_STRIDE.get(unitText); if (calendarStrideUnit != null) { Long n = extractPositiveInteger(interval); if (n != null) { RexNode stride = rexBuilder.makeLiteral(n + " " + calendarStrideUnit); - RexNode origin = rexBuilder.makeLiteral("1970-01-01T00:00:00Z"); + RexNode origin = rexBuilder.makeLiteral("1970-01-01 00:00:00"); return rexBuilder.makeCall(original.getType(), LOCAL_DATE_BIN_OP, List.of(stride, field, origin)); } } Suggestion importance[1-10]: 3 __ Why: The suggestion points out an inconsistency in timestamp literal format (ISO-T vs space separator). While consistency is valuable, the date_bin origin literal is an internal implementation detail that may not affect user-visible output. The impact is minor and the change is cosmetic.	Low

Suggestions up to commit 196a140

Category	Suggestion	Impact
Possible issue	Add null check for outUnit The rewriteStandaloneDiff method lacks null safety checks for the outUnit parameter before performing toUpperCase() operations. If outUnit is null, this will throw a NullPointerException at runtime. Add a null check at the method entry to handle this edge case gracefully. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/TimestampDiffAdapter.java [141-167] private static RexNode rewriteStandaloneDiff(RelOptCluster cluster, RexCall original, String outUnit, RexNode start, RexNode end) { + if (outUnit == null) { + return original; + } RexBuilder rb = cluster.getRexBuilder(); RexNode t1 = liftToTimestamp(rb, start, original.getType()); RexNode t2 = liftToTimestamp(rb, end, original.getType()); RexNode endSeconds = rb.makeCall(UnixTimestampAdapter.LOCAL_TO_UNIXTIME_OP, t2); RexNode startSeconds = rb.makeCall(UnixTimestampAdapter.LOCAL_TO_UNIXTIME_OP, t1); RexNode diffSeconds = rb.makeCall(SqlStdOperatorTable.MINUS, endSeconds, startSeconds); ... Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential NullPointerException in rewriteStandaloneDiff when outUnit is null. The method performs outUnit.toUpperCase(Locale.ROOT) at line 148 without null-checking. Adding a null guard at the method entry is a valid defensive measure that prevents runtime crashes.	Medium
	Validate value type before casting The formatTimestamp method assumes value is always a Number when it's not a LocalDateTime or LocalDate, but doesn't verify this before casting. If value is an unexpected type, a ClassCastException will occur. Add a type check or handle the cast failure gracefully to prevent runtime crashes. sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/exec/ArrowValues.java [132-149] private static String formatTimestamp(ArrowType.Timestamp type, Object value) { LocalDateTime ldt; if (value instanceof LocalDateTime t) { ldt = t; } else if (value instanceof LocalDate ld) { ldt = ld.atStartOfDay(); - } else { - long raw = ((Number) value).longValue(); + } else if (value instanceof Number num) { + long raw = num.longValue(); Instant instant = switch (type.getUnit()) { case SECOND -> Instant.ofEpochSecond(raw); case MILLISECOND -> Instant.ofEpochMilli(raw); case MICROSECOND -> Instant.ofEpochSecond(Math.floorDiv(raw, 1_000_000L), Math.floorMod(raw, 1_000_000L) * 1_000L); case NANOSECOND -> Instant.ofEpochSecond(Math.floorDiv(raw, 1_000_000_000L), Math.floorMod(raw, 1_000_000_000L)); }; ldt = LocalDateTime.ofInstant(instant, ZoneOffset.UTC); + } else { + throw new IllegalArgumentException("Unexpected timestamp value type: " + value.getClass()); } return ldt.getNano() == 0 ? ldt.format(TIMESTAMP_NO_NANO) : ldt.format(TIMESTAMP_WITH_NANO); } Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that formatTimestamp assumes value is a Number when it's not LocalDateTime or LocalDate, without verifying the type before casting. Adding an explicit instanceof Number check prevents a potential ClassCastException and improves error handling. The score is moderate because the method is likely called with well-typed Arrow data, but the defensive check is still valuable.	Low
General	Handle invalid date construction safely The function uses unwrap() on from_ymd_opt and and_hms_opt which can panic if dt.year() - 1 produces an invalid year (e.g., year 0 or below minimum). Handle potential None values gracefully to prevent runtime panics on edge-case dates. sandbox/plugins/analytics-backend-datafusion/rust/src/udf/os_strftime.rs [244-255] fn week_number_monday_first_01(dt: DateTime<Utc>) -> u32 { let wn = week_number_monday_first(dt); if wn == 0 { - let last = chrono::NaiveDate::from_ymd_opt(dt.year() - 1, 12, 31).unwrap(); - let last_dt = Utc.from_utc_datetime(&last.and_hms_opt(0, 0, 0).unwrap()); - week_number_monday_first(last_dt) + let last = chrono::NaiveDate::from_ymd_opt(dt.year() - 1, 12, 31) + .and_then(|d| d.and_hms_opt(0, 0, 0)) + .map(|ndt| Utc.from_utc_datetime(&ndt)); + match last { + Some(last_dt) => week_number_monday_first(last_dt), + None => 0, // Fallback for invalid prior year + } } else { wn } } Suggestion importance[1-10]: 7 __ Why: Valid concern about potential panics from unwrap() calls when dt.year() - 1 could produce invalid dates. The improved code properly handles the None case, though the likelihood of hitting year boundaries that cause this is low in practice.	Medium
	Verify matcher synchronization The rejectOutOfRangeMonthOrDay method validates month/day ranges but doesn't verify that the format and value matchers stay synchronized. If the format has more tokens than the value has numbers (or vice versa), the loop may silently skip validation. Add a check to ensure both matchers have the same number of matches before proceeding. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/RustUdfDateTimeAdapters.java [197-227] private static void rejectOutOfRangeMonthOrDay(RexCall original) { ... + int fmtCount = 0; + int valCount = 0; + while (fmtMatcher.find()) fmtCount++; + while (valMatcher.find()) valCount++; + if (fmtCount != valCount) { + return; + } + fmtMatcher.reset(); + valMatcher.reset(); while (fmtMatcher.find() && valMatcher.find()) { String token = fmtMatcher.group(); int n; try { n = Integer.parseInt(valMatcher.group()); } catch (NumberFormatException ignored) { continue; } if (("%m".equals(token) || "%c".equals(token)) && (n < 1 || n > 12)) { throw outOfRange("month", n, 1, 12, value, format); } if (("%d".equals(token) || "%e".equals(token)) && (n < 1 || n > 31)) { throw outOfRange("day", n, 1, 31, value, format); } } } Suggestion importance[1-10]: 5 __ Why: The suggestion identifies a potential issue where the format and value matchers may become desynchronized if they have different numbers of matches. The proposed fix (counting matches upfront and resetting) adds robustness. However, the current implementation's while (fmtMatcher.find() && valMatcher.find()) already stops when either matcher runs out, so the risk is limited to silent under-validation rather than a crash. The improvement is moderate.	Low
	Handle ZoneRulesException explicitly The foldTwoArgLiteral method catches DateTimeException but doesn't handle ZoneRulesException, which is a subclass that can be thrown by ZoneId.of() when zone rules are unavailable. While DateTimeException is the parent, explicitly handling zone-related failures improves code clarity and ensures all edge cases are covered. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DateTimeAdapters.java [273-284] private static RexNode foldTwoArgLiteral(String value, String tz, RexCall original, RelOptCluster cluster) { ... java.time.LocalDateTime ldt; try { ldt = java.time.LocalDateTime.parse(stripped.replace(' ', 'T')); } catch (java.time.format.DateTimeParseException e) { return rexBuilder.makeNullLiteral(original.getType()); } ... try { java.time.ZoneId fromZone = java.time.ZoneId.of(canonicalFrom); java.time.ZoneId toZone = java.time.ZoneId.of(canonicalTo); ... - } catch (java.time.DateTimeException invalid) { + } catch (java.time.DateTimeException | java.time.zone.ZoneRulesException invalid) { return rexBuilder.makeNullLiteral(original.getType()); } ... Suggestion importance[1-10]: 3 __ Why: The suggestion proposes catching ZoneRulesException explicitly alongside DateTimeException. While ZoneRulesException is a subclass of DateTimeException and is already caught, making it explicit improves code clarity. However, the functional impact is minimal since the parent exception already covers this case, so the score is moderate.	Low

Suggestions up to commit 243f8fa

Category	Suggestion	Impact
General	Prevent matcher desynchronization in parallel iteration **The parallel iteration of fmtMatcher and valMatcher assumes format tokens and value numbers align one-to-one. If the format string contains consecutive tokens (e.g., %m%...

[github-actions]

Persistent review updated to latest commit 510c16d

[github-actions]

Persistent review updated to latest commit b6c3cfb

[github-actions]

✅ Gradle check result for b6c3cfb: SUCCESS

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.38%. Comparing base (dd3a231) to head (196a140).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #22045      +/-   ##
============================================
+ Coverage     73.36%   73.38%   +0.02%     
- Complexity    75504    75556      +52     
============================================
  Files          6038     6038              
  Lines        342954   342954              
  Branches      49340    49340              
============================================
+ Hits         251592   251668      +76     
+ Misses        71303    71280      -23     
+ Partials      20059    20006      -53     

☔ 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 243f8fa

[github-actions]

Persistent review updated to latest commit 196a140

[github-actions]

✅ Gradle check result for 196a140: SUCCESS

[github-actions]

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

[expani]

Thanks for taking this up @vinaykpud

Most of my comments are around making the code short which could be follow-ups.

Just have a couple of things that seems odd specially the QTF test fix

[github-actions]

Persistent review updated to latest commit 28279be

[github-actions]

❌ Gradle check result for 28279be: 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]

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

[vinaykpud]

Thanks @expani , replied to the comments

[github-actions]

❌ Gradle check result for 643892c: 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]

❌ Gradle check result for 38c286a: 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]

❌ Gradle check result for 55994fb: 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]

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