lidavidm commented on code in PR #34817:
URL: https://github.com/apache/arrow/pull/34817#discussion_r1169350585
##########
cpp/src/arrow/flight/types.h:
##########
@@ -409,6 +409,15 @@ struct ARROW_FLIGHT_EXPORT Location {
/// \brief Get the scheme of this URI.
std::string scheme() const;
+ /// \brief Get the path of this URI.
+ std::string path() const;
+
+ /// \brief Get the query parameters of this URI.
+ arrow::Result<std::vector<std::pair<std::string, std::string>>> QueryItems()
const;
+
+ /// \brief Convert URI path and parameters to headers.
+ arrow::Result<std::vector<std::pair<std::string, std::string>>> AsHeaders()
const;
Review Comment:
This is still very specific to Flight SQL (and the docstring isn't very
clear on what the transformation is). Could it be implemented as a helper
function in the `arrow::flight::sql` namespace?
##########
cpp/src/arrow/flight/sql/types.h:
##########
@@ -44,6 +44,23 @@ using SqlInfoResult =
/// \brief Map SQL info identifier to its value.
using SqlInfoResultMap = std::unordered_map<int32_t, SqlInfoResult>;
+/// \brief Variant supporting all possible types for SetSessionOption
+using SessionOptionValue =
+ std::variant<std::string, bool, int32_t, int64_t, float, double,
std::vector<std::string>>;
+
+enum struct SessionOptionValueType : size_t {
+ kString, kBool, kInt32, kInt64, kFloat, kDouble, kStringList
+};
+
+struct ARROW_FLIGHT_SQL_EXPORT SessionOption {
+ std::string option_name;
+ SessionOptionValue option_value;
+
+ SessionOption(std::string name, SessionOptionValue val)
Review Comment:
Constructors with parameters must be marked `explicit`
##########
cpp/src/arrow/flight/sql/types.h:
##########
@@ -44,6 +44,23 @@ using SqlInfoResult =
/// \brief Map SQL info identifier to its value.
using SqlInfoResultMap = std::unordered_map<int32_t, SqlInfoResult>;
+/// \brief Variant supporting all possible types for SetSessionOption
+using SessionOptionValue =
+ std::variant<std::string, bool, int32_t, int64_t, float, double,
std::vector<std::string>>;
+
+enum struct SessionOptionValueType : size_t {
+ kString, kBool, kInt32, kInt64, kFloat, kDouble, kStringList
+};
+
+struct ARROW_FLIGHT_SQL_EXPORT SessionOption {
+ std::string option_name;
+ SessionOptionValue option_value;
+
+ SessionOption(std::string name, SessionOptionValue val)
+ : option_name{ name }, option_value{ val } {}
Review Comment:
Move when initializing
##########
cpp/src/arrow/flight/sql/types.h:
##########
@@ -44,6 +44,23 @@ using SqlInfoResult =
/// \brief Map SQL info identifier to its value.
using SqlInfoResultMap = std::unordered_map<int32_t, SqlInfoResult>;
+/// \brief Variant supporting all possible types for SetSessionOption
+using SessionOptionValue =
+ std::variant<std::string, bool, int32_t, int64_t, float, double,
std::vector<std::string>>;
Review Comment:
nit, but do we really need both 32 and 64 bit int and float?
##########
cpp/src/arrow/flight/sql/server.cc:
##########
@@ -1072,6 +1260,18 @@ Status FlightSqlServerBase::EndTransaction(const
ServerCallContext& context,
return Status::NotImplemented("EndTransaction not implemented");
}
+arrow::Result<ActionSetSessionOptionsResult>
FlightSqlServerBase::SetSessionOptions (
+ const ServerCallContext& context,
+ const ActionSetSessionOptionsRequest& request) {
+ return Status::NotImplemented("SetSessionOptions not implemented");
+}
Review Comment:
Put another way: how do I actually _implement_ this? I don't get anything
identifying the session here.
##########
cpp/src/arrow/flight/types.cc:
##########
@@ -430,6 +430,34 @@ std::string Location::scheme() const {
return scheme;
}
+std::string Location::path() const { return uri_->path(); }
+arrow::Result<std::vector<std::pair<std::string, std::string>>>
Location::QueryItems()
+ const {
+ return uri_->query_items();
+}
+
+arrow::Result<std::vector<std::pair<std::string, std::string>>>
Location::AsHeaders()
+ const {
+ std::string catalog = path();
+ if (catalog.empty()) {
+ return QueryItems();
+ }
+
+ std::vector<std::pair<std::string, std::string>> headers;
+
+ auto query_items_result = QueryItems();
+ if (!query_items_result.ok()) {
Review Comment:
Use ARROW_ASSIGN_OR_RAISE?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
