You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "paleolimbot (via GitHub)" <gi...@apache.org> on 2023/04/11 19:54:58 UTC

[GitHub] [arrow-adbc] paleolimbot opened a new pull request, #583: chore(r/adbcpostgresql): Add windows build support

paleolimbot opened a new pull request, #583:
URL: https://github.com/apache/arrow-adbc/pull/583

   Following the approach here: https://github.com/r-dbi/RPostgres


-- 
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] paleolimbot merged pull request #583: chore(r/adbcpostgresql): Add windows build support

Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot merged PR #583:
URL: https://github.com/apache/arrow-adbc/pull/583


-- 
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] paleolimbot commented on a diff in pull request #583: chore(r/adbcpostgresql): Add windows build support

Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #583:
URL: https://github.com/apache/arrow-adbc/pull/583#discussion_r1166815025


##########
c/driver/postgresql/util.h:
##########
@@ -40,11 +40,22 @@ namespace adbcpq {
 #define CONCAT(x, y) x##y
 #define MAKE_NAME(x, y) CONCAT(x, y)
 
-#if defined(_WIN32)
+#if defined(_WIN32) && defined(_MSC_VER)
 static inline uint32_t SwapNetworkToHost(uint32_t x) { return ntohl(x); }
 static inline uint32_t SwapHostToNetwork(uint32_t x) { return htonl(x); }
 static inline uint64_t SwapNetworkToHost(uint64_t x) { return ntohll(x); }
 static inline uint64_t SwapHostToNetwork(uint64_t x) { return htonll(x); }
+#elif defined(_WIN32)
+// e.g., msys2, where ntohll is not necessarily defined
+static inline uint32_t SwapNetworkToHost(uint32_t x) { return ntohl(x); }
+static inline uint32_t SwapHostToNetwork(uint32_t x) { return htonl(x); }
+static inline uint64_t SwapNetworkToHost(uint64_t x) {

Review Comment:
   (I take that back...I suppose it would pass now since the linux handlers would handle that)



-- 
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] paleolimbot commented on a diff in pull request #583: chore(r/adbcpostgresql): Add windows build support

Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #583:
URL: https://github.com/apache/arrow-adbc/pull/583#discussion_r1166814065


##########
c/driver/postgresql/util.h:
##########
@@ -40,11 +40,22 @@ namespace adbcpq {
 #define CONCAT(x, y) x##y
 #define MAKE_NAME(x, y) CONCAT(x, y)
 
-#if defined(_WIN32)
+#if defined(_WIN32) && defined(_MSC_VER)
 static inline uint32_t SwapNetworkToHost(uint32_t x) { return ntohl(x); }
 static inline uint32_t SwapHostToNetwork(uint32_t x) { return htonl(x); }
 static inline uint64_t SwapNetworkToHost(uint64_t x) { return ntohll(x); }
 static inline uint64_t SwapHostToNetwork(uint64_t x) { return htonll(x); }
+#elif defined(_WIN32)
+// e.g., msys2, where ntohll is not necessarily defined
+static inline uint32_t SwapNetworkToHost(uint32_t x) { return ntohl(x); }
+static inline uint32_t SwapHostToNetwork(uint32_t x) { return htonl(x); }
+static inline uint64_t SwapNetworkToHost(uint64_t x) {

Review Comment:
   There's a full suite of endian tools in flatcc (i.e., nanoarrow_ipc) too that might be worth considering if passing tests on big endian is something we want to consider.



-- 
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 #583: chore(r/adbcpostgresql): Add windows build support

Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #583:
URL: https://github.com/apache/arrow-adbc/pull/583#discussion_r1166432034


##########
c/driver/postgresql/util.h:
##########
@@ -40,11 +40,22 @@ namespace adbcpq {
 #define CONCAT(x, y) x##y
 #define MAKE_NAME(x, y) CONCAT(x, y)
 
-#if defined(_WIN32)
+#if defined(_WIN32) && defined(_MSC_VER)
 static inline uint32_t SwapNetworkToHost(uint32_t x) { return ntohl(x); }
 static inline uint32_t SwapHostToNetwork(uint32_t x) { return htonl(x); }
 static inline uint64_t SwapNetworkToHost(uint64_t x) { return ntohll(x); }
 static inline uint64_t SwapHostToNetwork(uint64_t x) { return htonll(x); }
+#elif defined(_WIN32)
+// e.g., msys2, where ntohll is not necessarily defined
+static inline uint32_t SwapNetworkToHost(uint32_t x) { return ntohl(x); }
+static inline uint32_t SwapHostToNetwork(uint32_t x) { return htonl(x); }
+static inline uint64_t SwapNetworkToHost(uint64_t x) {

Review Comment:
   Cool, it looks like s2 is Apache-licensed which simplifies things



##########
c/driver/postgresql/type.h:
##########
@@ -49,7 +49,7 @@ struct TypeMapping {
   std::unordered_map<uint32_t, PgType> type_mapping;
   // Maps standardized type names to the PostgreSQL type OID to use
   // Example: kInt8 == 20
-  std::unordered_map<PgType, uint32_t> canonical_types;
+  std::unordered_map<int32_t, uint32_t> canonical_types;

Review Comment:
   nit: can you just add a note here for why we aren't using the enum type?



-- 
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