Skip to content

add interval#24793

Open
daviszhen wants to merge 10 commits into
matrixorigin:mainfrom
daviszhen:0601-add-interval
Open

add interval#24793
daviszhen wants to merge 10 commits into
matrixorigin:mainfrom
daviszhen:0601-add-interval

Conversation

@daviszhen
Copy link
Copy Markdown
Contributor

@daviszhen daviszhen commented Jun 2, 2026

What type of PR is this?

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

Which issue(s) this PR fixes:

issue #24486

What this PR does / why we need it:

增加interval函数

@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 →

Copy link
Copy Markdown
Contributor

@XuPeng-SH XuPeng-SH left a comment

Choose a reason for hiding this comment

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

I found two blocking issues.

  1. The binder still steals some legal INTERVAL() function calls away from the new builtin path. In this patch base_binder.go rewrites interval(...) to T_interval whenever isIntervalExprSyntax(args) sees exactly two arguments and the second one is a string literal matching an interval unit. But the builtin itself is registered to accept string arguments, so calls like interval(5, 'day') never reach the new numeric INTERVAL() implementation even though they are valid under the registered signature. This keeps the old interval-expression path for a subset of function-style calls instead of fully disambiguating the new builtin.

  2. The string conversion semantics are not MySQL-compatible yet. makeIntervalParam() uses strconv.ParseFloat, so inputs such as '', ' ', '1abc', 'abc', and '10.9abc' raise hard errors. The new BVT file even documents the intended behavior as “return values with warnings, not fail”, but the checked-in .result records errors instead. That means the implementation and coverage are both locking in behavior that diverges from MySQL for the new function.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/enhancement kind/feature size/XXL Denotes a PR that changes 2000+ lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants