feat(c/driver): Date32 support #948
Conversation
| @@ -1095,8 +1100,12 @@ void StatementTest::TestSqlIngestBinary() { | |||
| NANOARROW_TYPE_BINARY, {std::nullopt, "", "\x00\x01\x02\x04", "\xFE\xFF"})); | |||
| } | |||
|
|
|||
| void StatementTest::TestSqlIngestDate32() { | |||
| ASSERT_NO_FATAL_FAILURE(TestSqlIngestNumericType<int32_t>(NANOARROW_TYPE_DATE32)); | |||
There was a problem hiding this comment.
Right now this tests the postgres implementation, but the SQLite implementation doesn't get tested since the roundtrip value is a string.
For the timestamps we have separate test bodies and a function like ValidateIngestedTimestampData that the tests use to override what should be returned. We could follow that same pattern here though it is a bit heavy-handed given how this fits into TestSqlIngestNumericType pretty easily
The general problem is finding the right pattern to round trip tests for things where the input type doesn't match the output type
There was a problem hiding this comment.
What about something like this? #787
What I envision there is a set of directories, one per vendor, where each has a .sql file that creates tables and inserts data and a reference .arrow file for the expected output.
There was a problem hiding this comment.
More verbose, but then for drivers where there's overlap (e.g. Postgres in Java, Postgres in C++) we can also use them to assert that the behavior matches. The problem is checking in binaries; I was thinking we could borrow the arrow-testing submodule to do that to avoid cluttering the main repo.
There was a problem hiding this comment.
I think this makes sense. Before I go too deep - when looking at this I figured we would have to vendor nanoarrow_ipc, which in turn would require us to at flatcc. Does that make sense or am I overcomplicating?
There was a problem hiding this comment.
Yeah, and to be clear, I think there's no need to go that far in this PR - just something for later evaluation
| @@ -1037,6 +1037,10 @@ void StatementTest::TestSqlIngestNumericType(ArrowType type) { | |||
| // values. Likely a bug on our side, but for now, avoid them. | |||
| values.push_back(static_cast<CType>(-1.5)); | |||
| values.push_back(static_cast<CType>(1.5)); | |||
| } else if (type == ArrowType::NANOARROW_TYPE_DATE32) { | |||
| // Windows does not seem to support negative date values | |||
| values.push_back(static_cast<CType>(0)); | |||
There was a problem hiding this comment.
Having to use 0 as a minimum value here is unfortunate. I couldn't find any real documentation on gmtime_s limitations in Windows, except that the upper limit on 64 bit systems is through the year 3000
Guessing it may also just not support negative dates in a way that is undocumented? Unfortunately I do not have a windows machine available to test
There was a problem hiding this comment.
Ugh.
I wonder if we shouldn't also consider vendoring a library for that down the line...that would be easier if/when we also consolidate the driver codebases
| @@ -1037,6 +1037,10 @@ void StatementTest::TestSqlIngestNumericType(ArrowType type) { | |||
| // values. Likely a bug on our side, but for now, avoid them. | |||
| values.push_back(static_cast<CType>(-1.5)); | |||
| values.push_back(static_cast<CType>(1.5)); | |||
| } else if (type == ArrowType::NANOARROW_TYPE_DATE32) { | |||
| // Windows does not seem to support negative date values | |||
| values.push_back(static_cast<CType>(0)); | |||
There was a problem hiding this comment.
Ugh.
I wonder if we shouldn't also consider vendoring a library for that down the line...that would be easier if/when we also consolidate the driver codebases
|
I restarted CI, but I think the problem is probably on |
No description provided.