github-actions[bot] commented on code in PR #69577:
URL: https://github.com/apache/doris/pull/69577#discussion_r4367405473
##########
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:
[P1] Preserve the safe projection restoration above OFFSET
Passing `limit` to the guard makes every nonzero date alias fail because
`anyMatch` matches the Limit itself. This also blocks the special rewrite that
keeps its filter **above** Limit. For a regular table `t(dt DATETIME(6))`
containing only `9999-12-31 23:59:59.000001`, consider:
```text
Filter(d = '2020-02-05')
Project(CAST(WEEKS_ADD(dt,5) AS DATETIME(0)) AS d)
Limit(1, offset=100)
Scan(t)
```
This must return no rows. `PushDownProjectThroughLimit` changes it to
`Filter -> Limit -> Project -> Scan`; previously
`pushDownFilterThroughLimitProject` restored `Project -> Filter -> Limit ->
Scan`, but this guard now prevents that restoration. After `SplitLimit`, the
sender applies local limit 101 and evaluates the arithmetic projection, while
the global Exchange applies offset 100 only after receiving the projected rows
(`PhysicalPlanTranslator.visitPhysicalLimit` and `ExchangeSourceOperatorX`).
The excluded maximum date therefore throws before OFFSET can discard it. Allow
the safe special rewrite, or prevent partial date projections from being moved
under Limit, and add an end-to-end nonzero-offset case.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/util/ExpressionUtils.java:
##########
@@ -133,6 +147,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:
[P1] Classify interval arithmetic as a protected date expression
`TimestampArithmetic` implements neither marker checked here.
`LogicalPlanBuilder.visitArithmeticBinary` creates that expression for `dt +
INTERVAL 5 WEEK`; `ExpressionAnalyzer` and
`TypeCoercionUtils.processTimestampArithmetic` retain it, and
`ExpressionTranslator.visitTimestampArithmetic` sends the same throwing
`weeks_add` function to BE. Thus the new protection is bypassed for equivalent
SQL syntax:
```text
Filter(d = '2020-02-05')
Project(CAST(dt + INTERVAL 5 WEEK AS DATETIME(0)) AS d)
Except(Scan(input.dt), Scan(right.dt))
```
Use the added EXCEPT regression's data: input contains the maximum timestamp
and `2020-01-01 00:00:00.000001`, and right contains only the maximum
timestamp. EXCEPT should remove the overflowing row and return February 5. This
classifier instead returns false, so `PushDownFilterThroughProject` expands
`d`, then `PushDownFilterThroughSetOperation` evaluates the arithmetic in both
scan branches before EXCEPT rejects that row. The same overflow the PR targets
remains for interval syntax. Include `TimestampArithmetic` with the appropriate
zero-offset exemption, or normalize it to the marked scalar functions, and
cover this spelling in the regression.
--
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]
