alinaliBQ commented on code in PR #47517: URL: https://github.com/apache/arrow/pull/47517#discussion_r2350102236
########## cpp/src/arrow/flight/sql/odbc/odbc_api.h: ########## @@ -0,0 +1,33 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +#pragma once + +#ifdef _WIN32 +# include <windows.h> +#endif + +#include <sql.h> +#include <sqltypes.h> +#include <sqlucode.h> + +// @file odbc_api.h +// +// Define internal ODBC API function headers. +namespace arrow { Review Comment: Yup good point. Updated code ########## cpp/src/arrow/flight/sql/odbc/CMakeLists.txt: ########## @@ -15,7 +15,76 @@ # specific language governing permissions and limitations # under the License. +# Use C++ 20 for ODBC and its subdirectory +# GH-44792: Arrow will switch to C++ 20 +set(CMAKE_CXX_STANDARD 20) +set(CMAKE_CXX_STANDARD_REQUIRED ON) + add_custom_target(arrow_flight_sql_odbc) +# Ensure fmt is loaded as header only +add_compile_definitions(FMT_HEADER_ONLY) Review Comment: Sure, I changed to use `target_compile_definitions` ########## cpp/src/arrow/flight/sql/odbc/entry_points.cc: ########## @@ -0,0 +1,42 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +// platform.h includes windows.h, so it needs to be included first +#include "arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/platform.h" + +#ifdef _WIN32 +# include <windows.h> +#endif + +#include <sql.h> +#include <sqlext.h> +#include <sqltypes.h> +#include <sqlucode.h> + +#include "arrow/flight/sql/odbc/odbc_api.h" +#include "arrow/flight/sql/odbc/visibility.h" + +#include "arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/odbc_impl/odbc_connection.h" +#include "arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/odbc_impl/odbc_descriptor.h" +#include "arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/odbc_impl/odbc_environment.h" +#include "arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/odbc_impl/odbc_statement.h" + +#include "arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/logger.h" + +SQLRETURN SQL_API SQLAllocHandle(SQLSMALLINT type, SQLHANDLE parent, SQLHANDLE* result) { + return SQL_INVALID_HANDLE; +} Review Comment: Yes. In upcoming PRs, we plan to change it to look like: ``` SQLRETURN SQL_API SQLAllocHandle(SQLSMALLINT type, SQLHANDLE parent, SQLHANDLE* result) { return arrow::flight::sql::odbc::SQLAllocHandle(...); } ``` I have separate the actual implementation of `SQLAllocHandle` to make this PR smaller ########## cpp/src/arrow/flight/sql/odbc/odbc_api.h: ########## @@ -0,0 +1,33 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +#pragma once + +#ifdef _WIN32 +# include <windows.h> +#endif + +#include <sql.h> +#include <sqltypes.h> +#include <sqlucode.h> + +// @file odbc_api.h Review Comment: fixed ########## cpp/src/arrow/flight/sql/odbc/odbc_api.cc: ########## @@ -0,0 +1,45 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +// flight_sql_connection.h needs to be included first due to conflicts with windows.h +#include "arrow/flight/sql/odbc/flight_sql/flight_sql_connection.h" + +#include "arrow/flight/sql/odbc/flight_sql/include/flight_sql/config/configuration.h" +#include "arrow/flight/sql/odbc/flight_sql/include/flight_sql/flight_sql_driver.h" +#include "arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/diagnostics.h" +#include "arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/logger.h" +#include "arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/odbc_impl/attribute_utils.h" +#include "arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/odbc_impl/encoding_utils.h" +#include "arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/odbc_impl/odbc_connection.h" +#include "arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/odbc_impl/odbc_descriptor.h" +#include "arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/odbc_impl/odbc_environment.h" +#include "arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/odbc_impl/odbc_statement.h" +#include "arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/spi/connection.h" Review Comment: These includes will be used in upcoming PRs as they are needed in branch at https://github.com/apache/arrow/pull/46099, is it okay to keep these includes there? Currently most of them are not used in this PR. ########## cpp/src/arrow/flight/sql/odbc/odbc_api.cc: ########## @@ -0,0 +1,45 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +// flight_sql_connection.h needs to be included first due to conflicts with windows.h +#include "arrow/flight/sql/odbc/flight_sql/flight_sql_connection.h" + +#include "arrow/flight/sql/odbc/flight_sql/include/flight_sql/config/configuration.h" +#include "arrow/flight/sql/odbc/flight_sql/include/flight_sql/flight_sql_driver.h" +#include "arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/diagnostics.h" +#include "arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/logger.h" +#include "arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/odbc_impl/attribute_utils.h" +#include "arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/odbc_impl/encoding_utils.h" +#include "arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/odbc_impl/odbc_connection.h" +#include "arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/odbc_impl/odbc_descriptor.h" +#include "arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/odbc_impl/odbc_environment.h" +#include "arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/odbc_impl/odbc_statement.h" +#include "arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/spi/connection.h" + +// odbc_api includes windows.h, which needs to be put behind winsock2.h. +// odbc_environment.h includes winsock2.h Review Comment: `odbc_environment.h` is a header included by `odbc_api.cc`. I have adjusted the comment order to make it clearer ########## cpp/src/arrow/flight/sql/odbc/odbc_api.h: ########## @@ -0,0 +1,33 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +#pragma once + +#ifdef _WIN32 +# include <windows.h> +#endif + +#include <sql.h> +#include <sqltypes.h> +#include <sqlucode.h> + +// @file odbc_api.h +// +// Define internal ODBC API function headers. Review Comment: yes, it is internal. Renamed the file ########## cpp/src/arrow/flight/sql/odbc/CMakeLists.txt: ########## @@ -15,7 +15,76 @@ # specific language governing permissions and limitations # under the License. +# Use C++ 20 for ODBC and its subdirectory +# GH-44792: Arrow will switch to C++ 20 +set(CMAKE_CXX_STANDARD 20) +set(CMAKE_CXX_STANDARD_REQUIRED ON) + add_custom_target(arrow_flight_sql_odbc) +# Ensure fmt is loaded as header only +add_compile_definitions(FMT_HEADER_ONLY) + +if(WIN32) + if(MSVC_VERSION GREATER_EQUAL 1900) + set(ODBCINST legacy_stdio_definitions odbccp32 shlwapi) Review Comment: `legacy_stdio_definitions` is needed to resolve this error: ``` odbccp32.lib(dllload.obj) : error LNK2019: unresolved external symbol _vsnwprintf_s referenced in function StringCchPrintfW ``` I think it is because odbccp32.lib are expecting these legacy symbols ########## cpp/src/arrow/flight/sql/odbc/CMakeLists.txt: ########## @@ -15,7 +15,76 @@ # specific language governing permissions and limitations # under the License. +# Use C++ 20 for ODBC and its subdirectory +# GH-44792: Arrow will switch to C++ 20 +set(CMAKE_CXX_STANDARD 20) +set(CMAKE_CXX_STANDARD_REQUIRED ON) + add_custom_target(arrow_flight_sql_odbc) +# Ensure fmt is loaded as header only +add_compile_definitions(FMT_HEADER_ONLY) + +if(WIN32) + if(MSVC_VERSION GREATER_EQUAL 1900) + set(ODBCINST legacy_stdio_definitions odbccp32 shlwapi) + elseif(MINGW) + set(ODBCINST odbccp32 shlwapi) + endif() +elseif(APPLE) + set(ODBCINST iodbcinst) +else() + set(ODBCINST odbcinst) +endif() + +include(FetchContent) +fetchcontent_declare(spdlog + URL https://github.com/gabime/spdlog/archive/refs/tags/v1.15.3.zip + CONFIGURE_COMMAND + "" + BUILD_COMMAND + "") +fetchcontent_makeavailable(spdlog) Review Comment: Yes. Let me look into this ########## cpp/src/arrow/flight/sql/odbc/entry_points.cc: ########## @@ -0,0 +1,42 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +// platform.h includes windows.h, so it needs to be included first +#include "arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/platform.h" + +#ifdef _WIN32 +# include <windows.h> +#endif Review Comment: Yes, this is needed by the ODBC API definitions ########## cpp/src/arrow/flight/sql/odbc/odbc_api.cc: ########## @@ -0,0 +1,45 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +// flight_sql_connection.h needs to be included first due to conflicts with windows.h +#include "arrow/flight/sql/odbc/flight_sql/flight_sql_connection.h" + +#include "arrow/flight/sql/odbc/flight_sql/include/flight_sql/config/configuration.h" +#include "arrow/flight/sql/odbc/flight_sql/include/flight_sql/flight_sql_driver.h" +#include "arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/diagnostics.h" +#include "arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/logger.h" +#include "arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/odbc_impl/attribute_utils.h" +#include "arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/odbc_impl/encoding_utils.h" +#include "arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/odbc_impl/odbc_connection.h" +#include "arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/odbc_impl/odbc_descriptor.h" +#include "arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/odbc_impl/odbc_environment.h" +#include "arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/odbc_impl/odbc_statement.h" +#include "arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/spi/connection.h" + +// odbc_api includes windows.h, which needs to be put behind winsock2.h. Review Comment: fixed -- 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-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
