lidavidm commented on code in PR #34817:
URL: https://github.com/apache/arrow/pull/34817#discussion_r1478339887
##########
format/Flight.proto:
##########
Review Comment:
@pitrou @zeroshade @jduo @emkornfield are we satisfied with the API now? I'd
like to avoid bikeshedding during the actual vote
##########
cpp/src/arrow/flight/integration_tests/test_integration.cc:
##########
@@ -744,6 +746,156 @@ class ExpirationTimeRenewFlightEndpointScenario : public
Scenario {
}
};
+/// \brief The server used for testing Session Options.
+///
+/// setSessionOptions has a blacklisted option name and string option value,
+/// both "lol_invalid", which will result in errors attempting to set either.
+class SessionOptionsServer : public sql::FlightSqlServerBase {
+ static inline const std::string invalid_option_name = "lol_invalid";
+ static inline const SessionOptionValue invalid_option_value = "lol_invalid";
+
+ const std::string session_middleware_key;
+ // These will never be threaded so using a plain map and no lock
+ std::map<std::string, SessionOptionValue> session_store_;
+
+ public:
+ explicit SessionOptionsServer(std::string session_middleware_key)
+ : FlightSqlServerBase(),
+ session_middleware_key(std::move(session_middleware_key)) {}
+
+ arrow::Result<SetSessionOptionsResult> SetSessionOptions(
+ const ServerCallContext& context,
+ const SetSessionOptionsRequest& request) override {
+ SetSessionOptionsResult res;
+
+ sql::ServerSessionMiddleware* middleware =
+
(sql::ServerSessionMiddleware*)context.GetMiddleware(session_middleware_key);
+ ARROW_ASSIGN_OR_RAISE(std::shared_ptr<sql::FlightSession> session,
+ middleware->GetSession());
+
+ for (const auto& [name, value] : request.session_options) {
+ // Blacklisted value name
+ if (name == invalid_option_name) {
+ res.errors.emplace(name, SetSessionOptionsResult::Error{
+
SetSessionOptionErrorValue::kInvalidName});
+ continue;
+ }
+ // Blacklisted option value
+ if (value == invalid_option_value) {
+ res.errors.emplace(name, SetSessionOptionsResult::Error{
+
SetSessionOptionErrorValue::kInvalidValue});
+ continue;
+ }
+ if (std::holds_alternative<std::monostate>(value)) {
+ session->EraseSessionOption(name);
+ continue;
+ }
+ session->SetSessionOption(name, value);
+ }
+
+ return res;
+ }
+
+ arrow::Result<GetSessionOptionsResult> GetSessionOptions(
+ const ServerCallContext& context,
+ const GetSessionOptionsRequest& request) override {
+ sql::ServerSessionMiddleware* middleware =
+
(sql::ServerSessionMiddleware*)context.GetMiddleware(session_middleware_key);
+ if (!middleware->HasSession()) {
+ return Status::Invalid("No existing session to get options from.");
+ }
+ ARROW_ASSIGN_OR_RAISE(std::shared_ptr<sql::FlightSession> session,
+ middleware->GetSession());
+
+ return GetSessionOptionsResult{session->GetSessionOptions()};
+ }
+
+ arrow::Result<CloseSessionResult> CloseSession(
+ const ServerCallContext& context, const CloseSessionRequest& request)
override {
+ // Broken (does not expire cookie) until C++ middleware SendingHeaders
handling fixed.
+ sql::ServerSessionMiddleware* middleware =
+
(sql::ServerSessionMiddleware*)context.GetMiddleware(session_middleware_key);
+ ARROW_RETURN_NOT_OK(middleware->CloseSession());
+ return CloseSessionResult{CloseSessionStatus::kClosed};
+ }
+};
+
+/// \brief The Session Options scenario.
+///
+/// This tests Session Options functionality as well as
ServerSessionMiddleware.
+class SessionOptionsScenario : public Scenario {
+ static inline const std::string server_middleware_key = "sessionmiddleware";
+
+ Status MakeServer(std::unique_ptr<FlightServerBase>* server,
+ FlightServerOptions* options) override {
+ *server = std::make_unique<SessionOptionsServer>(server_middleware_key);
+
+ auto id_gen_int = std::make_shared<std::atomic_int>(1000);
+ options->middleware.emplace_back(
+ server_middleware_key,
+ sql::MakeServerSessionMiddlewareFactory(
+ [=]() -> std::string { return std::to_string((*id_gen_int)++); }));
+
+ return Status::OK();
+ }
+
+ Status MakeClient(FlightClientOptions* options) override {
+ options->middleware.emplace_back(GetCookieFactory());
+ return Status::OK();
+ }
+
+ Status RunClient(std::unique_ptr<FlightClient> flight_client) override {
+ sql::FlightSqlClient client{std::move(flight_client)};
+
+ // Set
+ auto req1 = SetSessionOptionsRequest{
+ {{"foolong", 123L},
+ {"bardouble", 456.0},
+ {"lol_invalid", "this won't get set"},
+ {"key_with_invalid_value", "lol_invalid"},
+ {"big_ol_string_list", std::vector<std::string>{"a", "b", "sea",
"dee", " ",
+ " ", "geee",
"(づ｡◕‿‿◕｡)づ"}}}};
+ ARROW_ASSIGN_OR_RAISE(auto res1, client.SetSessionOptions({}, req1));
+ // Some errors
+ if (!(res1.errors ==
Review Comment:
nit: use !=
##########
cpp/src/arrow/flight/integration_tests/test_integration.cc:
##########
@@ -744,6 +746,156 @@ class ExpirationTimeRenewFlightEndpointScenario : public
Scenario {
}
};
+/// \brief The server used for testing Session Options.
+///
+/// setSessionOptions has a blacklisted option name and string option value,
+/// both "lol_invalid", which will result in errors attempting to set either.
+class SessionOptionsServer : public sql::FlightSqlServerBase {
+ static inline const std::string invalid_option_name = "lol_invalid";
+ static inline const SessionOptionValue invalid_option_value = "lol_invalid";
+
+ const std::string session_middleware_key;
+ // These will never be threaded so using a plain map and no lock
+ std::map<std::string, SessionOptionValue> session_store_;
+
+ public:
+ explicit SessionOptionsServer(std::string session_middleware_key)
+ : FlightSqlServerBase(),
+ session_middleware_key(std::move(session_middleware_key)) {}
+
+ arrow::Result<SetSessionOptionsResult> SetSessionOptions(
+ const ServerCallContext& context,
+ const SetSessionOptionsRequest& request) override {
+ SetSessionOptionsResult res;
+
+ sql::ServerSessionMiddleware* middleware =
+
(sql::ServerSessionMiddleware*)context.GetMiddleware(session_middleware_key);
+ ARROW_ASSIGN_OR_RAISE(std::shared_ptr<sql::FlightSession> session,
+ middleware->GetSession());
+
+ for (const auto& [name, value] : request.session_options) {
+ // Blacklisted value name
+ if (name == invalid_option_name) {
+ res.errors.emplace(name, SetSessionOptionsResult::Error{
+
SetSessionOptionErrorValue::kInvalidName});
+ continue;
+ }
+ // Blacklisted option value
+ if (value == invalid_option_value) {
+ res.errors.emplace(name, SetSessionOptionsResult::Error{
+
SetSessionOptionErrorValue::kInvalidValue});
+ continue;
+ }
+ if (std::holds_alternative<std::monostate>(value)) {
+ session->EraseSessionOption(name);
+ continue;
+ }
+ session->SetSessionOption(name, value);
+ }
+
+ return res;
+ }
+
+ arrow::Result<GetSessionOptionsResult> GetSessionOptions(
+ const ServerCallContext& context,
+ const GetSessionOptionsRequest& request) override {
+ sql::ServerSessionMiddleware* middleware =
+
(sql::ServerSessionMiddleware*)context.GetMiddleware(session_middleware_key);
Review Comment:
Instead of a C-style cast, use static_cast or dynamic_cast, and use `auto*`
to avoid repeating the type on both sides
##########
format/Flight.proto:
##########
@@ -525,3 +525,117 @@ message FlightData {
message PutResult {
bytes app_metadata = 1;
}
+
+/*
+ * EXPERIMENTAL: Union of possible value types for a Session Option to be set
to.
+ *
+ * By convention, an attempt to set a valueless SessionOptionValue should
+ * attempt to unset or clear the named option value on the server.
+ */
+message SessionOptionValue {
+ message StringListValue {
+ repeated string values = 1;
+ }
+
+ oneof option_value {
+ string string_value = 1;
+ bool bool_value = 2;
+ sfixed64 int64_value = 3;
+ double double_value = 4;
+ StringListValue string_list_value = 5;
+ }
+}
+
+/*
+ * EXPERIMENTAL: A request to set session options for an existing or new
(implicit)
+ * server session.
+ *
+ * Sessions are persisted and referenced via a transport-level state
management, typically
+ * RFC 6265 HTTP cookies when using an HTTP transport. The suggested cookie
name or state
+ * context key is 'arrow_flight_session_id', although implementations may
freely choose their
+ * own name.
+ *
+ * Session creation (if one does not already exist) is implied by this RPC
request, however
+ * server implementations may choose to initiate a session that also contains
client-provided
+ * session options at any other time, e.g. on authentication, or when any
other call is made
+ * and the server wishes to use a session to persist any state (or lack
thereof).
+ */
+message SetSessionOptionsRequest {
+ map<string, SessionOptionValue> session_options = 1;
+}
+
+/*
+ * EXPERIMENTAL: The results (individually) of setting a set of session
options.
+ *
+ * Option names should only be present in the response if they were not
successfully
+ * set on the server; that is, a response without an Error for a name provided
in the
+ * SetSessionOptionsRequest implies that the named option value was set
successfully.
+ */
+message SetSessionOptionsResult {
+ enum ErrorValue {
+ // The status of setting the option 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.
Review Comment:
nit: the comment seems to be talking about something else. INVALID_NAME is
probably the right value if "...the requested ~~query~~ option is not known"?
--
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]
