You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "alinaliBQ (via GitHub)" <gi...@apache.org> on 2025/10/01 23:49:26 UTC
[PR] [C++] Register arrow compute calls in ODBC [arrow]
alinaliBQ opened a new pull request, #47680:
URL: https://github.com/apache/arrow/pull/47680
### Rationale for this change
Need to call function to register Arrow compute calls which is needed due to https://github.com/apache/arrow/issues/25025
### What changes are included in this PR?
- Add calls to `arrow::compute::Initialize`
- Remove `RUN_ALL_TESTS` that wasn't needed; it is no longer needed after fix of GH-47434.
### Are these changes tested?
- `arrow-odbc-spi-impl-test` pass locally.
### Are there any user-facing changes?
No
--
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
Re: [PR] GH-47679: [C++] Register arrow compute calls in ODBC [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #47680:
URL: https://github.com/apache/arrow/pull/47680#discussion_r2400158923
##########
cpp/src/arrow/flight/sql/odbc/flight_sql/CMakeLists.txt:
##########
@@ -99,10 +99,12 @@ if(WIN32)
system_dsn.cc)
endif()
-target_link_libraries(arrow_odbc_spi_impl PUBLIC odbcabstraction arrow_flight_sql_shared)
+target_link_libraries(arrow_odbc_spi_impl PUBLIC odbcabstraction arrow_flight_sql_shared
+ arrow_compute_shared Boost::locale)
-if(MSVC)
- target_link_libraries(arrow_odbc_spi_impl PUBLIC Boost::locale)
+# Link libraries on MINGW64 only
Review Comment:
Is this comment correct?
It seems that the CMake configuration uses MinGW "or" Apple.
##########
cpp/src/arrow/flight/sql/odbc/flight_sql/utils_test.cc:
##########
@@ -27,6 +28,13 @@
namespace driver {
namespace flight_sql {
+class UtilTestsWithCompute : public ::testing::Test {
+ public:
+ // This must be done before using the compute kernels in order to
+ // register them to the FunctionRegistry.
+ void SetUp() override { ASSERT_OK(arrow::compute::Initialize()); }
Review Comment:
`arrow::compute::Initialize()` is called for each test, right?
Can we use `::testing::Environment` to call `arrow::compute::Initialize()` only once?
See also:
* https://google.github.io/googletest/advanced.html#global-set-up-and-tear-down
* https://google.github.io/googletest/advanced.html#global-set-up-and-tear-down
##########
cpp/src/arrow/flight/sql/odbc/flight_sql/CMakeLists.txt:
##########
@@ -99,10 +99,12 @@ if(WIN32)
system_dsn.cc)
endif()
-target_link_libraries(arrow_odbc_spi_impl PUBLIC odbcabstraction arrow_flight_sql_shared)
+target_link_libraries(arrow_odbc_spi_impl PUBLIC odbcabstraction arrow_flight_sql_shared
+ arrow_compute_shared Boost::locale)
-if(MSVC)
- target_link_libraries(arrow_odbc_spi_impl PUBLIC Boost::locale)
+# Link libraries on MINGW64 only
+if((MINGW AND CMAKE_CXX_COMPILER_ID STREQUAL "GNU") OR APPLE)
Review Comment:
Do we need the `CMAKE_CXX_COMPILER_ID STREQUAL "GNU"` part?
--
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
Re: [PR] GH-47679: [C++] Register arrow compute calls in ODBC [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #47680:
URL: https://github.com/apache/arrow/pull/47680#issuecomment-3364658018
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit a13525d230e9029c9940cca8ab45291dc0930aae.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/51866437762) has more details. It also includes information about 10 possible false positives for unstable benchmarks that are known to sometimes produce them.
--
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
Re: [PR] GH-47679: [C++] Register arrow compute calls in ODBC [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou merged PR #47680:
URL: https://github.com/apache/arrow/pull/47680
--
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
Re: [PR] GH-47679: [C++] Register arrow compute calls in ODBC [arrow]
Posted by "alinaliBQ (via GitHub)" <gi...@apache.org>.
alinaliBQ commented on code in PR #47680:
URL: https://github.com/apache/arrow/pull/47680#discussion_r2400262578
##########
cpp/src/arrow/flight/sql/odbc/flight_sql/utils_test.cc:
##########
@@ -27,6 +28,13 @@
namespace driver {
namespace flight_sql {
+class UtilTestsWithCompute : public ::testing::Test {
+ public:
+ // This must be done before using the compute kernels in order to
+ // register them to the FunctionRegistry.
+ void SetUp() override { ASSERT_OK(arrow::compute::Initialize()); }
Review Comment:
Yea it's a fair point. I have switched to use `::testing::Environment` instead
##########
cpp/src/arrow/flight/sql/odbc/flight_sql/CMakeLists.txt:
##########
@@ -99,10 +99,12 @@ if(WIN32)
system_dsn.cc)
endif()
-target_link_libraries(arrow_odbc_spi_impl PUBLIC odbcabstraction arrow_flight_sql_shared)
+target_link_libraries(arrow_odbc_spi_impl PUBLIC odbcabstraction arrow_flight_sql_shared
+ arrow_compute_shared Boost::locale)
-if(MSVC)
- target_link_libraries(arrow_odbc_spi_impl PUBLIC Boost::locale)
+# Link libraries on MINGW64 only
Review Comment:
Good catch, fixed
##########
cpp/src/arrow/flight/sql/odbc/flight_sql/CMakeLists.txt:
##########
@@ -99,10 +99,12 @@ if(WIN32)
system_dsn.cc)
endif()
-target_link_libraries(arrow_odbc_spi_impl PUBLIC odbcabstraction arrow_flight_sql_shared)
+target_link_libraries(arrow_odbc_spi_impl PUBLIC odbcabstraction arrow_flight_sql_shared
+ arrow_compute_shared Boost::locale)
-if(MSVC)
- target_link_libraries(arrow_odbc_spi_impl PUBLIC Boost::locale)
+# Link libraries on MINGW64 only
+if((MINGW AND CMAKE_CXX_COMPILER_ID STREQUAL "GNU") OR APPLE)
Review Comment:
I think it is not needed. Removed
--
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
Re: [PR] GH-47679: [C++] Register arrow compute calls in ODBC [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #47680:
URL: https://github.com/apache/arrow/pull/47680#issuecomment-3358542209
:warning: GitHub issue #47679 **has been automatically assigned in GitHub** to PR creator.
--
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
Re: [PR] GH-47679: [C++] Register arrow compute calls in ODBC [arrow]
Posted by "alinaliBQ (via GitHub)" <gi...@apache.org>.
alinaliBQ commented on PR #47680:
URL: https://github.com/apache/arrow/pull/47680#issuecomment-3362141906
@lidavidm @kou Please review this PR, it is extracted from https://github.com/apache/arrow/pull/46099. Thanks!
--
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