You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "WillAyd (via GitHub)" <gi...@apache.org> on 2023/07/15 05:23:25 UTC
[GitHub] [arrow-adbc] WillAyd opened a new pull request, #907: feat(c/driver/postgresql): Duration support
WillAyd opened a new pull request, #907:
URL: https://github.com/apache/arrow-adbc/pull/907
(no comment)
--
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-adbc] jorisvandenbossche commented on pull request #907: feat(c/driver/postgresql): Duration support
Posted by "jorisvandenbossche (via GitHub)" <gi...@apache.org>.
jorisvandenbossche commented on PR #907:
URL: https://github.com/apache/arrow-adbc/pull/907#issuecomment-1639899711
> 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?
--
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-adbc] WillAyd commented on pull request #907: feat(c/driver/postgresql): Duration support
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on PR #907:
URL: https://github.com/apache/arrow-adbc/pull/907#issuecomment-1640138464
The MonthDayNano interval like in #258 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
--
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-adbc] WillAyd commented on pull request #907: feat(c/driver/postgresql): Duration support
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on PR #907:
URL: https://github.com/apache/arrow-adbc/pull/907#issuecomment-1655883596
closing in favor of https://github.com/apache/arrow-adbc/pull/908
--
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-adbc] WillAyd closed pull request #907: feat(c/driver/postgresql): Duration support
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd closed pull request #907: feat(c/driver/postgresql): Duration support
URL: https://github.com/apache/arrow-adbc/pull/907
--
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-adbc] jorisvandenbossche commented on pull request #907: feat(c/driver/postgresql): Duration support
Posted by "jorisvandenbossche (via GitHub)" <gi...@apache.org>.
jorisvandenbossche commented on PR #907:
URL: https://github.com/apache/arrow-adbc/pull/907#issuecomment-1639892583
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 years/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-adbc] lidavidm commented on a diff in pull request #907: feat(c/driver/postgresql): Duration support
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #907:
URL: https://github.com/apache/arrow-adbc/pull/907#discussion_r1318511490
##########
c/validation/adbc_validation.h:
##########
@@ -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>
Review Comment:
Oh for future reference, I think you just need `MACRO((Template<A, B>), ...)`
--
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-adbc] WillAyd commented on a diff in pull request #907: feat(c/driver/postgresql): Duration support
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on code in PR #907:
URL: https://github.com/apache/arrow-adbc/pull/907#discussion_r1264332716
##########
c/validation/adbc_validation.h:
##########
@@ -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>
Review Comment:
Originally I tried to provide ArrowType and ArrowTimeUnit as template parameters, but that kept yielding errors like
```sh
/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
--
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-adbc] lidavidm merged pull request #907: feat(c/driver/postgresql): Duration support
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm merged PR #907:
URL: https://github.com/apache/arrow-adbc/pull/907
--
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-adbc] lidavidm commented on pull request #907: feat(c/driver/postgresql): Duration support
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #907:
URL: https://github.com/apache/arrow-adbc/pull/907#issuecomment-1637980589
From a quick scan, yeah, it seems PostgreSQL intervals map most closely to Arrow's MonthDayNano interval
--
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-adbc] lidavidm commented on pull request #907: feat(c/driver/postgresql): Duration support
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #907:
URL: https://github.com/apache/arrow-adbc/pull/907#issuecomment-1709223111
Alright, I'll give it a review for 0.7.0 (which I do want to push out in the next two weeks, hopefully)
--
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-adbc] WillAyd commented on a diff in pull request #907: feat(c/driver/postgresql): Duration support
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on code in PR #907:
URL: https://github.com/apache/arrow-adbc/pull/907#discussion_r1264335576
##########
c/driver/postgresql/postgres_copy_reader.h:
##########
@@ -212,6 +212,32 @@ class PostgresCopyNetworkEndianFieldReader : public PostgresCopyFieldReader {
}
};
+// Reader for Durations; Similar to PostgresCopyNetworkEndianFieldReader but
+// discards an extra 64 bits per read (representing the postgres day / month)
+class PostgresIntervalReader : public PostgresCopyFieldReader {
+ public:
+ ArrowErrorCode Read(ArrowBufferView* data, int32_t field_size_bytes, ArrowArray* array,
+ ArrowError* error) override {
+ if (field_size_bytes <= 0) {
+ return ArrowArrayAppendNull(array, 1);
+ }
+
+ if (field_size_bytes != static_cast<int32_t>(sizeof(int64_t) * 2)) {
+ ArrowErrorSet(error, "Expected field with %d bytes but found field with %d bytes",
+ static_cast<int>(sizeof(int64_t)),
+ static_cast<int>(field_size_bytes)); // NOLINT(runtime/int)
+ return EINVAL;
+ }
+
+ int64_t value = ReadUnsafe<int64_t>(data);
+ NANOARROW_RETURN_NOT_OK(ArrowBufferAppend(data_, &value, sizeof(int64_t)));
+
+ // discard unnecessary bits
Review Comment:
On second though probably want to raise if these are non-zero
##########
c/driver/postgresql/postgres_copy_reader.h:
##########
@@ -212,6 +212,32 @@ class PostgresCopyNetworkEndianFieldReader : public PostgresCopyFieldReader {
}
};
+// Reader for Durations; Similar to PostgresCopyNetworkEndianFieldReader but
+// discards an extra 64 bits per read (representing the postgres day / month)
+class PostgresIntervalReader : public PostgresCopyFieldReader {
+ public:
+ ArrowErrorCode Read(ArrowBufferView* data, int32_t field_size_bytes, ArrowArray* array,
+ ArrowError* error) override {
+ if (field_size_bytes <= 0) {
+ return ArrowArrayAppendNull(array, 1);
+ }
+
+ if (field_size_bytes != static_cast<int32_t>(sizeof(int64_t) * 2)) {
+ ArrowErrorSet(error, "Expected field with %d bytes but found field with %d bytes",
+ static_cast<int>(sizeof(int64_t)),
+ static_cast<int>(field_size_bytes)); // NOLINT(runtime/int)
+ return EINVAL;
+ }
+
+ int64_t value = ReadUnsafe<int64_t>(data);
+ NANOARROW_RETURN_NOT_OK(ArrowBufferAppend(data_, &value, sizeof(int64_t)));
+
+ // discard unnecessary bits
Review Comment:
On second thought probably want to raise if these are non-zero
--
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-adbc] WillAyd commented on pull request #907: feat(c/driver/postgresql): Duration support
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on PR #907:
URL: https://github.com/apache/arrow-adbc/pull/907#issuecomment-1636816906
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 is already in nanoarrow and the pandas test suite currently maps its timedelta to duration[ns] in the sql tests. @jorisvandenbossche if you have any thoughts would love to hear
--
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-adbc] WillAyd commented on pull request #907: feat(c/driver/postgresql): Duration support
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on PR #907:
URL: https://github.com/apache/arrow-adbc/pull/907#issuecomment-1709200285
After chatting some more with @jorisvandenbossche I think there is value in this, even though it can't be roundtripped
--
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-adbc] lidavidm commented on a diff in pull request #907: feat(c/driver/postgresql): Duration support
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #907:
URL: https://github.com/apache/arrow-adbc/pull/907#discussion_r1318617798
##########
c/driver/snowflake/snowflake_test.cc:
##########
@@ -184,28 +184,37 @@ class SnowflakeStatementTest : public ::testing::Test,
}
void TestSqlIngestInterval() { GTEST_SKIP(); }
+ void TestSqlIngestDuration() { GTEST_SKIP(); }
protected:
- void ValidateIngestedTimestampData(struct ArrowArrayView* values,
- enum ArrowTimeUnit unit,
- const char* timezone) override {
- std::vector<std::optional<int64_t>> expected;
- switch (unit) {
- case NANOARROW_TIME_UNIT_SECOND:
- expected = {std::nullopt, -42, 0, 42};
- break;
- case NANOARROW_TIME_UNIT_MILLI:
- expected = {std::nullopt, -42000, 0, 42000};
- break;
- case NANOARROW_TIME_UNIT_MICRO:
- expected = {std::nullopt, -42, 0, 42};
- break;
- case NANOARROW_TIME_UNIT_NANO:
- expected = {std::nullopt, -42, 0, 42};
+ void ValidateIngestedTemporalData(struct ArrowArrayView* values, ArrowType type,
+ enum ArrowTimeUnit unit,
+ const char* timezone) override {
+ switch (type) {
+ case NANOARROW_TYPE_TIMESTAMP: {
+ std::vector<std::optional<int64_t>> expected;
+ switch (unit) {
+ case NANOARROW_TIME_UNIT_SECOND:
+ expected = {std::nullopt, -42, 0, 42};
+ break;
+ case NANOARROW_TIME_UNIT_MILLI:
+ expected = {std::nullopt, -42000, 0, 42000};
+ break;
+ case NANOARROW_TIME_UNIT_MICRO:
+ expected = {std::nullopt, -42, 0, 42};
+ break;
+ case NANOARROW_TIME_UNIT_NANO:
+ expected = {std::nullopt, -42, 0, 42};
+ break;
+ }
+ ASSERT_NO_FATAL_FAILURE(
+ adbc_validation::CompareArray<std::int64_t>(values, expected));
break;
+ }
+ default:
+ ASSERT_TRUE(false) << "ValidateIngestedTemporalData not implemented for type "
Review Comment:
I think `FAIL()` is meant for this
--
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