Support PPL timewrap command

[ahkcs]

Description

Adds the timewrap command to PPL, which reshapes timechart output by wrapping each time period into separate columns for side-by-side comparison (day-over-day, week-over-week, etc.). Behavior verified against Splunk.

Syntax: ... | timechart <agg> | timewrap <span> [align=end|now]

Key features

• Column naming matches Splunk: <agg>_<N><unit>_before, <agg>_latest_<unit>, <agg>_<N><unit>_after
• Column order: oldest first (leftmost)
• Dynamic column count: only populated period columns appear (no trailing nulls)
• align=end (default): uses WHERE upper bound as reference for deterministic column names
• align=now: uses current time as reference
• Supports all timescales: sec, min, hr, day, week, month, quarter, year

Implementation

• Grammar: timewrapCommand rule reusing existing spanLiteral
• AST: Timewrap node with unit, value, align
• Calcite: PIVOT-based transformation with UNIX_TIMESTAMP/FROM_UNIXTIME for epoch conversion
• Execution engine: post-processing to strip all-null columns, rename to absolute offsets using __base_offset__, and handle latest_<unit> naming
• WHERE upper bound extraction: walks AST to find @timestamp <= filter for align=end reference

Known limitations

• BY clause not yet supported
• Month/quarter/year use approximate fixed-length conversions (30d, 91d, 365d)
• Max 20 period columns
• Timestamps show data dates, not reference date (Splunk rewrites to reference date)

Testing

• 26 integration tests covering all timescales, aggregation types, WHERE filters, align modes, null fill, and edge cases
• 5 doctest examples in user documentation
• All existing timechart and transpose tests pass (backward compatible)

Issues Resolved

Resolves #5237

Check List

• New functionality includes testing
• New functionality has been documented
• Commits are signed per the DCO using --signoff

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 41f00ee)

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 leap year check uses modulo arithmetic that can produce incorrect results for years divisible by 100 but not 400 (e.g., 1900, 2100). The condition mod400 == 0 OR (mod4 == 0 AND mod100 != 0) is correct, but the CASE expression at line 239-248 evaluates mod4 == 0 AND (mod100 != 0 OR mod400 == 0), which is logically equivalent but relies on short-circuit evaluation that SQL CASE does not guarantee. If mod100 == 0 and mod400 != 0, the year is not a leap year, but the expression may still evaluate the mod400 == 0 branch. This affects cumulative day calculations for quarters spanning February in century years. RexBuilder rx, RexNode month, RexNode year, RelDataType bigintType) { RexNode mod4 = rx.makeCall( SqlStdOperatorTable.MOD, year, rx.makeExactLiteral(BigDecimal.valueOf(4), bigintType)); RexNode mod100 = rx.makeCall( SqlStdOperatorTable.MOD, year, rx.makeExactLiteral(BigDecimal.valueOf(100), bigintType)); RexNode mod400 = rx.makeCall( SqlStdOperatorTable.MOD, year, rx.makeExactLiteral(BigDecimal.valueOf(400), bigintType)); RexNode zero = rx.makeExactLiteral(BigDecimal.ZERO, bigintType); RexNode isLeap = rx.makeCall( SqlStdOperatorTable.CASE, rx.makeCall( SqlStdOperatorTable.AND, rx.makeCall(SqlStdOperatorTable.EQUALS, mod4, zero), rx.makeCall( SqlStdOperatorTable.OR, rx.makeCall(SqlStdOperatorTable.NOT_EQUALS, mod100, zero), rx.makeCall(SqlStdOperatorTable.EQUALS, mod400, zero))), rx.makeExactLiteral(BigDecimal.ONE, bigintType), zero); Possible Issue For fixed-length units with align=now over future-dated data, the floor division at lines 3941-3945 casts to DOUBLE and back to BIGINT to avoid truncation-toward-zero. However, DOUBLE has 53 bits of precision; epoch differences beyond ~285 years (2^53 seconds ≈ 285 million years) lose precision, causing incorrect period labels. While this is an edge case, the comment explicitly calls out the need for floor division correctness, yet the implementation silently degrades for large time ranges. // Floor-divide (ref - maxEpoch) by span: integer DIVIDE truncates toward zero, which is wrong // when the reference is below maxEpoch (e.g. align=now over future-dated data) — it would // shift period labels by one across the latest/before/after boundary. Cast to DOUBLE and // FLOOR // to get true floor division, then back to BIGINT. RelDataType doubleType = rx.getTypeFactory().createSqlType(SqlTypeName.DOUBLE); RexNode refDiff = rx.makeCall(SqlStdOperatorTable.MINUS, refLit, maxEpoch); RexNode refDiffDouble = rx.makeCast(doubleType, refDiff, true); baseOffset = rx.makeCast( Possible Issue The pivot logic at line 99 reads __period__ from each row and assumes it is non-null and a valid long. If a row has a null __period__ (e.g., due to a null timestamp in the input data that survived the timechart aggregation), pv.longValue() throws a NullPointerException. The code should either skip rows with null __period__ or handle the exception gracefully. for (ExprValue row : values) { ExprValue pv = row.tupleValue().get("__period__"); if (pv != null && !pv.isNull()) { periodSet.add(pv.longValue()); } } Possible Issue The extractTimestampUpperBound method walks the AST to find the deepest Filter node with a timestamp upper bound. If the user writes a query with multiple WHERE clauses (e.g., where @timestamp <= X | where @timestamp <= Y), the method returns the last (deepest) bound found. However, if Y > X, the returned bound is Y, not the tighter X, causing incorrect align=end reference and wrong period labels. The method should return the minimum of all found upper bounds, not the last one. public static Long extractTimestampUpperBound(Timewrap node) { Node current = node; Long lastBound = null; while (current != null && !current.getChild().isEmpty()) { current = current.getChild().get(0); if (current instanceof Filter filter) { Long bound = findUpperBound(filter.getCondition()); if (bound != null) { lastBound = bound; } } } return lastBound; }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 41f00ee

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Add null check for bookkeeping column The code assumes period is always present and non-null, but if the unpivoted rows are missing this bookkeeping column (e.g., due to a plan construction bug), longValue() will throw a NullPointerException. Add a null check and fail fast with a descriptive error message to aid debugging. core/src/main/java/org/opensearch/sql/calcite/utils/TimewrapPivot.java [132-135] for (ExprValue row : values) { Map<String, ExprValue> tuple = row.tupleValue(); String tsKey = tuple.get(tsColName).toString(); - long period = tuple.get("__period__").longValue(); + ExprValue periodVal = tuple.get("__period__"); + if (periodVal == null || periodVal.isNull()) { + throw new IllegalStateException("Timewrap pivot requires __period__ column in unpivoted rows"); + } + long period = periodVal.longValue(); ... } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that __period__ could be null and proposes adding a null check with a descriptive error message. This is a valid error handling improvement that would aid debugging if the unpivoted rows are malformed. However, it's a defensive check rather than fixing a known bug, so it receives a moderate score.	Medium
General	Avoid floating-point precision loss The floor division logic for baseOffset casts to DOUBLE and back to BIGINT, which may lose precision for large epoch differences (e.g., years of data). Consider using integer arithmetic with explicit floor semantics (subtract the modulo remainder before dividing) to avoid floating-point precision issues. core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [3942-3950] RexNode refDiff = rx.makeCall(SqlStdOperatorTable.MINUS, refLit, maxEpoch); -RexNode refDiffDouble = rx.makeCast(doubleType, refDiff, true); -baseOffset = - rx.makeCast( - bigintType, - rx.makeCall( - SqlStdOperatorTable.FLOOR, - rx.makeCall(SqlStdOperatorTable.DIVIDE, refDiffDouble, spanLit)), - true); +RexNode remainder = rx.makeCall(SqlStdOperatorTable.MOD, refDiff, spanLit); +RexNode adjustedDiff = rx.makeCall(SqlStdOperatorTable.MINUS, refDiff, remainder); +baseOffset = rx.makeCall(SqlStdOperatorTable.DIVIDE, adjustedDiff, spanLit); Suggestion importance[1-10]: 4 __ Why: The suggestion addresses a potential precision issue in floor division for baseOffset, but the proposed integer arithmetic approach (subtract modulo, then divide) may not correctly handle negative differences when referenceEpoch < maxEpoch. The current FLOOR-based approach is intentionally designed to handle this case correctly, as noted in the code comment. The suggestion's alternative could introduce bugs.	Low

---

Previous suggestions

Suggestions up to commit 026f481

Category	Suggestion	Impact
General	Validate base offset consistency assumption The code assumes base_offset is constant across all rows and reads only the first row. If the input is malformed or the bookkeeping column varies (e.g., due to a bug in the RelNode construction), this will silently produce incorrect period names. Verify that all rows have the same base_offset value or document this assumption explicitly. core/src/main/java/org/opensearch/sql/calcite/utils/TimewrapPivot.java [85-89] ExprValue boVal = values.getFirst().tupleValue().get("__base_offset__"); if (boVal != null && !boVal.isNull()) { baseOffset = boVal.longValue(); } +// Verify assumption: __base_offset__ is constant across all rows +for (ExprValue row : values) { + ExprValue bo = row.tupleValue().get("__base_offset__"); + if (bo != null && !bo.isNull() && bo.longValue() != baseOffset) { + throw new IllegalStateException("__base_offset__ varies across rows; expected constant value"); + } +} Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies that __base_offset__ is assumed constant and only reads the first row. Adding validation would catch malformed input, but this adds O(n) overhead to every timewrap query. The assumption is guaranteed by the RelNode construction in visitTimewrap, making this defensive check of limited value.	Low
	Avoid precision loss in floor division The floor division logic correctly handles negative differences when the reference is below maxEpoch, but the intermediate cast to DOUBLE may introduce precision loss for very large epoch differences (beyond 2^53). Consider using BIGINT arithmetic with explicit sign handling to maintain precision across the full epoch range. core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [3941-3950] RexNode refDiff = rx.makeCall(SqlStdOperatorTable.MINUS, refLit, maxEpoch); -RexNode refDiffDouble = rx.makeCast(doubleType, refDiff, true); -baseOffset = - rx.makeCast( - bigintType, - rx.makeCall( - SqlStdOperatorTable.FLOOR, - rx.makeCall(SqlStdOperatorTable.DIVIDE, refDiffDouble, spanLit)), - true); +RexNode isNegative = rx.makeCall(SqlStdOperatorTable.LESS_THAN, refDiff, rx.makeBigintLiteral(BigDecimal.ZERO)); +RexNode absDiff = rx.makeCall(SqlStdOperatorTable.ABS, refDiff); +RexNode absQuotient = rx.makeCall(SqlStdOperatorTable.DIVIDE, absDiff, spanLit); +RexNode adjustment = rx.makeCall( + SqlStdOperatorTable.CASE, + rx.makeCall(SqlStdOperatorTable.EQUALS, rx.makeCall(SqlStdOperatorTable.MOD, absDiff, spanLit), rx.makeBigintLiteral(BigDecimal.ZERO)), + rx.makeBigintLiteral(BigDecimal.ZERO), + rx.makeBigintLiteral(BigDecimal.ONE)); +RexNode floorQuotient = rx.makeCall(SqlStdOperatorTable.PLUS, absQuotient, adjustment); +baseOffset = rx.makeCall( + SqlStdOperatorTable.CASE, + isNegative, + rx.makeCall(SqlStdOperatorTable.MINUS, rx.makeBigintLiteral(BigDecimal.ZERO), floorQuotient), + absQuotient); Suggestion importance[1-10]: 4 __ Why: While the concern about precision loss beyond 2^53 is theoretically valid, epoch timestamps in practice stay well within this range (current epoch ~1.7e9 seconds). The suggested replacement with complex BIGINT arithmetic is significantly more verbose and harder to maintain for a problem that won't occur in real-world usage.	Low
	Explicit floor for quarter calculation The quarter calculation uses integer division which truncates toward zero, potentially producing incorrect results for month values 1-3 (Q1). When month=1, (1-1)/3=0, but when month=2, (2-1)/3=0, and month=3 gives (3-1)/3=0. However, month=4 gives (4-1)/3=1. This is correct, but consider adding a FLOOR operation to make the truncation behavior explicit and consistent with the fixed-length unit path. core/src/main/java/org/opensearch/sql/calcite/utils/TimewrapUtils.java [105-112] RexNode quarter = rx.makeCall( - SqlStdOperatorTable.DIVIDE, + SqlStdOperatorTable.FLOOR, rx.makeCall( - SqlStdOperatorTable.MINUS, - month, - rx.makeExactLiteral(BigDecimal.ONE, bigintType)), - rx.makeExactLiteral(BigDecimal.valueOf(3), bigintType)); + SqlStdOperatorTable.DIVIDE, + rx.makeCall( + SqlStdOperatorTable.MINUS, + month, + rx.makeExactLiteral(BigDecimal.ONE, bigintType)), + rx.makeExactLiteral(BigDecimal.valueOf(3), bigintType))); Suggestion importance[1-10]: 3 __ Why: The suggestion to add explicit FLOOR is unnecessary because integer division in SQL already truncates toward zero, which produces the correct quarter calculation for all months. The existing code is correct and adding FLOOR would be redundant without changing behavior.	Low

Suggestions up to commit 83d18be

Category	Suggestion	Impact
General	Cast span to DOUBLE for consistent division The baseOffset calculation divides refDiffDouble (a DOUBLE) by spanLit (a BIGINT literal). Calcite's type coercion may produce unexpected results when mixing DOUBLE and BIGINT in division. Cast spanLit to DOUBLE before the division to ensure consistent floating-point arithmetic and avoid potential precision issues. core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [3942-3950] +RelDataType doubleType = rx.getTypeFactory().createSqlType(SqlTypeName.DOUBLE); +RexNode spanDouble = rx.makeCast(doubleType, spanLit, true); RexNode baseOffset = rx.makeCast( bigintType, rx.makeCall( SqlStdOperatorTable.FLOOR, - rx.makeCall(SqlStdOperatorTable.DIVIDE, refDiffDouble, spanLit)), + rx.makeCall(SqlStdOperatorTable.DIVIDE, refDiffDouble, spanDouble)), true); Suggestion importance[1-10]: 4 __ Why: The suggestion improves type consistency by explicitly casting spanLit to DOUBLE before division. While Calcite's type coercion should handle this correctly, explicit casting makes the intent clearer and avoids potential precision issues across different execution engines.	Low
	Use floor division for quarter calculation The quarter calculation uses integer division which truncates toward zero. For months 1-3 (Q1), the result is correct (0), but this truncation behavior may cause off-by-one errors in edge cases. Use FLOOR after converting to DOUBLE to ensure consistent floor division semantics, matching the pattern used elsewhere in the codebase for period calculations. core/src/main/java/org/opensearch/sql/calcite/utils/TimewrapUtils.java [104-112] +RelDataType doubleType = rx.getTypeFactory().createSqlType(SqlTypeName.DOUBLE); +RexNode monthMinusOne = rx.makeCall(SqlStdOperatorTable.MINUS, month, rx.makeExactLiteral(BigDecimal.ONE, bigintType)); +RexNode monthMinusOneDouble = rx.makeCast(doubleType, monthMinusOne, true); RexNode quarter = - rx.makeCall( - SqlStdOperatorTable.DIVIDE, + rx.makeCast( + bigintType, rx.makeCall( - SqlStdOperatorTable.MINUS, - month, - rx.makeExactLiteral(BigDecimal.ONE, bigintType)), - rx.makeExactLiteral(BigDecimal.valueOf(3), bigintType)); + SqlStdOperatorTable.FLOOR, + rx.makeCall( + SqlStdOperatorTable.DIVIDE, + monthMinusOneDouble, + rx.makeExactLiteral(BigDecimal.valueOf(3), bigintType))), + true); Suggestion importance[1-10]: 3 __ Why: The suggestion addresses a potential edge case in quarter calculation. However, for the specific case of (month-1)/3 where month is 1-12, integer division already produces correct results (0,0,0,1,1,1,2,2,2,3,3,3). The floor division approach adds complexity without fixing an actual bug in this context.	Low
	Guard against empty values list access Calling values.getFirst() without checking if values is empty will throw NoSuchElementException when the input list is empty. Although the method returns early if values.isEmpty() at the top, defensive programming suggests adding an explicit check here or moving this logic inside a conditional block to prevent future refactoring errors. core/src/main/java/org/opensearch/sql/calcite/utils/TimewrapPivot.java [85-89] long baseOffset = 0; -ExprValue boVal = values.getFirst().tupleValue().get("__base_offset__"); -if (boVal != null && !boVal.isNull()) { - baseOffset = boVal.longValue(); +if (!values.isEmpty()) { + ExprValue boVal = values.getFirst().tupleValue().get("__base_offset__"); + if (boVal != null && !boVal.isNull()) { + baseOffset = boVal.longValue(); + } } Suggestion importance[1-10]: 2 __ Why: The suggestion adds defensive programming, but the method already returns early at line 64 if values.isEmpty(), making this check redundant. The suggestion would add unnecessary code without improving safety.	Low

Suggestions up to commit b072dbf

Category	Suggestion	Impact
General	Narrow exception handling to parse exceptions The method silently swallows all exceptions (including DateTimeParseException and potentially NullPointerException if s is null). This can hide bugs where malformed timestamps are passed. Consider logging parse failures or at least narrowing the catch to DateTimeParseException to avoid masking unexpected errors. core/src/main/java/org/opensearch/sql/calcite/utils/TimewrapUtils.java [331-347] private static Long parseTimestampLiteral(UnresolvedExpression expr) { if (expr instanceof Literal lit && lit.getValue() instanceof String s) { try { java.time.LocalDateTime ldt = java.time.LocalDateTime.parse( s, java.time.format.DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss")); return ldt.toEpochSecond(java.time.ZoneOffset.UTC); - } catch (Exception e) { + } catch (java.time.format.DateTimeParseException e) { try { return java.time.Instant.parse(s).getEpochSecond(); - } catch (Exception ignored) { + } catch (java.time.format.DateTimeParseException ignored) { return null; } } } return null; } Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies overly broad exception handling that could mask unexpected errors. Narrowing to DateTimeParseException improves error visibility. However, error handling improvements typically score in the moderate range (3-8).	Low
	Add defensive check before accessing first element The code assumes values is non-empty when calling getFirst(), but the method already checks values.isEmpty() earlier and returns early. However, if the list becomes empty between the check and this line (e.g., concurrent modification), this will throw NoSuchElementException. Add a defensive check or document the invariant. core/src/main/java/org/opensearch/sql/calcite/utils/TimewrapPivot.java [86-89] -ExprValue boVal = values.getFirst().tupleValue().get("__base_offset__"); -if (boVal != null && !boVal.isNull()) { - baseOffset = boVal.longValue(); +if (!values.isEmpty()) { + ExprValue boVal = values.getFirst().tupleValue().get("__base_offset__"); + if (boVal != null && !boVal.isNull()) { + baseOffset = boVal.longValue(); + } } Suggestion importance[1-10]: 2 __ Why: The method already returns early if values.isEmpty() at line 64, making this defensive check redundant. The suggestion addresses a theoretical concurrent modification scenario that isn't applicable here, as the list is passed as a parameter and not modified concurrently.	Low

Suggestions up to commit 0fe5dde

Category	Suggestion	Impact
Possible issue	Add null check for period value The code assumes tuple.get("period") is never null and calls longValue() directly. If a row has a null period (due to upstream data issues or incomplete filtering), this will throw a NullPointerException. Add a null check before calling longValue() to prevent runtime crashes. core/src/main/java/org/opensearch/sql/calcite/utils/TimewrapPivot.java [132-136] Map<String, Map<String, ExprValue>> pivoted = new LinkedHashMap<>(); String tsColName = columns.get(tsIdx).getName(); for (ExprValue row : values) { Map<String, ExprValue> tuple = row.tupleValue(); String tsKey = tuple.get(tsColName).toString(); - long period = tuple.get("__period__").longValue(); + ExprValue periodVal = tuple.get("__period__"); + if (periodVal == null || periodVal.isNull()) { + continue; // Skip rows with null period + } + long period = periodVal.longValue(); Suggestion importance[1-10]: 7 __ Why: This is a valid error handling suggestion that prevents a potential NullPointerException when __period__ is null. However, error handling suggestions should not receive a score above 8, and the likelihood of this occurring is low given the upstream processing.	Medium
General	Verify baseOffset calculation edge case The floor division logic for baseOffset may produce incorrect results when referenceEpoch equals maxEpoch. When the difference is zero, floor(0/span) = 0, but the period labeling expects baseOffset to reflect the alignment. Verify that baseOffset correctly handles the case where referenceEpoch == maxEpoch to avoid off-by-one errors in period column names. core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [3942-3950] RexNode refDiff = rx.makeCall(SqlStdOperatorTable.MINUS, refLit, maxEpoch); RexNode refDiffDouble = rx.makeCast(doubleType, refDiff, true); +// When referenceEpoch == maxEpoch, floor(0/span) = 0, which is correct for baseOffset +// since the latest period should be labeled relative to itself (offset 0). baseOffset = rx.makeCast( bigintType, rx.makeCall( SqlStdOperatorTable.FLOOR, rx.makeCall(SqlStdOperatorTable.DIVIDE, refDiffDouble, spanLit)), true); Suggestion importance[1-10]: 3 __ Why: The suggestion asks to verify the edge case where referenceEpoch == maxEpoch, but the improved code only adds a comment without changing the logic. The floor division already handles this case correctly (floor(0/span) = 0), so this is more of a documentation improvement than a functional fix.	Low
	Clarify quarter numbering convention The quarter calculation (zdt.getMonthValue() - 1) / 3 uses integer division, which truncates toward zero. For months 1-3 this gives 0, 4-6 gives 1, etc., which is correct. However, verify that this matches the quarter numbering convention used elsewhere in the codebase (e.g., Q1=0 vs Q1=1) to avoid off-by-one errors in period alignment. core/src/main/java/org/opensearch/sql/calcite/utils/TimewrapUtils.java [263-274] public static long calendarUnitNumberFromEpoch(long epochSec, SpanUnit unit, int spanValue) { java.time.Instant instant = java.time.Instant.ofEpochSecond(epochSec); java.time.ZonedDateTime zdt = instant.atZone(java.time.ZoneOffset.UTC); long unitNum; switch (unit.getName()) { case "M" -> unitNum = zdt.getYear() * 12L + zdt.getMonthValue(); - case "q" -> unitNum = zdt.getYear() * 4L + (zdt.getMonthValue() - 1) / 3; + case "q" -> { + // Quarter numbering: Q1=0, Q2=1, Q3=2, Q4=3 (zero-based) + // Months 1-3 → (1-1)/3=0, 4-6 → (4-1)/3=1, etc. + unitNum = zdt.getYear() * 4L + (zdt.getMonthValue() - 1) / 3; + } case "y" -> unitNum = zdt.getYear(); default -> throw new SemanticCheckException("Not a variable-length unit: " + unit.getName()); } return unitNum / spanValue; } Suggestion importance[1-10]: 2 __ Why: The suggestion asks to verify the quarter numbering convention, but the improved code only adds a comment explaining the existing logic without changing it. The calculation is already correct, making this a minor documentation improvement with minimal impact.	Low

Suggestions up to commit 11061de

Category	Suggestion	Impact
General	Validate align=end has valid upper bound When align=end but extractTimestampUpperBound returns null (e.g., no WHERE clause or malformed timestamp), the code falls back to nowEpochSec. This silent fallback may produce unexpected period labels. Consider throwing a SemanticCheckException when align=end is specified but no valid upper bound is found. core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [3926-3934] long nowEpochSec = context.functionProperties.getQueryStartClock().millis() / 1000; Long referenceEpoch = null; if ("end".equals(node.getAlign())) { referenceEpoch = TimewrapUtils.extractTimestampUpperBound(node); -} -if (referenceEpoch == null) { + if (referenceEpoch == null) { + throw new SemanticCheckException("align=end requires a valid timestamp upper bound in WHERE clause"); + } +} else { referenceEpoch = nowEpochSec; } Suggestion importance[1-10]: 5 __ Why: The suggestion raises a valid point about silent fallback behavior when align=end is specified but no valid upper bound is found. However, the current fallback to nowEpochSec may be intentional for backward compatibility. The suggestion to throw an exception would change behavior and requires careful consideration of user expectations.	Low
Possible issue	Fix floor division for negative differences The floor division logic for baseOffset may produce incorrect results when refLit < maxEpoch due to FLOOR behavior with negative numbers. Verify that the division correctly handles negative differences to avoid off-by-one errors in period labeling across the latest/before/after boundary. core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [3942-3950] RexNode refDiff = rx.makeCall(SqlStdOperatorTable.MINUS, refLit, maxEpoch); RexNode refDiffDouble = rx.makeCast(doubleType, refDiff, true); +RexNode spanDouble = rx.makeCast(doubleType, spanLit, true); baseOffset = rx.makeCast( bigintType, rx.makeCall( SqlStdOperatorTable.FLOOR, - rx.makeCall(SqlStdOperatorTable.DIVIDE, refDiffDouble, spanLit)), + rx.makeCall(SqlStdOperatorTable.DIVIDE, refDiffDouble, spanDouble)), true); Suggestion importance[1-10]: 3 __ Why: The suggestion identifies a potential issue with floor division when refLit < maxEpoch, but the existing code already casts to DOUBLE before FLOOR to handle this case correctly (as noted in the comment). The suggested change to cast spanLit to DOUBLE is redundant since integer division of DOUBLE by BIGINT already produces DOUBLE in Calcite.	Low
	Guard against empty values list access Calling values.getFirst() without checking if values is empty can throw NoSuchElementException. Although the method returns early if values.isEmpty(), ensure this check occurs before accessing getFirst() to prevent potential race conditions or future refactoring errors. core/src/main/java/org/opensearch/sql/calcite/utils/TimewrapPivot.java [85-89] +if (values.isEmpty()) { + return new Result(columns, values); +} ExprValue boVal = values.getFirst().tupleValue().get("__base_offset__"); if (boVal != null && !boVal.isNull()) { baseOffset = boVal.longValue(); } Suggestion importance[1-10]: 2 __ Why: The suggestion correctly identifies that values.getFirst() could throw an exception, but the code already has an early return check for values.isEmpty() at line 63. The suggestion to add a redundant check is unnecessary and shows the reviewer missed the existing guard.	Low

[github-actions]

Persistent review updated to latest commit 6276cba

[github-actions]

Persistent review updated to latest commit e0a03f0

[github-actions]

PR Code Analyzer ❗

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

Path	Line	Severity	Description
opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java	477	low	renameTimewrapColumn is defined but never called anywhere in the diff — dead code that references SpanUnit.of(singular.toUpperCase()) with a value derived from user-controlled query input. Not directly exploitable through the parser constraints, but the unused method adds unnecessary attack surface.
opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java	333	low	ThreadLocal variables (stripNullColumns, timewrapUnitName, timewrapSeries, timewrapTimeFormat) are set during query planning in visitTimewrap but only cleaned up inside buildResultSet's try/finally. If buildResultSet is never reached on an error path, ThreadLocals persist for subsequent requests on the same pooled thread, potentially leaking query configuration state across unrelated user queries.
opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java	516	low	The series=exact branch silently falls back to series=short behavior with no warning or error (TODO comment acknowledges this). Users requesting exact date-formatted column names receive short numeric names instead, which is a silent behavioral discrepancy but not exploitable.

The table above displays the top 10 most important findings.

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

---

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

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

---

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

Thanks.

[github-actions]

Persistent review updated to latest commit 219a704

[github-actions]

Persistent review updated to latest commit 4ba32b8

[github-actions]

Persistent review updated to latest commit 11061de

[github-actions]

Persistent review updated to latest commit b072dbf

[noCharger]

Is this tracked somewhere?

[ahkcs]

Not previously due to low usage — opened a follow-up issue #5606 to track series=exact with time_format

[noCharger]

Should this be approximate or exact? What's expected for leap-year February boundary?

[ahkcs]

spanToSeconds is only used on the fixed-length path (s/m/h/d/w), where the second count is exact — no calendar variability. The variable-length units (M/q/y) don't use this method; they go through calendarUnitNumber/calendarUnitStartEpoch, which handle the Feb/leap-year boundary precisely via cumDaysBeforeMonth. I've updated spanToSeconds to throw on M/q/y so the approximate values can never be silently used, and clarified the javadoc.

[noCharger]

Narrow to catch (DateTimeParseException)

[ahkcs]

Done — narrowed both to DateTimeParseException

[noCharger]

Can we add a leak-guard test: a timewrap query followed by a non-timewrap query on the same pooled thread, asserting the second result has no base_offset/period artifacts?

[ahkcs]

Added TimewrapSignalsLeakTest in 41f00ee

[github-actions]

Persistent review updated to latest commit 83d18be

[github-actions]

Persistent review updated to latest commit 026f481

[github-actions]

Persistent review updated to latest commit 41f00ee