-
Notifications
You must be signed in to change notification settings - Fork 192
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix!: Support full precision for TSQL date types #3648
Conversation
7a0e45f
to
c788b07
Compare
4fce7f6
to
0af79e4
Compare
return (to_datetime(start), make_inclusive_end(end)) | ||
|
||
|
||
def make_inclusive_end(end: TimeLike) -> datetime: | ||
return make_exclusive(end) - timedelta(microseconds=1) | ||
return make_exclusive(end) - pd.Timedelta(1, unit="ns") | ||
|
||
|
||
def make_exclusive(time: TimeLike) -> datetime: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this type still right is datetime and pd.Timedelta a subclass?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, for example:
>>> pd.Timestamp("2020-01-01") + pd.Timedelta(1, unit="day")
Timestamp('2020-01-02 00:00:00')
>>> issubclass(pd.Timestamp, datetime.datetime)
True
sqlmesh/utils/date.py
Outdated
if is_date(time): | ||
dt = dt + timedelta(days=1) | ||
dt = dt + pd.Timedelta(1, unit="day") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does time delta no longer work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point, adding a one day timedelta
works here so no need for pandas in this
do we need a migration for this? cc @georgesittas |
There's definitely a diff between main and this PR. E.g. if I change the filter in the example incremental model to: -- instead of `@start_date` and `@end_date`
event_date BETWEEN @start_dt AND @end_dt Then I see this after running first plan, switching to this branch, wiping out cache and re-running plan:
I think an empty migration script should do the trick here. |
0af79e4
to
b13bb6c
Compare
b13bb6c
to
e11c9e6
Compare
This update aims to enhance the support for MSSQL/T-SQL full precision when building the
end_dt
macro and date ranges, specifically for date types with up to 100 nano seconds precision, fixes: #3624