feat(c/driver/postgresql): Duration support #907
Conversation
c/validation/adbc_validation.h
Outdated
| @@ -274,8 +275,8 @@ class StatementTest { | |||
| template <typename CType> | |||
| void TestSqlIngestNumericType(ArrowType type); | |||
|
|
|||
| template <enum ArrowTimeUnit TU> | |||
| void TestSqlIngestTemporalType(const char* timezone); | |||
| template <enum ArrowType T> | |||
There was a problem hiding this comment.
Originally I tried to provide ArrowType and ArrowTimeUnit as template parameters, but that kept yielding errors like
/home/willayd/clones/arrow-adbc/c/validation/adbc_validation.cc:1173:114: error: macro "ASSERT_NO_FATAL_FAILURE" passed 2 arguments, but takes just 1
1173 | ASSERT_NO_FATAL_FAILURE(TestSqlIngestTemporalType<NANOARROW_TYPE_DURATION, NANOARROW_TIME_UNIT_SECOND>(nullptr));
| ^
In file included from /home/willayd/clones/arrow-adbc/c/validation/adbc_validation.h:26,
from /home/willayd/clones/arrow-adbc/c/validation/adbc_validation.cc:18:
/usr/local/include/gtest/gtest.h:2216: note: macro "ASSERT_NO_FATAL_FAILURE" defined here
2216 | #define ASSERT_NO_FATAL_FAILURE(statement) \I'm not sure if that is a bug with gtest or my lack of C++ template knowledge, but figured changing the template around like this wasn't a huge deal
There was a problem hiding this comment.
Oh for future reference, I think you just need MACRO((Template<A, B>), ...)
| int64_t value = ReadUnsafe<int64_t>(data); | ||
| NANOARROW_RETURN_NOT_OK(ArrowBufferAppend(data_, &value, sizeof(int64_t))); | ||
|
|
||
| // discard unnecessary bits |
There was a problem hiding this comment.
On second thought probably want to raise if these are non-zero
|
Not entirely sure if this makes sense to map; the Interval Arrow type seems to align more closely to postgres' interval type (though still not exactly the same). The only reason I started with duration was because it already has a schema setter in nanoarrow (via |
|
From a quick scan, yeah, it seems PostgreSQL intervals map most closely to Arrow's MonthDayNano interval |
|
Indeed, and FWIW, pandas's to_sql maps timedelta data to integers and warns about doing that: https://github.com/pandas-dev/pandas/blob/c92b3701efff16293bd110a3f06f458040b43d97/pandas/io/sql.py#L1312-L1319 I am wondering if in theory we could store duration as an interval with only microseconds set (no months/days) |
Ah, that's actually what you are doing here ;) That doesn't give a faithful roundtrip, but might be the most useful type on the postgres side? |
|
The MonthDayNano interval like in #908 should be able to round trip. Not sure of the implications for casting from timedelta to that in Python but would be a more faithful of what Postgres expects |
|
closing in favor of #908 |
|
After chatting some more with @jorisvandenbossche I think there is value in this, even though it can't be roundtripped |
|
Alright, I'll give it a review for 0.7.0 (which I do want to push out in the next two weeks, hopefully) |
c/driver/snowflake/snowflake_test.cc
Outdated
| break; | ||
| } | ||
| default: | ||
| ASSERT_TRUE(false) << "ValidateIngestedTemporalData not implemented for type " |
There was a problem hiding this comment.
I think FAIL() is meant for this
No description provided.