lidavidm commented on code in PR #40939: URL: https://github.com/apache/arrow/pull/40939#discussion_r2099116665
########## cpp/src/arrow/flight/sql/odbc/flight_sql/accessors/main.h: ########## Review Comment: By codebase convention this should be called "api.h". ########## cpp/src/arrow/flight/sql/odbc/flight_sql/accessors/common.h: ########## @@ -0,0 +1,64 @@ +// 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 <algorithm> +#include <cstdint> +#include "arrow/array.h" +#include "arrow/flight/sql/odbc/flight_sql/accessors/types.h" +#include "arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/diagnostics.h" +#include "arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/types.h" +#include "arrow/scalar.h" + +namespace driver { +namespace flight_sql { Review Comment: Let's fix the namespace: this should be something like `arrow::flight::sql::odbc` ########## cpp/src/arrow/flight/sql/odbc/flight_sql/accessors/types.h: ########## @@ -0,0 +1,148 @@ +// 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 <cstddef> +#include <cstdint> +#include <sstream> + +#include "arrow/array.h" +#include "arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/diagnostics.h" +#include "arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/exceptions.h" +#include "arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/platform.h" +#include "arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/types.h" + +namespace driver { +namespace flight_sql { + +using arrow::Array; +using odbcabstraction::CDataType; + +class FlightSqlResultSet; + +struct ColumnBinding { + void* buffer; + ssize_t* strlen_buffer; + size_t buffer_length; + CDataType target_type; + int precision; + int scale; + + ColumnBinding() = default; + + ColumnBinding(CDataType target_type, int precision, int scale, void* buffer, + size_t buffer_length, ssize_t* strlen_buffer) + : target_type(target_type), + precision(precision), + scale(scale), + buffer(buffer), + buffer_length(buffer_length), + strlen_buffer(strlen_buffer) {} +}; + +/// \brief Accessor interface meant to provide a way of populating data of a +/// single column to buffers bound by `ColumnarResultSet::BindColumn`. +class Accessor { + public: + const CDataType target_type_; + + explicit Accessor(CDataType target_type) : target_type_(target_type) {} + + virtual ~Accessor() = default; + + /// \brief Populates next cells + virtual size_t GetColumnarData(ColumnBinding* binding, int64_t starting_row, + size_t cells, int64_t& value_offset, + bool update_value_offset, + odbcabstraction::Diagnostics& diagnostics, + uint16_t* row_status_array) = 0; + + virtual size_t GetCellLength(ColumnBinding* binding) const = 0; +}; + +template <typename ARROW_ARRAY, CDataType TARGET_TYPE, typename DERIVED> +class FlightSqlAccessor : public Accessor { + public: + explicit FlightSqlAccessor(Array* array) + : Accessor(TARGET_TYPE), + array_(arrow::internal::checked_cast<ARROW_ARRAY*>(array)) {} + + size_t GetColumnarData(ColumnBinding* binding, int64_t starting_row, size_t cells, + int64_t& value_offset, bool update_value_offset, + odbcabstraction::Diagnostics& diagnostics, + uint16_t* row_status_array) override { + return static_cast<DERIVED*>(this)->GetColumnarData_impl( + binding, starting_row, cells, value_offset, update_value_offset, diagnostics, + row_status_array); + } + + size_t GetCellLength(ColumnBinding* binding) const override { + return static_cast<const DERIVED*>(this)->GetCellLength_impl(binding); + } + + protected: + size_t GetColumnarData_impl(ColumnBinding* binding, int64_t starting_row, int64_t cells, + int64_t& value_offset, bool update_value_offset, + odbcabstraction::Diagnostics& diagnostics, + uint16_t* row_status_array) { + for (int64_t i = 0; i < cells; ++i) { + int64_t current_arrow_row = starting_row + i; + if (array_->IsNull(current_arrow_row)) { + if (binding->strlen_buffer) { + binding->strlen_buffer[i] = odbcabstraction::NULL_DATA; + } else { + throw odbcabstraction::NullWithoutIndicatorException(); + } + } else { + // TODO: Optimize this by creating different versions of MoveSingleCell + // depending on if strlen_buffer is null. + auto row_status = MoveSingleCell(binding, current_arrow_row, i, value_offset, + update_value_offset, diagnostics); + if (row_status_array) { + row_status_array[i] = row_status; + } + } + } + + return static_cast<size_t>(cells); + } + + inline ARROW_ARRAY* GetArray() { return array_; } + + private: + ARROW_ARRAY* array_; + + odbcabstraction::RowStatus MoveSingleCell(ColumnBinding* binding, int64_t arrow_row, + int64_t i, int64_t& value_offset, + bool update_value_offset, + odbcabstraction::Diagnostics& diagnostics) { + return static_cast<DERIVED*>(this)->MoveSingleCell_impl( + binding, arrow_row, i, value_offset, update_value_offset, diagnostics); + } + + odbcabstraction::RowStatus MoveSingleCell_impl( + ColumnBinding* binding, int64_t arrow_row, int64_t i, int64_t& value_offset, + bool update_value_offset, odbcabstraction::Diagnostics& diagnostics) { + std::stringstream ss; + ss << "Unknown type conversion from StringArray to target C type " << TARGET_TYPE; + throw odbcabstraction::DriverException(ss.str()); Review Comment: Do we want to convert this to use arrow::Result/arrow::Status? Or should we keep as exceptions? (IMO, since this code isn't going to integrate with the rest of the Arrow source tree, it can stay as exceptions.) Maybe we should consider if the driver should be part of the `arrow/...` source tree, or if it should be its own toplevel source tree (next to arrow/ and parquet/). ########## cpp/src/arrow/flight/sql/odbc/flight_sql/accessors/timestamp_array_accessor_test.cc: ########## @@ -0,0 +1,210 @@ +// 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/timestamp_array_accessor.h" +#include "arrow/flight/sql/odbc/flight_sql/utils.h" +#include "arrow/testing/builder.h" +#include "gtest/gtest.h" +#include "odbcabstraction/calendar_utils.h" + +namespace driver { +namespace flight_sql { + +using arrow::ArrayFromVector; +using arrow::TimestampType; +using arrow::TimeUnit; + +using odbcabstraction::OdbcVersion; +using odbcabstraction::TIMESTAMP_STRUCT; + +using odbcabstraction::GetTimeForSecondsSinceEpoch; + +TEST(TEST_TIMESTAMP, TIMESTAMP_WITH_MILLI) { Review Comment: FWIW, the test suite/case name are effectively class names, so you'd normally expect to see `TestTimestamp, TimestampWithMilli` ########## cpp/src/arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/types.h: ########## @@ -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. + +#pragma once + +#include <arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/platform.h> +#include <boost/optional.hpp> + +namespace driver { +namespace odbcabstraction { + +/// \brief Supported ODBC versions. +enum OdbcVersion { V_2, V_3, V_4 }; + +// Based on ODBC sql.h and sqlext.h definitions. +enum SqlDataType : int16_t { + SqlDataType_CHAR = 1, + SqlDataType_VARCHAR = 12, + SqlDataType_LONGVARCHAR = (-1), + SqlDataType_WCHAR = (-8), + SqlDataType_WVARCHAR = (-9), + SqlDataType_WLONGVARCHAR = (-10), + SqlDataType_DECIMAL = 3, + SqlDataType_NUMERIC = 2, + SqlDataType_SMALLINT = 5, + SqlDataType_INTEGER = 4, + SqlDataType_REAL = 7, + SqlDataType_FLOAT = 6, + SqlDataType_DOUBLE = 8, + SqlDataType_BIT = (-7), + SqlDataType_TINYINT = (-6), + SqlDataType_BIGINT = (-5), + SqlDataType_BINARY = (-2), + SqlDataType_VARBINARY = (-3), + SqlDataType_LONGVARBINARY = (-4), + SqlDataType_TYPE_DATE = 91, + SqlDataType_TYPE_TIME = 92, + SqlDataType_TYPE_TIMESTAMP = 93, + SqlDataType_INTERVAL_MONTH = (100 + 2), + SqlDataType_INTERVAL_YEAR = (100 + 1), + SqlDataType_INTERVAL_YEAR_TO_MONTH = (100 + 7), + SqlDataType_INTERVAL_DAY = (100 + 3), + SqlDataType_INTERVAL_HOUR = (100 + 4), + SqlDataType_INTERVAL_MINUTE = (100 + 5), + SqlDataType_INTERVAL_SECOND = (100 + 6), + SqlDataType_INTERVAL_DAY_TO_HOUR = (100 + 8), + SqlDataType_INTERVAL_DAY_TO_MINUTE = (100 + 9), + SqlDataType_INTERVAL_DAY_TO_SECOND = (100 + 10), + SqlDataType_INTERVAL_HOUR_TO_MINUTE = (100 + 11), + SqlDataType_INTERVAL_HOUR_TO_SECOND = (100 + 12), + SqlDataType_INTERVAL_MINUTE_TO_SECOND = (100 + 13), + SqlDataType_GUID = (-11), +}; + +enum SqlDateTimeSubCode : int16_t { + SqlDateTimeSubCode_DATE = 1, + SqlDateTimeSubCode_TIME = 2, + SqlDateTimeSubCode_TIMESTAMP = 3, + SqlDateTimeSubCode_YEAR = 1, + SqlDateTimeSubCode_MONTH = 2, + SqlDateTimeSubCode_DAY = 3, + SqlDateTimeSubCode_HOUR = 4, + SqlDateTimeSubCode_MINUTE = 5, + SqlDateTimeSubCode_SECOND = 6, + SqlDateTimeSubCode_YEAR_TO_MONTH = 7, + SqlDateTimeSubCode_DAY_TO_HOUR = 8, + SqlDateTimeSubCode_DAY_TO_MINUTE = 9, + SqlDateTimeSubCode_DAY_TO_SECOND = 10, + SqlDateTimeSubCode_HOUR_TO_MINUTE = 11, + SqlDateTimeSubCode_HOUR_TO_SECOND = 12, + SqlDateTimeSubCode_MINUTE_TO_SECOND = 13, +}; + +// Based on ODBC sql.h and sqlext.h definitions. +enum CDataType { + CDataType_CHAR = 1, + CDataType_WCHAR = -8, + CDataType_SSHORT = (5 + (-20)), + CDataType_USHORT = (5 + (-22)), + CDataType_SLONG = (4 + (-20)), + CDataType_ULONG = (4 + (-22)), + CDataType_FLOAT = 7, + CDataType_DOUBLE = 8, + CDataType_BIT = -7, + CDataType_DATE = 91, + CDataType_TIME = 92, + CDataType_TIMESTAMP = 93, + CDataType_STINYINT = ((-6) + (-20)), + CDataType_UTINYINT = ((-6) + (-22)), + CDataType_SBIGINT = ((-5) + (-20)), + CDataType_UBIGINT = ((-5) + (-22)), + CDataType_BINARY = (-2), + CDataType_NUMERIC = 2, + CDataType_DEFAULT = 99, +}; + +enum Nullability { + NULLABILITY_NO_NULLS = 0, + NULLABILITY_NULLABLE = 1, + NULLABILITY_UNKNOWN = 2, +}; + +enum Searchability { + SEARCHABILITY_NONE = 0, + SEARCHABILITY_LIKE_ONLY = 1, + SEARCHABILITY_ALL_EXPECT_LIKE = 2, + SEARCHABILITY_ALL = 3, +}; + +enum Updatability { + UPDATABILITY_READONLY = 0, + UPDATABILITY_WRITE = 1, + UPDATABILITY_READWRITE_UNKNOWN = 2, +}; + +constexpr ssize_t NULL_DATA = -1; +constexpr ssize_t NO_TOTAL = -4; +constexpr ssize_t ALL_TYPES = 0; +constexpr ssize_t DAYS_TO_SECONDS_MULTIPLIER = 86400; +constexpr ssize_t MILLI_TO_SECONDS_DIVISOR = 1000; +constexpr ssize_t MICRO_TO_SECONDS_DIVISOR = 1000000; +constexpr ssize_t NANO_TO_SECONDS_DIVISOR = 1000000000; + +typedef struct tagDATE_STRUCT { + int16_t year; + uint16_t month; + uint16_t day; +} DATE_STRUCT; + +typedef struct tagTIME_STRUCT { + uint16_t hour; + uint16_t minute; + uint16_t second; +} TIME_STRUCT; + +typedef struct tagTIMESTAMP_STRUCT { + int16_t year; + uint16_t month; + uint16_t day; + uint16_t hour; + uint16_t minute; + uint16_t second; + uint32_t fraction; +} TIMESTAMP_STRUCT; + +typedef struct tagNUMERIC_STRUCT { + uint8_t precision; + int8_t scale; + uint8_t sign; // The sign field is 1 if positive, 0 if negative. + uint8_t val[16]; //[e], [f] +} NUMERIC_STRUCT; + +enum RowStatus : uint16_t { + RowStatus_SUCCESS = 0, // Same as SQL_ROW_SUCCESS + RowStatus_SUCCESS_WITH_INFO = 6, // Same as SQL_ROW_SUCCESS_WITH_INFO + RowStatus_ERROR = 5, // Same as SQL_ROW_ERROR + RowStatus_NOROW = 3 // Same as SQL_ROW_NOROW +}; + +struct MetadataSettings { + boost::optional<int32_t> string_column_length_{boost::none}; Review Comment: Use stdlib ########## cpp/src/arrow/flight/sql/odbc/flight_sql/main.cc: ########## Review Comment: What is the purpose of this application? Should it be converted to tests instead? ########## cpp/src/arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/spd_logger.h: ########## @@ -0,0 +1,54 @@ +// 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 "odbcabstraction/logger.h" + +#include <cstdint> +#include <string> + +#include <spdlog/spdlog.h> + +namespace driver { +namespace odbcabstraction { + +class SPDLogger : public Logger { + protected: + std::shared_ptr<spdlog::logger> logger_; + + public: + static constexpr std::string_view LOG_LEVEL = "LogLevel"; + static constexpr std::string_view LOG_PATH = "LogPath"; + static constexpr std::string_view MAXIMUM_FILE_SIZE = "MaximumFileSize"; + static constexpr std::string_view FILE_QUANTITY = "FileQuantity"; + static constexpr std::string_view LOG_ENABLED = "LogEnabled"; + + SPDLogger() = default; + ~SPDLogger() = default; + SPDLogger(SPDLogger& other) = delete; + + void operator=(const SPDLogger&) = delete; + void init(int64_t fileQuantity, int64_t maxFileSize, const std::string& fileNamePrefix, Review Comment: Follow Arrow codebase convention (Init, file_quantity) ########## cpp/src/arrow/flight/sql/odbc/odbcabstraction/utils.cc: ########## @@ -0,0 +1,116 @@ +// 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/vendored/whereami/whereami.h" + +#include "arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/utils.h" + +#include <fstream> +#include <sstream> + +#include <boost/algorithm/string/predicate.hpp> +#include <boost/token_functions.hpp> +#include <boost/tokenizer.hpp> +#include <iostream> + +namespace driver { +namespace odbcabstraction { + +boost::optional<bool> AsBool(const std::string& value) { Review Comment: Let's replace all uses of `boost::optional` with `std::optional` ########## cpp/src/arrow/flight/sql/odbc/flight_sql/include/flight_sql/config/configuration.h: ########## Review Comment: In general, we don't have a separate "include" subdirectory. Headers go next to source files. ########## cpp/src/arrow/flight/sql/odbc/flight_sql/accessors/date_array_accessor.cc: ########## @@ -0,0 +1,95 @@ +// 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/date_array_accessor.h" +#include <time.h> +#include "arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/calendar_utils.h" + +using arrow::Date32Array; +using arrow::Date64Array; + +namespace { +template <typename T> +int64_t convertDate(typename T::value_type value) { Review Comment: Please update functions to Arrow naming conventions (ConvertDate, not convertDate) ########## cpp/src/arrow/flight/sql/odbc/flight_sql/accessors/common.h: ########## @@ -0,0 +1,64 @@ +// 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 <algorithm> +#include <cstdint> +#include "arrow/array.h" +#include "arrow/flight/sql/odbc/flight_sql/accessors/types.h" +#include "arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/diagnostics.h" +#include "arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/types.h" +#include "arrow/scalar.h" + +namespace driver { +namespace flight_sql { + +template <typename ARRAY_TYPE> +inline size_t CopyFromArrayValuesToBinding(ARRAY_TYPE* array, ColumnBinding* binding, + int64_t starting_row, int64_t cells) { + constexpr ssize_t element_size = sizeof(typename ARRAY_TYPE::value_type); + + if (binding->strlen_buffer) { + for (int64_t i = 0; i < cells; ++i) { + int64_t current_row = starting_row + i; + if (array->IsNull(current_row)) { + binding->strlen_buffer[i] = odbcabstraction::NULL_DATA; + } else { + binding->strlen_buffer[i] = element_size; + } + } + } else { + // Duplicate this loop to avoid null checks within the loop. + for (int64_t i = starting_row; i < starting_row + cells; ++i) { + if (array->IsNull(i)) { + throw odbcabstraction::NullWithoutIndicatorException(); + } + } Review Comment: What does this comment mean? It seems there is a null check inside the loop. ########## cpp/src/arrow/flight/sql/odbc/flight_sql/accessors/time_array_accessor.cc: ########## @@ -0,0 +1,148 @@ +// 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/time_array_accessor.h" +#include "arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/calendar_utils.h" + +namespace driver { +namespace flight_sql { + +using arrow::Array; +using arrow::Time32Array; +using arrow::Time64Array; +using arrow::TimeType; +using arrow::TimeUnit; + +using odbcabstraction::DriverException; +using odbcabstraction::GetTimeForSecondsSinceEpoch; +using odbcabstraction::TIME_STRUCT; + +Accessor* CreateTimeAccessor(arrow::Array* array, arrow::Type::type type) { + auto time_type = arrow::internal::checked_pointer_cast<TimeType>(array->type()); + auto time_unit = time_type->unit(); + + if (type == arrow::Type::TIME32) { + switch (time_unit) { + case TimeUnit::SECOND: + return new TimeArrayFlightSqlAccessor<odbcabstraction::CDataType_TIME, + Time32Array, TimeUnit::SECOND>(array); + case TimeUnit::MILLI: + return new TimeArrayFlightSqlAccessor<odbcabstraction::CDataType_TIME, + Time32Array, TimeUnit::MILLI>(array); + case TimeUnit::MICRO: + return new TimeArrayFlightSqlAccessor<odbcabstraction::CDataType_TIME, + Time32Array, TimeUnit::MICRO>(array); + case TimeUnit::NANO: + return new TimeArrayFlightSqlAccessor<odbcabstraction::CDataType_TIME, + Time32Array, TimeUnit::NANO>(array); + } + } else if (type == arrow::Type::TIME64) { + switch (time_unit) { + case TimeUnit::SECOND: + return new TimeArrayFlightSqlAccessor<odbcabstraction::CDataType_TIME, + Time64Array, TimeUnit::SECOND>(array); + case TimeUnit::MILLI: + return new TimeArrayFlightSqlAccessor<odbcabstraction::CDataType_TIME, + Time64Array, TimeUnit::MILLI>(array); + case TimeUnit::MICRO: + return new TimeArrayFlightSqlAccessor<odbcabstraction::CDataType_TIME, + Time64Array, TimeUnit::MICRO>(array); + case TimeUnit::NANO: + return new TimeArrayFlightSqlAccessor<odbcabstraction::CDataType_TIME, + Time64Array, TimeUnit::NANO>(array); + } + } + assert(false); + throw DriverException("Unsupported input supplied to CreateTimeAccessor"); +} + +namespace { +template <typename T> +int64_t ConvertTimeValue(typename T::value_type value, TimeUnit::type unit) { + return value; +} + +template <> +int64_t ConvertTimeValue<Time32Array>(int32_t value, TimeUnit::type unit) { + return unit == TimeUnit::SECOND ? value + : value / odbcabstraction::MILLI_TO_SECONDS_DIVISOR; +} + +template <> +int64_t ConvertTimeValue<Time64Array>(int64_t value, TimeUnit::type unit) { + return unit == TimeUnit::MICRO ? value / odbcabstraction::MICRO_TO_SECONDS_DIVISOR + : value / odbcabstraction::NANO_TO_SECONDS_DIVISOR; +} +} // namespace + +template <CDataType TARGET_TYPE, typename ARROW_ARRAY, TimeUnit::type UNIT> +TimeArrayFlightSqlAccessor<TARGET_TYPE, ARROW_ARRAY, UNIT>::TimeArrayFlightSqlAccessor( + Array* array) + : FlightSqlAccessor<ARROW_ARRAY, TARGET_TYPE, + TimeArrayFlightSqlAccessor<TARGET_TYPE, ARROW_ARRAY, UNIT>>( + array) {} + +template <CDataType TARGET_TYPE, typename ARROW_ARRAY, TimeUnit::type UNIT> +RowStatus TimeArrayFlightSqlAccessor<TARGET_TYPE, ARROW_ARRAY, UNIT>::MoveSingleCell_impl( + ColumnBinding* binding, int64_t arrow_row, int64_t cell_counter, + int64_t& value_offset, bool update_value_offset, + odbcabstraction::Diagnostics& diagnostic) { + auto* buffer = static_cast<TIME_STRUCT*>(binding->buffer); + + tm time{}; Review Comment: In general, why are we using this instead of C++ stdlib chrono/date? ########## cpp/src/arrow/flight/sql/odbc/flight_sql/accessors/common.h: ########## @@ -0,0 +1,64 @@ +// 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 <algorithm> +#include <cstdint> +#include "arrow/array.h" +#include "arrow/flight/sql/odbc/flight_sql/accessors/types.h" +#include "arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/diagnostics.h" +#include "arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/types.h" +#include "arrow/scalar.h" + +namespace driver { +namespace flight_sql { + +template <typename ARRAY_TYPE> +inline size_t CopyFromArrayValuesToBinding(ARRAY_TYPE* array, ColumnBinding* binding, + int64_t starting_row, int64_t cells) { + constexpr ssize_t element_size = sizeof(typename ARRAY_TYPE::value_type); + + if (binding->strlen_buffer) { + for (int64_t i = 0; i < cells; ++i) { + int64_t current_row = starting_row + i; + if (array->IsNull(current_row)) { + binding->strlen_buffer[i] = odbcabstraction::NULL_DATA; + } else { + binding->strlen_buffer[i] = element_size; + } + } + } else { + // Duplicate this loop to avoid null checks within the loop. + for (int64_t i = starting_row; i < starting_row + cells; ++i) { + if (array->IsNull(i)) { + throw odbcabstraction::NullWithoutIndicatorException(); + } + } + } + + // Copy the entire array to the bound ODBC buffers. + // Note that the array should already have been sliced down to the same number + // of elements in the ODBC data array by the point in which this function is called. + const auto* values = array->raw_values(); + memcpy(binding->buffer, &values[starting_row], element_size * cells); Review Comment: nits, but (1) std::memcpy, (2) include cstring? ########## cpp/src/arrow/flight/sql/odbc/flight_sql/accessors/types.h: ########## @@ -0,0 +1,148 @@ +// 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 <cstddef> +#include <cstdint> +#include <sstream> + +#include "arrow/array.h" +#include "arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/diagnostics.h" +#include "arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/exceptions.h" +#include "arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/platform.h" +#include "arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/types.h" + +namespace driver { +namespace flight_sql { + +using arrow::Array; +using odbcabstraction::CDataType; + +class FlightSqlResultSet; + +struct ColumnBinding { + void* buffer; + ssize_t* strlen_buffer; + size_t buffer_length; + CDataType target_type; + int precision; + int scale; + + ColumnBinding() = default; + + ColumnBinding(CDataType target_type, int precision, int scale, void* buffer, + size_t buffer_length, ssize_t* strlen_buffer) + : target_type(target_type), + precision(precision), + scale(scale), + buffer(buffer), + buffer_length(buffer_length), + strlen_buffer(strlen_buffer) {} +}; + +/// \brief Accessor interface meant to provide a way of populating data of a +/// single column to buffers bound by `ColumnarResultSet::BindColumn`. +class Accessor { + public: + const CDataType target_type_; + + explicit Accessor(CDataType target_type) : target_type_(target_type) {} + + virtual ~Accessor() = default; + + /// \brief Populates next cells + virtual size_t GetColumnarData(ColumnBinding* binding, int64_t starting_row, + size_t cells, int64_t& value_offset, + bool update_value_offset, + odbcabstraction::Diagnostics& diagnostics, + uint16_t* row_status_array) = 0; Review Comment: nit, but is the non-const reference meant to be an in-out parameter? I think that is OK but this function confusingly uses a mix of pointers and non-const references ########## cpp/src/arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/types.h: ########## @@ -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. + +#pragma once + +#include <arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/platform.h> +#include <boost/optional.hpp> + +namespace driver { +namespace odbcabstraction { + +/// \brief Supported ODBC versions. +enum OdbcVersion { V_2, V_3, V_4 }; + +// Based on ODBC sql.h and sqlext.h definitions. +enum SqlDataType : int16_t { + SqlDataType_CHAR = 1, + SqlDataType_VARCHAR = 12, + SqlDataType_LONGVARCHAR = (-1), + SqlDataType_WCHAR = (-8), + SqlDataType_WVARCHAR = (-9), + SqlDataType_WLONGVARCHAR = (-10), + SqlDataType_DECIMAL = 3, + SqlDataType_NUMERIC = 2, + SqlDataType_SMALLINT = 5, + SqlDataType_INTEGER = 4, + SqlDataType_REAL = 7, + SqlDataType_FLOAT = 6, + SqlDataType_DOUBLE = 8, + SqlDataType_BIT = (-7), + SqlDataType_TINYINT = (-6), + SqlDataType_BIGINT = (-5), + SqlDataType_BINARY = (-2), + SqlDataType_VARBINARY = (-3), + SqlDataType_LONGVARBINARY = (-4), + SqlDataType_TYPE_DATE = 91, + SqlDataType_TYPE_TIME = 92, + SqlDataType_TYPE_TIMESTAMP = 93, + SqlDataType_INTERVAL_MONTH = (100 + 2), + SqlDataType_INTERVAL_YEAR = (100 + 1), + SqlDataType_INTERVAL_YEAR_TO_MONTH = (100 + 7), + SqlDataType_INTERVAL_DAY = (100 + 3), + SqlDataType_INTERVAL_HOUR = (100 + 4), + SqlDataType_INTERVAL_MINUTE = (100 + 5), + SqlDataType_INTERVAL_SECOND = (100 + 6), + SqlDataType_INTERVAL_DAY_TO_HOUR = (100 + 8), + SqlDataType_INTERVAL_DAY_TO_MINUTE = (100 + 9), + SqlDataType_INTERVAL_DAY_TO_SECOND = (100 + 10), + SqlDataType_INTERVAL_HOUR_TO_MINUTE = (100 + 11), + SqlDataType_INTERVAL_HOUR_TO_SECOND = (100 + 12), + SqlDataType_INTERVAL_MINUTE_TO_SECOND = (100 + 13), + SqlDataType_GUID = (-11), +}; + +enum SqlDateTimeSubCode : int16_t { + SqlDateTimeSubCode_DATE = 1, + SqlDateTimeSubCode_TIME = 2, + SqlDateTimeSubCode_TIMESTAMP = 3, + SqlDateTimeSubCode_YEAR = 1, + SqlDateTimeSubCode_MONTH = 2, + SqlDateTimeSubCode_DAY = 3, + SqlDateTimeSubCode_HOUR = 4, + SqlDateTimeSubCode_MINUTE = 5, + SqlDateTimeSubCode_SECOND = 6, + SqlDateTimeSubCode_YEAR_TO_MONTH = 7, + SqlDateTimeSubCode_DAY_TO_HOUR = 8, + SqlDateTimeSubCode_DAY_TO_MINUTE = 9, + SqlDateTimeSubCode_DAY_TO_SECOND = 10, + SqlDateTimeSubCode_HOUR_TO_MINUTE = 11, + SqlDateTimeSubCode_HOUR_TO_SECOND = 12, + SqlDateTimeSubCode_MINUTE_TO_SECOND = 13, +}; + +// Based on ODBC sql.h and sqlext.h definitions. +enum CDataType { + CDataType_CHAR = 1, + CDataType_WCHAR = -8, + CDataType_SSHORT = (5 + (-20)), + CDataType_USHORT = (5 + (-22)), + CDataType_SLONG = (4 + (-20)), + CDataType_ULONG = (4 + (-22)), + CDataType_FLOAT = 7, + CDataType_DOUBLE = 8, + CDataType_BIT = -7, + CDataType_DATE = 91, + CDataType_TIME = 92, + CDataType_TIMESTAMP = 93, + CDataType_STINYINT = ((-6) + (-20)), + CDataType_UTINYINT = ((-6) + (-22)), + CDataType_SBIGINT = ((-5) + (-20)), + CDataType_UBIGINT = ((-5) + (-22)), + CDataType_BINARY = (-2), + CDataType_NUMERIC = 2, + CDataType_DEFAULT = 99, +}; + +enum Nullability { + NULLABILITY_NO_NULLS = 0, + NULLABILITY_NULLABLE = 1, + NULLABILITY_UNKNOWN = 2, +}; + +enum Searchability { + SEARCHABILITY_NONE = 0, + SEARCHABILITY_LIKE_ONLY = 1, + SEARCHABILITY_ALL_EXPECT_LIKE = 2, + SEARCHABILITY_ALL = 3, +}; + +enum Updatability { + UPDATABILITY_READONLY = 0, + UPDATABILITY_WRITE = 1, + UPDATABILITY_READWRITE_UNKNOWN = 2, +}; + +constexpr ssize_t NULL_DATA = -1; +constexpr ssize_t NO_TOTAL = -4; +constexpr ssize_t ALL_TYPES = 0; +constexpr ssize_t DAYS_TO_SECONDS_MULTIPLIER = 86400; +constexpr ssize_t MILLI_TO_SECONDS_DIVISOR = 1000; +constexpr ssize_t MICRO_TO_SECONDS_DIVISOR = 1000000; +constexpr ssize_t NANO_TO_SECONDS_DIVISOR = 1000000000; + +typedef struct tagDATE_STRUCT { Review Comment: Why are these `typedef struct`? Just use normal `struct`? ########## cpp/src/arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/types.h: ########## @@ -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. + +#pragma once + +#include <arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/platform.h> +#include <boost/optional.hpp> + +namespace driver { +namespace odbcabstraction { + +/// \brief Supported ODBC versions. +enum OdbcVersion { V_2, V_3, V_4 }; Review Comment: Can we use `enum class`? -- 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
