github-actions[bot] commented on code in PR #69576:
URL: https://github.com/apache/doris/pull/69576#discussion_r4367399241
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/util/ExpressionUtils.java:
##########
@@ -136,6 +149,39 @@ public class ExpressionUtils {
// folding the conjunct). We restrict MAX_MARK_SLOT_COUNT to 4 to bound
this cost.
private static final int MAX_MARK_SLOT_COUNT = 4;
+ /**
+ * Date arithmetic can throw for values outside its valid range. Keep it
above a lower
+ * condition, join, limit, rejecting set operation, generate, or CTE
consumer so rejected
+ * rows are not evaluated early.
+ * A date add/subtract with literal zero is an identity and remains safe
to push through.
+ */
+ public static boolean requiresDateArithmeticProtection(Expression
expression, Plan input) {
+ return expression.anyMatch(expr -> isUnsafeDateArithmetic((Expression)
expr))
+ && input.anyMatch(plan -> plan instanceof Filter || plan
instanceof Join || plan instanceof Limit
+ || plan instanceof LogicalExcept || plan instanceof
PhysicalExcept
+ || plan instanceof LogicalIntersect || plan instanceof
PhysicalIntersect
+ || plan instanceof Generate || plan instanceof
LogicalCTEConsumer
+ || plan instanceof PhysicalCTEConsumer);
+ }
+
+ private static boolean isUnsafeDateArithmetic(Expression expression) {
+ if (expression instanceof DateCeilFloorMonotonic) {
+ return true;
+ }
+ if (!(expression instanceof DateAddSubMonotonic)) {
Review Comment:
[P2] Include interval syntax in date-arithmetic protection
`dt + INTERVAL 5 WEEK` remains a `TimestampArithmetic` after
`ExpressionAnalyzer.visitTimestampArithmetic` and
`TypeCoercionUtils.processTimestampArithmetic`; it implements neither monotonic
interface, so this check classifies it as safe although translation calls the
same `weeks_add` backend function. For example, replace `WEEKS_ADD(dt,5)` with
`dt + INTERVAL 5 WEEK` in the new EXCEPT regression:
```text
Filter(d = '2020-02-05')
Project(CAST(dt + INTERVAL 5 WEEK AS DATETIME(0)) AS d)
Except(left: {max_datetime, 2020-01-01}, right: {max_datetime})
```
The max datetime is `9999-12-31 23:59:59.000001`, and EXCEPT should remove
it before arithmetic. Instead, the helper returns false, the project rule
expands `d`, and `PushDownFilterThroughSetOperation` installs the date
predicate on both input scans. Evaluating the right-hand max date alone then
throws before EXCEPT can reject it. The equivalent scalar spelling is protected
by this PR, but standard interval addition/subtraction still encounters the
reported failure. Include `TimestampArithmetic` in the safety check (or
normalize it first) and add interval-syntax coverage.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/PushDownFilterThroughProject.java:
##########
@@ -101,7 +101,7 @@ private static Plan pushDownFilterThroughLimitProject(
Map<Slot, Expression> childAlias = project.getAliasToProducer();
// split the conjuncts by child's output
Pair<Set<Expression>, Set<Expression>> splitConjuncts =
- splitConjunctsByChildOutput(filter.getConjuncts(),
childOutputs, childAlias);
+ splitConjunctsByChildOutput(filter.getConjuncts(),
childOutputs, childAlias, limit);
Review Comment:
[P2] Preserve lifting date evaluation above OFFSET
Passing the enclosing `limit` makes the new guard reject every nonzero date
alias, disabling the compensating rewrite in
`pushDownFilterThroughLimitProject`. With a physical table `t(dt DATETIME(6))`
containing only `9999-12-31 23:59:59.000001`, this query should return no rows:
```sql
SELECT d FROM (
SELECT CAST(WEEKS_ADD(dt,5) AS DATETIME(0)) AS d
FROM (SELECT dt FROM t LIMIT 1 OFFSET 1) limited
) s WHERE d='2020-02-05';
```
`PushDownProjectThroughLimit` creates the reduced tree `Filter(d=...) ->
Limit(1 OFFSET 1) -> Project(date AS d) -> Scan`. Previously this special rule
restored `Project(date) -> Filter(date=...) -> Limit -> Scan`, keeping
evaluation after OFFSET. Now the guard leaves the former tree: `SplitLimit`
retains up to two scan rows locally, the translator attaches the date
projection to the scan, and BE executes it before the global Exchange skips the
only row. The query therefore overflows on an empty lower result. Preserve the
projection lift above the enclosing limit, and cover the full rewrite/execution
chain with a nonzero OFFSET test; the new isolated LIMIT 0 assertion misses
this case.
--
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]
