lidavidm commented on code in PR #40939: URL: https://github.com/apache/arrow/pull/40939#discussion_r2099266258
########## cpp/src/arrow/flight/sql/odbc/flight_sql/flight_sql_ssl_config.h: ########## @@ -0,0 +1,62 @@ +// 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/types.h> +#include <arrow/status.h> +#include <string> + +namespace driver { +namespace flight_sql { + +/// \brief An Auxiliary class that holds all the information to perform +/// a SSL connection. +class FlightSqlSslConfig { + public: + FlightSqlSslConfig(bool disableCertificateVerification, const std::string& trustedCerts, + bool systemTrustStore, bool useEncryption); + + /// \brief Tells if ssl is enabled. By default it will be true. + /// \return Whether ssl is enabled. + bool useEncryption() const; Review Comment: Should be either UseEncryption or use_encryption (because this is a trivial getter), ditto for below ########## cpp/src/arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/logger.h: ########## @@ -0,0 +1,67 @@ +// 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 <functional> +#include <string> + +#include <spdlog/fmt/bundled/format.h> Review Comment: Once we move to C++20 (soon), we need to use stdlib format, not poke into spdlog's internals ########## cpp/src/arrow/flight/sql/odbc/flight_sql/json_converter.cc: ########## @@ -0,0 +1,326 @@ +// 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/json_converter.h" + +#include <rapidjson/rapidjson.h> +#include <rapidjson/writer.h> +#include <boost/beast/core/detail/base64.hpp> +#include "arrow/builder.h" +#include "arrow/flight/sql/odbc/flight_sql/utils.h" +#include "arrow/scalar.h" +#include "arrow/visitor.h" + +using arrow::Status; + +using boost::beast::detail::base64::encode; +using boost::beast::detail::base64::encoded_size; +namespace base64 = boost::beast::detail::base64; + +using driver::flight_sql::ThrowIfNotOK; + +namespace { +template <typename ScalarT> +Status ConvertScalarToStringAndWrite(const ScalarT& scalar, + rapidjson::Writer<rapidjson::StringBuffer>& writer) { + ARROW_ASSIGN_OR_RAISE(auto string_scalar, scalar.CastTo(arrow::utf8())) + const auto& view = reinterpret_cast<arrow::StringScalar*>(string_scalar.get())->view(); + writer.String(view.data(), view.length(), true); + return Status::OK(); +} + +template <typename BinaryScalarT> +Status ConvertBinaryToBase64StringAndWrite( + const BinaryScalarT& scalar, rapidjson::Writer<rapidjson::StringBuffer>& writer) { + const auto& view = scalar.view(); + size_t encoded_size = base64::encoded_size(view.length()); + std::vector<char> encoded(std::max(encoded_size, static_cast<size_t>(1))); + base64::encode(&encoded[0], view.data(), view.length()); + writer.String(&encoded[0], encoded_size, true); + return Status::OK(); +} + +template <typename ListScalarT> +Status WriteListScalar(const ListScalarT& scalar, + rapidjson::Writer<rapidjson::StringBuffer>& writer, + arrow::ScalarVisitor* visitor) { + writer.StartArray(); + for (int64_t i = 0; i < scalar.value->length(); ++i) { + if (scalar.value->IsNull(i)) { + writer.Null(); + } else { + const auto& result = scalar.value->GetScalar(i); + ThrowIfNotOK(result.status()); + ThrowIfNotOK(result.ValueOrDie()->Accept(visitor)); + } + } + + writer.EndArray(); + return Status::OK(); +} + +class ScalarToJson : public arrow::ScalarVisitor { + private: + rapidjson::StringBuffer string_buffer_; + rapidjson::Writer<rapidjson::StringBuffer> writer_{string_buffer_}; + + public: + void Reset() { + string_buffer_.Clear(); + writer_.Reset(string_buffer_); + } + + std::string ToString() { return string_buffer_.GetString(); } + + Status Visit(const arrow::NullScalar& scalar) override { + writer_.Null(); + + return Status::OK(); + } + + Status Visit(const arrow::BooleanScalar& scalar) override { + writer_.Bool(scalar.value); + + return Status::OK(); + } + + Status Visit(const arrow::Int8Scalar& scalar) override { + writer_.Int(scalar.value); + + return Status::OK(); + } + + Status Visit(const arrow::Int16Scalar& scalar) override { + writer_.Int(scalar.value); + + return Status::OK(); + } + + Status Visit(const arrow::Int32Scalar& scalar) override { + writer_.Int(scalar.value); + + return Status::OK(); + } + + Status Visit(const arrow::Int64Scalar& scalar) override { + writer_.Int64(scalar.value); Review Comment: What is rapidJSON's behavior for int64s that are not representable as JSON numbers? ########## cpp/src/arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/odbc_impl/odbc_handle.h: ########## @@ -0,0 +1,93 @@ +// 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/diagnostics.h> +#include <arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/platform.h> +#include <sql.h> +#include <sqltypes.h> +#include <functional> +#include <mutex> + +/** + * @brief An abstraction over a generic ODBC handle. Review Comment: N.B. this codebase typically uses `\brief`, not `@brief`, for Doxygen. ########## cpp/src/arrow/flight/sql/odbc/flight_sql/json_converter.cc: ########## @@ -0,0 +1,326 @@ +// 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/json_converter.h" + +#include <rapidjson/rapidjson.h> +#include <rapidjson/writer.h> +#include <boost/beast/core/detail/base64.hpp> +#include "arrow/builder.h" +#include "arrow/flight/sql/odbc/flight_sql/utils.h" +#include "arrow/scalar.h" +#include "arrow/visitor.h" + +using arrow::Status; + +using boost::beast::detail::base64::encode; +using boost::beast::detail::base64::encoded_size; +namespace base64 = boost::beast::detail::base64; + +using driver::flight_sql::ThrowIfNotOK; + +namespace { +template <typename ScalarT> +Status ConvertScalarToStringAndWrite(const ScalarT& scalar, + rapidjson::Writer<rapidjson::StringBuffer>& writer) { + ARROW_ASSIGN_OR_RAISE(auto string_scalar, scalar.CastTo(arrow::utf8())) + const auto& view = reinterpret_cast<arrow::StringScalar*>(string_scalar.get())->view(); + writer.String(view.data(), view.length(), true); + return Status::OK(); +} + +template <typename BinaryScalarT> +Status ConvertBinaryToBase64StringAndWrite( + const BinaryScalarT& scalar, rapidjson::Writer<rapidjson::StringBuffer>& writer) { + const auto& view = scalar.view(); + size_t encoded_size = base64::encoded_size(view.length()); Review Comment: Arrow has base64 utilities already, can we use them instead of Boost? ########## cpp/src/arrow/flight/sql/odbc/flight_sql/flight_sql_result_set_column.h: ########## @@ -0,0 +1,77 @@ +// 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/array.h" +#include "arrow/flight/sql/odbc/flight_sql/accessors/types.h" +#include "arrow/flight/sql/odbc/flight_sql/utils.h" + +namespace driver { +namespace flight_sql { + +using arrow::Array; + +class FlightSqlResultSetColumn { + private: + std::shared_ptr<Array> original_array_; + std::shared_ptr<Array> cached_casted_array_; + std::unique_ptr<Accessor> cached_accessor_; + + std::unique_ptr<Accessor> CreateAccessor(CDataType target_type); + + Accessor* GetAccessorForTargetType(CDataType target_type); + + public: + FlightSqlResultSetColumn() = default; + explicit FlightSqlResultSetColumn(bool use_wide_char); + + ColumnBinding binding_; + bool use_wide_char_; + bool is_bound_; + + inline Accessor* GetAccessorForBinding() { return cached_accessor_.get(); } Review Comment: Why are these declared `inline`? ########## cpp/src/arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/odbc_impl/encoding_utils.h: ########## @@ -0,0 +1,80 @@ +// 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/encoding.h> +#include <arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/platform.h> +#include <sql.h> +#include <sqlext.h> +#include <algorithm> +#include <codecvt> +#include <cstring> +#include <locale> +#include <memory> +#include <string> + +#define _SILENCE_CXX17_CODECVT_HEADER_DEPRECATION_WARNING Review Comment: Can we get rid of codecvt? It's been deprecated/removed ########## cpp/src/arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/logger.h: ########## Review Comment: Do we really need multiple layers of logging abstractions? *especially* as this header still requires spdlog... ########## cpp/src/arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/odbc_impl/odbc_statement.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/odbc_impl/odbc_handle.h> + +#include <arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/platform.h> +#include <sql.h> +#include <memory> +#include <string> + +namespace driver { +namespace odbcabstraction { +class Statement; +class ResultSet; +} // namespace odbcabstraction +} // namespace driver + +namespace ODBC { +class ODBCConnection; +class ODBCDescriptor; +} // namespace ODBC Review Comment: You may want to create type_fwd.h headers like the rest of the Arrow codebase ########## cpp/src/arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/error_codes.h: ########## @@ -0,0 +1,37 @@ +// 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 <cstdint> + +namespace driver { +namespace odbcabstraction { + +enum ODBCErrorCodes : int32_t { Review Comment: (1) enum class (2) Given the very specific codes, is there a reference/source for these values? -- 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
