WillAyd commented on code in PR #861:
URL: https://github.com/apache/arrow-adbc/pull/861#discussion_r1245807375
##########
c/driver/postgresql/statement.cc:
##########
@@ -216,6 +216,16 @@ struct BindStream {
type_id = PostgresTypeId::kBytea;
param_lengths[i] = 0;
break;
+ case ArrowType::NANOARROW_TYPE_TIMESTAMP:
+ if (strcmp("", bind_schema_fields[i].timezone)) {
Review Comment:
I was a bit surprised that nanoarrow assigns an empty string to the
`timezone` member when constructing a schema with a timestamp; was expecting it
to be nullptr
##########
c/validation/adbc_validation.cc:
##########
@@ -1094,6 +1094,83 @@ void StatementTest::TestSqlIngestBinary() {
NANOARROW_TYPE_BINARY, {std::nullopt, "", "\x00\x01\x02\x04",
"\xFE\xFF"}));
}
+template <enum ArrowTimeUnit TU>
Review Comment:
The templating here might be over-engineering. I was thinking we could also
change the signature to something like:
```cpp
void StatementTest::TestSqlIngestTemporalType(
std::vector<std::optional<int64_t>>& values,
enum ArrowTimeUnit unit, const char* timezone) {
...
}
```
While nothing else is using that pattern currently, it might be nice for
gtest to do something like:
```cpp
ASSERT_NO_FATAL_FAILURE(TestSqlIngestTemporalType(
{std::nullopt, 0, 42}, NANOARROW_TIME_UNIT_SECOND, nullptr
));
EXPECT_FATAL_FAILURE(TestSqlIngestTemporalType(
{std::nullopt, INT64_MIN, INT64_MAX}, NANOARROW_TIME_UNIT_SECOND,
nullptr),
"overflow")
);
EXPECT_FATAL_FAILURE(TestSqlIngestTemporalType(
{std::nullopt, 0, 42}, NANOARROW_TIME_UNIT_SECOND, "America/Los
Angeles"),
"not implemented")
);
```
(N.B. I have no experience with EXPECT_FATAL_FAILURE)
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
