Skip to content

[fix](nereids) Avoid overflow when rearranging date comparisons - #70556

Open
jacktengg wants to merge 1 commit into
apache:masterfrom
jacktengg:DORIS-30242
Open

jacktengg wants to merge 1 commit into
apache:masterfrom
jacktengg:DORIS-30242

Conversation

@jacktengg

Copy link
Copy Markdown
Contributor

Background

Nereids can move date arithmetic from a comparison's left operand to its right operand. For example, days_sub(d, 1) < c becomes d < days_add(c, 1). This can expose a column predicate for pruning.

What problem does this PR solve?

Issue Number: None
Related PR: None
PR Type: Bugfix

Problem Summary:

The inverse operation can exceed the date type's range even when the original operation is valid. For example, subtracting one day from an ordinary date fails when compared with the maximum datetime. Check the inverse with FE constant evaluation before accepting the rewrite. Keep the original comparison when the inverse is not a valid date literal.

Details

  • Change map: 19 production diff lines, 78 FE test lines, 122 regression test lines, and 184 generated result lines.
  • Frequency: Comparisons whose inverse crosses a date boundary, including a boundary supplied by a column.
  • Root cause: The rule created an inverse function without checking its result. Failed FE evaluation left that function for BE execution.
  • Why existing tests missed it: Existing comparison tests used dates inside the valid range.
  • Fix: Coerce and evaluate the inverse on FE. Accept only a date literal. Reuse that literal in the rewritten comparison.
  • Other paths and branches: The shared check covers week, day, hour, minute, and second addition and subtraction. It covers legacy date types, DATEV2, DATETIMEV2, TIMESTAMPTZ, and TIMESTAMP_NS. SQL aliases use the same bound functions. Month, quarter, year, smaller units, and compound intervals do not use this inverse rule. Adjacent date-boundary inference already checks its generated constants. No release branches were changed.
  • Behavior change: Valid source arithmetic no longer fails because a generated inverse exceeds the date range. A valid constant inverse still permits the rewrite. A boundary supplied by a column retains the original expression.

Testing

All checks ran locally.

  • Before the fix, the reported SQL failed with E-218. Equivalent DATE, DATETIME(6), TIMESTAMPTZ, and TIMESTAMP_NS queries also failed.
  • bash run-fe-ut.sh --run org.apache.doris.nereids.rules.expression.rules.SimplifyArithmeticComparisonRuleTest: 4 tests passed.
  • DISABLE_BUILD_UI=ON FE_MAVEN_THREADS=4 bash build.sh --fe --output /mnt/disk2/tengjianping/doris-master2/output/date-boundary-build: passed, including Checkstyle with zero violations.
  • Generated the new result file with run-regression-test.sh --run -d nereids_rules_p0/expression -s test_date_comparison_boundary -forceGenOut.
  • Checked all 61 generated result blocks against their intended values.
  • run-regression-test.sh --run -d nereids_rules_p0 -s test_date_comparison_boundary,test_simplify_arithmetic,test_simplify_comparison_predicate,test_date_arithmetic_rejecting_boundary,test_calendar_arithmetic_prune: all 5 suites passed.
  • The reported SQL returned its original row after the FE restart. EXPLAIN retained days_sub on the column.

Notes for Reviewers

Self-review checkpoints:

  • Goal and evidence: The original error is reproduced before the fix and removed after it.
  • Scope and reuse: One shared rule changes. Existing coercion and date evaluation supply the range checks.
  • Concurrency and locks: No shared mutable state, threads, or locks are added.
  • Lifecycle and initialization: No lifecycle or static initialization behavior changes.
  • Configuration: No configuration item is added.
  • Compatibility: Function signatures and stored formats are unchanged.
  • Thrift and Protobuf: No definitions change.
  • Parallel paths: All ten date functions in the rearrangement map use the check.
  • Conditional checks: Comments explain the maximum-date failure and the unknown column boundary.
  • Test coverage: Tests cover both limits, both interval signs, six comparison operators, NULL, aliases, nested operations, and BE folding.
  • Test results: The regression suite uses one stored row. Its results are generated by the standard script and verified on a second run.
  • Observability: EXPLAIN shows the retained predicate. No new log or metric is needed.
  • Transactions and persistence: No transaction or journal behavior changes.
  • Data writes and atomicity: Production write paths are unchanged.
  • FE/BE variables: No new value crosses the FE/BE interface.
  • Performance: The check runs locally during planning and reuses its folded result. Unknown boundaries retain per-row date evaluation.
  • Other findings: No additional issue was found in the changed paths.

Release note

Fix date comparison errors caused by optimizer-generated arithmetic that exceeds the date type's range.

Check List (For Author)

  • Behavior changed: Yes (see Details)
  • Does this need documentation: No
  • Pre-submission status: Ready

AI Disclosure

  • Tool: Codex (GPT-6)
  • Generated: Code, tests, comments, and this commit description.
  • Human review: Pending.

Background

What problem does this PR solve?

Issue Number:
Related PR:
PR Type:

Problem Summary:

Details

Testing

Notes for Reviewers

Release note

Check List (For Author)

  • Behavior changed:
  • Does this need documentation:
  • Pre-submission status:

AI Disclosure

  • Tool:
  • Generated:
  • Human review:

### Background

Nereids can move date arithmetic from a comparison's left operand to its right operand.
For example, days_sub(d, 1) < c becomes d < days_add(c, 1).
This can expose a column predicate for pruning.

### What problem does this PR solve?

Issue Number: None
Related PR: None
PR Type: Bugfix

Problem Summary:

The inverse operation can exceed the date type's range even when the original operation is valid.
For example, subtracting one day from an ordinary date fails when compared with the maximum datetime.
Check the inverse with FE constant evaluation before accepting the rewrite.
Keep the original comparison when the inverse is not a valid date literal.

### Details

- **Change map:** 19 production diff lines, 78 FE test lines, 122 regression test lines, and 184 generated result lines.
- **Frequency:** Comparisons whose inverse crosses a date boundary, including a boundary supplied by a column.
- **Root cause:** The rule created an inverse function without checking its result. Failed FE evaluation left that function for BE execution.
- **Why existing tests missed it:** Existing comparison tests used dates inside the valid range.
- **Fix:** Coerce and evaluate the inverse on FE. Accept only a date literal. Reuse that literal in the rewritten comparison.
- **Other paths and branches:** The shared check covers week, day, hour, minute, and second addition and subtraction. It covers legacy date types, DATEV2, DATETIMEV2, TIMESTAMPTZ, and TIMESTAMP_NS. SQL aliases use the same bound functions. Month, quarter, year, smaller units, and compound intervals do not use this inverse rule. Adjacent date-boundary inference already checks its generated constants. No release branches were changed.
- **Behavior change:** Valid source arithmetic no longer fails because a generated inverse exceeds the date range. A valid constant inverse still permits the rewrite. A boundary supplied by a column retains the original expression.

### Testing

All checks ran locally.

- Before the fix, the reported SQL failed with E-218. Equivalent DATE, DATETIME(6), TIMESTAMPTZ, and TIMESTAMP_NS queries also failed.
- `bash run-fe-ut.sh --run org.apache.doris.nereids.rules.expression.rules.SimplifyArithmeticComparisonRuleTest`: 4 tests passed.
- `DISABLE_BUILD_UI=ON FE_MAVEN_THREADS=4 bash build.sh --fe --output /mnt/disk2/tengjianping/doris-master2/output/date-boundary-build`: passed, including Checkstyle with zero violations.
- Generated the new result file with `run-regression-test.sh --run -d nereids_rules_p0/expression -s test_date_comparison_boundary -forceGenOut`.
- Checked all 61 generated result blocks against their intended values.
- `run-regression-test.sh --run -d nereids_rules_p0 -s test_date_comparison_boundary,test_simplify_arithmetic,test_simplify_comparison_predicate,test_date_arithmetic_rejecting_boundary,test_calendar_arithmetic_prune`: all 5 suites passed.
- The reported SQL returned its original row after the FE restart. EXPLAIN retained days_sub on the column.

### Notes for Reviewers

Self-review checkpoints:

- Goal and evidence: The original error is reproduced before the fix and removed after it.
- Scope and reuse: One shared rule changes. Existing coercion and date evaluation supply the range checks.
- Concurrency and locks: No shared mutable state, threads, or locks are added.
- Lifecycle and initialization: No lifecycle or static initialization behavior changes.
- Configuration: No configuration item is added.
- Compatibility: Function signatures and stored formats are unchanged.
- Thrift and Protobuf: No definitions change.
- Parallel paths: All ten date functions in the rearrangement map use the check.
- Conditional checks: Comments explain the maximum-date failure and the unknown column boundary.
- Test coverage: Tests cover both limits, both interval signs, six comparison operators, NULL, aliases, nested operations, and BE folding.
- Test results: The regression suite uses one stored row. Its results are generated by the standard script and verified on a second run.
- Observability: EXPLAIN shows the retained predicate. No new log or metric is needed.
- Transactions and persistence: No transaction or journal behavior changes.
- Data writes and atomicity: Production write paths are unchanged.
- FE/BE variables: No new value crosses the FE/BE interface.
- Performance: The check runs locally during planning and reuses its folded result. Unknown boundaries retain per-row date evaluation.
- Other findings: No additional issue was found in the changed paths.

### Release note

Fix date comparison errors caused by optimizer-generated arithmetic that exceeds the date type's range.

### Check List (For Author)

- Behavior changed: Yes (see Details)
- Does this need documentation: No
- Pre-submission status: Ready

### AI Disclosure

- Tool: Codex (GPT-6)
- Generated: Code, tests, comments, and this commit description.
- Human review: Pending.
@jacktengg

Copy link
Copy Markdown
Contributor Author

/review

@jacktengg

Copy link
Copy Markdown
Contributor Author

run buildall

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Static review of PR #70556 at cd42918 found no actionable new findings. The review converged in 1 round: the production, test and independent boundary reviewers all returned NO_NEW_VALUABLE_FINDINGS, and the main reviewer verified their conclusions and completed the final sweep of all four changed files.

Critical checkpoint conclusions:

  • Goal and evidence: the shared rule now coerces and folds the inverse on FE, accepting only a date literal. Invalid, NULL and unprovable boundaries preserve the comparison. The added FE and SQL tests target the reported failure while requiring valid inverses to remain optimized. This conclusion is based on static inspection.
  • Scope and reuse: the production change is confined to one shared rule and reuses existing coercion and constant evaluation.
  • Concurrency and locks: new state consists of local expression references; no shared mutable state, thread or lock change.
  • Lifecycle and initialization: no new resource ownership or static initialization dependency.
  • Configuration: no new configuration; the regression's BE-fold setting is restored in finally.
  • Compatibility: existing function signatures, temporal representations and stored formats are preserved.
  • Thrift and Protobuf: no definitions or field ids change.
  • Parallel paths: all ten mapped week/day/hour/minute/second add/sub functions use the guard; numeric arithmetic is unaffected. Aliases bind to these functions. Nonreversible calendar, subsecond and compound operations are outside this rule; adjacent date inference already validates optional boundaries.
  • Conditional checks: comments explain concrete maximum-date overflow and the inability to prove column boundaries. Nested rewrites preserve only already-validated outer steps; an invalid generated inverse cannot escape to BE folding.
  • Test coverage: FE tests cover seven temporal types, five units, six operators, both signs and limits, NULL/column boundaries and timestamp precision. Regression cases add aliases, nested fallback, BE folding, valid-boundary EXPLAIN and an original source-error check. Ordering and test conventions are followed.
  • Test results: all 61 result blocks were inspected and agree with the one-row SQL semantics. Generation and execution were not independently rerun.
  • Observability: existing EXPLAIN exposes whether arithmetic is retained; no new logging or metric is needed for this local rule.
  • Transactions and persistence: no transaction, journal, failover, visible-version or delete-bitmap path changes.
  • Writes and atomicity: no production write path changes.
  • FE/BE variables: no new transmitted value; temporal range, fractional precision and timezone handling were traced through existing FE and BE code.
  • Performance: the local planning fold result is reused. Nonconstant boundaries intentionally retain per-row arithmetic to avoid generated overflow. No benchmark was run.
  • Other issues: a year-zero calendar discrepancy and source-error suppression in older valid algebraic rewrites were checked against the base and dismissed as preexisting; no distinct regression from this change was established.

No additional user focus was supplied. Existing review threads and refreshed live comments were empty, so there are no confirmed existing P0/P1 blockers to carry forward. No unresolved candidate or uncovered changed scope remains. Review limits: static inspection only; no builds, Doris tests, benchmarks or runtime validation were performed, and the author's execution claims were not independently reproduced.

Review converged after 1 round(s).

jacktengg added a commit to jacktengg/incubator-doris that referenced this pull request Nov 13, 2026
This reverts commit 3b6bca3 in preparation for cherry-picking
cd42918 from PR apache#70556.

### What problem does this PR solve?

Issue Number: N/A

Related PR: apache#70556

Problem Summary: Replace the TIMESTAMPTZ DAY-only inverse validation with
the shared date arithmetic validation from PR apache#70556. The replacement
covers all date arithmetic functions in the rearrangement map, applies
type coercion, and validates the inverse exclusively on FE. Keep the
existing DST monotonicity guard from earlier commits.

### Release note

None; this preparatory revert is followed by the broader fix.

### Check List (For Author)

- Test: Manual test; inspected the reverse patch and replacement code. Run unit and regression tests after applying the replacement.
- Behavior changed: Yes; this intermediate commit removes the narrow inverse guard before its replacement.
- Does this need documentation: No
jacktengg added a commit to jacktengg/incubator-doris that referenced this pull request Nov 13, 2026
### What problem does this PR solve?

Issue Number: N/A

Related PR: apache#70556

Problem Summary: Moving date arithmetic across a comparison can introduce
an inverse operation outside the date type's range even when the source
operation is valid. Fixed-offset TIMESTAMPTZ sessions can also have a valid
UTC cutoff whose local year is outside the supported range.

Coerce and evaluate each candidate date inverse on FE, accepting only a
DateLiteral and reusing it in the rewritten comparison. Preserve the
original comparison for invalid, NULL or nonconstant boundaries. The
shared guard covers week, day, hour, minute and second addition and
subtraction across the supported date types.

Preserve this branch's existing DST monotonicity guard. Resolve import
conflicts and explicitly use UTC in the two imported boundary unit tests
so their DAY assertions exercise inverse validation in a fixed-offset
session. No additional production changes were needed for integration.

### Release note

Prevent date comparison errors caused by optimizer-generated arithmetic
that exceeds the date type's range.

### Check List (For Author)

- Test: Unit Test / Regression test / Manual test
    - 22 FE tests passed via run-fe-ut.sh: SimplifyArithmeticComparisonRuleTest and TimestampTzLiteralTest.
    - FE build via build.sh --fe and Checkstyle passed.
    - Six regression suites passed: test_date_comparison_boundary, test_simplify_arithmetic, test_simplify_comparison_predicate, test_date_arithmetic_rejecting_boundary, test_timestamptz_day_predicate_boundary, and test_timestamptz_days_add_dst.
    - Temporarily restored the reverted boundary suite to verify all 12 original queries, then removed the temporary copies.
    - Verified the original +08:00 boundary query returns its row and retains days_add in EXPLAIN with BE constant folding enabled.
    - git diff --check passed with the generated .out trailing separator allowed.
- Behavior changed: Yes; reject unsafe date-comparison rearrangements while preserving valid inverse optimizations.
- Does this need documentation: No

(cherry picked from commit cd42918)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant