Skip to content

fix(plan): preserve datetime columns in timestamp comparisons - #26755

Closed
aptend wants to merge 1 commit into
matrixorigin:mainfrom
aptend:fix/datetime-timestamp-pruning
Closed

fix(plan): preserve datetime columns in timestamp comparisons#26755
aptend wants to merge 1 commit into
matrixorigin:mainfrom
aptend:fix/datetime-timestamp-pruning

Conversation

@aptend

@aptend aptend commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

issue #26741

What this PR does / why we need it:

When a DATETIME column is compared with a statement-constant TIMESTAMP expression, the binder currently casts the column to TIMESTAMP. This hides the storage column behind a cast and prevents effective object/block pruning, as seen with system.statement_info.request_at and DATE_SUB(NOW(), ...).

This change casts the statement-constant value to DATETIME when its fractional-second precision fits the column, preserving the column expression for pruning. Lossy precision conversions, volatile expressions, and column-dependent expressions keep the existing TIMESTAMP comparison path.

Validation:

  • focused binder and public planner-path tests
  • full pkg/sql/plan/... build, vet, and test
  • local EXPLAIN ANALYZE: 1 block / 778 input rows after the fix versus 11 blocks / 50,342 input rows with an explicit column-side TIMESTAMP cast

@aptend
aptend requested review from XuPeng-SH and aunjgr as code owners August 6, 2026 08:44
@qodo-code-review

Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

@XuPeng-SH XuPeng-SH 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.

Blocking correctness issue: the new cast-direction rule is not semantics-preserving in session time zones with DST folds. Casting a TIMESTAMP constant to DATETIME discards which UTC instant produced an ambiguous local wall-clock value.

Concrete current-runtime counterexample with America/New_York on 2024-11-03: let the DATETIME column be 01:30 and the TIMESTAMP constant be 2024-11-03 06:15:00 UTC, which renders as the second 01:15 occurrence. The new DATETIME-domain predicate evaluates 01:30 > 01:15 as true. The existing TIMESTAMP-domain path converts the DATETIME through Go time.Date to 2024-11-03 05:30:00 UTC, so 05:30 > 06:15 is false. Thus this optimization can return extra rows, even though scale is lossless and the expression is statement-constant.

This also conflicts with the documented MySQL comparison rule: when a TIMESTAMP/DATETIME column is compared with a constant, the constant is converted to a timestamp before comparison, rather than changing the comparison domain to DATETIME: https://dev.mysql.com/doc/refman/8.4/en/type-conversion.html

Please preserve the original timestamp-domain predicate as the semantic residual and derive a separate safe storage-pruning bound, or otherwise prove an equivalent rule across named time zones. Add an execution-level regression for a DST fold plus an ordinary fixed-offset control; the current tests assert only plan shape and cannot detect this result change. The focused planner tests pass, but they do not close this correctness invariant.

@aunjgr aunjgr 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.

Reviewed the exact head. Preserving the DATETIME column for constant TIMESTAMP comparisons is correctly limited by precision and constancy, with appropriate fallback coverage. I found no blocking issue.

@aptend

aptend commented Aug 10, 2026

Copy link
Copy Markdown
Contributor Author

Closing as superseded by #26918, which has been merged as dcef98c269. The value-side cast approach in this PR is not semantics-preserving across DST folds because converting a TIMESTAMP boundary to DATETIME can discard instant identity. #26918 fixes the original pruning issue with direct DATETIME/TIMESTAMP comparison support and conservative timezone-aware pruning, while preserving the existing public result types.

@aptend aptend closed this Aug 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/bug Something isn't working size/M Denotes a PR that changes [100,499] lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants