You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/09/11 18:41:30 UTC
[GitHub] [arrow] houqp opened a new pull request #11138: ARROW-13977: [Format] clarify leap seconds and leap days for interval type
houqp opened a new pull request #11138:
URL: https://github.com/apache/arrow/pull/11138
Like other time units, we don't account for leap seconds for interval types. For MONTH_DAY_NANO, we do account for leap days.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] edponce commented on a change in pull request #11138: ARROW-13977: [Format] clarify leap seconds for interval type
Posted by GitBox <gi...@apache.org>.
edponce commented on a change in pull request #11138:
URL: https://github.com/apache/arrow/pull/11138#discussion_r712500796
##########
File path: format/Schema.fbs
##########
@@ -357,18 +357,19 @@ enum IntervalUnit: short { YEAR_MONTH, DAY_TIME, MONTH_DAY_NANO}
// days can differ in length during day light savings time transitions).
// All integers in the types below are stored in the endianness indicated
// by the schema.
+//
// YEAR_MONTH - Indicates the number of elapsed whole months, stored as
// 4-byte signed integers.
-// DAY_TIME - Indicates the number of elapsed days and milliseconds,
-// stored as 2 contiguous 32-bit integers (8-bytes in total). Support
+// DAY_TIME - Indicates the number of elapsed days and milliseconds (no leap seconds),
+// stored as 2 contiguous 32-bit integers (8-bytes in total). Support
// of this IntervalUnit is not required for full arrow compatibility.
// MONTH_DAY_NANO - A triple of the number of elapsed months, days, and nanoseconds.
-// The values are stored contiguously in 16 byte blocks. Months and
-// days are encoded as 32 bit integers and nanoseconds is encoded as a
-// 64 bit integer. All integers are signed. Each field is independent
-// (e.g. there is no constraint that nanoseconds have the same sign
-// as days or that the quantity of nanoseconds represents less
-// than a day's worth of time).
+// The values are stored contiguously in 16 byte blocks. Months and days are
+// encoded as 32 bit integers and nanoseconds is encoded as a 64 bit integer.
+// Nanoseconds does not allow for leap seconds. All integers are signed. Each
+// field is independent (e.g. there is no constraint that nanoseconds have the
+// same sign as days or that the quantity of nanoseconds represents less than
+// a day's worth of time).
Review comment:
Nit: Use dash consistently when referring to `XX-bit`.
Also, might as well state `32-bit signed integers ... 64-bit signed integer` and remove the sentence `All integers are signed`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] edponce commented on a change in pull request #11138: ARROW-13977: [Format] clarify leap seconds for interval type
Posted by GitBox <gi...@apache.org>.
edponce commented on a change in pull request #11138:
URL: https://github.com/apache/arrow/pull/11138#discussion_r712501093
##########
File path: format/Schema.fbs
##########
@@ -357,18 +357,19 @@ enum IntervalUnit: short { YEAR_MONTH, DAY_TIME, MONTH_DAY_NANO}
// days can differ in length during day light savings time transitions).
// All integers in the types below are stored in the endianness indicated
// by the schema.
+//
// YEAR_MONTH - Indicates the number of elapsed whole months, stored as
// 4-byte signed integers.
-// DAY_TIME - Indicates the number of elapsed days and milliseconds,
-// stored as 2 contiguous 32-bit integers (8-bytes in total). Support
+// DAY_TIME - Indicates the number of elapsed days and milliseconds (no leap seconds),
+// stored as 2 contiguous 32-bit integers (8-bytes in total). Support
// of this IntervalUnit is not required for full arrow compatibility.
Review comment:
Are these also `32-bit signed integers`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jorgecarleitao commented on pull request #11138: ARROW-13977: [Format] clarify leap seconds and leap days for interval type
Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on pull request #11138:
URL: https://github.com/apache/arrow/pull/11138#issuecomment-917581003
You are right 👍
I understood that comment to differentiate it from `Duration`, that denotes a physical time interval. Specifically, my understanding is that `Duration[s](1)` equals 1 second as per its definition of [atomic clocks and stuff](https://en.wikipedia.org/wiki/Second).
In opposition to an `Interval`, that corresponds time units that are constructs associated with a calendar (historically associated with the Earth and moon movements), such as "Month", "day", etc.
The main difference (in my understanding) is that `add_duration(now,duration(MS_IN_DAY)) - now = duration(MS_IN_DAY)` and `add_interval(now,interval(1d)) - now = interval(1d)`, but `duration(MS_IN_DAY) != interval(1d)` because we may cross a (calendar) offset, either due to a leap second, leap hour (daylight, which I forgot about in the comment above), leap day (February), etc.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #11138: ARROW-13977: [Format] clarify leap seconds and leap days for interval type
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #11138:
URL: https://github.com/apache/arrow/pull/11138#issuecomment-917454539
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jorgecarleitao commented on pull request #11138: ARROW-13977: [Format] clarify leap seconds and leap days for interval type
Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on pull request #11138:
URL: https://github.com/apache/arrow/pull/11138#issuecomment-918218742
Updating the summary table above with the result of the follow-up discussion,
| care about leap | Type |
|------------------|----------------------|
| No | Duration |
| hours | Interval DaysMs |
| days and hours | Interval MonthsDaysNs |
| days, hours, and seconds | Extension |
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] houqp commented on pull request #11138: ARROW-13977: [Format] clarify leap seconds and leap days for interval type
Posted by GitBox <gi...@apache.org>.
houqp commented on pull request #11138:
URL: https://github.com/apache/arrow/pull/11138#issuecomment-917571062
@jorgecarleitao even if we don't account for leap seconds, `Interval(1d,MS_IN_DAY)` is still not equal to `Interval(2d,0)` due to daylight saving adjustments. Here is the quote from the comment:
```
// A "calendar" interval which models types that don't necessarily
// have a precise duration without the context of a base timestamp (e.g.
// days can differ in length during day light savings time transitions).
```
> Since DataFusion follows postgres, I shouldn't we stick to MONTHS_DAYS_NS so that we capture its behavior of storing months, days and (nano)seconds separately,
Are you suggesting that we should use `MONTHS_DAYS_NS` everywhere in datafusion and drop day_ms entirely?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] houqp commented on pull request #11138: ARROW-13977: [Format] clarify leap seconds for interval type
Posted by GitBox <gi...@apache.org>.
houqp commented on pull request #11138:
URL: https://github.com/apache/arrow/pull/11138#issuecomment-923618693
@edponce pushed a new commit to apply your suggestions, PTAL.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jorgecarleitao commented on pull request #11138: ARROW-13977: [Format] clarify leap seconds and leap days for interval type
Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on pull request #11138:
URL: https://github.com/apache/arrow/pull/11138#issuecomment-917565327
If we do not account for leap seconds, isn't this type equivalent to `Duration[ms]`?
I.e. Given `MS_IN_DAY = 60*60*24*1000`, if `Interval(1d,MS_IN_DAY) == Interval(2d,0)`, then `Interval(a,b) == Duration[ms](a*MS_IN_DAY + b)` which makes them semantically equivalent (and thus redundant?). This is how I would conclude that `Interval(1d,MS_IN_DAY) != Interval(2d,0)`, i.e. to capture different semantics.
OTOH, [many systems do not handle leap seconds](https://github.com/chronotope/chrono/issues/198#issuecomment-354692880) and in 2012 [there were no plans to support leap seconds in Postgres](https://www.postgresql.org/message-id/23016.1327976502@sss.pgh.pa.us).
So, my conclusion atm is that the decision tree is
| care about leap | Type |
|------------------|----------------------|
| days | Interval MonthDaysNs |
| seconds | Interval DaysMs |
| days and seconds | Extension |
| None | Duration |
If we ignore leap seconds in `DaysMs`, the table would become
| care about leap | Type |
|------------------|----------------------|
| days | Interval MonthDaysNs |
| seconds | Extension |
| days and seconds | Extension |
| None | Duration |
and we may deprecate `DaysMs` without loss of generality?
-----------------
Since `DataFusion` follows postgres, I shouldn't we stick to `MONTHS_DAYS_NS` so that we capture its behavior of storing months, days and (nano)seconds separately,
```
postgres=# SELECT interval '11 months + 28 days + 86399 seconds' + interval '1 month + 1 day + 1 second';
?column?
-------------------------
1 year 29 days 24:00:00
(1 row)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] houqp commented on pull request #11138: ARROW-13977: [Format] clarify leap seconds and leap days for interval type
Posted by GitBox <gi...@apache.org>.
houqp commented on pull request #11138:
URL: https://github.com/apache/arrow/pull/11138#issuecomment-917689994
Good summary :+1: this aligns with what I have in mind as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] emkornfield commented on pull request #11138: ARROW-13977: [Format] clarify leap seconds and leap days for interval type
Posted by GitBox <gi...@apache.org>.
emkornfield commented on pull request #11138:
URL: https://github.com/apache/arrow/pull/11138#issuecomment-917694583
Could you send an e-mail out to dev@ to make sure this gets wide visibility
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jorgecarleitao commented on a change in pull request #11138: ARROW-13977: [Format] clarify leap seconds and leap days for interval type
Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on a change in pull request #11138:
URL: https://github.com/apache/arrow/pull/11138#discussion_r708018490
##########
File path: format/Schema.fbs
##########
@@ -357,18 +357,23 @@ enum IntervalUnit: short { YEAR_MONTH, DAY_TIME, MONTH_DAY_NANO}
// days can differ in length during day light savings time transitions).
// All integers in the types below are stored in the endianness indicated
// by the schema.
+//
// YEAR_MONTH - Indicates the number of elapsed whole months, stored as
// 4-byte signed integers.
-// DAY_TIME - Indicates the number of elapsed days and milliseconds,
-// stored as 2 contiguous 32-bit integers (8-bytes in total). Support
+// DAY_TIME - Indicates the number of elapsed days and milliseconds (no leap seconds),
+// stored as 2 contiguous 32-bit integers (8-bytes in total). Support
// of this IntervalUnit is not required for full arrow compatibility.
+// DAY_TIME interval doesn't have total order due to day light savings.
Review comment:
maybe removing this sentence to not discuss ordering?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] houqp closed pull request #11138: ARROW-13977: [Format] clarify leap seconds for interval type
Posted by GitBox <gi...@apache.org>.
houqp closed pull request #11138:
URL: https://github.com/apache/arrow/pull/11138
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] houqp commented on pull request #11138: ARROW-13977: [Format] clarify leap seconds and leap days for interval type
Posted by GitBox <gi...@apache.org>.
houqp commented on pull request #11138:
URL: https://github.com/apache/arrow/pull/11138#issuecomment-917455003
For the context, this came out of the exercise to define partial order for Interval Types in https://github.com/jorgecarleitao/arrow2/pull/398.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] houqp commented on pull request #11138: ARROW-13977: [Format] clarify leap seconds and leap days for interval type
Posted by GitBox <gi...@apache.org>.
houqp commented on pull request #11138:
URL: https://github.com/apache/arrow/pull/11138#issuecomment-921421024
I removed sentences that mentioned ordering and leap days. now the only change is mention of leap seconds to be consistent with other temporal types.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org