feat(c/driver/postgresql): Interval support #908
Conversation
| NANOARROW_RETURN_NOT_OK(ArrowBufferAppend(data_, &days, sizeof(int32_t))); | ||
| NANOARROW_RETURN_NOT_OK(ArrowBufferAppend(data_, &months, sizeof(int32_t))); |
There was a problem hiding this comment.
Is the order of these switched?
There was a problem hiding this comment.
I don't think so. Not sure the definitive place to look in arrow but modeled this after https://github.com/apache/arrow/blob/fb7fb0db60aac7e48f6434b48aa23ada5c4885a2/cpp/src/arrow/scalar.cc#L84
There was a problem hiding this comment.
so it should go months, then days
There was a problem hiding this comment.
(The prose docs were never updated to account for this, because I suppose we don't document the types, only the 'layouts'...)
There was a problem hiding this comment.
Cool thanks for the reference. Is what I linked a bug in arrow?
There was a problem hiding this comment.
I don't think so, it's merely misleading.
Unwrapping a bunch of layers of type references: https://github.com/apache/arrow/blob/fb7fb0db60aac7e48f6434b48aa23ada5c4885a2/cpp/src/arrow/type.h#L1529-L1536
The code defines it as months-days-nanos, just hashes them in a different order for some reason (alphabetically?)
| @@ -426,6 +430,23 @@ struct BindStream { | |||
| std::memcpy(param_values[col], &value, sizeof(int64_t)); | |||
| break; | |||
| } | |||
| case ArrowType::NANOARROW_TYPE_INTERVAL_MONTH_DAY_NANO: { | |||
| const auto buf = | |||
There was a problem hiding this comment.
One thing that is missing from https://github.com/apache/arrow-nanoarrow/pull/258/files is a ArrowArrayViewGetIntervalUnsafe function, which would help both here and in the test
| const int64_t time = ReadUnsafe<int64_t>(data) * 1000; | ||
| const int64_t time_usec = ReadUnsafe<int64_t>(data); | ||
|
|
||
| if ((time_usec > INT64_MAX / 1000) | (time_usec < INT64_MIN / 1000)) { |
There was a problem hiding this comment.
| if ((time_usec > INT64_MAX / 1000) | (time_usec < INT64_MIN / 1000)) { | |
| if ((time_usec > INT64_MAX / 1000) || (time_usec < INT64_MIN / 1000)) { |
There was a problem hiding this comment.
FWIW, Arrow vendors https://github.com/nemequ/portable-snippets/tree/master/safe-math for overflow-safe helpers (we can do that as a followup though)
There was a problem hiding this comment.
Is your idea to also vendor that in this repo? Or something we should do in nanoarrow?
|
Don't think the R failures are related? |
|
Yeah, we can ignore that for now |
alternate to #907 still needs some work to get the test templates working