-
Notifications
You must be signed in to change notification settings - Fork 46
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
Support datetime interval extended type and arithmetic #230
Conversation
5e331d7
to
406b2ef
Compare
406b2ef
to
f5deabe
Compare
d4e0aed
to
47d9864
Compare
47d9864
to
e71a4ac
Compare
e8b1be0
to
bd9aeba
Compare
e71a4ac
to
8c92b86
Compare
bd9aeba
to
c629676
Compare
8c92b86
to
aa7302a
Compare
c629676
to
afa8107
Compare
afa8107
to
d4c577c
Compare
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.
Thank you for the patch! I don't see a critical problems, just few issues.
@skip_or_run_datetime_test | ||
@unittest.expectedFailure # See https://github.com/tarantool/tarantool/issues/7698 | ||
def test_tarantool_datetime_sub_different_timezones(self): | ||
case = self.datetime_sub_different_timezones_case | ||
|
||
self.assertSequenceEqual(self.con.call('sub', case['arg_1'], case['arg_2']), | ||
[case['res']]) |
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.
Wil the test fail on a new versions of Tarantool with fixes? It seems better to skip the test.
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.
Yes, it will. I think it's better to wait until the bug would be fixed and then replace xfail with conditional xfail based on version rather than set a skip and forget to ever run the test.
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.
Maybe we should run the test only on CI? (by a environment variable or an another way that unittest support)
On the one hand, we'll see when our tests break. On the other hand, a user will not be confused with a failed test after updating Tarantool.
It looks a bit tricky, but for me it seems better than failed by default tests in the future.
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.
On the other hand, a user will not be confused with a failed test after updating Tarantool.
I think that when one should get this fail, it would be the moment we need to replace xfail with conditional xfail based on version.
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.
We will need to add a task to a sprint, fix it, and then release a new version... It may take a long time for users to get a working version.
It's not a good idea to make CI easier for us in the moment, but make life harder for users in the future.
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.
I don't get how choosing between xfail
and skip
affects user with release versions.
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.
If you are running module tests, you are a developer. As a developer, you may either ignore a couple of failing tests after you find out what's the reason (it shouldn't be complicated since relevant issue link is provided), or report the issue to us (to be honest, I'm sure that developer would already be one of us), or fix it (it's an hour long work together with all required stuff like commit messages and PR).
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.
I would be glad to provide skip
tests with conditions, but these conditions not yet exist. Make a test that always will skip until one remembers to "uncomment it" is also doesn't seems like a good approach to me.
e3525ef
to
fb9449b
Compare
9efe2f2
to
ac7a1e3
Compare
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.
LGTM.
Tarantool supports datetime interval type since version 2.10.0 [1]. This patch introduced the support of Tarantool interval type in msgpack decoders and encoders. Tarantool datetime interval objects are decoded to `tarantool.Interval` type. `tarantool.Interval` may be encoded to Tarantool interval objects. You can create `tarantool.Interval` objects either from msgpack data or by using the same API as in Tarantool: ``` di = tarantool.Interval(year=-1, month=2, day=3, hour=4, minute=-5, sec=6, nsec=308543321, adjust=tarantool.IntervalAdjust.NONE) ``` Its attributes (same as in init API) are exposed, so you can use them if needed. datetime, numpy and pandas tools doesn't seem to be sufficient to cover all adjust cases supported by Tarantool. This patch does not yet introduce the support of datetime interval arithmetic. 1. tarantool/tarantool#5941 Part of #229
Support datetime and interval arithmetic with the same rules as in Tarantool [1]. Valid operations: - `tarantool.Datetime` + `tarantool.Interval` = `tarantool.Datetime` - `tarantool.Datetime` - `tarantool.Interval` = `tarantool.Datetime` - `tarantool.Datetime` - `tarantool.Datetime` = `tarantool.Interval` - `tarantool.Interval` + `tarantool.Interval` = `tarantool.Interval` - `tarantool.Interval` - `tarantool.Interval` = `tarantool.Interval` Since `tarantool.Interval` could contain `month` and `year` fields and such operations could be ambiguous, you can use `adjust` field to tune the logic. The behavior is the same as in Tarantool, see Interval arithmetic RFC [1]. - `tarantool.IntervalAdjust.NONE` -- only truncation toward the end of month performed (default mode). ``` >>> dt = tarantool.Datetime(year=2022, month=3, day=31) datetime: Timestamp('2022-03-31 00:00:00'), tz: "" >>> di = tarantool.Interval(month=1, adjust=tarantool.IntervalAdjust.NONE) >>> dt + di datetime: Timestamp('2022-04-30 00:00:00'), tz: "" ``` - `tarantool.IntervalAdjust.EXCESS` -- overflow mode, without any snap or truncation to the end of month, straight addition of days in month, stopping over month boundaries if there is less number of days. ``` >>> dt = tarantool.Datetime(year=2022, month=1, day=31) datetime: Timestamp('2022-01-31 00:00:00'), tz: "" >>> di = tarantool.Interval(month=1, adjust=tarantool.IntervalAdjust.EXCESS) >>> dt + di datetime: Timestamp('2022-03-02 00:00:00'), tz: "" ``` - `tarantool.IntervalAdjust.LAST` -- mode when day snaps to the end of month, if happens. ``` >>> dt = tarantool.Datetime(year=2022, month=2, day=28) datetime: Timestamp('2022-02-28 00:00:00'), tz: "" >>> di = tarantool.Interval(month=1, adjust=tarantool.IntervalAdjust.LAST) >>> dt + di datetime: Timestamp('2022-03-31 00:00:00'), tz: "" ``` Tarantool does not yet correctly support subtraction of datetime objects with different timezones [2] and addition of intervals to datetimes with non-fixed offset timezones [3]. tarantool-python implementation support them, but it could be reworked later if core team choose another solution. 1. https://github.com/tarantool/tarantool/wiki/Datetime-Internals#interval-arithmetic 2. tarantool/tarantool#7698 3. tarantool/tarantool#7700 Closes #229
ac7a1e3
to
b121ac8
Compare
msgpack: support datetime interval extended type
Tarantool supports datetime interval type since version 2.10.0 [1]. This patch introduced the support of Tarantool interval type in msgpack decoders and encoders.
Tarantool datetime interval objects are decoded to
tarantool.Interval
type.tarantool.Interval
may be encoded to Tarantool interval objects.You can create
tarantool.Interval
objects either from msgpack data or by using the same API as in Tarantool:Its attributes (same as in init API) are exposed, so you can use them if needed.
datetime, numpy and pandas tools doesn't seem to be sufficient to cover all adjust cases supported by Tarantool.
This patch does not yet introduce the support of datetime interval arithmetic.
Part of #229
msgpack: support datetime interval arithmetic
Support datetime and interval arithmetic with the same rules as in Tarantool [1].
Valid operations:
tarantool.Datetime
+tarantool.Interval
=tarantool.Datetime
tarantool.Datetime
-tarantool.Interval
=tarantool.Datetime
tarantool.Datetime
-tarantool.Datetime
=tarantool.Interval
tarantool.Interval
+tarantool.Interval
=tarantool.Interval
tarantool.Interval
-tarantool.Interval
=tarantool.Interval
Since
tarantool.Interval
could containmonth
andyear
fields and such operations could be ambiguous, you can useadjust
field to tune the logic.tarantool.IntervalAdjust.NONE
-- only truncation toward the end of month performed (default mode).tarantool.IntervalAdjust.EXCESS
-- overflow mode, without any snap or truncation to the end of month, straight addition of days in month, stopping over month boundaries if there is less number of days.tarantool.IntervalAdjust.LAST
-- mode when day snaps to the end of month, if happens.Tarantool does not yet correctly support subtraction of datetime objects with different timezones [2] and addition of intervals to datetimes with non-fixed offset timezones [3]. tarantool-python implementation support them, but it could be reworked later if core team choose another solution.
Closes #229