lidavidm commented on code in PR #40939: URL: https://github.com/apache/arrow/pull/40939#discussion_r2099211671
########## cpp/src/arrow/flight/sql/odbc/flight_sql/config/connection_string_parser.cc: ########## @@ -0,0 +1,102 @@ +// 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. + +#include "arrow/flight/sql/odbc/flight_sql/include/flight_sql/config/connection_string_parser.h" + +#include <stdint.h> +#include <iostream> +#include <string> +#include <vector> + +#include <algorithm> +#include <cstring> +#include <iterator> +#include <sstream> + +namespace driver { +namespace flight_sql { +namespace config { + +ConnectionStringParser::ConnectionStringParser(Configuration& cfg) : cfg(cfg) { + // No-op. Review Comment: Redundant comment ########## cpp/src/arrow/flight/sql/odbc/flight_sql/config/connection_string_parser.cc: ########## @@ -0,0 +1,102 @@ +// 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. + +#include "arrow/flight/sql/odbc/flight_sql/include/flight_sql/config/connection_string_parser.h" + +#include <stdint.h> +#include <iostream> +#include <string> +#include <vector> + +#include <algorithm> +#include <cstring> +#include <iterator> +#include <sstream> + +namespace driver { +namespace flight_sql { +namespace config { + +ConnectionStringParser::ConnectionStringParser(Configuration& cfg) : cfg(cfg) { + // No-op. +} + +ConnectionStringParser::~ConnectionStringParser() { + // No-op. +} + +void ConnectionStringParser::ParseConnectionString(const char* str, size_t len, + char delimiter) { + std::string connect_str(str, len); + + while (connect_str.rbegin() != connect_str.rend() && *connect_str.rbegin() == 0) + connect_str.erase(connect_str.size() - 1); Review Comment: Instead of erasing, you can use a string_view to slice ########## cpp/src/arrow/flight/sql/odbc/flight_sql/flight_sql_connection_test.cc: ########## @@ -0,0 +1,211 @@ +// 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. + +#include "arrow/flight/sql/odbc/flight_sql/flight_sql_connection.h" +#include "arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/platform.h" +#include "arrow/flight/types.h" +#include "gtest/gtest.h" + +namespace driver { +namespace flight_sql { + +using arrow::flight::Location; +using arrow::flight::TimeoutDuration; +using odbcabstraction::Connection; + +TEST(AttributeTests, SetAndGetAttribute) { + FlightSqlConnection connection(odbcabstraction::V_3); + connection.SetClosed(false); + + connection.SetAttribute(Connection::CONNECTION_TIMEOUT, static_cast<uint32_t>(200)); + const boost::optional<Connection::Attribute> firstValue = + connection.GetAttribute(Connection::CONNECTION_TIMEOUT); + + EXPECT_TRUE(firstValue); + + EXPECT_EQ(boost::get<uint32_t>(*firstValue), static_cast<uint32_t>(200)); + + connection.SetAttribute(Connection::CONNECTION_TIMEOUT, static_cast<uint32_t>(300)); + + const boost::optional<Connection::Attribute> changeValue = + connection.GetAttribute(Connection::CONNECTION_TIMEOUT); + + EXPECT_TRUE(changeValue); + EXPECT_EQ(boost::get<uint32_t>(*changeValue), static_cast<uint32_t>(300)); + + connection.Close(); +} + +TEST(AttributeTests, GetAttributeWithoutSetting) { + FlightSqlConnection connection(odbcabstraction::V_3); + + const boost::optional<Connection::Attribute> optional = + connection.GetAttribute(Connection::CONNECTION_TIMEOUT); + connection.SetClosed(false); + + EXPECT_EQ(0, boost::get<uint32_t>(*optional)); + + connection.Close(); +} + +TEST(MetadataSettingsTest, StringColumnLengthTest) { + FlightSqlConnection connection(odbcabstraction::V_3); + connection.SetClosed(false); + + const int32_t expected_string_column_length = 100000; + + const Connection::ConnPropertyMap properties = { + {FlightSqlConnection::HOST, std::string("localhost")}, // expect not used + {FlightSqlConnection::PORT, std::string("32010")}, // expect not used + {FlightSqlConnection::USE_ENCRYPTION, std::string("false")}, // expect not used + {FlightSqlConnection::STRING_COLUMN_LENGTH, + std::to_string(expected_string_column_length)}, + }; + + const boost::optional<int32_t> actual_string_column_length = + connection.GetStringColumnLength(properties); + + EXPECT_TRUE(actual_string_column_length); + EXPECT_EQ(expected_string_column_length, *actual_string_column_length); + + connection.Close(); +} + +TEST(MetadataSettingsTest, UseWideCharTest) { + FlightSqlConnection connection(odbcabstraction::V_3); + connection.SetClosed(false); + + const Connection::ConnPropertyMap properties1 = { + {FlightSqlConnection::USE_WIDE_CHAR, std::string("true")}, + }; + const Connection::ConnPropertyMap properties2 = { + {FlightSqlConnection::USE_WIDE_CHAR, std::string("false")}, + }; + + EXPECT_EQ(true, connection.GetUseWideChar(properties1)); + EXPECT_EQ(false, connection.GetUseWideChar(properties2)); + + connection.Close(); +} + +TEST(BuildLocationTests, ForTcp) { + std::vector<std::string_view> missing_attr; + Connection::ConnPropertyMap properties = { + {FlightSqlConnection::HOST, std::string("localhost")}, + {FlightSqlConnection::PORT, std::string("32010")}, + {FlightSqlConnection::USE_ENCRYPTION, std::string("false")}, + }; + + const std::shared_ptr<FlightSqlSslConfig>& ssl_config = + LoadFlightSslConfigs(properties); + + const Location& actual_location1 = + FlightSqlConnection::BuildLocation(properties, missing_attr, ssl_config); + const Location& actual_location2 = FlightSqlConnection::BuildLocation( + { + {FlightSqlConnection::HOST, std::string("localhost")}, + {FlightSqlConnection::PORT, std::string("32011")}, + }, + missing_attr, ssl_config); + + Location expected_location; + ASSERT_TRUE(Location::ForGrpcTcp("localhost", 32010).Value(&expected_location).ok()); + ASSERT_EQ(expected_location, actual_location1); + ASSERT_NE(expected_location, actual_location2); +} + +TEST(BuildLocationTests, ForTls) { + std::vector<std::string_view> missing_attr; + Connection::ConnPropertyMap properties = { + {FlightSqlConnection::HOST, std::string("localhost")}, + {FlightSqlConnection::PORT, std::string("32010")}, + {FlightSqlConnection::USE_ENCRYPTION, std::string("1")}, + }; + + const std::shared_ptr<FlightSqlSslConfig>& ssl_config = + LoadFlightSslConfigs(properties); + + const Location& actual_location1 = + FlightSqlConnection::BuildLocation(properties, missing_attr, ssl_config); + + Connection::ConnPropertyMap second_properties = { + {FlightSqlConnection::HOST, std::string("localhost")}, + {FlightSqlConnection::PORT, std::string("32011")}, + {FlightSqlConnection::USE_ENCRYPTION, std::string("1")}, + }; + + const std::shared_ptr<FlightSqlSslConfig>& second_ssl_config = + LoadFlightSslConfigs(properties); + + const Location& actual_location2 = + FlightSqlConnection::BuildLocation(second_properties, missing_attr, ssl_config); + + Location expected_location; + ASSERT_TRUE(Location::ForGrpcTls("localhost", 32010).Value(&expected_location).ok()); + ASSERT_EQ(expected_location, actual_location1); + ASSERT_NE(expected_location, actual_location2); +} + +TEST(PopulateCallOptionsTest, ConnectionTimeout) { + FlightSqlConnection connection(odbcabstraction::V_3); + connection.SetClosed(false); + + // Expect default timeout to be -1 + ASSERT_EQ(TimeoutDuration{-1.0}, + connection.PopulateCallOptions(Connection::ConnPropertyMap()).timeout); + + connection.SetAttribute(Connection::CONNECTION_TIMEOUT, static_cast<uint32_t>(10)); + ASSERT_EQ(TimeoutDuration{10.0}, + connection.PopulateCallOptions(Connection::ConnPropertyMap()).timeout); +} + +TEST(PopulateCallOptionsTest, GenericOption) { + FlightSqlConnection connection(odbcabstraction::V_3); + connection.SetClosed(false); + + Connection::ConnPropertyMap properties; + properties["Foo"] = "Bar"; + auto options = connection.PopulateCallOptions(properties); + auto headers = options.headers; + ASSERT_EQ(1, headers.size()); + + // Header name must be lower-case because gRPC will crash if it is not lower-case. + ASSERT_EQ("foo", headers[0].first); + + // Header value should preserve case. + ASSERT_EQ("Bar", headers[0].second); +} + +TEST(PopulateCallOptionsTest, GenericOptionWithSpaces) { + FlightSqlConnection connection(odbcabstraction::V_3); + connection.SetClosed(false); + + Connection::ConnPropertyMap properties; + properties["Persist Security Info"] = "False"; + auto options = connection.PopulateCallOptions(properties); + auto headers = options.headers; + // Header names with spaces must be omitted or gRPC will crash. + ASSERT_TRUE(headers.empty()); +} + +} // namespace flight_sql +} // namespace driver + +int main(int argc, char** argv) { + ::testing::InitGoogleTest(&argc, argv); + return RUN_ALL_TESTS(); +} Review Comment: You shouldn't need this ########## cpp/src/arrow/flight/sql/odbc/flight_sql/config/configuration.cc: ########## @@ -0,0 +1,181 @@ +// 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. + +#include "arrow/flight/sql/odbc/flight_sql/include/flight_sql/config/configuration.h" +#include "arrow/flight/sql/odbc/flight_sql/flight_sql_connection.h" + +#include <odbcinst.h> +#include <boost/range/adaptor/map.hpp> +#include <boost/range/algorithm/copy.hpp> +#include <iterator> +#include <sstream> + +namespace driver { +namespace flight_sql { +namespace config { + +static const char DEFAULT_DSN[] = "Apache Arrow Flight SQL"; Review Comment: Is there a reason why we sometimes use `static const char[]` and other times use `constexpr std::string_view`? IMO the latter is preferable unless there is a reason ########## cpp/src/arrow/flight/sql/odbc/flight_sql/accessors/string_array_accessor.cc: ########## @@ -0,0 +1,155 @@ +// 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. + +#include "arrow/flight/sql/odbc/flight_sql/accessors/string_array_accessor.h" + +#include <boost/locale.hpp> +#include "arrow/array.h" +#include "arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/encoding.h" + +namespace driver { +namespace flight_sql { + +using arrow::Array; +using arrow::StringArray; +using odbcabstraction::RowStatus; + +namespace { + +#if defined _WIN32 || defined _WIN64 +std::string utf8_to_clocale(const char* utf8str, int len) { Review Comment: Naming conventions: Utf8ToCLocale, utf8_str ########## cpp/src/arrow/flight/sql/odbc/flight_sql/config/configuration.cc: ########## @@ -0,0 +1,181 @@ +// 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. + +#include "arrow/flight/sql/odbc/flight_sql/include/flight_sql/config/configuration.h" +#include "arrow/flight/sql/odbc/flight_sql/flight_sql_connection.h" + +#include <odbcinst.h> +#include <boost/range/adaptor/map.hpp> +#include <boost/range/algorithm/copy.hpp> +#include <iterator> +#include <sstream> + +namespace driver { +namespace flight_sql { +namespace config { + +static const char DEFAULT_DSN[] = "Apache Arrow Flight SQL"; +static const char DEFAULT_ENABLE_ENCRYPTION[] = TRUE_STR; +static const char DEFAULT_USE_CERT_STORE[] = TRUE_STR; +static const char DEFAULT_DISABLE_CERT_VERIFICATION[] = FALSE_STR; + +namespace { +std::string ReadDsnString(const std::string& dsn, const std::string_view& key, + const std::string& dflt = "") { +#define BUFFER_SIZE (1024) + std::vector<char> buf(BUFFER_SIZE); + + std::string key_str = std::string(key); + int ret = + SQLGetPrivateProfileString(dsn.c_str(), key_str.c_str(), dflt.c_str(), buf.data(), + static_cast<int>(buf.size()), "ODBC.INI"); + + if (ret > BUFFER_SIZE) { + // If there wasn't enough space, try again with the right size buffer. + buf.resize(ret + 1); + ret = + SQLGetPrivateProfileString(dsn.c_str(), key_str.c_str(), dflt.c_str(), buf.data(), + static_cast<int>(buf.size()), "ODBC.INI"); + } + + return std::string(buf.data(), ret); +} + +void RemoveAllKnownKeys(std::vector<std::string>& keys) { + // Remove all known DSN keys from the passed in set of keys, case insensitively. + keys.erase(std::remove_if(keys.begin(), keys.end(), + [&](auto& x) { + return std::find_if( + FlightSqlConnection::ALL_KEYS.begin(), + FlightSqlConnection::ALL_KEYS.end(), + [&](auto& s) { return boost::iequals(x, s); }) != + FlightSqlConnection::ALL_KEYS.end(); + }), + keys.end()); +} + +std::vector<std::string> ReadAllKeys(const std::string& dsn) { + std::vector<char> buf(BUFFER_SIZE); + + int ret = SQLGetPrivateProfileString(dsn.c_str(), NULL, "", buf.data(), + static_cast<int>(buf.size()), "ODBC.INI"); + + if (ret > BUFFER_SIZE) { + // If there wasn't enough space, try again with the right size buffer. + buf.resize(ret + 1); + ret = SQLGetPrivateProfileString(dsn.c_str(), NULL, "", buf.data(), + static_cast<int>(buf.size()), "ODBC.INI"); + } + + // When you pass NULL to SQLGetPrivateProfileString it gives back a \0 delimited list of + // all the keys. The below loop simply tokenizes all the keys and places them into a + // vector. + std::vector<std::string> keys; + char* begin = buf.data(); + while (begin && *begin != '\0') { + char* cur; + for (cur = begin; *cur != '\0'; ++cur) { + } + keys.emplace_back(begin, cur); + begin = ++cur; + } + return keys; +} +} // namespace + +Configuration::Configuration() { + // No-op. +} + +Configuration::~Configuration() { + // No-op. +} + +void Configuration::LoadDefaults() { + Set(FlightSqlConnection::DSN, DEFAULT_DSN); + Set(FlightSqlConnection::USE_ENCRYPTION, DEFAULT_ENABLE_ENCRYPTION); + Set(FlightSqlConnection::USE_SYSTEM_TRUST_STORE, DEFAULT_USE_CERT_STORE); + Set(FlightSqlConnection::DISABLE_CERTIFICATE_VERIFICATION, + DEFAULT_DISABLE_CERT_VERIFICATION); +} + +void Configuration::LoadDsn(const std::string& dsn) { + Set(FlightSqlConnection::DSN, dsn); + Set(FlightSqlConnection::HOST, ReadDsnString(dsn, FlightSqlConnection::HOST)); + Set(FlightSqlConnection::PORT, ReadDsnString(dsn, FlightSqlConnection::PORT)); + Set(FlightSqlConnection::TOKEN, ReadDsnString(dsn, FlightSqlConnection::TOKEN)); + Set(FlightSqlConnection::UID, ReadDsnString(dsn, FlightSqlConnection::UID)); + Set(FlightSqlConnection::PWD, ReadDsnString(dsn, FlightSqlConnection::PWD)); + Set(FlightSqlConnection::USE_ENCRYPTION, + ReadDsnString(dsn, FlightSqlConnection::USE_ENCRYPTION, DEFAULT_ENABLE_ENCRYPTION)); + Set(FlightSqlConnection::TRUSTED_CERTS, + ReadDsnString(dsn, FlightSqlConnection::TRUSTED_CERTS)); + Set(FlightSqlConnection::USE_SYSTEM_TRUST_STORE, + ReadDsnString(dsn, FlightSqlConnection::USE_SYSTEM_TRUST_STORE, + DEFAULT_USE_CERT_STORE)); + Set(FlightSqlConnection::DISABLE_CERTIFICATE_VERIFICATION, + ReadDsnString(dsn, FlightSqlConnection::DISABLE_CERTIFICATE_VERIFICATION, + DEFAULT_DISABLE_CERT_VERIFICATION)); + + auto customKeys = ReadAllKeys(dsn); + RemoveAllKnownKeys(customKeys); + for (auto key : customKeys) { + std::string_view key_sv(key); + Set(key, ReadDsnString(dsn, key_sv)); + } +} + +void Configuration::Clear() { this->properties.clear(); } + +bool Configuration::IsSet(const std::string_view& key) const { + return 0 != this->properties.count(key); +} + +const std::string& Configuration::Get(const std::string_view& key) const { + const auto itr = this->properties.find(key); + if (itr == this->properties.cend()) { + static const std::string empty(""); + return empty; + } + return itr->second; +} + +void Configuration::Set(const std::string_view& key, const std::string& value) { + const std::string copy = boost::trim_copy(value); + if (!copy.empty()) { + this->properties[key] = value; + } +} + +const driver::odbcabstraction::Connection::ConnPropertyMap& Configuration::GetProperties() + const { + return this->properties; +} + +std::vector<std::string_view> Configuration::GetCustomKeys() const { + driver::odbcabstraction::Connection::ConnPropertyMap copyProps(properties); + for (auto& key : FlightSqlConnection::ALL_KEYS) { + copyProps.erase(key); + } + std::vector<std::string_view> keys; + boost::copy(copyProps | boost::adaptors::map_keys, std::back_inserter(keys)); Review Comment: Do we *need* boost here? ########## cpp/src/arrow/flight/sql/odbc/flight_sql/config/configuration.cc: ########## @@ -0,0 +1,181 @@ +// 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. + +#include "arrow/flight/sql/odbc/flight_sql/include/flight_sql/config/configuration.h" +#include "arrow/flight/sql/odbc/flight_sql/flight_sql_connection.h" + +#include <odbcinst.h> +#include <boost/range/adaptor/map.hpp> +#include <boost/range/algorithm/copy.hpp> +#include <iterator> +#include <sstream> + +namespace driver { +namespace flight_sql { +namespace config { + +static const char DEFAULT_DSN[] = "Apache Arrow Flight SQL"; +static const char DEFAULT_ENABLE_ENCRYPTION[] = TRUE_STR; +static const char DEFAULT_USE_CERT_STORE[] = TRUE_STR; +static const char DEFAULT_DISABLE_CERT_VERIFICATION[] = FALSE_STR; + +namespace { +std::string ReadDsnString(const std::string& dsn, const std::string_view& key, + const std::string& dflt = "") { +#define BUFFER_SIZE (1024) + std::vector<char> buf(BUFFER_SIZE); + + std::string key_str = std::string(key); + int ret = + SQLGetPrivateProfileString(dsn.c_str(), key_str.c_str(), dflt.c_str(), buf.data(), + static_cast<int>(buf.size()), "ODBC.INI"); + + if (ret > BUFFER_SIZE) { + // If there wasn't enough space, try again with the right size buffer. + buf.resize(ret + 1); + ret = + SQLGetPrivateProfileString(dsn.c_str(), key_str.c_str(), dflt.c_str(), buf.data(), + static_cast<int>(buf.size()), "ODBC.INI"); + } + + return std::string(buf.data(), ret); +} + +void RemoveAllKnownKeys(std::vector<std::string>& keys) { + // Remove all known DSN keys from the passed in set of keys, case insensitively. + keys.erase(std::remove_if(keys.begin(), keys.end(), + [&](auto& x) { + return std::find_if( + FlightSqlConnection::ALL_KEYS.begin(), + FlightSqlConnection::ALL_KEYS.end(), + [&](auto& s) { return boost::iequals(x, s); }) != + FlightSqlConnection::ALL_KEYS.end(); + }), + keys.end()); +} + +std::vector<std::string> ReadAllKeys(const std::string& dsn) { + std::vector<char> buf(BUFFER_SIZE); + + int ret = SQLGetPrivateProfileString(dsn.c_str(), NULL, "", buf.data(), + static_cast<int>(buf.size()), "ODBC.INI"); + + if (ret > BUFFER_SIZE) { + // If there wasn't enough space, try again with the right size buffer. + buf.resize(ret + 1); + ret = SQLGetPrivateProfileString(dsn.c_str(), NULL, "", buf.data(), + static_cast<int>(buf.size()), "ODBC.INI"); + } + + // When you pass NULL to SQLGetPrivateProfileString it gives back a \0 delimited list of + // all the keys. The below loop simply tokenizes all the keys and places them into a + // vector. + std::vector<std::string> keys; + char* begin = buf.data(); + while (begin && *begin != '\0') { + char* cur; + for (cur = begin; *cur != '\0'; ++cur) { + } + keys.emplace_back(begin, cur); + begin = ++cur; + } + return keys; +} +} // namespace + +Configuration::Configuration() { + // No-op. +} + +Configuration::~Configuration() { + // No-op. +} + +void Configuration::LoadDefaults() { + Set(FlightSqlConnection::DSN, DEFAULT_DSN); + Set(FlightSqlConnection::USE_ENCRYPTION, DEFAULT_ENABLE_ENCRYPTION); + Set(FlightSqlConnection::USE_SYSTEM_TRUST_STORE, DEFAULT_USE_CERT_STORE); + Set(FlightSqlConnection::DISABLE_CERTIFICATE_VERIFICATION, + DEFAULT_DISABLE_CERT_VERIFICATION); +} + +void Configuration::LoadDsn(const std::string& dsn) { + Set(FlightSqlConnection::DSN, dsn); + Set(FlightSqlConnection::HOST, ReadDsnString(dsn, FlightSqlConnection::HOST)); + Set(FlightSqlConnection::PORT, ReadDsnString(dsn, FlightSqlConnection::PORT)); + Set(FlightSqlConnection::TOKEN, ReadDsnString(dsn, FlightSqlConnection::TOKEN)); + Set(FlightSqlConnection::UID, ReadDsnString(dsn, FlightSqlConnection::UID)); + Set(FlightSqlConnection::PWD, ReadDsnString(dsn, FlightSqlConnection::PWD)); + Set(FlightSqlConnection::USE_ENCRYPTION, + ReadDsnString(dsn, FlightSqlConnection::USE_ENCRYPTION, DEFAULT_ENABLE_ENCRYPTION)); + Set(FlightSqlConnection::TRUSTED_CERTS, + ReadDsnString(dsn, FlightSqlConnection::TRUSTED_CERTS)); + Set(FlightSqlConnection::USE_SYSTEM_TRUST_STORE, + ReadDsnString(dsn, FlightSqlConnection::USE_SYSTEM_TRUST_STORE, + DEFAULT_USE_CERT_STORE)); + Set(FlightSqlConnection::DISABLE_CERTIFICATE_VERIFICATION, + ReadDsnString(dsn, FlightSqlConnection::DISABLE_CERTIFICATE_VERIFICATION, + DEFAULT_DISABLE_CERT_VERIFICATION)); + + auto customKeys = ReadAllKeys(dsn); + RemoveAllKnownKeys(customKeys); + for (auto key : customKeys) { + std::string_view key_sv(key); + Set(key, ReadDsnString(dsn, key_sv)); + } Review Comment: Are we effectively re-parsing the DSN from scratch for each key? Can't we parse it up front into keys/values then assign? ########## cpp/src/arrow/flight/sql/odbc/flight_sql/flight_sql_auth_method.h: ########## @@ -0,0 +1,48 @@ +// 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 + +#include <map> +#include <memory> +#include <string> +#include "arrow/flight/client.h" +#include "arrow/flight/sql/odbc/flight_sql/flight_sql_connection.h" +#include "arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/spi/connection.h" + +namespace driver { +namespace flight_sql { + +class FlightSqlAuthMethod { Review Comment: Docstrings may be useful on class definitions & methods ########## cpp/src/arrow/flight/sql/odbc/flight_sql/config/configuration.cc: ########## @@ -0,0 +1,181 @@ +// 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. + +#include "arrow/flight/sql/odbc/flight_sql/include/flight_sql/config/configuration.h" +#include "arrow/flight/sql/odbc/flight_sql/flight_sql_connection.h" + +#include <odbcinst.h> +#include <boost/range/adaptor/map.hpp> +#include <boost/range/algorithm/copy.hpp> +#include <iterator> +#include <sstream> + +namespace driver { +namespace flight_sql { +namespace config { + +static const char DEFAULT_DSN[] = "Apache Arrow Flight SQL"; +static const char DEFAULT_ENABLE_ENCRYPTION[] = TRUE_STR; +static const char DEFAULT_USE_CERT_STORE[] = TRUE_STR; +static const char DEFAULT_DISABLE_CERT_VERIFICATION[] = FALSE_STR; + +namespace { +std::string ReadDsnString(const std::string& dsn, const std::string_view& key, + const std::string& dflt = "") { +#define BUFFER_SIZE (1024) + std::vector<char> buf(BUFFER_SIZE); + + std::string key_str = std::string(key); + int ret = + SQLGetPrivateProfileString(dsn.c_str(), key_str.c_str(), dflt.c_str(), buf.data(), + static_cast<int>(buf.size()), "ODBC.INI"); + + if (ret > BUFFER_SIZE) { + // If there wasn't enough space, try again with the right size buffer. + buf.resize(ret + 1); + ret = + SQLGetPrivateProfileString(dsn.c_str(), key_str.c_str(), dflt.c_str(), buf.data(), + static_cast<int>(buf.size()), "ODBC.INI"); + } + + return std::string(buf.data(), ret); +} + +void RemoveAllKnownKeys(std::vector<std::string>& keys) { + // Remove all known DSN keys from the passed in set of keys, case insensitively. + keys.erase(std::remove_if(keys.begin(), keys.end(), + [&](auto& x) { + return std::find_if( + FlightSqlConnection::ALL_KEYS.begin(), + FlightSqlConnection::ALL_KEYS.end(), + [&](auto& s) { return boost::iequals(x, s); }) != + FlightSqlConnection::ALL_KEYS.end(); + }), + keys.end()); +} + +std::vector<std::string> ReadAllKeys(const std::string& dsn) { + std::vector<char> buf(BUFFER_SIZE); + + int ret = SQLGetPrivateProfileString(dsn.c_str(), NULL, "", buf.data(), + static_cast<int>(buf.size()), "ODBC.INI"); + + if (ret > BUFFER_SIZE) { + // If there wasn't enough space, try again with the right size buffer. + buf.resize(ret + 1); + ret = SQLGetPrivateProfileString(dsn.c_str(), NULL, "", buf.data(), + static_cast<int>(buf.size()), "ODBC.INI"); + } + + // When you pass NULL to SQLGetPrivateProfileString it gives back a \0 delimited list of + // all the keys. The below loop simply tokenizes all the keys and places them into a + // vector. + std::vector<std::string> keys; + char* begin = buf.data(); + while (begin && *begin != '\0') { + char* cur; + for (cur = begin; *cur != '\0'; ++cur) { + } + keys.emplace_back(begin, cur); + begin = ++cur; + } + return keys; +} +} // namespace + +Configuration::Configuration() { + // No-op. +} + +Configuration::~Configuration() { + // No-op. +} Review Comment: If these are no-ops, use `=default` ########## cpp/src/arrow/flight/sql/odbc/flight_sql/config/connection_string_parser.cc: ########## @@ -0,0 +1,102 @@ +// 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. + +#include "arrow/flight/sql/odbc/flight_sql/include/flight_sql/config/connection_string_parser.h" + +#include <stdint.h> +#include <iostream> +#include <string> +#include <vector> + +#include <algorithm> +#include <cstring> +#include <iterator> +#include <sstream> + +namespace driver { +namespace flight_sql { +namespace config { + +ConnectionStringParser::ConnectionStringParser(Configuration& cfg) : cfg(cfg) { + // No-op. +} + +ConnectionStringParser::~ConnectionStringParser() { Review Comment: =default ########## cpp/src/arrow/flight/sql/odbc/flight_sql/accessors/string_array_accessor.cc: ########## @@ -0,0 +1,155 @@ +// 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. + +#include "arrow/flight/sql/odbc/flight_sql/accessors/string_array_accessor.h" + +#include <boost/locale.hpp> +#include "arrow/array.h" +#include "arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/encoding.h" + +namespace driver { +namespace flight_sql { + +using arrow::Array; +using arrow::StringArray; +using odbcabstraction::RowStatus; + +namespace { + +#if defined _WIN32 || defined _WIN64 +std::string utf8_to_clocale(const char* utf8str, int len) { + thread_local boost::locale::generator g; + g.locale_cache_enabled(true); + std::locale loc = g(boost::locale::util::get_system_locale()); Review Comment: Aren't locales thread safe? Can't we initialize this once in a static instead of using a thread local? ########## cpp/src/arrow/flight/sql/odbc/flight_sql/flight_sql_connection.h: ########## @@ -0,0 +1,123 @@ +// 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 + +#include "arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/spi/connection.h" + +#include <vector> +#include "arrow/flight/api.h" +#include "arrow/flight/sql/api.h" + +#include "arrow/flight/sql/odbc/flight_sql/get_info_cache.h" +#include "arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/types.h" + +namespace driver { +namespace flight_sql { + +class FlightSqlSslConfig; + +/// \brief Create an instance of the FlightSqlSslConfig class, from the properties passed +/// into the map. +/// \param connPropertyMap the map with the Connection properties. +/// \return An instance of the FlightSqlSslConfig. +std::shared_ptr<FlightSqlSslConfig> LoadFlightSslConfigs( + const odbcabstraction::Connection::ConnPropertyMap& connPropertyMap); Review Comment: conn_property_map ########## cpp/src/arrow/flight/sql/odbc/flight_sql/config/connection_string_parser.cc: ########## @@ -0,0 +1,102 @@ +// 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. + +#include "arrow/flight/sql/odbc/flight_sql/include/flight_sql/config/connection_string_parser.h" + +#include <stdint.h> +#include <iostream> +#include <string> +#include <vector> + +#include <algorithm> +#include <cstring> +#include <iterator> +#include <sstream> + +namespace driver { +namespace flight_sql { +namespace config { + +ConnectionStringParser::ConnectionStringParser(Configuration& cfg) : cfg(cfg) { + // No-op. +} + +ConnectionStringParser::~ConnectionStringParser() { + // No-op. +} + +void ConnectionStringParser::ParseConnectionString(const char* str, size_t len, + char delimiter) { + std::string connect_str(str, len); + + while (connect_str.rbegin() != connect_str.rend() && *connect_str.rbegin() == 0) + connect_str.erase(connect_str.size() - 1); + + while (!connect_str.empty()) { + size_t attr_begin = connect_str.rfind(delimiter); + + if (attr_begin == std::string::npos) Review Comment: Don't omit braces ########## cpp/src/arrow/flight/sql/odbc/flight_sql/flight_sql_get_tables_reader.h: ########## @@ -0,0 +1,50 @@ +// 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. + +#include <optional> +#include "arrow/flight/sql/odbc/flight_sql/record_batch_transformer.h" + +namespace driver { +namespace flight_sql { + +using arrow::RecordBatch; + +using std::optional; + +class GetTablesReader { Review Comment: docstring? ########## cpp/src/arrow/flight/sql/odbc/flight_sql/flight_sql_get_tables_reader.h: ########## @@ -0,0 +1,50 @@ +// 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. + +#include <optional> +#include "arrow/flight/sql/odbc/flight_sql/record_batch_transformer.h" + +namespace driver { +namespace flight_sql { + +using arrow::RecordBatch; + +using std::optional; Review Comment: Don't `using` std types, just write `std::optional` -- 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
