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/04/10 18:50:52 UTC
[GitHub] [arrow-adbc] WillAyd opened a new pull request, #582: feat(c/driver/shared): created shared util library for drivers
WillAyd opened a new pull request, #582:
URL: https://github.com/apache/arrow-adbc/pull/582
Per @lidavidm comment https://github.com/apache/arrow-adbc/pull/577#discussion_r1161364187 made this act similar to the current nanoarrow setup
--
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 #582: feat(c/driver/shared): created shared util library for drivers
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #582:
URL: https://github.com/apache/arrow-adbc/pull/582#discussion_r1162672901
##########
c/cmake_modules/AdbcDefines.cmake:
##########
@@ -69,6 +69,15 @@ set_target_properties(nanoarrow
"${REPOSITORY_ROOT}/c/vendor/"
POSITION_INDEPENDENT_CODE ON)
+# Shared ADBC Driver libraries
+add_library(adbc_driver_shared STATIC EXCLUDE_FROM_ALL
Review Comment:
It's a bit confusing to name this 'driver_shared' while it's a static library (usually in Arrow libraries that convention means this is a dynamic library). Maybe `driver_common`?
--
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 #582: feat(c/driver/shared): created shared util library for drivers
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on code in PR #582:
URL: https://github.com/apache/arrow-adbc/pull/582#discussion_r1163221827
##########
c/cmake_modules/AdbcDefines.cmake:
##########
@@ -69,6 +69,15 @@ set_target_properties(nanoarrow
"${REPOSITORY_ROOT}/c/vendor/"
POSITION_INDEPENDENT_CODE ON)
+# Shared ADBC Driver libraries
+add_library(adbc_driver_shared STATIC EXCLUDE_FROM_ALL
Review Comment:
Yea that makes more sense
--
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 #582: feat(c/driver/shared): created shared util library for drivers
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #582:
URL: https://github.com/apache/arrow-adbc/pull/582#issuecomment-1504238869
Looks like R needs adjusting since it vendors the SQLite sources.
--
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 #582: refactor(c/driver/shared): created shared util library for drivers
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #582:
URL: https://github.com/apache/arrow-adbc/pull/582#issuecomment-1506100212
So in that case there may be no point consolidating.
--
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 #582: refactor(c/driver/shared): created shared util library for drivers
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm merged PR #582:
URL: https://github.com/apache/arrow-adbc/pull/582
--
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 #582: refactor(c/driver/shared): created shared util library for drivers
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on PR #582:
URL: https://github.com/apache/arrow-adbc/pull/582#issuecomment-1505640006
Trying to consolidate some of the implementations in a follow up and I noticed that StringBuilder in the postgres util implementation uses `sstream` so is dependent upon a C++ compiler. Do you think it is better to stick with that or should we make the common utils cross-compatible between the languages?
--
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 #582: refactor(c/driver/shared): created shared util library for drivers
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on PR #582:
URL: https://github.com/apache/arrow-adbc/pull/582#issuecomment-1506109958
Interesting. It would take some effort but I wonder if the common utils then shouldn't just be written in C to increase compatability.
Ignoring the C/C++ distinction there are like-named macros in both util files today with different definitions; will probably be easier to align that first and worry about things like the StringBuilder C/C++ differences later
--
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 #582: refactor(c/driver/shared): created shared util library for drivers
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #582:
URL: https://github.com/apache/arrow-adbc/pull/582#issuecomment-1506099888
The SQLite utils are explicitly in C since the SQLite driver is C.
--
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