You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "lidavidm (via GitHub)" <gi...@apache.org> on 2023/03/31 12:12:55 UTC
[GitHub] [arrow] lidavidm commented on a diff in pull request #34817: Add Session management messages, Location URI path accessors
lidavidm commented on code in PR #34817:
URL: https://github.com/apache/arrow/pull/34817#discussion_r1154391450
##########
format/FlightSql.proto:
##########
@@ -1842,6 +1842,94 @@ message ActionCancelQueryResult {
CancelResult result = 1;
}
+/*
+ * Request message for the "Close Session" action.
+ */
+message ActionCloseSessionRequest {
+ option (experimental) = true;
+}
+
+/*
+ * The result of closing a session.
+ *
+ * The result should be wrapped in a google.protobuf.Any message.
+ */
+message ActionCloseSessionResult {
+ option (experimental) = true;
+
+ enum CloseSessionResult {
+ // The cancellation status is unknown. Servers should avoid using
Review Comment:
s/cancellation/close?
##########
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::query_items()
+ const {
+ return uri_->query_items();
+}
+
+arrow::Result<std::vector<std::pair<std::string, std::string>>> Location::as_headers()
Review Comment:
This is far, far too specific to Flight SQL to belong as a method
##########
format/FlightSql.proto:
##########
@@ -1842,6 +1842,94 @@ message ActionCancelQueryResult {
CancelResult result = 1;
}
+/*
+ * Request message for the "Close Session" action.
+ */
+message ActionCloseSessionRequest {
Review Comment:
Is the assumption for all of these that the session/client identifier will be transmitted out-of-band (via headers, etc.)?
##########
format/FlightSql.proto:
##########
@@ -1842,6 +1842,94 @@ message ActionCancelQueryResult {
CancelResult result = 1;
}
+/*
+ * Request message for the "Close Session" action.
+ */
+message ActionCloseSessionRequest {
+ option (experimental) = true;
+}
+
+/*
+ * The result of closing a session.
+ *
+ * The result should be wrapped in a google.protobuf.Any message.
+ */
+message ActionCloseSessionResult {
+ option (experimental) = true;
+
+ enum CloseSessionResult {
+ // The cancellation status is unknown. Servers should avoid using
+ // this value (send a NOT_FOUND error if the requested query is
+ // not known). Clients can retry the request.
+ CLOSE_RESULT_UNSPECIFIED = 0;
+ // The session close request is complete. Subsequent requests with
+ // a NOT_FOUND error.
+ CLOSE_RESULT_CLOSED = 1;
+ // The session close request is in progress. The client may retry
+ // the close request.
+ CLOSE_RESULT_CLOSING = 2;
+ // The session is not closeable. The client should not retry the
+ // close request.
+ CLOSE_RESULT_NOT_CLOSEABLE = 3;
+ }
+
+ CloseSessionResult result = 1;
+}
+
+message SessionOption {
+ option (experimental) = true;
+
+ message StringListValue {
+ repeated string values = 1;
+ }
+
+ string option_name = 1;
+ oneof option_value {
+ string string_value = 2;
+ bool bool_value = 3;
+ sfixed32 int32_value = 4;
Review Comment:
Why sfixed32 instead of just int32?
##########
cpp/src/arrow/flight/sql/server.cc:
##########
@@ -359,6 +370,66 @@ arrow::Result<ActionEndTransactionRequest> ParseActionEndTransactionRequest(
return result;
}
+arrow::Result<ActionSetSessionOptionsRequest> ParseActionSetSessionOptionsRequest(
+ const google::protobuf::Any& any) {
+ pb::sql::ActionSetSessionOptionsRequest command;
+ if (!any.UnpackTo(&command)) {
+ return Status::Invalid("Unable to unpack ActionSetSessionOptionsRequest");
+ }
+
+ ActionSetSessionOptionsRequest result;
+ if (command.session_options_size() > 0) {
+ result.session_options.reserve(command.session_options_size());
+ for (const pb::sql::SessionOption& in_opt : command.session_options()) {
+ const std::string& name = in_opt.option_name();
+ SessionOption opt;
+ switch (in_opt.option_value_case()) {
+ case pb::sql::SessionOption::OPTION_VALUE_NOT_SET:
+ return Status::Invalid("Unset SessionOptionValue for name '" + name + "'");
+ case pb::sql::SessionOption::kStringValue:
+ opt = {name, in_opt.string_value()};
+ break;
+ case pb::sql::SessionOption::kBoolValue:
+ opt = {name, in_opt.bool_value()};
+ break;
+ case pb::sql::SessionOption::kInt32Value:
+ opt = {name, in_opt.int32_value()};
+ break;
+ case pb::sql::SessionOption::kInt64Value:
+ opt = {name, in_opt.int64_value()};
+ break;
+ case pb::sql::SessionOption::kFloatValue:
+ opt = {name, in_opt.float_value()};
+ break;
+ case pb::sql::SessionOption::kDoubleValue:
+ opt = {name, in_opt.double_value()};
+ break;
+ case pb::sql::SessionOption::kStringListValue:
+ std::vector<std::string> vlist;
+ vlist.reserve(in_opt.string_list_value().values_size());
+ for (const std::string& s : in_opt.string_list_value().values())
+ vlist.push_back(s);
+ opt = {name, vlist};
+ break;
+ }
+ result.session_options.push_back(opt);
Review Comment:
std::move
##########
cpp/src/arrow/flight/sql/client.h:
##########
@@ -329,6 +329,25 @@ class ARROW_FLIGHT_SQL_EXPORT FlightSqlClient {
/// \param[in] info The FlightInfo of the query to cancel.
::arrow::Result<CancelResult> CancelQuery(const FlightCallOptions& options,
const FlightInfo& info);
+
+ /// \brief Sets session options.
+ ///
+ /// \param[in] options RPC-layer hints for this call.
+ /// \param[in] session_options The session options to set.
+ ::arrow::Result<std::vector<SetSessionOptionResult>> SetSessionOptions(
Review Comment:
It may be good to make the session identifier explicit so that clients can use it across individual connections? Also to avoid implicit state.
##########
cpp/src/arrow/flight/sql/client.cc:
##########
@@ -802,6 +802,157 @@ ::arrow::Result<CancelResult> FlightSqlClient::CancelQuery(
return Status::IOError("Server returned unknown result ", result.result());
}
+::arrow::Result<std::vector<SetSessionOptionResult>> FlightSqlClient::SetSessionOptions(
+ const FlightCallOptions& options,
+ const std::vector<SessionOption>& session_options) {
+ flight_sql_pb::ActionSetSessionOptionsRequest request;
+ for (const SessionOption& in_opt : session_options) {
+ flight_sql_pb::SessionOption& opt = *request.add_session_options();
+ const std::string& name = in_opt.option_name;
+ opt.set_option_name(name);
+
+ const SessionOptionValue& value = in_opt.option_value;
+ if (value.index() == std::variant_npos)
+ return Status::Invalid("Undefined SessionOptionValue type ");
+ switch ((SessionOptionValueType)(value.index())) {
Review Comment:
no C-style casts
##########
cpp/src/arrow/flight/sql/server.h:
##########
@@ -225,6 +225,27 @@ struct ARROW_FLIGHT_SQL_EXPORT ActionCreatePreparedStatementResult {
std::string prepared_statement_handle;
};
+/// \brief A request to close the open client session.
+struct ARROW_FLIGHT_SQL_EXPORT ActionCloseSessionRequest {};
Review Comment:
Ditto here - how is the server supposed to get the session identifier?
##########
cpp/src/arrow/flight/sql/server.cc:
##########
@@ -359,6 +370,66 @@ arrow::Result<ActionEndTransactionRequest> ParseActionEndTransactionRequest(
return result;
}
+arrow::Result<ActionSetSessionOptionsRequest> ParseActionSetSessionOptionsRequest(
+ const google::protobuf::Any& any) {
+ pb::sql::ActionSetSessionOptionsRequest command;
+ if (!any.UnpackTo(&command)) {
+ return Status::Invalid("Unable to unpack ActionSetSessionOptionsRequest");
+ }
+
+ ActionSetSessionOptionsRequest result;
+ if (command.session_options_size() > 0) {
+ result.session_options.reserve(command.session_options_size());
+ for (const pb::sql::SessionOption& in_opt : command.session_options()) {
+ const std::string& name = in_opt.option_name();
+ SessionOption opt;
+ switch (in_opt.option_value_case()) {
+ case pb::sql::SessionOption::OPTION_VALUE_NOT_SET:
+ return Status::Invalid("Unset SessionOptionValue for name '" + name + "'");
+ case pb::sql::SessionOption::kStringValue:
+ opt = {name, in_opt.string_value()};
+ break;
+ case pb::sql::SessionOption::kBoolValue:
+ opt = {name, in_opt.bool_value()};
+ break;
+ case pb::sql::SessionOption::kInt32Value:
+ opt = {name, in_opt.int32_value()};
+ break;
+ case pb::sql::SessionOption::kInt64Value:
+ opt = {name, in_opt.int64_value()};
+ break;
+ case pb::sql::SessionOption::kFloatValue:
+ opt = {name, in_opt.float_value()};
+ break;
+ case pb::sql::SessionOption::kDoubleValue:
+ opt = {name, in_opt.double_value()};
+ break;
+ case pb::sql::SessionOption::kStringListValue:
+ std::vector<std::string> vlist;
+ vlist.reserve(in_opt.string_list_value().values_size());
+ for (const std::string& s : in_opt.string_list_value().values())
+ vlist.push_back(s);
+ opt = {name, vlist};
+ break;
+ }
+ result.session_options.push_back(opt);
Review Comment:
(Though, why not just emplace_back inside the switch-case?)
##########
cpp/src/arrow/flight/sql/server.cc:
##########
@@ -423,6 +494,91 @@ arrow::Result<Result> PackActionResult(ActionCreatePreparedStatementResult resul
return PackActionResult(pb_result);
}
+arrow::Result<Result> PackActionResult(ActionSetSessionOptionsResult result) {
+ pb::sql::ActionSetSessionOptionsResult pb_result;
+ for (SetSessionOptionResult& res : result.results) {
+ switch (res) {
+ case SetSessionOptionResult::kUnspecified:
+ pb_result.add_results(
+ pb::sql::ActionSetSessionOptionsResult::SET_SESSION_OPTION_RESULT_UNSPECIFIED);
+ break;
+ case SetSessionOptionResult::kOk:
+ pb_result.add_results(
+ pb::sql::ActionSetSessionOptionsResult::SET_SESSION_OPTION_RESULT_OK);
+ break;
+ case SetSessionOptionResult::kInvalidResult:
+ pb_result.add_results(
+ pb::sql::ActionSetSessionOptionsResult::SET_SESSION_OPTION_RESULT_INVALID_VALUE);
+ break;
+ case SetSessionOptionResult::kError:
+ pb_result.add_results(
+ pb::sql::ActionSetSessionOptionsResult::SET_SESSION_OPTION_RESULT_ERROR);
+ break;
+ }
+ }
+ return PackActionResult(pb_result);
+}
+
+arrow::Result<Result> PackActionResult(ActionGetSessionOptionsResult result) {
+ pb::sql::ActionGetSessionOptionsResult pb_result;
+ for (const SessionOption& in_opt : result.session_options) {
+ pb::sql::SessionOption& opt = *pb_result.add_session_options();
Review Comment:
two things:
- we generally avoid non-const references in Arrow
- Can this be shared with the client-side code that is effectively the same?
##########
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>>> query_items() const;
Review Comment:
nit: snake_case is really only used for trivial getters/setters
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org