Skip to content

PPL date/time fixes 15 new scalars, TIME comparison, sub-ms precision, format tokens#22086

Merged
mch2 merged 12 commits into
opensearch-project:mainfrom
vinaykpud:fix/ai/datetime-sweep-2
Jun 10, 2026
Merged

PPL date/time fixes 15 new scalars, TIME comparison, sub-ms precision, format tokens#22086
mch2 merged 12 commits into
opensearch-project:mainfrom
vinaykpud:fix/ai/datetime-sweep-2

Conversation

@vinaykpud

@vinaykpud vinaykpud commented Jun 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

Wave 2 of PPL date/time fixes on the analytics-engine route. Adds 15 missing datetime scalars, lowers query shapes that previously hit Substrait binding gaps, and tightens MySQL-semantic correctness on existing functions. Unmutes 16 of the 18 @AwaitsFix ITs gated in #22045.

What's in each commit

Add 15 PPL datetime scalars over DataFusion

  • New scalars: ADDDATE, SUBDATE, DATEDIFF, TO_DAYS / TO_SECONDS / FROM_DAYS / TIME_TO_SEC, SEC_TO_TIME, WEEKDAY, PERIOD_ADD / PERIOD_DIFF, GET_FORMAT, ADDTIME / SUBTIME / TIMEDIFF, LAST_DAY, YEARWEEK.
  • Per-function adapters lower each call to to_unixtime / from_unixtime + integer arithmetic, date_trunc, maketime, or a Rust UDF (new os_yearweek).
  • Unmutes 15 ExtensiveCoveragePplIT golden queries (q112/119/125/126/128/137/138/139/144/147/148/151/152/156/158).

Fix TIME-vs-{DATE,TIMESTAMP} comparison via operand coercion

  • Calcite's binary-comparison type coercion wraps a TIME operand with to_timestamp(precision_time<P>), which has no yaml binding (substrait-java 0.89.1 lacks ToTypeString for PrecisionTime).
  • Extend ComparisonTemporalCoercionAdapter to anchor the TIME side to a today-UTC TIMESTAMP.
  • Queries like time('00:00:00') = date('2004-07-09') now resolve.

Register PPL ADDDATE/SUBDATE with integer-days overload

  • The integer form (adddate(date, 7)) was bailed by DateAddSubAdapter.
  • Adapter now rebuilds an integer second operand as INTERVAL N DAY before the standard interval-lowering path runs.

Preserve sub-millisecond precision in timestamp literal folds

  • timestamp('2020-09-16 17:30:00.123456') was folded at parquet's resolved precision (3) and silently truncated to .123.
  • TimestampFunctionAdapter now bumps fold precision to 6 when the input carries non-zero sub-ms digits.
  • microsecond(...) and date_format(_, '%f') round-trip the full 6 digits.

Reject invalid string literals in DAYNAME/MONTHNAME at plan time

  • DAYNAME('2025-13-02') previously crashed the streaming fragment with a generic StreamException.
  • Adapters now validate at plan time and return HTTP 400 with the typed unsupported format hint, matching the DATE / TIME / TIMESTAMP siblings.

Pin convert_tz precision-9 path with regression IT

  • Locks in the precision-9 SAFE-cast contract (already shipped in earlier dd3a231eb2e) against future regression.
  • Mirrors CalcitePPLBuiltinFunctionsNullIT's null-string DATETIME shape.

Unmute 14 datetime ITs

Five narrow follow-up fixes that flip previously-pending regression gates green:

  • ArrowValues TIMESTAMP/TIME render: variable-fraction (1..9 digits, trailing zeros stripped) instead of fixed-9 — .999999000 → .999999.
  • DatetimeAdapter (1-arg): TIME operand anchored to TIMESTAMP before to_timestamp (yaml has no precision_time<P> arm).
  • DatetimeAdapter (2-arg, column path): synthesised tz literal as VARCHAR (was CHAR(N)) so isthmus binds the convert_tz(_, string, string) arm.
  • DatetimeAdapter (2-arg, fold + column): MySQL semantics — when the value has no embedded TZ offset, the user's tz arg is the source zone (target=UTC), not the target. Inverted the offset direction pre-fix.
  • os_strftime Rust UDF: %c emits unpadded month; %U / %u / %V / %v emit zero-padded width-2 week numbers (matches MySQL docs and PPL DateTimeFormatterUtil).

Drop as_any from OsYearweekUdf for DataFusion 54 compat

Also includes a behavior-only rename of DatetimeCoverageIT test methods (dropped internal cluster-letter labels in favor of behavior-named methods).

Query shapes added / fixed

New scalars (each lowers to a DataFusion-native plan)

Function Example query Notes
ADDDATE, SUBDATE adddate(date0, 7), adddate(date0, interval 7 day), subdate(date0, interval 1 month), adddate(datetime0, interval 5 hour), subdate(datetime0, interval 30 minute), adddate('2020-08-26', interval 7 day) Integer-days, INTERVAL form, DATE / TIMESTAMP / VARCHAR base
DATEDIFF datediff('2000-01-02 00:00:00', '2000-01-01 23:59:59') (= 1, time-of-day discarded), datediff(time('23:59:59'), time('00:00:00')) (= 0, both anchored to today)
TO_DAYS to_days('1970-01-01') (= 719528, MySQL day-1 origin)
TO_SECONDS to_seconds('1970-01-01 00:00:00') (= 62167219200)
FROM_DAYS from_days(719528) (= 1970-01-01)
TIME_TO_SEC time_to_sec(datetime0), time_to_sec(time('19:36:22'))
SEC_TO_TIME sec_to_time(123456) (= 10:17:36, MOD-86400 wrap)
WEEKDAY weekday(datetime0) (Mon=0..Sun=6)
PERIOD_ADD period_add(202612, 1) (= 202701, year-boundary roll)
PERIOD_DIFF period_diff(202612, 202601) (= 11)
GET_FORMAT get_format(DATE, 'EUR') (= '%d.%m.%Y') All 5 region variants
ADDTIME, SUBTIME addtime(datetime0, time('14:00:00')) (cross-midnight), subtime(datetime0, time('11:00:00')), addtime(datetime0, time1), addtime(time1, time('02:30:00')) TIMESTAMP+TIME and TIME+TIME
TIMEDIFF timediff(time('23:59:59'), time('13:00:00')) (= 10:59:59)
LAST_DAY last_day('2024-02-15') (= 2024-02-29, leap-year), last_day(date0), last_day(datetime0)
YEARWEEK yearweek('2003-10-03') (= 200339), yearweek('2003-10-03', 3) (= 200340), yearweek(datetime0) All 8 modes

Function-overload gaps closed

Shape Now resolves
datetime('2008-01-01 02:00:00', '+10:00') 2-arg DATETIME literal fold (= '2007-12-31 16:00:00', MySQL source-tz semantics)
datetime('2008-01-01 02:00:00', 'America/Los_Angeles') 2-arg DATETIME with named-zone string
datetime(datetime0, '+00:00') 2-arg DATETIME with column input
datetime('2008-01-01 02:00:00', '+15:00') Out-of-range tz folds to NULL (was HTTP 400)
DATETIME(null_str, '+10:00') Null operands fold to typed-null TIMESTAMP
sysdate(0) / now(0) FSP-arg overload
date_add(time('09:00:00'), interval 1 hour), date_sub(time('09:00:00'), interval 30 minute) DATE_ADD/SUB on TIME operand
timestampadd(YEAR, 1, '2024-01-15 12:00:00'), timestampdiff(DAY, '...', '...'), timestampdiff(MONTH, '...', '...') Standalone string-overload
hour(time('17:30:45')), minute(time('17:30:45')), second(time('17:30:45')) DATE_PART(unit, TIME)
from_unixtime(1521467703, '%Y-%m-%d %H:%i:%s') 2-arg FROM_UNIXTIME
timestamp(date0, time1) 2-arg TIMESTAMP composition
timestamp(time('10:20:30')) 1-arg TIMESTAMP(TIME) — folds with today-UTC

Comparisons across mixed temporal types

Shape Result
time('00:00:00') = date('2004-07-09') TIME side anchored to today-UTC; resolves to BOOLEAN
date('2020-09-16') < time('09:07:00') Symmetric — DATE < anchored-TIME
time('10:20:30') = timestamp('<today> 10:20:30') TRUE on same time-of-day
timestamp('1984-12-15 10:20:30') != time('10:20:30') TIMESTAMP != TIME (anchored)

Span types

Shape Now
span(date0, 1d), span(date1, 1month) Bare date string, date schema type (was 2004-04-15 00:00:00 / timestamp)
span(time1, 1h), span(time1, 1minute) Time-of-day, time schema type (was 1970-01-01 19:00:00)

Format-token / rendering

Shape Now
date_format(date('2024-01-28'), '%c %U %u %V %v') 1 04 04 04 04 (was 01 4 4 4 4)
date_format(timestamp('1998-01-31 13:14:15'), '%a %b %c %D %d %H %i %M %m %S') Sat Jan 1 31st 31 13 14 January 01 15 (was Sat Jan 01 …)
date_format('2024-01-15 12:00:00.123456', '%f') 123456 (was 123000)
microsecond('2024-01-15 12:00:00.123456') 123456 (was 123000)
microsecond('2024-01-15 12:00:00.000001') 1 (was 0)
cast('2023-10-01 12:00:00.123456' as TIMESTAMP) rendered .123456 preserved (was truncated to .123)
cast('2262-04-11 23:47:16.854' as TIMESTAMP) 2262-04-11 23:47:16.854 (was …854000)
cast('23:59:59.999999' as TIME) 23:59:59.999999 (was …999999000)
date_add(datetime0, interval 500 millisecond) 2004-07-09 10:17:35.5 (was …500000000)
cast('1985-10-09 12:00:00' as TIME) 12:00:00
maketime(20.2, 49.5, 42.1) rendered via time_format(_, '%H:%i:%s') 20:50:42

Error-contract / validation

Shape HTTP 400 with unsupported format
dayname('2025-13-02'), monthname('2025-13-02') New plan-time validation (was StreamException 500)
date('2025-13-02'), time('16:00:61'), timestamp('2025-12-01 15:02:61') Existing
cast('xxx' as DATE), cast('xxx' as TIME), cast('xxx' as TIMESTAMP) Existing
timestampadd(YEAR, 1, '2025-13-02'), timestampdiff(DAY, '2025-13-02', ...) Existing
hour('99:99:99') Existing
datetime('2025-13-02') Folds to NULL (1-arg form)
convert_tz(_, '+15:00', '+00:00') Out-of-range tz folds to NULL

Boundary / range

Shape Now
cast('0001-01-01' as DATE), cast('9999-12-31' as DATE) ANSI-SQL min/max DATE round-trip
cast('1677-09-21 00:12:44' as TIMESTAMP) Min TIMESTAMP (i64-ns floor)
cast('2200-01-01 00:00:00' as TIMESTAMP) Below year-2262 ceiling
cast('00:00:00' as TIME) Midnight TIME
timestamp('3077-04-12 09:07:00') Plan-time reject — outside the supported range (no opaque Arrow overflow)

Other

Shape Now
`eval is_after = earliest('1900-01-01 00:00:00', datetime0) where is_after

Still skipped (2 of 18)

  • DatetimeCoverageIT.testSpanCustomFormatDate — span() over an eval-derived date(...) returns the bucket as "2004-04-01 00:00:00" instead of the bare "2004-04-01". The renderer in AnalyticsExecutionEngine#convert only strips the midnight suffix when the type is DateOnlyType (column-mapping UDT), but the eval-derived path produces ExprDateType. Fix is one line in the SQL plugin (broaden the predicate to OpenSearchTypeFactory.isDateExprType(type)); out of scope here. TODO captured on the test.
  • TwoShardCommandIT.testReduceCorrectnessAcrossTwoShards — timechart bucket renders as bare DATE instead of TIMESTAMP (3 of 18 reduce sub-checks fail). Same renderer/typing gap, surfaces in the multi-shard reduce harness.

Test plan

  • :sandbox:plugins:analytics-backend-datafusion:check — green (Java + Rust unit tests).
  • :sandbox:libs:analytics-framework:check — green.
  • :sandbox:plugins:analytics-engine:check — green.
  • :sandbox:qa:analytics-engine-rest:integTest — green for the 16 unmuted ITs against a 2-node test cluster; the 2 remaining @AwaitsFix tests stay skipped with rationale.
  • Spotless clean on every touched module.

@github-actions

github-actions Bot commented Jun 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

(Review updated until commit be091ed)

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
📝 TODO sections

🔀 No multiple PR themes
⚡ Recommended focus areas for review

Possible Issue

The 2-arg DATETIME adapter inverts the timezone semantics when the value has no embedded offset. The code treats the user's tz arg as the SOURCE zone and converts to UTC, but the comment states this is "MySQL semantics". However, MySQL's CONVERT_TZ(dt, from_tz, to_tz) interprets the tz arg as the target zone when dt has no offset, not the source. The adapter may produce incorrect timestamps for queries like DATETIME('2008-01-01 02:00:00', '+10:00') when the value lacks an embedded offset.

 
// Column input with no embedded TZ in the value: tz arg is the SOURCE zone (MySQL
// semantics — value is interpreted AT the given tz, then expressed in UTC). The legacy
// SQL plugin path treated tz as target, which inverted the offset direction.
RexNode strippedValue;
if (SqlTypeName.CHAR_TYPES.contains(value.getType().getSqlTypeName())) {
    RexNode pattern = rexBuilder.makeLiteral(OFFSET_SUFFIX_PATTERN);
    RexNode replacement = rexBuilder.makeLiteral("");
    strippedValue = rexBuilder.makeCall(
        value.getType(),
        SqlLibraryOperators.REGEXP_REPLACE_3,
        List.of(value, pattern, replacement)
    );
} else {
    strippedValue = value;
}
RexNode asTimestamp = rexBuilder.makeCall(original.getType(), LOCAL_TO_TIMESTAMP_OP, List.of(strippedValue));
// convert_tz yaml binds tz operands as unbounded `string`; cast both away from CHAR(N).
RelDataType varchar = rexBuilder.getTypeFactory().createSqlType(SqlTypeName.VARCHAR);
RexNode tzArgVarchar = SqlTypeName.CHAR_TYPES.contains(tzArg.getType().getSqlTypeName())
    ? rexBuilder.makeCast(varchar, tzArg, true)
    : tzArg;
RexNode toTz = rexBuilder.makeLiteral("+00:00", varchar, true);
return rexBuilder.makeCall(original.getType(), ConvertTzAdapter.LOCAL_CONVERT_TZ_OP, List.of(asTimestamp, tzArgVarchar, toTz));
Possible Issue

The TIME-vs-DATE/TIMESTAMP comparison anchors TIME to "today UTC" at plan time, which can produce incorrect results when the plan is cached across midnight or when the query execution time differs from plan time. A query like time('23:59:59') > date('2024-01-15') will flip its result if the plan is generated on 2024-01-14 but executed on 2024-01-15. The TODO comment at line 54 acknowledges this drift issue but the fix is deferred.

 
private static RexNode coerceTimeIfApplicable(RexNode self, RexNode other, RelOptCluster cluster) {
    RexNode timeInner = unwrapToTime(self);
    if (timeInner == null) {
        return self;
    }
    if (unwrapToTime(other) != null || !isDateOrTimestampOrWrapped(other)) {
        return self;
    }
    RexNode rewritten = DatePartAdapters.coerceCharacterOperandToTimestamp(timeInner, cluster);
    if (self instanceof OperatorAnnotation annotation && annotation.unwrap() != null) {
        return annotation.withAdaptedOriginal(rewritten);
    }
    return rewritten;
}
Possible Issue

The asIntervalLiteral helper returns null for non-literal integer operands, causing the adapter to pass the call through unchanged. However, the code does not validate that the second operand is actually an integer type before attempting to extract it as BigDecimal. If the operand is a non-integer numeric type or a column reference of unexpected type, getValueAs(BigDecimal.class) may return null silently, and the adapter will pass through a malformed call that should have been rejected earlier.

 
private static RexLiteral asIntervalLiteral(RexNode node, RexBuilder rexBuilder) {
    if (node instanceof RexLiteral lit) {
        if (SqlTypeName.INTERVAL_TYPES.contains(lit.getType().getSqlTypeName())) {
            return lit;
        }
        if (SqlTypeName.INT_TYPES.contains(lit.getType().getSqlTypeName())) {
            BigDecimal days = lit.getValueAs(BigDecimal.class);
            if (days == null) {
                return null;
            }
            return rexBuilder.makeIntervalLiteral(days, new SqlIntervalQualifier(TimeUnit.DAY, null, SqlParserPos.ZERO));
        }
    }
    return null;
}

@github-actions

github-actions Bot commented Jun 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Latest suggestions up to be091ed

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Avoid unnecessary coercion for temporal types

The coerceCharacterOperandToTimestamp method is called unconditionally, but if
operand is already a TIMESTAMP or DATE, this may introduce unnecessary overhead or
type conversions. Consider checking the operand type first and only coercing when
necessary.

sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DateDiffAdapter.java [70-73]

 private static RexNode dayNumber(RexNode operand, RelOptCluster cluster) {
     RexBuilder rexBuilder = cluster.getRexBuilder();
-    RexNode anchored = DatePartAdapters.coerceCharacterOperandToTimestamp(operand, cluster);
+    SqlTypeName typeName = operand.getType().getSqlTypeName();
+    RexNode anchored = (typeName == SqlTypeName.TIMESTAMP || typeName == SqlTypeName.DATE)
+        ? operand
+        : DatePartAdapters.coerceCharacterOperandToTimestamp(operand, cluster);
     RexNode epochSeconds = rexBuilder.makeCall(UnixTimestampAdapter.LOCAL_TO_UNIXTIME_OP, anchored);
-    RexNode secondsPerDay = rexBuilder.makeExactLiteral(
-        BigDecimal.valueOf(TimeOfDayLowering.SECONDS_PER_DAY),
-        rexBuilder.getTypeFactory().createSqlType(SqlTypeName.BIGINT)
-    );
     ...
Suggestion importance[1-10]: 7

__

Why: Valid optimization suggestion that avoids unnecessary coercion when operand is already a TIMESTAMP or DATE type. This could improve performance and reduce complexity in the generated plan, though the impact depends on query patterns.

Medium
Ensure consistent nullability for timezone operands

The toTz literal is created with a nullable VARCHAR type but the tzArgVarchar cast
may produce a non-nullable type depending on the input. Ensure both timezone
operands have consistent nullability to avoid potential type mismatches in the
convert_tz call.

sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DateTimeAdapters.java [246-252]

-// convert_tz yaml binds tz operands as unbounded `string`; cast both away from CHAR(N).
-RelDataType varchar = rexBuilder.getTypeFactory().createSqlType(SqlTypeName.VARCHAR);
+RelDataType varchar = rexBuilder.getTypeFactory().createTypeWithNullability(
+    rexBuilder.getTypeFactory().createSqlType(SqlTypeName.VARCHAR), true);
 RexNode tzArgVarchar = SqlTypeName.CHAR_TYPES.contains(tzArg.getType().getSqlTypeName())
     ? rexBuilder.makeCast(varchar, tzArg, true)
     : tzArg;
 RexNode toTz = rexBuilder.makeLiteral("+00:00", varchar, true);
 return rexBuilder.makeCall(original.getType(), ConvertTzAdapter.LOCAL_CONVERT_TZ_OP, List.of(asTimestamp, tzArgVarchar, toTz));
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies a potential type consistency issue where tzArgVarchar may have different nullability than toTz. However, the existing code already uses makeCast(..., true) which typically preserves nullability, and the impact is moderate since type mismatches would likely be caught at runtime.

Low
Clarify sub-millisecond precision detection logic

The modulo operation ldt.getNano() % 1_000_000 checks for sub-millisecond precision,
but getNano() returns nanoseconds (0-999,999,999). If the intent is to detect
non-zero microseconds, verify that the logic correctly handles all cases where
nanoseconds are not millisecond-aligned.

sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/TimestampFunctionAdapter.java [272-275]

 private static int foldPrecision(LocalDateTime ldt, int resolved) {
-    return ldt.getNano() % 1_000_000 == 0 ? resolved : Math.max(resolved, 6);
+    int nanos = ldt.getNano();
+    boolean hasSubMillisecond = (nanos % 1_000_000) != 0;
+    return hasSubMillisecond ? Math.max(resolved, 6) : resolved;
 }
Suggestion importance[1-10]: 5

__

Why: The suggestion improves code readability by extracting the modulo check into a named variable, making the intent clearer. However, the logic is already correct and the improvement is primarily stylistic rather than fixing a bug.

Low
Guard against ClassCastException in literal extraction

The getValueAs(String.class) call can throw ClassCastException or return null if the
literal's internal value is not a string (e.g., a malformed literal). Wrap the call
in a try-catch or add a null check to prevent runtime exceptions from propagating
when the literal is unexpectedly non-string.

sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/GetFormatAdapter.java [80-89]

 private static String stringLiteral(RexNode operand) {
     if (!(operand instanceof RexLiteral literal)) {
         return null;
     }
     SqlTypeName typeName = literal.getType().getSqlTypeName();
     if (typeName != SqlTypeName.CHAR && typeName != SqlTypeName.VARCHAR) {
         return null;
     }
-    return literal.getValueAs(String.class);
+    try {
+        return literal.getValueAs(String.class);
+    } catch (ClassCastException e) {
+        return null;
+    }
 }
Suggestion importance[1-10]: 3

__

Why: The suggestion addresses a potential ClassCastException, but the type check on lines 84-87 already guards against non-string types. The risk is low since RexLiteral.getValueAs is designed to handle type conversions safely within Calcite's type system.

Low
Possible issue
Prevent integer overflow in date conversion

The addition days + 719_163 can overflow when days is near i32::MAX, causing
from_num_days_from_ce_opt to receive a wrapped value and return None. This produces
a misleading error message. Use checked_add to detect overflow explicitly and return
a clearer error before calling the date constructor.

sandbox/plugins/analytics-backend-datafusion/rust/src/udf/os_week.rs [212-215]

-let date = NaiveDate::from_num_days_from_ce_opt(days + 719_163)
+let days_from_ce = days.checked_add(719_163)
+    .ok_or_else(|| datafusion::error::DataFusionError::Execution(
+        format!("os_yearweek: Date32 day count {days} causes overflow"),
+    ))?;
+let date = NaiveDate::from_num_days_from_ce_opt(days_from_ce)
     .ok_or_else(|| datafusion::error::DataFusionError::Execution(
         format!("os_yearweek: invalid Date32 day count {days}"),
     ))?;
Suggestion importance[1-10]: 6

__

Why: Valid concern about potential overflow when adding 719_163 to days near i32::MAX. Using checked_add would provide clearer error handling and prevent silent wraparound, improving robustness for edge cases in date arithmetic.

Low
Prevent potential integer overflow

The floor-mod normalization ((x MOD 86400) + 86400) MOD 86400 can produce incorrect
results when the epoch value exceeds Long.MAX_VALUE - 86400. The intermediate
addition mod1 + 86400 may overflow, wrapping to a negative value and yielding a
wrong seconds-of-day. Consider validating the epoch range or using Math.addExact to
detect overflow.

sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/TimeOfDayLowering.java [50-56]

 static RexNode secondsOfDay(RexNode operand, RelOptCluster cluster) {
     RexBuilder rexBuilder = cluster.getRexBuilder();
     RexNode anchored = DatePartAdapters.coerceCharacterOperandToTimestamp(operand, cluster);
     RexNode epoch = rexBuilder.makeCall(UnixTimestampAdapter.LOCAL_TO_UNIXTIME_OP, anchored);
     RexNode mod1 = rexBuilder.makeCall(SqlStdOperatorTable.MOD, epoch, bigintLit(rexBuilder, SECONDS_PER_DAY));
+    // Use CASE to guard against overflow: if mod1 is already non-negative, skip the +86400 step
     RexNode plusDay = rexBuilder.makeCall(SqlStdOperatorTable.PLUS, mod1, bigintLit(rexBuilder, SECONDS_PER_DAY));
     return rexBuilder.makeCall(SqlStdOperatorTable.MOD, plusDay, bigintLit(rexBuilder, SECONDS_PER_DAY));
 }
Suggestion importance[1-10]: 2

__

Why: While overflow is theoretically possible, Unix timestamps in practice are far from Long.MAX_VALUE (current timestamps are ~1.7e9 seconds). The suggestion's improved_code is identical to existing_code and doesn't implement the proposed fix, making it unhelpful.

Low

Previous suggestions

Suggestions up to commit 978d627
CategorySuggestion                                                                                                                                    Impact
General
Extract duplicated floor-mod normalization logic

The floor-mod normalization ((x MOD 86400) + 86400) MOD 86400 is computed twice in
this class (here and in secondsToTime). Extract this into a separate helper method
to eliminate duplication and improve maintainability.

sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/TimeOfDayLowering.java [50-57]

 static RexNode secondsOfDay(RexNode operand, RelOptCluster cluster) {
     RexBuilder rexBuilder = cluster.getRexBuilder();
     RexNode anchored = DatePartAdapters.coerceCharacterOperandToTimestamp(operand, cluster);
     RexNode epoch = rexBuilder.makeCall(UnixTimestampAdapter.LOCAL_TO_UNIXTIME_OP, anchored);
-    RexNode mod1 = rexBuilder.makeCall(SqlStdOperatorTable.MOD, epoch, bigintLit(rexBuilder, SECONDS_PER_DAY));
+    return normalizeSecondsToDay(epoch, rexBuilder);
+}
+
+private static RexNode normalizeSecondsToDay(RexNode seconds, RexBuilder rexBuilder) {
+    RexNode mod1 = rexBuilder.makeCall(SqlStdOperatorTable.MOD, seconds, bigintLit(rexBuilder, SECONDS_PER_DAY));
     RexNode plusDay = rexBuilder.makeCall(SqlStdOperatorTable.PLUS, mod1, bigintLit(rexBuilder, SECONDS_PER_DAY));
     return rexBuilder.makeCall(SqlStdOperatorTable.MOD, plusDay, bigintLit(rexBuilder, SECONDS_PER_DAY));
 }
Suggestion importance[1-10]: 6

__

Why: Valid refactoring suggestion to eliminate code duplication. The floor-mod pattern ((x MOD 86400) + 86400) MOD 86400 appears in both secondsOfDay and secondsToTime (lines 66-68). Extracting this into a helper method would improve maintainability and reduce the risk of inconsistencies.

Low
Validate operand type before to_unixtime

The toUnixtime helper does not validate that operand is a supported temporal type
before calling to_unixtime. If a non-temporal operand (e.g., INTEGER) is passed, the
resulting plan may fail at runtime with an opaque error. Add a type check or
document the precondition.

sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/EpochArithmeticAdapters.java [54-56]

 private static RexNode toUnixtime(RexBuilder rexBuilder, RexNode operand) {
+    SqlTypeName type = operand.getType().getSqlTypeName();
+    if (!SqlTypeName.DATETIME_TYPES.contains(type) && type != SqlTypeName.VARCHAR) {
+        throw new IllegalArgumentException("to_unixtime requires temporal or VARCHAR operand, got: " + type);
+    }
     return rexBuilder.makeCall(UnixTimestampAdapter.LOCAL_TO_UNIXTIME_OP, operand);
 }
Suggestion importance[1-10]: 5

__

Why: The helper is private and all call sites (ToDaysAdapter, ToSecondsAdapter) already validate or coerce operands via DatetimeLiteralValidator or coerceCharacterOperandToTimestamp. Adding a redundant check here would duplicate validation logic.

Low
Handle deeply nested TIME wrappers

The recursion in unwrapToTime only descends one level. If annotations or wrappers
are nested deeper (e.g., CAST(to_timestamp(CAST(TIME)))), the method returns null
prematurely. Consider iterative unwrapping until a TIME or non-wrapper is found to
handle arbitrary nesting.

sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/ComparisonTemporalCoercionAdapter.java [99-111]

 private static RexNode unwrapToTime(RexNode node) {
-    RexNode stripped = stripAnnotation(node);
-    if (isTime(stripped)) {
-        return stripped;
-    }
-    if (stripped instanceof RexCall call && call.getOperands().size() == 1) {
+    RexNode current = stripAnnotation(node);
+    while (current instanceof RexCall call && call.getOperands().size() == 1) {
         RexNode inner = stripAnnotation(call.getOperands().get(0));
         if (isTime(inner) && (call.getOperator() == DateTimeAdapters.LOCAL_TO_TIMESTAMP_OP || call.isA(SqlKind.CAST))) {
             return inner;
         }
+        if (isTime(current)) {
+            return current;
+        }
+        current = inner;
     }
-    return null;
+    return isTime(current) ? current : null;
 }
Suggestion importance[1-10]: 4

__

Why: The method already strips annotations at every level via stripAnnotation, so single-level descent is sufficient for the production shapes (Calcite emits at most one wrapper). Iterative unwrapping adds complexity without addressing a real case.

Low
Ensure consistent VARCHAR cast for timezone

The conditional cast to VARCHAR may produce a null-typed result when tzArg is
already VARCHAR, causing the subsequent convert_tz call to receive a mismatched
type. Ensure the cast is applied consistently or verify that tzArg is always
VARCHAR-compatible before passing to convert_tz.

sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DateTimeAdapters.java [249-251]

-RexNode tzArgVarchar = SqlTypeName.CHAR_TYPES.contains(tzArg.getType().getSqlTypeName())
-    ? rexBuilder.makeCast(varchar, tzArg, true)
-    : tzArg;
+RexNode tzArgVarchar = rexBuilder.makeCast(varchar, tzArg, true);
Suggestion importance[1-10]: 3

__

Why: The conditional cast logic is correct—when tzArg is already VARCHAR, the cast is a no-op. Removing the condition would force an unnecessary cast on VARCHAR inputs. The suggestion misunderstands the intent.

Low
Clarify rewriter scope in documentation

The class javadoc states the rewriter only handles VarChar→Str conversion, but the
class name and structure suggest it could handle multiple rewrite cases. Consider
renaming to VarCharLiteralRewriter or updating the javadoc to clarify this is a
general-purpose rewriter that currently only implements one transformation.

sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/SubstraitPlanPojoRewriter.java [24-32]

 /**
- * POJO-layer Substrait plan rewriter. Rewrites {@code VarCharLiteral} → {@code StrLiteral}
+ * POJO-layer Substrait plan rewriter for VarChar literal conversion. Rewrites {@code VarCharLiteral} → {@code StrLiteral}
  * — DataFusion 53.1.0's consumer has no VarCharLiteral arm; the two literal types are
  * byte-identical. Sibling {@link SubstraitPlanProtoRewriter} runs after this on the proto
  * layer for fixes the POJO API can't express.
  *
+ * <p>This rewriter is designed to be extensible for additional POJO-layer transformations.
  * <p>TODO: remove this class once datafusion-substrait adds a VarCharLiteral arm to its consumer.
  *
  * @opensearch.internal
  */
Suggestion importance[1-10]: 3

__

Why: The suggestion to clarify documentation is valid but minor. The current javadoc accurately describes what the class does (VarChar→Str rewriting), and the TODO already indicates this is temporary. The suggestion to mention extensibility adds marginal value since the class structure already supports it.

Low
Cast epoch seconds to BIGINT

The floor-divide logic assumes epochSeconds and secondsPerDay are BIGINT. If
epochSeconds is DOUBLE (e.g., from a widened to_unixtime result), the MOD and DIVIDE
operations may produce incorrect results or fail to bind. Ensure epochSeconds is
cast to BIGINT before the floor-mod chain.

sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DateDiffAdapter.java [86-91]

+RexNode epochSecondsBigint = rexBuilder.makeCast(
+    rexBuilder.getTypeFactory().createSqlType(SqlTypeName.BIGINT),
+    epochSeconds,
+    true
+);
+RexNode mod1 = rexBuilder.makeCall(SqlStdOperatorTable.MOD, epochSecondsBigint, secondsPerDay);
+RexNode plusDay = rexBuilder.makeCall(SqlStdOperatorTable.PLUS, mod1, secondsPerDay);
 RexNode floorMod = rexBuilder.makeCall(SqlStdOperatorTable.MOD, plusDay, secondsPerDay);
-RexNode numerator = rexBuilder.makeCall(SqlStdOperatorTable.MINUS, epochSeconds, floorMod);
+RexNode numerator = rexBuilder.makeCall(SqlStdOperatorTable.MINUS, epochSecondsBigint, floorMod);
 return rexBuilder.makeCall(SqlStdOperatorTable.DIVIDE, numerator, secondsPerDay);
Suggestion importance[1-10]: 2

__

Why: The to_unixtime UDF returns BIGINT (see UnixTimestampAdapter.LOCAL_TO_UNIXTIME_OP signature), so epochSeconds is already BIGINT-typed. The cast is unnecessary and the concern about DOUBLE is unfounded.

Low
Suggestions up to commit b329c6d
CategorySuggestion                                                                                                                                    Impact
Possible issue
Prevent infinite loop in annotation unwrapping

The stripAnnotation method may enter an infinite loop if annotation.unwrap() returns
the same node or creates a cycle. Add a cycle detection mechanism or a maximum
iteration limit to prevent potential infinite loops during annotation unwrapping.

sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/ComparisonTemporalCoercionAdapter.java [113-118]

 private static RexNode stripAnnotation(RexNode node) {
+    Set<RexNode> visited = new HashSet<>();
     while (node instanceof OperatorAnnotation annotation && annotation.unwrap() != null) {
+        if (!visited.add(node)) {
+            break;
+        }
         node = annotation.unwrap();
     }
     return node;
 }
Suggestion importance[1-10]: 7

__

Why: Valid concern about potential infinite loops in stripAnnotation. Adding cycle detection with a HashSet prevents infinite loops if annotation.unwrap() creates cycles, improving robustness.

Medium
General
Fix precision bump logic condition

The foldPrecision method may produce incorrect results when resolved is greater than
6 and the nanosecond value has non-zero sub-millisecond digits. The
Math.max(resolved, 6) will preserve the higher precision, but the logic should
verify that the precision bump to 6 is only applied when resolved is less than 6 to
avoid unintended precision changes.

sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/TimestampFunctionAdapter.java [272-275]

 private static int foldPrecision(LocalDateTime ldt, int resolved) {
-    return ldt.getNano() % 1_000_000 == 0 ? resolved : Math.max(resolved, 6);
+    return (resolved < 6 && ldt.getNano() % 1_000_000 != 0) ? 6 : resolved;
 }
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies that the current logic may not handle cases where resolved > 6 properly. The improved condition ensures precision is only bumped to 6 when resolved < 6 and sub-ms digits exist, which is more precise than the original implementation.

Low
Cache numeric constants as static fields

The constants 6 and 7 are created as new BigDecimal literals on every invocation.
Extract these as static final fields to avoid repeated object allocation, improving
performance when WEEKDAY is used in large queries or repeatedly.

sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/WeekdayAdapter.java [48-50]

+private static final BigDecimal SIX = BigDecimal.valueOf(6);
+private static final BigDecimal SEVEN = BigDecimal.valueOf(7);
+
 RexNode dow = rexBuilder.makeCall(SqlLibraryOperators.DATE_PART, partLiteral, operand);
-RexNode shifted = rexBuilder.makeCall(SqlStdOperatorTable.PLUS, dow, rexBuilder.makeExactLiteral(BigDecimal.valueOf(6)));
-RexNode weekday = rexBuilder.makeCall(SqlStdOperatorTable.MOD, shifted, rexBuilder.makeExactLiteral(BigDecimal.valueOf(7)));
+RexNode shifted = rexBuilder.makeCall(SqlStdOperatorTable.PLUS, dow, rexBuilder.makeExactLiteral(SIX));
+RexNode weekday = rexBuilder.makeCall(SqlStdOperatorTable.MOD, shifted, rexBuilder.makeExactLiteral(SEVEN));
Suggestion importance[1-10]: 4

__

Why: Caching BigDecimal constants (6 and 7) as static fields would reduce object allocation. However, the performance gain is minimal since BigDecimal.valueOf() already uses internal caching for small values, making this optimization of limited practical impact.

Low
Handle VARCHAR type explicitly in cast

The conditional cast to VARCHAR may produce a null-typed result when tzArg is
already VARCHAR, causing the subsequent convert_tz call to receive a mismatched
type. Ensure the cast always produces a non-null VARCHAR type by wrapping the result
in a null-safe type assertion or by explicitly handling the VARCHAR case separately.

sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DateTimeAdapters.java [250-252]

 RexNode tzArgVarchar = SqlTypeName.CHAR_TYPES.contains(tzArg.getType().getSqlTypeName())
     ? rexBuilder.makeCast(varchar, tzArg, true)
-    : tzArg;
+    : (tzArg.getType().getSqlTypeName() == SqlTypeName.VARCHAR ? tzArg : rexBuilder.makeCast(varchar, tzArg, true));
Suggestion importance[1-10]: 3

__

Why: The suggestion addresses a potential type mismatch, but the existing code already handles VARCHAR correctly by checking CHAR_TYPES which includes VARCHAR. The suggested change adds redundant logic without fixing an actual issue.

Low
Add logging for non-literal operands

The adapter silently returns the original call when operands aren't string literals,
which may hide configuration errors. Consider logging a warning when non-literal
operands are encountered to aid debugging, as GET_FORMAT is expected to work with
compile-time constants.

sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/GetFormatAdapter.java [62-66]

 String type = stringLiteral(original.getOperands().get(0));
 String region = stringLiteral(original.getOperands().get(1));
 if (type == null || region == null) {
+    // Log warning: GET_FORMAT expects string literal operands
     return original;
 }
Suggestion importance[1-10]: 3

__

Why: Adding a warning log could help debugging, but the current behavior (returning the original call) is intentional to let downstream planners surface errors. The improvement is marginal and the suggestion only asks to add logging without concrete implementation.

Low
Guard against division by zero

The floor-divide implementation assumes secondsPerDay is never zero, but there's no
explicit guard against division by zero. While SECONDS_PER_DAY is a constant
(86400), defensive programming suggests adding a runtime assertion or check to
prevent potential arithmetic exceptions if the constant is ever modified
incorrectly.

sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DateDiffAdapter.java [86-91]

+if (TimeOfDayLowering.SECONDS_PER_DAY == 0) {
+    throw new IllegalStateException("SECONDS_PER_DAY constant must not be zero");
+}
 RexNode floorMod = rexBuilder.makeCall(SqlStdOperatorTable.MOD, plusDay, secondsPerDay);
 RexNode numerator = rexBuilder.makeCall(SqlStdOperatorTable.MINUS, epochSeconds, floorMod);
 return rexBuilder.makeCall(SqlStdOperatorTable.DIVIDE, numerator, secondsPerDay);
Suggestion importance[1-10]: 2

__

Why: The suggestion adds a runtime check for a compile-time constant (SECONDS_PER_DAY = 86400). This is unnecessary defensive programming since the constant cannot be zero, and the check would never trigger in practice.

Low
Suggestions up to commit 8feacbe
CategorySuggestion                                                                                                                                    Impact
Possible issue
Handle pre-1970 dates correctly

The division operation truncates toward zero for negative epoch seconds (pre-1970
dates), which differs from floor division. For dates before 1970-01-01, this
produces incorrect day numbers. Use FLOOR after division or adjust the arithmetic to
handle negative values correctly.

sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DateDiffAdapter.java [72-81]

 private static RexNode dayNumber(RexNode operand, RelOptCluster cluster) {
     RexBuilder rexBuilder = cluster.getRexBuilder();
     RexNode anchored = DatePartAdapters.coerceCharacterOperandToTimestamp(operand, cluster);
     RexNode epochSeconds = rexBuilder.makeCall(UnixTimestampAdapter.LOCAL_TO_UNIXTIME_OP, anchored);
     RexNode secondsPerDay = rexBuilder.makeExactLiteral(
         BigDecimal.valueOf(TimeOfDayLowering.SECONDS_PER_DAY),
         rexBuilder.getTypeFactory().createSqlType(SqlTypeName.BIGINT)
     );
-    return rexBuilder.makeCall(SqlStdOperatorTable.DIVIDE, epochSeconds, secondsPerDay);
+    RexNode divided = rexBuilder.makeCall(SqlStdOperatorTable.DIVIDE, epochSeconds, secondsPerDay);
+    return rexBuilder.makeCall(SqlStdOperatorTable.FLOOR, divided);
 }
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies that integer division in Java truncates toward zero, which differs from floor division for negative numbers (pre-1970 dates). However, the comment at line 63 states "for the UTC epoch seconds of any post-1970 date that equals floor", suggesting the implementation may intentionally only support post-1970 dates. The suggestion to add FLOOR is valid for handling pre-1970 dates correctly.

Medium
General
Cast timezone argument to VARCHAR

The tzArg operand is used directly in the convert_tz call without type validation.
If tzArg is also a CHAR(N) type (not VARCHAR), it will cause the same signature
mismatch that toTz was explicitly cast to avoid. Ensure tzArg is also cast to
VARCHAR before passing to convert_tz.

sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DateTimeAdapters.java [230-251]

-// Column input with no embedded TZ in the value: tz arg is the SOURCE zone (MySQL
-// semantics — value is interpreted AT the given tz, then expressed in UTC). The legacy
-// SQL plugin path treated tz as target, which inverted the offset direction.
 RexNode strippedValue;
 if (SqlTypeName.CHAR_TYPES.contains(value.getType().getSqlTypeName())) {
     RexNode pattern = rexBuilder.makeLiteral(OFFSET_SUFFIX_PATTERN);
     RexNode replacement = rexBuilder.makeLiteral("");
     strippedValue = rexBuilder.makeCall(SqlStdOperatorTable.REGEXP_REPLACE, value, pattern, replacement);
 } else {
     strippedValue = value;
 }
 RexNode asTimestamp = rexBuilder.makeCall(original.getType(), LOCAL_TO_TIMESTAMP_OP, List.of(strippedValue));
-// makeLiteral(String) infers CHAR(N) from the literal's length; the convert_tz yaml
-// binds the 2nd/3rd operands as `string` (unbounded VARCHAR), so a CHAR(6) "+00:00"
-// would fail isthmus signature lookup (`convert_tz(precision_timestamp, char<6>, string)`).
 RelDataType varchar = rexBuilder.getTypeFactory().createSqlType(SqlTypeName.VARCHAR);
+RexNode fromTz = rexBuilder.makeCast(varchar, tzArg, true);
 RexNode toTz = rexBuilder.makeLiteral("+00:00", varchar, true);
-return rexBuilder.makeCall(original.getType(), ConvertTzAdapter.LOCAL_CONVERT_TZ_OP, List.of(asTimestamp, tzArg, toTz));
+return rexBuilder.makeCall(original.getType(), ConvertTzAdapter.LOCAL_CONVERT_TZ_OP, List.of(asTimestamp, fromTz, toTz));
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that tzArg should be cast to VARCHAR to match the convert_tz yaml signature, preventing a potential signature mismatch. The fix is straightforward and aligns with the existing pattern for toTz.

Medium
Prevent infinite loop in annotation unwrapping

The stripAnnotation method uses a while loop that could become infinite if
annotation.unwrap() returns a circular reference (e.g., an annotation that unwraps
to itself or a cycle). Add a safeguard to detect and break cycles, such as tracking
visited nodes or limiting iterations.

sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/ComparisonTemporalCoercionAdapter.java [113-118]

 private static RexNode stripAnnotation(RexNode node) {
+    Set<RexNode> visited = new HashSet<>();
     while (node instanceof OperatorAnnotation annotation && annotation.unwrap() != null) {
+        if (!visited.add(node)) {
+            break;
+        }
         node = annotation.unwrap();
     }
     return node;
 }
Suggestion importance[1-10]: 5

__

Why: While the suggestion addresses a theoretical infinite loop risk, circular references in OperatorAnnotation are unlikely in practice given the codebase's design. The safeguard adds defensive programming but may be over-engineering for the current context.

Low
Validate precision is non-negative

The foldPrecision method always returns at least 6 when sub-millisecond digits are
present, but it doesn't validate that resolved is non-negative. If resolved is
negative, Math.max(resolved, 6) will return 6, which may mask an invalid input.
Validate resolved is non-negative before computing the result.

sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/TimestampFunctionAdapter.java [272-275]

 private static int foldPrecision(LocalDateTime ldt, int resolved) {
+    if (resolved < 0) {
+        throw new IllegalArgumentException("Resolved precision must be non-negative, got: " + resolved);
+    }
     return ldt.getNano() % 1_000_000 == 0 ? resolved : Math.max(resolved, 6);
 }
Suggestion importance[1-10]: 4

__

Why: The suggestion adds input validation for resolved, which is good practice. However, the method is private and called from controlled contexts where resolved is derived from Calcite's type system, making negative values highly unlikely. The validation is defensive but not critical.

Low
Validate literal operands explicitly

When either operand is not a string literal, the function returns the original call
unchanged, which will fail downstream. Consider validating that both operands are
literals during planning and raising a clear error if they are not, since GET_FORMAT
requires compile-time constant arguments.

sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/GetFormatAdapter.java [62-66]

 String type = stringLiteral(original.getOperands().get(0));
 String region = stringLiteral(original.getOperands().get(1));
 if (type == null || region == null) {
-    return original;
+    throw new IllegalArgumentException("GET_FORMAT requires both type and region to be string literals");
 }
Suggestion importance[1-10]: 4

__

Why: The suggestion proposes throwing an exception instead of returning the original call. However, the adapter pattern typically returns the original call unchanged when it cannot be adapted, allowing downstream planners to handle it. The current approach is consistent with the adapter pattern and the comment at line 33 states "An unknown (type, region) pair leaves the call unchanged so the downstream planner surfaces a loud error."

Low
Document overflow assumption in floor-mod

The secondsOfDay method applies floor-mod to handle negative epochs, but the
intermediate plusDay step can overflow for very large negative epoch values (e.g.,
dates far before 1970). While unlikely in practice, verify that epoch +
SECONDS_PER_DAY doesn't overflow BIGINT bounds, or document the assumption that
input dates are within a reasonable range.

sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/TimeOfDayLowering.java [50-56]

 static RexNode secondsOfDay(RexNode operand, RelOptCluster cluster) {
     RexBuilder rexBuilder = cluster.getRexBuilder();
     RexNode anchored = DatePartAdapters.coerceCharacterOperandToTimestamp(operand, cluster);
     RexNode epoch = rexBuilder.makeCall(UnixTimestampAdapter.LOCAL_TO_UNIXTIME_OP, anchored);
+    // Floor-mod: ((epoch % 86400) + 86400) % 86400. Assumes epoch is within BIGINT bounds
+    // and that epoch + 86400 does not overflow (valid for dates ~292 billion years from 1970).
     RexNode mod1 = rexBuilder.makeCall(SqlStdOperatorTable.MOD, epoch, bigintLit(rexBuilder, SECONDS_PER_DAY));
     RexNode plusDay = rexBuilder.makeCall(SqlStdOperatorTable.PLUS, mod1, bigintLit(rexBuilder, SECONDS_PER_DAY));
     return rexBuilder.makeCall(SqlStdOperatorTable.MOD, plusDay, bigintLit(rexBuilder, SECONDS_PER_DAY));
 }
Suggestion importance[1-10]: 3

__

Why: The suggestion correctly notes that overflow is theoretically possible for extreme dates, but the scenario is impractical (dates ~292 billion years from 1970). Adding a comment is helpful for documentation but doesn't address a real-world issue.

Low
Use MINUS for day subtraction

The arithmetic date_trunc('month', x) + 1 month - 1 day may produce incorrect
results for edge cases like leap-year February or month-end dates. Verify that the
interval arithmetic correctly handles all month lengths, especially when the input
date is near month boundaries.

sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/LastDayAdapter.java [70-79]

 RexNode plusMonth = rexBuilder.makeCall(
     SqlStdOperatorTable.DATETIME_PLUS,
     firstOfMonth,
     rexBuilder.makeIntervalLiteral(BigDecimal.ONE, new SqlIntervalQualifier(TimeUnit.MONTH, null, SqlParserPos.ZERO))
 );
 RexNode lastDay = rexBuilder.makeCall(
-    SqlStdOperatorTable.DATETIME_PLUS,
+    SqlStdOperatorTable.MINUS,
     plusMonth,
-    rexBuilder.makeIntervalLiteral(BigDecimal.valueOf(-1), new SqlIntervalQualifier(TimeUnit.DAY, null, SqlParserPos.ZERO))
+    rexBuilder.makeIntervalLiteral(BigDecimal.ONE, new SqlIntervalQualifier(TimeUnit.DAY, null, SqlParserPos.ZERO))
 );
Suggestion importance[1-10]: 2

__

Why: The suggestion proposes using MINUS instead of DATETIME_PLUS with a negative interval. However, the existing code already uses BigDecimal.valueOf(-1) to create a negative interval, which is semantically equivalent. The improved_code changes the operator but the logic remains the same. The concern about leap-year handling is unfounded as the comment at line 32 explicitly states the approach is "correct for all month lengths, including leap-year February."

Low
Suggestions up to commit 98e0a57
CategorySuggestion                                                                                                                                    Impact
Possible issue
Check for null literal values

The method doesn't handle null literal values, which can cause NullPointerException
when calling getValueAs(String.class) on a null-valued literal. A RexLiteral can
represent SQL NULL, and attempting to extract its value as a String without checking
isNull() first will fail.

sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/GetFormatAdapter.java [80-89]

 private static String stringLiteral(RexNode operand) {
     if (!(operand instanceof RexLiteral literal)) {
+        return null;
+    }
+    if (literal.isNull()) {
         return null;
     }
     SqlTypeName typeName = literal.getType().getSqlTypeName();
     if (typeName != SqlTypeName.CHAR && typeName != SqlTypeName.VARCHAR) {
         return null;
     }
     return literal.getValueAs(String.class);
 }
Suggestion importance[1-10]: 7

__

Why: Valid suggestion to add null checking for RexLiteral values before calling getValueAs(String.class). This prevents potential NullPointerException when the literal represents SQL NULL. The check improves robustness, though the impact is moderate since the adapter already returns original when operands can't be processed.

Medium
General
Ensure explicit DOUBLE cast

The from_unixtime call expects a floating-point epoch value but receives the result
of arithmetic that may overflow or lose precision when converting large BIGINT epoch
seconds to DOUBLE. For dates far from 1970, the conversion can introduce rounding
errors in the sub-second component or fail to represent the full range of valid
timestamps.

sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/AddSubTimeAdapter.java [72-75]

 RexNode epochDouble = NumericToDoubleAdapter.widenToDoubleIfNumeric(epoch, cluster);
+if (epochDouble.getType().getSqlTypeName() != SqlTypeName.DOUBLE) {
+    epochDouble = rexBuilder.makeCast(
+        rexBuilder.getTypeFactory().createSqlType(SqlTypeName.DOUBLE),
+        epochDouble,
+        true
+    );
+}
 RelDataType tsType = rexBuilder.getTypeFactory()
     .createTypeWithNullability(rexBuilder.getTypeFactory().createSqlType(SqlTypeName.TIMESTAMP), true);
 RexNode ts = rexBuilder.makeCall(tsType, RustUdfDateTimeAdapters.LOCAL_FROM_UNIXTIME_OP, List.of(epochDouble));
Suggestion importance[1-10]: 3

__

Why: The suggestion adds redundant type checking. NumericToDoubleAdapter.widenToDoubleIfNumeric already handles the conversion to DOUBLE, so the additional check and cast are unnecessary. The concern about precision loss is valid for very large epoch values, but the suggested code doesn't solve that problem—it just adds a redundant cast that widenToDoubleIfNumeric already performs.

Low
Suggestions up to commit 19cfc05
CategorySuggestion                                                                                                                                    Impact
Possible issue
Fix pre-1970 date handling

The division operation truncates toward zero for negative epoch seconds (pre-1970
dates), which differs from floor division. This causes incorrect day-number
calculations for dates before the Unix epoch. Use FLOOR after division or implement
proper floor division to ensure consistent calendar-day indexing across all dates.

sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DateDiffAdapter.java [72-81]

 private static RexNode dayNumber(RexNode operand, RelOptCluster cluster) {
     RexBuilder rexBuilder = cluster.getRexBuilder();
     RexNode anchored = DatePartAdapters.coerceCharacterOperandToTimestamp(operand, cluster);
     RexNode epochSeconds = rexBuilder.makeCall(UnixTimestampAdapter.LOCAL_TO_UNIXTIME_OP, anchored);
     RexNode secondsPerDay = rexBuilder.makeExactLiteral(
         BigDecimal.valueOf(TimeOfDayLowering.SECONDS_PER_DAY),
         rexBuilder.getTypeFactory().createSqlType(SqlTypeName.BIGINT)
     );
-    return rexBuilder.makeCall(SqlStdOperatorTable.DIVIDE, epochSeconds, secondsPerDay);
+    RexNode divided = rexBuilder.makeCall(SqlStdOperatorTable.DIVIDE, epochSeconds, secondsPerDay);
+    return rexBuilder.makeCall(SqlStdOperatorTable.FLOOR, divided);
 }
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a potential issue with integer division for pre-1970 dates. The comment at line 63 states "the divide is integer division (truncates toward zero)" and claims this "equals floor" for post-1970 dates, but truncation toward zero differs from floor division for negative numbers. This could cause incorrect DATEDIFF results for dates before 1970-01-01.

Medium
Verify convert_tz operand order

The 2-arg DATETIME semantics changed: when value has NO embedded offset, tzArg is
now the SOURCE zone and target is UTC. However, the convert_tz call passes tzArg as
the second operand (FROM zone) and "+00:00" as the third (TO zone), which matches
the new semantics. Verify that ConvertTzAdapter.LOCAL_CONVERT_TZ_OP signature is
convert_tz(ts, from_tz, to_tz) and not inverted.

sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DateTimeAdapters.java [230-251]

 // Column input with no embedded TZ in the value: tz arg is the SOURCE zone (MySQL
-// semantics — value is interpreted AT the given tz, then expressed in UTC). The legacy
-// SQL plugin path treated tz as target, which inverted the offset direction.
+// semantics — value is interpreted AT the given tz, then expressed in UTC).
 RexNode strippedValue;
 if (SqlTypeName.CHAR_TYPES.contains(value.getType().getSqlTypeName())) {
     RexNode pattern = rexBuilder.makeLiteral(OFFSET_SUFFIX_PATTERN);
     ...
 }
 ...
 RelDataType varchar = rexBuilder.getTypeFactory().createSqlType(SqlTypeName.VARCHAR);
 RexNode toTz = rexBuilder.makeLiteral("+00:00", varchar, true);
+// convert_tz(ts, from_tz=tzArg, to_tz="+00:00") — tzArg is the source zone per MySQL semantics
 return rexBuilder.makeCall(original.getType(), ConvertTzAdapter.LOCAL_CONVERT_TZ_OP, List.of(asTimestamp, tzArg, toTz));
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies a critical semantic change in the 2-arg DATETIME handling where tzArg is now the SOURCE zone (not target). The comment addition helps clarify the operand order for convert_tz, which is important for maintainability and correctness verification. However, the actual code logic appears correct as-is, so this is more of a documentation/verification concern than a bug fix.

Medium
Handle null literal values

The method doesn't handle null literal values, which can cause NullPointerException
when calling getValueAs(String.class) on a null-valued literal. Add a null check for
the literal's value before attempting to extract it as a string.

sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/GetFormatAdapter.java [80-89]

 private static String stringLiteral(RexNode operand) {
     if (!(operand instanceof RexLiteral literal)) {
         return null;
     }
     SqlTypeName typeName = literal.getType().getSqlTypeName();
     if (typeName != SqlTypeName.CHAR && typeName != SqlTypeName.VARCHAR) {
         return null;
     }
+    if (literal.isNull()) {
+        return null;
+    }
     return literal.getValueAs(String.class);
 }
Suggestion importance[1-10]: 7

__

Why: The suggestion identifies a potential NullPointerException when extracting string values from null literals. While the adapt method at line 64 checks if operands are null before calling stringLiteral, adding an explicit null check in stringLiteral itself improves robustness and prevents potential issues if the method is called from other contexts.

Medium
Prevent infinite loop in annotation unwrapping

The stripAnnotation loop could infinite-loop if an OperatorAnnotation chain cycles
back to itself. Add a visited-set guard or depth limit to prevent stack overflow or
infinite recursion when unwrapping nested annotations.

sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/ComparisonTemporalCoercionAdapter.java [113-119]

 private static RexNode stripAnnotation(RexNode node) {
+    java.util.Set<RexNode> visited = new java.util.HashSet<>();
     while (node instanceof OperatorAnnotation annotation && annotation.unwrap() != null) {
+        if (!visited.add(node)) {
+            break; // cycle detected
+        }
         node = annotation.unwrap();
     }
     return node;
 }
Suggestion importance[1-10]: 6

__

Why: The suggestion raises a valid defensive programming concern about potential infinite loops if OperatorAnnotation chains cycle. While such cycles are unlikely in practice (would require malformed AST construction), adding a visited-set guard is a reasonable safety measure. The score is moderate because the issue is theoretical rather than observed in the PR's test coverage.

Low
General
Validate precision bump logic for all levels

When resolved is already greater than 6 (e.g., 9 for nanosecond precision), the
function returns Math.max(resolved, 6) which preserves the higher precision.
However, if the input has non-zero sub-millisecond digits but resolved is less than
6, the function bumps to 6. Verify that this logic correctly handles all precision
levels (3, 6, 9) and doesn't inadvertently truncate nanosecond-precision inputs.

sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/TimestampFunctionAdapter.java [272-275]

 private static int foldPrecision(LocalDateTime ldt, int resolved) {
+    // Bump to 6 only if sub-ms digits exist AND resolved < 6; preserve resolved if already ≥6
     return ldt.getNano() % 1_000_000 == 0 ? resolved : Math.max(resolved, 6);
 }
Suggestion importance[1-10]: 5

__

Why: The suggestion asks to verify the precision logic handles all levels (3, 6, 9) correctly. The existing code Math.max(resolved, 6) already preserves higher precisions (e.g., 9) when present, so the logic appears sound. The improved code is identical to the existing code, making this more of a verification request than an actual improvement. The concern is valid for code review but doesn't identify a concrete issue.

Low

@vinaykpud vinaykpud force-pushed the fix/ai/datetime-sweep-2 branch from 87b1de6 to 6014422 Compare June 9, 2026 20:23
@github-actions

github-actions Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit ebc0b11

@vinaykpud vinaykpud force-pushed the fix/ai/datetime-sweep-2 branch 2 times, most recently from e0d4b3a to c0b2d21 Compare June 9, 2026 21:26
@github-actions

github-actions Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

PR Code Analyzer ❗

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

PathLineSeverityDescription
sandbox/libs/dataformat-native/rust/Cargo.lock1654highNew Rust dependency added: fancy-regex 0.14.0 (checksum: 6e24cb5a94bcae1e5408b0effca5cd7172ea3c5755049c5f3af4cd283a165298). Maintainers must verify this artifact on crates.io matches the expected package before merging.
sandbox/libs/dataformat-native/rust/Cargo.lock407highNew Rust dependency added: bit-set 0.8.0 (checksum: 08807e080ed7f9d5433fa9b275196cfc35414f66a0c79d864dc51a0d825231a3), a transitive dependency pulled in by fancy-regex. Maintainers must verify this artifact on crates.io.
sandbox/libs/dataformat-native/rust/Cargo.lock423highNew Rust dependency added: bit-vec 0.8.0 (checksum: 5e764a1d40d510daf35e07be9eb06e75770908c27d411ee6c92109c9840eaaf7), a transitive dependency pulled in by fancy-regex. Maintainers must verify this artifact on crates.io.

The table above displays the top 10 most important findings.

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

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.

@vinaykpud vinaykpud force-pushed the fix/ai/datetime-sweep-2 branch from c0b2d21 to 109bb0b Compare June 9, 2026 21:43
@github-actions

github-actions Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 109bb0b

@vinaykpud vinaykpud force-pushed the fix/ai/datetime-sweep-2 branch from 109bb0b to 19cfc05 Compare June 9, 2026 21:49
@github-actions

github-actions Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 19cfc05

@vinaykpud vinaykpud changed the title Fix/ai/datetime sweep 2 [analytics-engine] PPL date/time fixes — span types, 2-arg DATETIME tz, sub-ms precision, format tokens, ADDDATE/SUBDATE, TIME-vs-DATE comparison Jun 9, 2026
@vinaykpud vinaykpud changed the title [analytics-engine] PPL date/time fixes — span types, 2-arg DATETIME tz, sub-ms precision, format tokens, ADDDATE/SUBDATE, TIME-vs-DATE comparison [analytics-engine] PPL date/time fixes — 15 new scalars, TIME comparison, sub-ms precision, format tokens Jun 9, 2026
@vinaykpud vinaykpud changed the title [analytics-engine] PPL date/time fixes — 15 new scalars, TIME comparison, sub-ms precision, format tokens PPL date/time fixes 15 new scalars, TIME comparison, sub-ms precision, format tokens Jun 9, 2026
vinaykpud added 8 commits June 9, 2026 21:58
@vinaykpud
Add 15 PPL datetime scalars over DataFusion
d206871 
Lowers ADDDATE/SUBDATE, DATEDIFF, TO_DAYS/TO_SECONDS/FROM_DAYS/TIME_TO_SEC,
SEC_TO_TIME, WEEKDAY, PERIOD_ADD/PERIOD_DIFF, GET_FORMAT, ADDTIME/SUBTIME/
TIMEDIFF, LAST_DAY, YEARWEEK to DataFusion-native expressions via per-function
adapters. Each rewrites to to_unixtime/from_unixtime + integer arithmetic,
date_trunc, maketime, or a Rust UDF that lowers through the Substrait catalog.

Includes:
- DateAddSubAdapter: handle ADDDATE/SUBDATE integer form (treated as
  INTERVAL N DAY per MySQL).
- TimeFormatAdapter: coerce a TIME first operand to TIMESTAMP (substrait-java
  0.89.1 has no precision_time<P> binding, so calls like time_format(maketime,
  ...) need anchoring) — same workaround DatePartAdapters uses.
- EarliestLatestAdapter: pin result type to the call's declared nullable
  BOOLEAN so a non-null timestamp operand doesn't trip Calcite's BOOLEAN-vs-
  BOOLEAN-NOT-NULL assert in the enclosing Project/Filter.
- SubstraitPlanPojoRewriter: only rescale precision-9 (ns) literals to ms;
  precision-6 (µs) literals pass through so cast('… .123456' AS TIMESTAMP)
  preserves the µs fraction.
- New os_yearweek Rust UDF sharing os_week's per-mode math.

Unit tests cover every adapter at the rewrite-shape level; QA ITs in
DateTimeScalarFunctionsIT pin MySQL semantics (year-boundary roll, leap-Feb
last_day, MOD-86400 wrap, today-anchor cancellation, integer-form ADDDATE,
…). Unmutes 15 ExtensiveCoveragePplIT golden queries.

Signed-off-by: Vinay Krishna Pudyodu <vinkrish.neo@gmail.com>
@vinaykpud
Fix TIME-vs-{DATE,TIMESTAMP} comparison via operand coercion
f6197b9 
Calcite's binary-comparison type-coercion wraps a TIME operand with
to_timestamp(precision_time<P>) to align with a DATE/TIMESTAMP peer, but
the yaml has no impl for that signature — isthmus throws "Unable to
convert call to_timestamp(precision_time<9>?)" and the query fails with
HTTP 400.

Extend ComparisonTemporalCoercionAdapter (already handling
VARCHAR-vs-TIMESTAMP) to also rewrite the TIME side to a today-anchored
TIMESTAMP via the existing DatePartAdapters.coerceCharacterOperandToTimestamp
helper, mirroring PPL's TIME-vs-{DATE,TIMESTAMP} semantics. TIME-vs-TIME
is left on the native Substrait path.

Signed-off-by: Vinay Krishna Pudyodu <vinkrish.neo@gmail.com>
@vinaykpud
Register PPL ADDDATE/SUBDATE with integer-days overload
8ca3e9d 
ADDDATE / SUBDATE were unbound in the analytics-engine scalar registry,
so PPL queries using either UDF returned `No backend supports scalar
function [ADDDATE]`. Both alias DATE_ADD / DATE_SUB but additionally
accept an integer second operand (days), which the existing
DateAddSubAdapter rejected.

Add ADDDATE / SUBDATE to ScalarFunction, register them against the
shared DateAddSubAdapter, and extend the adapter to rebuild integer
operands as INTERVAL N DAY before the existing interval-lowering path.
Cluster verified — 10/10 affected queries return HTTP 200 with expected
values; null-row semantics preserved.

Signed-off-by: Vinay Krishna Pudyodu <vinkrish.neo@gmail.com>
@vinaykpud
Preserve sub-millisecond precision in timestamp literal folds
ae262d9 
Calcite's makeTimestampLiteral(value, precision) truncates the value to
`precision` digits, so timestamp('2020-09-16 17:30:00.123456') folded at
the parquet-resolved precision (3) silently dropped the .456 µs digits
the user typed. microsecond() and date_format(_, '%f') then read 123000
instead of 123456.

Bump fold precision to 6 in TimestampFunctionAdapter when the input
carries non-zero sub-ms digits; ms-aligned values stay at the resolved
precision. Drop the stale precision-3 rescale in SubstraitPlanPojoRewriter
— DataFusion 53.x coerces precision-6 literals against ms parquet columns
without a runtime cost, verified end-to-end against 14 PPL queries.

Signed-off-by: Vinay Krishna Pudyodu <vinkrish.neo@gmail.com>
@vinaykpud
Reject invalid string literals in DAYNAME/MONTHNAME at plan time
2149c44 
Pre-fix, an invalid input like DAYNAME('2025-13-02') flowed through
plan-time and crashed the streaming fragment with a generic
StreamException. PPL's negative-path tests want a structured
"unsupported format" rejection at plan time so the user sees a
typed error instead of a streaming-layer crash.

Add validateFirstArgIfStringLiteral(...) calls to DaynameAdapter and
MonthnameAdapter, mirroring the existing DateFormatAdapter /
TimeFormatAdapter siblings. Valid inputs pass through unchanged.

Signed-off-by: Vinay Krishna Pudyodu <vinkrish.neo@gmail.com>
@vinaykpud
Pin convert_tz precision-9 path with regression IT
779b934 
Two-arg DATETIME(name, tz) lowers to convert_tz, which had a
precision-9 binding gap pre-fix (already addressed in earlier
commit dd3a231 via SAFE-cast in ConvertTzAdapter). Add an
IT method exercising convert_tz on a null-typed VARCHAR column
to lock in the cast-then-fold-to-null contract against future
regression.

Signed-off-by: Vinay Krishna Pudyodu <vinkrish.neo@gmail.com>
@vinaykpud
Unmute 14 datetime ITs
70a82c9 
Five narrow fixes that unmute six previously-pending DatetimeCoverageIT /
DateTimeScalarFunctionsIT regression gates:

- ArrowValues TIMESTAMP/TIME render: switch fixed-width SSSSSSSSS to
  appendFraction(NANO_OF_SECOND, 1, 9, true). Trailing zeros stripped, so
  '23:59:59.999999000' → '23:59:59.999999' and '2262-04-11 23:47:16.854000'
  → '2262-04-11 23:47:16.854'. Dispatcher (nano==0 ? NO_NANO : WITH_NANO)
  guarantees ≥1 fractional digit so the 1..9 minimum never trips.

- DatetimeAdapter (1-arg): TIME first operand serialises to substrait
  precision_time<P>, no to_timestamp yaml binding. Anchor to TIMESTAMP via
  DatePartAdapters.coerceCharacterOperandToTimestamp before to_timestamp.

- DatetimeAdapter (2-arg, column path): synthesised "+00:00" target-tz
  literal as VARCHAR (not CHAR(6)) so isthmus binds the
  convert_tz(precision_timestamp, string, string) yaml arm; previously
  emitted convert_tz(_, char<6>, _) which mismatched.

- DatetimeAdapter (2-arg fold + column): MySQL semantics — when the value
  has no embedded TZ offset, the user's tz arg is the SOURCE (target=UTC),
  not the target. The previous direction inverted the offset
  (DATETIME('2008-01-01 02:00:00', '+10:00') was returning
  '2008-01-01 12:00:00' instead of '2007-12-31 16:00:00').

- os_strftime (Rust UDF): %c emits month with no leading zero (MySQL
  documented behavior, was zero-padded "01"); %U/%u/%V/%v emit
  zero-padded week numbers width 2 (was unpadded "4" instead of "04").
  Companion Rust unit tests updated.

testClusterA_spanCustomFormatDate stays @AwaitsFix — requires
OpenSearchTypeFactory DateOnlyType bridging in the SQL plugin, which is
read-only on this branch; the rationale is captured in the test's Javadoc.

Signed-off-by: Vinay Krishna Pudyodu <vinkrish.neo@gmail.com>
@vinaykpud
Rename DatetimeCoverageIT methods by behavior, drop cluster labels
98e0a57 
Test method names previously embedded the internal grouping (testClusterA_*,
testClusterB_*, etc.) which leaked the maintenance taxonomy into the surface
contract. Renamed each method to describe what it tests
(testSpanDateTypePreservesDate, testTwoArgDatetimeColInput, etc.) and trimmed
the section banner / Javadoc prefixes to drop the cluster-letter naming.
No assertion or query changes.

Signed-off-by: Vinay Krishna Pudyodu <vinkrish.neo@gmail.com>
@vinaykpud vinaykpud force-pushed the fix/ai/datetime-sweep-2 branch from 19cfc05 to 98e0a57 Compare June 9, 2026 21:58
@github-actions

github-actions Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 98e0a57

@vinaykpud vinaykpud marked this pull request as ready for review June 9, 2026 22:08
@vinaykpud vinaykpud requested a review from a team as a code owner June 9, 2026 22:08
@vinaykpud
Drop as_any from OsYearweekUdf for DataFusion 54 compat
8feacbe 
Mainline upgrade to DataFusion 54 (opensearch-project#22063) removed as_any from the
ScalarUDFImpl trait — the udf_identity! macro now provides it. Sibling
OsWeekUdf already shed the override; OsYearweekUdf added on this branch
still declared it and broke cargoTest with E0407.

Drop the override; trait dispatch falls through to the macro's impl.

Signed-off-by: Vinay Krishna Pudyodu <vinkrish.neo@gmail.com>
@github-actions

github-actions Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 8feacbe

@github-actions

github-actions Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

✅ Gradle check result for 8feacbe: SUCCESS

@codecov

codecov Bot commented Jun 9, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.38%. Comparing base (1559fb4) to head (be091ed).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff            @@
##               main   #22086   +/-   ##
=========================================
  Coverage     73.37%   73.38%           
- Complexity    75572    75592   +20     
=========================================
  Files          6038     6038           
  Lines        343009   343009           
  Branches      49348    49348           
=========================================
+ Hits         251674   251703   +29     
- Misses        71305    71313    +8     
+ Partials      20030    19993   -37

☔ 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

github-actions Bot commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit b329c6d

@github-actions

github-actions Bot commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 978d627

vinaykpud added 3 commits June 10, 2026 02:25
@vinaykpud
Floor-divide DATEDIFF day-numbers for pre-1970 correctness
a9570fd 
SQL `/` truncates toward zero, so naive `to_unixtime(x) / 86400` rounds
negative epoch seconds the wrong way and puts e.g. `1969-12-31 12:00`
on the same epoch-day as `1970-01-01 00:00`. Express the floor in pure
integer arithmetic — `(x - ((x MOD d + d) MOD d)) / d` — since isthmus
has no `FLOOR(i64)` signature in the bound catalog. Pin with two
DateTimeScalarFunctionsIT cases that span and sit below the epoch.

Signed-off-by: Vinay Krishna Pudyodu <vinkrish.neo@gmail.com>
@vinaykpud
Cast tzArg to VARCHAR in DATETIME 2-arg column path
459ec2c 
Calcite infers CHAR(N) from a literal's length, but the convert_tz yaml
binds tz operands as unbounded `string` (VARCHAR). Without the cast, a
user-supplied tz literal like `'+05:30'` lowers as
`convert_tz(precision_timestamp, char<6>, char<6>)` — same CHAR-vs-string
mismatch already fixed for the synthesised target tz literal, just from
the other side. Cast the source tz to VARCHAR for symmetry; the literal
target was already cast.

Signed-off-by: Vinay Krishna Pudyodu <vinkrish.neo@gmail.com>
@vinaykpud
TODO: centralise annotation stripping in FragmentConversionDriver
be091ed 
Per PR 22086 review feedback (expani, mch2): adapters shouldn't be
stripping OperatorAnnotation themselves; the driver should normalise
the tree. Tag the local strip helper as a follow-up so the cleanup
covers every adapter doing this together.

Signed-off-by: Vinay Krishna Pudyodu <vinkrish.neo@gmail.com>
@vinaykpud vinaykpud force-pushed the fix/ai/datetime-sweep-2 branch from 978d627 to be091ed Compare June 10, 2026 02:27
@github-actions

github-actions Bot commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit be091ed

@github-actions

github-actions Bot commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

❕ Gradle check result for be091ed: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

@mch2 mch2 merged commit 3cd77ad into opensearch-project:main Jun 10, 2026
15 of 16 checks passed
@opensearch-ci-bot opensearch-ci-bot mentioned this pull request Jun 17, 2026
Open
KhishorekumarBS pushed a commit to KhishorekumarBS/OpenSearch that referenced this pull request Jul 3, 2026
@vinaykpud
PPL date/time fixes 15 new scalars, TIME comparison, sub-ms precision…
f862e9f 
…, format tokens (opensearch-project#22086)

* Add 15 PPL datetime scalars over DataFusion

Lowers ADDDATE/SUBDATE, DATEDIFF, TO_DAYS/TO_SECONDS/FROM_DAYS/TIME_TO_SEC,
SEC_TO_TIME, WEEKDAY, PERIOD_ADD/PERIOD_DIFF, GET_FORMAT, ADDTIME/SUBTIME/
TIMEDIFF, LAST_DAY, YEARWEEK to DataFusion-native expressions via per-function
adapters. Each rewrites to to_unixtime/from_unixtime + integer arithmetic,
date_trunc, maketime, or a Rust UDF that lowers through the Substrait catalog.

Includes:
- DateAddSubAdapter: handle ADDDATE/SUBDATE integer form (treated as
  INTERVAL N DAY per MySQL).
- TimeFormatAdapter: coerce a TIME first operand to TIMESTAMP (substrait-java
  0.89.1 has no precision_time<P> binding, so calls like time_format(maketime,
  ...) need anchoring) — same workaround DatePartAdapters uses.
- EarliestLatestAdapter: pin result type to the call's declared nullable
  BOOLEAN so a non-null timestamp operand doesn't trip Calcite's BOOLEAN-vs-
  BOOLEAN-NOT-NULL assert in the enclosing Project/Filter.
- SubstraitPlanPojoRewriter: only rescale precision-9 (ns) literals to ms;
  precision-6 (µs) literals pass through so cast('… .123456' AS TIMESTAMP)
  preserves the µs fraction.
- New os_yearweek Rust UDF sharing os_week's per-mode math.

Unit tests cover every adapter at the rewrite-shape level; QA ITs in
DateTimeScalarFunctionsIT pin MySQL semantics (year-boundary roll, leap-Feb
last_day, MOD-86400 wrap, today-anchor cancellation, integer-form ADDDATE,
…). Unmutes 15 ExtensiveCoveragePplIT golden queries.

Signed-off-by: Vinay Krishna Pudyodu <vinkrish.neo@gmail.com>

* Fix TIME-vs-{DATE,TIMESTAMP} comparison via operand coercion

Calcite's binary-comparison type-coercion wraps a TIME operand with
to_timestamp(precision_time<P>) to align with a DATE/TIMESTAMP peer, but
the yaml has no impl for that signature — isthmus throws "Unable to
convert call to_timestamp(precision_time<9>?)" and the query fails with
HTTP 400.

Extend ComparisonTemporalCoercionAdapter (already handling
VARCHAR-vs-TIMESTAMP) to also rewrite the TIME side to a today-anchored
TIMESTAMP via the existing DatePartAdapters.coerceCharacterOperandToTimestamp
helper, mirroring PPL's TIME-vs-{DATE,TIMESTAMP} semantics. TIME-vs-TIME
is left on the native Substrait path.

Signed-off-by: Vinay Krishna Pudyodu <vinkrish.neo@gmail.com>

* Register PPL ADDDATE/SUBDATE with integer-days overload

ADDDATE / SUBDATE were unbound in the analytics-engine scalar registry,
so PPL queries using either UDF returned `No backend supports scalar
function [ADDDATE]`. Both alias DATE_ADD / DATE_SUB but additionally
accept an integer second operand (days), which the existing
DateAddSubAdapter rejected.

Add ADDDATE / SUBDATE to ScalarFunction, register them against the
shared DateAddSubAdapter, and extend the adapter to rebuild integer
operands as INTERVAL N DAY before the existing interval-lowering path.
Cluster verified — 10/10 affected queries return HTTP 200 with expected
values; null-row semantics preserved.

Signed-off-by: Vinay Krishna Pudyodu <vinkrish.neo@gmail.com>

* Preserve sub-millisecond precision in timestamp literal folds

Calcite's makeTimestampLiteral(value, precision) truncates the value to
`precision` digits, so timestamp('2020-09-16 17:30:00.123456') folded at
the parquet-resolved precision (3) silently dropped the .456 µs digits
the user typed. microsecond() and date_format(_, '%f') then read 123000
instead of 123456.

Bump fold precision to 6 in TimestampFunctionAdapter when the input
carries non-zero sub-ms digits; ms-aligned values stay at the resolved
precision. Drop the stale precision-3 rescale in SubstraitPlanPojoRewriter
— DataFusion 53.x coerces precision-6 literals against ms parquet columns
without a runtime cost, verified end-to-end against 14 PPL queries.

Signed-off-by: Vinay Krishna Pudyodu <vinkrish.neo@gmail.com>

* Reject invalid string literals in DAYNAME/MONTHNAME at plan time

Pre-fix, an invalid input like DAYNAME('2025-13-02') flowed through
plan-time and crashed the streaming fragment with a generic
StreamException. PPL's negative-path tests want a structured
"unsupported format" rejection at plan time so the user sees a
typed error instead of a streaming-layer crash.

Add validateFirstArgIfStringLiteral(...) calls to DaynameAdapter and
MonthnameAdapter, mirroring the existing DateFormatAdapter /
TimeFormatAdapter siblings. Valid inputs pass through unchanged.

Signed-off-by: Vinay Krishna Pudyodu <vinkrish.neo@gmail.com>

* Pin convert_tz precision-9 path with regression IT

Two-arg DATETIME(name, tz) lowers to convert_tz, which had a
precision-9 binding gap pre-fix (already addressed in earlier
commit dd3a231 via SAFE-cast in ConvertTzAdapter). Add an
IT method exercising convert_tz on a null-typed VARCHAR column
to lock in the cast-then-fold-to-null contract against future
regression.

Signed-off-by: Vinay Krishna Pudyodu <vinkrish.neo@gmail.com>

* Unmute 14 datetime ITs

Five narrow fixes that unmute six previously-pending DatetimeCoverageIT /
DateTimeScalarFunctionsIT regression gates:

- ArrowValues TIMESTAMP/TIME render: switch fixed-width SSSSSSSSS to
  appendFraction(NANO_OF_SECOND, 1, 9, true). Trailing zeros stripped, so
  '23:59:59.999999000' → '23:59:59.999999' and '2262-04-11 23:47:16.854000'
  → '2262-04-11 23:47:16.854'. Dispatcher (nano==0 ? NO_NANO : WITH_NANO)
  guarantees ≥1 fractional digit so the 1..9 minimum never trips.

- DatetimeAdapter (1-arg): TIME first operand serialises to substrait
  precision_time<P>, no to_timestamp yaml binding. Anchor to TIMESTAMP via
  DatePartAdapters.coerceCharacterOperandToTimestamp before to_timestamp.

- DatetimeAdapter (2-arg, column path): synthesised "+00:00" target-tz
  literal as VARCHAR (not CHAR(6)) so isthmus binds the
  convert_tz(precision_timestamp, string, string) yaml arm; previously
  emitted convert_tz(_, char<6>, _) which mismatched.

- DatetimeAdapter (2-arg fold + column): MySQL semantics — when the value
  has no embedded TZ offset, the user's tz arg is the SOURCE (target=UTC),
  not the target. The previous direction inverted the offset
  (DATETIME('2008-01-01 02:00:00', '+10:00') was returning
  '2008-01-01 12:00:00' instead of '2007-12-31 16:00:00').

- os_strftime (Rust UDF): %c emits month with no leading zero (MySQL
  documented behavior, was zero-padded "01"); %U/%u/%V/%v emit
  zero-padded week numbers width 2 (was unpadded "4" instead of "04").
  Companion Rust unit tests updated.

testClusterA_spanCustomFormatDate stays @AwaitsFix — requires
OpenSearchTypeFactory DateOnlyType bridging in the SQL plugin, which is
read-only on this branch; the rationale is captured in the test's Javadoc.

Signed-off-by: Vinay Krishna Pudyodu <vinkrish.neo@gmail.com>

* Rename DatetimeCoverageIT methods by behavior, drop cluster labels

Test method names previously embedded the internal grouping (testClusterA_*,
testClusterB_*, etc.) which leaked the maintenance taxonomy into the surface
contract. Renamed each method to describe what it tests
(testSpanDateTypePreservesDate, testTwoArgDatetimeColInput, etc.) and trimmed
the section banner / Javadoc prefixes to drop the cluster-letter naming.
No assertion or query changes.

Signed-off-by: Vinay Krishna Pudyodu <vinkrish.neo@gmail.com>

* Drop as_any from OsYearweekUdf for DataFusion 54 compat

Mainline upgrade to DataFusion 54 (opensearch-project#22063) removed as_any from the
ScalarUDFImpl trait — the udf_identity! macro now provides it. Sibling
OsWeekUdf already shed the override; OsYearweekUdf added on this branch
still declared it and broke cargoTest with E0407.

Drop the override; trait dispatch falls through to the macro's impl.

Signed-off-by: Vinay Krishna Pudyodu <vinkrish.neo@gmail.com>

* Floor-divide DATEDIFF day-numbers for pre-1970 correctness

SQL `/` truncates toward zero, so naive `to_unixtime(x) / 86400` rounds
negative epoch seconds the wrong way and puts e.g. `1969-12-31 12:00`
on the same epoch-day as `1970-01-01 00:00`. Express the floor in pure
integer arithmetic — `(x - ((x MOD d + d) MOD d)) / d` — since isthmus
has no `FLOOR(i64)` signature in the bound catalog. Pin with two
DateTimeScalarFunctionsIT cases that span and sit below the epoch.

Signed-off-by: Vinay Krishna Pudyodu <vinkrish.neo@gmail.com>

* Cast tzArg to VARCHAR in DATETIME 2-arg column path

Calcite infers CHAR(N) from a literal's length, but the convert_tz yaml
binds tz operands as unbounded `string` (VARCHAR). Without the cast, a
user-supplied tz literal like `'+05:30'` lowers as
`convert_tz(precision_timestamp, char<6>, char<6>)` — same CHAR-vs-string
mismatch already fixed for the synthesised target tz literal, just from
the other side. Cast the source tz to VARCHAR for symmetry; the literal
target was already cast.

Signed-off-by: Vinay Krishna Pudyodu <vinkrish.neo@gmail.com>

* TODO: centralise annotation stripping in FragmentConversionDriver

Per PR 22086 review feedback (expani, mch2): adapters shouldn't be
stripping OperatorAnnotation themselves; the driver should normalise
the tree. Tag the local strip helper as a follow-up so the cleanup
covers every adapter doing this together.

Signed-off-by: Vinay Krishna Pudyodu <vinkrish.neo@gmail.com>

---------

Signed-off-by: Vinay Krishna Pudyodu <vinkrish.neo@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mch2 mch2 mch2 approved these changes
+1 more reviewer
@expani expani expani left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@vinaykpud @mch2 @expani