alinaliBQ commented on code in PR #49564:
URL: https://github.com/apache/arrow/pull/49564#discussion_r2997271343
##########
cpp/src/arrow/flight/sql/odbc/install/unix/install_odbc.sh:
##########
@@ -40,7 +40,16 @@ if [ ! -f "$ODBC_64BIT" ]; then
exit 1
fi
-USER_ODBCINST_FILE="$HOME/Library/ODBC/odbcinst.ini"
+case "$(uname)" in
+ Linux)
+ USER_ODBCINST_FILE="/etc/odbcinst.ini"
+ ;;
+ *)
+ # macOS
+ USER_ODBCINST_FILE="$HOME/Library/ODBC/odbcinst.ini"
+ ;;
+esac
+
DRIVER_NAME="Apache Arrow Flight SQL ODBC Driver"
mkdir -p "$HOME"/Library/ODBC
Review Comment:
Yes, I have moved the command for macOS only
##########
cpp/src/arrow/flight/sql/odbc/odbc_impl/system_dsn.cc:
##########
@@ -23,6 +23,29 @@
#include <boost/algorithm/string/predicate.hpp>
#include <sstream>
+#include "arrow/flight/sql/odbc/odbc_impl/encoding_utils.h"
+
+#ifdef __linux__
+// Linux driver manager uses utf16string
+# define CONVERT_UTF8_TO_SQLWCHAR_OR_RETURN(wvar, var) \
+ auto wvar##_result = arrow::util::UTF8StringToUTF16(var); \
+ if (!wvar##_result.status().ok()) { \
+ PostArrowUtilError(wvar##_result.status()); \
+ return false; \
+ } \
+ std::u16string wvar = wvar##_result.ValueOrDie();
+
+#else
+// Windows and macOS
+# define CONVERT_UTF8_TO_SQLWCHAR_OR_RETURN(wvar, var) \
+ auto wvar##_result = arrow::util::UTF8ToWideString(var); \
+ if (!wvar##_result.status().ok()) { \
+ PostArrowUtilError(wvar##_result.status()); \
+ return false; \
+ } \
+ std::wstring wvar = wvar##_result.ValueOrDie();
+#endif // __linux__
Review Comment:
Good point. I have replaced this macro with function `ConvertToSQLWCHAR`
##########
compose.yaml:
##########
@@ -496,6 +497,43 @@ services:
volumes: *ubuntu-volumes
command: *cpp-command
+ ubuntu-cpp-odbc:
+ # Arrow Flight SQL ODBC build with BUNDLED dependencies with downloaded
dependencies.
+ image: ${REPO}:${ARCH}-ubuntu-${UBUNTU}-cpp
+ build:
+ context: .
+ dockerfile: ci/docker/ubuntu-${UBUNTU}-cpp.dockerfile
+ cache_from:
+ - ${REPO}:${ARCH}-ubuntu-${UBUNTU}-cpp
+ args:
+ arch: ${ARCH}
+ base: "${ARCH}/ubuntu:${UBUNTU}"
+ clang_tools: ${CLANG_TOOLS}
+ cmake: ${CMAKE}
+ gcc: ${GCC}
+ llvm: ${LLVM}
+ shm_size: *shm-size
+ cap_add:
+ - SYS_ADMIN
+ devices:
+ - "/dev/fuse:/dev/fuse"
Review Comment:
ok, removed
##########
cpp/src/arrow/flight/sql/odbc/odbc_impl/odbc_descriptor.cc:
##########
@@ -28,6 +26,10 @@
#include "arrow/flight/sql/odbc/odbc_impl/spi/statement.h"
#include "arrow/flight/sql/odbc/odbc_impl/type_utilities.h"
+// Include ODBC headers after arrow headers to avoid conflicts
+#include <sql.h>
+#include <sqlext.h>
Review Comment:
Since `odbc_descriptor.cc` doesn't contain `BOOL`, including `sql*.h` is ok
here. In general we want to limit the usage of `odbc_includes.h` to cases where
the build would otherwise be blocked.
##########
cpp/src/arrow/flight/sql/odbc/odbc_impl/encoding_utils.h:
##########
@@ -117,4 +129,27 @@ inline std::string SqlStringToString(const unsigned char*
sql_str,
return res;
}
+
+inline std::vector<SQLWCHAR> ToSqlWCharVector(const std::wstring& ws) {
+ switch (arrow::flight::sql::odbc::GetSqlWCharSize()) {
+ case sizeof(wchar_t): {
+ return std::vector<SQLWCHAR>(ws.begin(), ws.end());
+ }
+#ifdef __linux__
+ case sizeof(char16_t): {
+ // Linux ODBC driver manager uses char16_t as SQLWCHAR
+ CONVERT_UTF8_STR(const std::string utf8s, ws);
+ CONVERT_UTF16_STR(const std::u16string utf16s, utf8s);
+ return std::vector<SQLWCHAR>(utf16s.begin(), utf16s.end());
Review Comment:
I think `wstring` and returned string input is expected to be
null-terminated, so we don't need to append a null terminator.
##########
.github/workflows/cpp_extra.yml:
##########
@@ -336,6 +336,62 @@ jobs:
cd cpp/examples/minimal_build
../minimal_build.build/arrow-example
+ odbc-linux:
+ needs: check-labels
+ name: ODBC Linux
+ runs-on: ubuntu-latest
+ if: >-
+ needs.check-labels.outputs.force == 'true' ||
+ contains(fromJSON(needs.check-labels.outputs.ci-extra-labels || '[]'),
'CI: Extra') ||
+ contains(fromJSON(needs.check-labels.outputs.ci-extra-labels || '[]'),
'CI: Extra: C++')
+ timeout-minutes: 75
+ strategy:
+ fail-fast: false
+ env:
+ ARCH: amd64
+ ARCHERY_DEBUG: 1
+ ARROW_ENABLE_TIMING_TESTS: OFF
+ DOCKER_VOLUME_PREFIX: ".docker/"
+ UBUNTU: 24.04
+ steps:
+ - name: Checkout Arrow
+ uses: actions/checkout@v6
+ with:
+ fetch-depth: 0
+ submodules: recursive
+ - name: Cache Docker Volumes
+ uses: actions/cache@v5
+ with:
+ path: .docker
+ key: ubuntu-cpp-odbc-${{ hashFiles('cpp/**') }}
+ restore-keys: ubuntu-cpp-odbc-
+ - name: Setup Python on hosted runner
+ uses: actions/setup-python@v6
+ with:
+ python-version: 3
+ - name: Setup Archery
+ run: python3 -m pip install -e dev/archery[docker]
+ - name: Execute Docker Build
+ env:
+ ARCHERY_DOCKER_USER: ${{ secrets.DOCKERHUB_USER }}
+ ARCHERY_DOCKER_PASSWORD: ${{ secrets.DOCKERHUB_TOKEN }}
+ run: |
+ # GH-40558: reduce ASLR to avoid ASAN/LSAN crashes
+ sudo sysctl -w vm.mmap_rnd_bits=28
+ source ci/scripts/util_enable_core_dumps.sh
+ archery docker run ubuntu-cpp-odbc
+ - name: Docker Push
+ if: >-
+ success() &&
+ github.event_name == 'push' &&
+ github.repository == 'apache/arrow' &&
+ github.ref_name == 'main'
+ env:
+ ARCHERY_DOCKER_USER: ${{ secrets.DOCKERHUB_USER }}
+ ARCHERY_DOCKER_PASSWORD: ${{ secrets.DOCKERHUB_TOKEN }}
+ continue-on-error: true
+ run: archery docker push ubuntu-cpp-odbc
Review Comment:
Hi kou, I used `ubuntu-cpp-odbc` for running a custom `commands` section,
since for ODBC we need to run the ODBC registration script before executing
tests.
In order to use `ubuntu-cpp`, I tried code like `archery docker run
ubuntu-cpp bash -c "<custom_commands>"` to replace the `commands` section and
that didn't work, so I created a new image instead. Is there a workaround for
this?
##########
cpp/src/arrow/flight/sql/odbc/odbc_impl/encoding_utils.h:
##########
@@ -19,17 +19,29 @@
#include "arrow/flight/sql/odbc/odbc_impl/encoding.h"
#include "arrow/flight/sql/odbc/odbc_impl/platform.h"
-#include <sql.h>
-#include <sqlext.h>
+#include "arrow/flight/sql/odbc/odbc_impl/odbc_includes.h"
+
#include <algorithm>
#include <codecvt>
#include <cstring>
#include <locale>
#include <memory>
#include <string>
+// Include fwd.h headers after ODBC headers
+#include "arrow/flight/sql/odbc/odbc_impl/util.h"
+
#define _SILENCE_CXX17_CODECVT_HEADER_DEPRECATION_WARNING
+#ifdef __linux__
+# define GET_SQWCHAR_PTR(wstring_var)
(ODBC::ToSqlWCharVector(wstring_var).data())
+#else
+// Windows and macOS
+# define GET_SQWCHAR_PTR(wstring_var) (wstring_var.c_str())
+#endif
+
+#define ODBC_INI
reinterpret_cast<LPCWSTR>(GET_SQWCHAR_PTR(std::wstring(L"ODBC.INI")))
Review Comment:
I have removed `GET_SQWCHAR_PTR`
##########
cpp/src/arrow/flight/sql/odbc/odbc_impl/CMakeLists.txt:
##########
@@ -130,20 +130,26 @@ if(WIN32)
win_system_dsn.cc)
endif()
-if(APPLE)
+if(WIN32)
+ find_package(ODBC REQUIRED)
+ target_include_directories(arrow_odbc_spi_impl PUBLIC ${ODBC_INCLUDE_DIR})
+ target_link_libraries(arrow_odbc_spi_impl
+ PUBLIC arrow_flight_sql_shared arrow_compute_shared
Boost::locale
+ ${ODBCINST})
+else()
+ # Unix
target_include_directories(arrow_odbc_spi_impl SYSTEM BEFORE PUBLIC
${ODBC_INCLUDE_DIR})
target_link_libraries(arrow_odbc_spi_impl
PUBLIC arrow_flight_sql_static
arrow_compute_static
Boost::locale
Boost::headers
RapidJSON)
-else()
- find_package(ODBC REQUIRED)
- target_include_directories(arrow_odbc_spi_impl PUBLIC ${ODBC_INCLUDE_DIR})
- target_link_libraries(arrow_odbc_spi_impl
- PUBLIC arrow_flight_sql_shared arrow_compute_shared
Boost::locale
- ${ODBCINST})
+
+ if(NOT APPLE)
+ # Explicitly link to unix-odbc on Linux
+ target_link_libraries(arrow_odbc_spi_impl PUBLIC ${ODBCINST})
+ endif()
Review Comment:
`find_package(ODBC REQUIRED)` is only called on Windows before this PR
##########
cpp/src/arrow/flight/sql/odbc/odbc_impl/attribute_utils.h:
##########
@@ -158,11 +158,21 @@ inline void SetAttributeSQLWCHAR(SQLPOINTER new_value,
SQLINTEGER input_length_i
thread_local std::vector<uint8_t> utf8_str;
if (input_length_in_bytes == SQL_NTS) {
arrow::flight::sql::odbc::WcsToUtf8(new_value, &utf8_str);
+ } else if (input_length_in_bytes <= 0) {
+ // empty string
+ attribute_to_write.clear();
+ return;
} else {
arrow::flight::sql::odbc::WcsToUtf8(
new_value, input_length_in_bytes /
arrow::flight::sql::odbc::GetSqlWCharSize(),
&utf8_str);
}
+
+ // add null-terminator
+ if (utf8_str.back() != '\0') {
+ utf8_str.push_back('\0');
+ }
Review Comment:
`utf8_str` empty case is already handled in the `else if
(input_length_in_bytes <= 0)` check. If the string is empty, the function will
exit before it reaches `utf8_str.back() != '\0'` check.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
