github-actions[bot] commented on code in PR #65679:
URL: https://github.com/apache/doris/pull/65679#discussion_r3656077766
##########
fe/fe-core/src/main/java/org/apache/doris/analysis/AnalyticWindow.java:
##########
@@ -139,7 +141,9 @@ public TAnalyticWindowBoundary toThrift(Type windowType) {
TAnalyticWindowBoundary result = new
TAnalyticWindowBoundary(type.toThrift());
if (type.isOffset() && windowType == Type.ROWS) {
- result.setRowsOffsetValue(offsetValue.longValue());
+
Preconditions.checkState(offsetValue.compareTo(MAX_ROWS_OFFSET_VALUE) <= 0,
Review Comment:
[P1] Keep admitted FOLLOWING offsets from overflowing BE frame positions.
This check still accepts `Long.MAX_VALUE`, and `visitLead` turns that into
an `UNBOUNDED PRECEDING .. 9223372036854775807 FOLLOWING` frame. BE then
computes `_current_row_position + _rows_end_offset + 1`; row zero overflows
immediately for `Long.MAX_VALUE`, and other near-limit offsets overflow as the
current row advances. The added regression file already contains this
UBSAN/invalid-index crash and comments out the exact LEAD/LAG boundary cases,
so the active tests cannot catch it.
Please make the BE frame arithmetic overflow-safe (treat a target beyond the
partition as the default/empty range), or otherwise ensure every FE-admitted
value is safe, then activate end-to-end accepted-boundary coverage. Exact
Thrift serialization alone still leaves a user query able to crash BE.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/window/Lag.java:
##########
@@ -92,6 +92,14 @@ public Lag withChildren(List<Expression> children) {
return new Lag(getFunctionParams(children));
}
+ @Override
+ public void checkLegalityBeforeTypeCoercion() {
+ if (!getArgument(1).getDataType().isIntegralType()) {
Review Comment:
[P2] Reject over-int64 LEAD/LAG offsets before lossy coercion.
An offset such as `9223372036854775808` is a `LargeIntLiteral`, so it passes
this integral-type check. The function signature then casts it to BIGINT; under
the default non-strict cast mode that overflow folds to typed `NULL`, which the
later constant/positivity check accepts. Because LEAD/LAG synthesize their
frame after `checkWindowBeforeFunc()`, the new ROWS upper-bound check never
sees it, and physical translation eventually attempts `new BigDecimal("null")`
instead of returning the advertised `must not exceed 9223372036854775807`
analysis error.
Please validate signed-int64 representability on the original offset here
(and in `Lead`) before coercion, or run generated frames through equivalent
validation, and cover both functions with an over-int64 regression.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/WindowFunctionChecker.java:
##########
@@ -211,14 +213,17 @@ private void checkFrameBoundOffset(FrameBoundary
frameBoundary) {
Preconditions.checkArgument(offset.isLiteral(), "BoundOffset of
WindowFrame must be Literal");
// case 2
- boolean isPositive = ((Literal) offset).getDouble() > 0;
+ BigDecimal offsetValue = getBoundOffsetValue(frameBoundary);
Review Comment:
[P2] Validate the ROWS boundary type before parsing its text.
The frame parser accepts any `Literal`, so a query such as `ROWS BETWEEN
'abc' PRECEDING AND CURRENT ROW` reaches this line. `new BigDecimal("abc")`
throws `NumberFormatException` before the existing integral-type check can
return `BoundOffset of ROWS WindowFrame must be an Integer`; NULL, boolean,
date, and other nonnumeric literals have the same path.
Please run the frame-unit-specific type validation before
`getBoundOffsetValue()` (or convert parse failures to the intended analysis
error), and add parser-level negative coverage for nonnumeric ROWS boundaries.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
