Skip to content

GH-46522: [C++][FlightRPC] Add Arrow Flight SQL ODBC driver #40939

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
lidavidm merged 19 commits into from
May 21, 2025
Merged

GH-46522: [C++][FlightRPC] Add Arrow Flight SQL ODBC driver #40939

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
fbb59e1
Add Arrow Flight SQL ODBC driver
jbonofre Apr 2, 2024
f795885
Add initial framework for odbc dll
alinaliBQ Mar 27, 2025
b251d50
Address James' comments
alinaliBQ Apr 10, 2025
d85771f
Fix `odbcabstraction` build errors and partially fix `flightsql_odbc`…
alinaliBQ Apr 11, 2025
4485696
Create initial odbc tests folder
rscales Apr 15, 2025
f7d52d5
Implement SQLAllocEnv
alinaliBQ Apr 14, 2025
0983cb0
Implement SQLFreeEnv
rscales Apr 24, 2025
bf48db0
Fix rest of build errors from `flightsql-odbc`
alinaliBQ May 2, 2025
0851610
Implement SQLAllocConnect and SQLFreeConnect
rscales May 5, 2025
5ec1fdd
Trim ODBC fragments
alinaliBQ May 6, 2025
e716a08
Re-add deleted submodule parquet-testing
alinaliBQ May 7, 2025
1691897
clang-format lint fix
alinaliBQ May 6, 2025
dda37df
cpplint lint fix
alinaliBQ May 6, 2025
845131d
Exclude whereami in rat exclude list
alinaliBQ May 8, 2025
31c90bf
C++/CLI lint fix
alinaliBQ May 8, 2025
faca7ad
Merge pull request #1 from Bit-Quill/fix-flightsql-odbc-err
jbonofre May 10, 2025
c788ba3
Address code review comments (#2)
alinaliBQ May 12, 2025
f435e15
Doxygen Error Fixes and Address comments (#3)
alinaliBQ May 14, 2025
141e900
Remove `whereami.cc` from build
alinaliBQ May 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Address code review comments (#2) 
* Update parquet-testing to match commit from `main`

* Address Kou's comments
  • Loading branch information
@alinaliBQ
alinaliBQ authored May 12, 2025
commit c788ba309904170fa6c2612e3709a938e61c4799
9 changes: 1 addition & 8 deletions cpp/CMakePresets.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,14 +122,6 @@
"ARROW_FLIGHT_SQL": "ON"
}
},
{
"name": "features-flight-sql-odbc",
"inherits": "features-flight-sql",
"hidden": true,
"cacheVariables": {
"ARROW_FLIGHT_SQL_ODBC": "ON"
}
},
{
"name": "features-gandiva",
"inherits": "features-basic",
Expand Down Expand Up @@ -186,6 +178,7 @@
"cacheVariables": {
"ARROW_BUILD_EXAMPLES": "ON",
"ARROW_BUILD_UTILITIES": "ON",
"ARROW_FLIGHT_SQL_ODBC": "ON",
"ARROW_SKYHOOK": "ON",
"ARROW_TENSORFLOW": "ON",
"PARQUET_BUILD_EXAMPLES": "ON",
Expand Down
13 changes: 12 additions & 1 deletion cpp/cmake_modules/ThirdpartyToolchain.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -410,7 +410,7 @@ if(ARROW_AZURE)
set(ARROW_WITH_AZURE_SDK ON)
endif()

if(ARROW_JSON)
if(ARROW_JSON OR ARROW_FLIGHT_SQL_ODBC)
set(ARROW_WITH_RAPIDJSON ON)
endif()

Expand Down Expand Up @@ -1261,6 +1261,7 @@ endif()
if(ARROW_BUILD_INTEGRATION
OR ARROW_BUILD_TESTS
OR (ARROW_FLIGHT AND (ARROW_TESTING OR ARROW_BUILD_BENCHMARKS))
OR ARROW_FLIGHT_SQL_ODBC
OR (ARROW_S3 AND ARROW_BUILD_BENCHMARKS)
OR (ARROW_TESTING AND ARROW_BUILD_SHARED))
set(ARROW_USE_BOOST TRUE)
Expand Down Expand Up @@ -5562,3 +5563,13 @@ if(ARROW_WITH_AZURE_SDK)
endif()

message(STATUS "All bundled static libraries: ${ARROW_BUNDLED_STATIC_LIBS}")

# ----------------------------------------------------------------------
# Apache Flight SQL ODBC

if(ARROW_FLIGHT_SQL_ODBC)
find_package(ODBC REQUIRED)
if(MSVC)
find_package(Boost REQUIRED COMPONENTS locale)
Copy link
Member

@kou kou May 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you do this in if(ARROW_USE_BOOST) something like the following?

diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake
index 7f97fbd6cb..b103fa3183 100644
--- a/cpp/cmake_modules/ThirdpartyToolchain.cmake
+++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake
@@ -1288,6 +1288,9 @@ if(ARROW_USE_BOOST)
   endif()
   if(ARROW_BOOST_REQUIRE_LIBRARY)
     set(ARROW_BOOST_COMPONENTS filesystem system)
+    if(ARROW_FLIGHT_SQL_ODBC AND MSVC)
+      list(APPEND ARROW_BOOST_COMPONENTS locale)
+    endif()
     set(ARROW_BOOST_OPTIONAL_COMPONENTS process)
   else()
     set(ARROW_BOOST_COMPONENTS)

Copy link
Collaborator

@alinaliBQ alinaliBQ May 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, updated the code

endif()
endif()
Copy link
Member

@kou kou May 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you move this before message(STATUS "All bundled static libraries: ${ARROW_BUNDLED_STATIC_LIBS}")?

Copy link
Collaborator

@alinaliBQ alinaliBQ May 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, moved

11 changes: 0 additions & 11 deletions cpp/src/arrow/flight/sql/odbc/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,18 +15,7 @@
# specific language governing permissions and limitations
# under the License.

cmake_minimum_required(VERSION 3.16)
set(CMAKE_CXX_STANDARD 17)

add_custom_target(arrow_flight_sql_odbc)

if(CMAKE_BUILD_TYPE STREQUAL "Release")
add_compile_definitions(NDEBUG)
endif()

# Add Boost dependencies. Should be pre-installed (Brew on Mac).
find_package(Boost REQUIRED)
find_package(ODBC REQUIRED)

add_subdirectory(flight_sql)
add_subdirectory(odbcabstraction)
42 changes: 3 additions & 39 deletions cpp/src/arrow/flight/sql/odbc/flight_sql/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,28 +15,6 @@
# specific language governing permissions and limitations
# under the License.

cmake_minimum_required(VERSION 3.16)
set(CMAKE_CXX_STANDARD 17)

set(CMAKE_POSITION_INDEPENDENT_CODE ON)

include_directories(include include/flight_sql
${CMAKE_SOURCE_DIR}/odbcabstraction/include)

if(DEFINED CMAKE_TOOLCHAIN_FILE)
include(${CMAKE_TOOLCHAIN_FILE})
endif()

find_package(RapidJSON CONFIG REQUIRED)

if(MSVC)
# the following definitions stop arrow from using __declspec when statically
# linking and will break on Windows without them
add_compile_definitions(ARROW_STATIC ARROW_FLIGHT_STATIC)
endif()

enable_testing()

add_library(arrow_odbc_spi_impl
Copy link
Member

@kou kou May 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you use our add_arrow_lib() instead?

arrow/cpp/cmake_modules/BuildUtils.cmake

Line 190 in 5989387

function(ADD_ARROW_LIB LIB_NAME)

Copy link
Collaborator

@alinaliBQ alinaliBQ May 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have an upcoming PR#46099 that uses add_arrow_lib(arrow_flight_sql_odbc) to add an arrow library at the odbc directory. The flight_sql and odbcabstraction directories are subfolders to arrow_flight_sql_odbc, so I suggest we keep add_library for these 2 subfolders.

Copy link
Member

@kou kou May 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I may misunderstand your comment... What will be focused in #46099? Will #46099 focus on replacing add_library() with add_arrow_lib()?

The flight_sql and odbcabstraction directories are subfolders to arrow_flight_sql_odbc, so I suggest we keep add_library for these 2 subfolders.

Could you explain why add_library() is better for them? (I'm not sure why "subfolders" is important here.)

include/flight_sql/flight_sql_driver.h
accessors/binary_array_accessor.cc
Expand Down Expand Up @@ -99,6 +77,9 @@ add_library(arrow_odbc_spi_impl
system_trust_store.cc
system_trust_store.h
utils.cc)
target_include_directories(arrow_odbc_spi_impl
PUBLIC include include/flight_sql
${CMAKE_SOURCE_DIR}/odbcabstraction/include)
target_include_directories(arrow_odbc_spi_impl PUBLIC ${CMAKE_CURRENT_LIST_DIR})

if(WIN32)
Expand All @@ -118,23 +99,9 @@ if(WIN32)
system_dsn.cc)
endif()

find_package(ArrowFlightSql)

target_link_libraries(arrow_odbc_spi_impl PUBLIC odbcabstraction arrow_flight_sql_shared)

if(MSVC)
set(CMAKE_CXX_FLAGS_RELEASE "/MD")
set(CMAKE_CXX_FLAGS_DEBUG "/MDd")
# else
# target_link_libraries(arrow_odbc_spi_impl PUBLIC ArrowFlightSql::arrow_flight_sql_static)
endif()

# set(ARROW_ODBC_SPI_THIRDPARTY_LIBS
# ${ARROW_LIBS} gRPC::grpc++ ${ZLIB_LIBRARIES} ${Protobuf_LIBRARIES}
# ${OPENSSL_LIBRARIES} ${RapidJSON_LIBRARIES})

if(MSVC)
find_package(Boost REQUIRED COMPONENTS locale)
target_link_libraries(arrow_odbc_spi_impl PUBLIC Boost::locale)
endif()

Expand All @@ -146,9 +113,6 @@ set_target_properties(arrow_odbc_spi_impl
RUNTIME_OUTPUT_DIRECTORY
${CMAKE_BINARY_DIR}/$<CONFIG>/lib)
Comment on lines +108 to +114
Copy link
Member

@kou kou May 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may be able to remove this by add_arrow_lib().

Copy link
Collaborator

@alinaliBQ alinaliBQ May 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #40939 (comment)


# target_include_directories(arrow_odbc_spi_impl
# PUBLIC ${CMAKE_CURRENT_SOURCE_DIR})

# CLI
add_executable(arrow_odbc_spi_impl_cli main.cc)
set_target_properties(arrow_odbc_spi_impl_cli
Expand Down
2 changes: 0 additions & 2 deletions cpp/src/arrow/flight/sql/odbc/odbcabstraction/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,6 @@
# specific language governing permissions and limitations
# under the License.

set(CMAKE_POSITION_INDEPENDENT_CODE ON)

include_directories(include)

# Ensure fmt is loaded as header only
Expand Down
2 changes: 1 addition & 1 deletion cpp/submodules/parquet-testing
Open in desktop
Copy link
Collaborator

@alinaliBQ alinaliBQ May 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to restore the submodule cpp/submodules/parquet-testing

Copy link
Member Author

@jbonofre jbonofre May 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup gonna do that.

Copy link
Member

@lidavidm lidavidm May 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is too old of a commit, can we make sure it's up to date with main?

Copy link
Collaborator

@alinaliBQ alinaliBQ May 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let me fix this

Copy link
Collaborator

@alinaliBQ alinaliBQ May 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should be resolved now

Submodule parquet-testing updated 4 files
+1 −1 bad_data/README.md
+4 −2 data/README.md
+ − data/page_v2_empty_compressed.parquet
+ − data/unknown-logical-type.parquet
2 changes: 1 addition & 1 deletion cpp/vcpkg.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@
]
},
"benchmark",
"boost-cmake",
"boost-beast",
Copy link
Member

@kou kou May 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you keep this list in alphabetical order?

Copy link
Collaborator

@alinaliBQ alinaliBQ May 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

"boost-cmake",
"boost-crc",
"boost-filesystem",
"boost-locale",
Expand Down
Loading