Flight session management - #1
indigophox wants to merge 51 commits into
Conversation
Add methods to easily get the path and query parameters from a Location that has been parsed from a URI. Add a method to return this data as headers that can be supplied to FlightCallOptions
| /// \param[in] session_options The session options to set. | ||
| ::arrow::Result<std::vector<SetSessionOptionResult>> SetSessionOptions( | ||
| const FlightCallOptions& options, | ||
| const std::vector<SessionOption>& session_options); |
There was a problem hiding this comment.
Can the client modify the SessionOptions and update them here or is this use once and error if they try to set them again? I guess they could call GetSessionOptions and pass that back in here.
There was a problem hiding this comment.
Originally we were not going to but given we're necessarily going to be setting the desired Resource-Path session option value /after/ connecting I don't see that it's going to be immutable in any way. If we start thinking in terms of it being updated we do need to ensure that it's captured at call time by the FlightProducer and can't change from that call handing code's perspective so that there isn't a race if e.g. SetSessionOptions is called again while a query is busy executing or whatnot (i.e. snapshot the session state for the FlightProducer's handler's perspective or read/copy the values at handler init or whatever).
| /// \brief Gets current session options. | ||
| /// | ||
| /// \param[in] options RPC-layer hints for this call. | ||
| ::arrow::Result<std::vector<SessionOption>> GetSessionOptions( |
There was a problem hiding this comment.
I guess the user could call GetSessionOptions then pass the std::vector back into SetSessionOptions. I assume this would get rejected?
There was a problem hiding this comment.
Should probably work (server implementation-defined) and should effectively be a no-op unless one of the option names is defined by the server to be set-once-and-don't-allow-changing in which case it might complain.
| /// | ||
| /// \param[in] options RPC-layer hints for this call. | ||
| /// \param[in] session_options The session options to set. | ||
| ::arrow::Result<std::vector<SetSessionOptionResult>> SetSessionOptions( |
There was a problem hiding this comment.
I think we need a may to set SessionOptionResult in a GetFlightInfo call as the likes of Power BI won't handle two calls from a driver easily.
There was a problem hiding this comment.
SetSessionOptions will be part of the ODBC driver connection init, after which cookies (presumably persisted by the ODBC driver which it might need to be updated to support, or by the ODBC-using application which I don't think is the case(?!)) will persist the session state reference.
| break; | ||
| case flight_sql_pb::SessionOption::kStringListValue: | ||
| std::vector<std::string> vlist; | ||
| if (in_opt.string_list_value().values_size() > 0) { |
There was a problem hiding this comment.
Is this if statement here for performance? It looks like it can be removed which would clean up/make this more readable
There was a problem hiding this comment.
Yes, performance. It's consistent with a lot of other Arrow code but it's likely that calling std::vector.reserve(0) doesn't cost much at all, particularly as the allocator isn't touched. Can remove it if you think that's cleaner.
| return CloseSessionResult::kClosing; | ||
| case flight_sql_pb::ActionCloseSessionResult::CLOSE_RESULT_NOT_CLOSEABLE: | ||
| return CloseSessionResult::kNotClosable; | ||
| default: |
There was a problem hiding this comment.
Why does this switch statement have a default case when the other switch statements don't have one? Especially since the default case doesn't do anything here.
There was a problem hiding this comment.
Because it's for a Protobuf enum that has sentinel values that the build chain barfs on. No other reason :)
| break; | ||
| case pb::sql::SessionOption::kStringListValue: | ||
| std::vector<std::string> vlist; | ||
| if (in_opt.string_list_value().values_size() > 0) { |
There was a problem hiding this comment.
Same comment here with this if statement. Why is it needed? To me the code would look cleaner without it
There was a problem hiding this comment.
This is pretty fair, while it's consistent with other usage it's probably a noop to call a .reserve() that doesn't change the vector's allocated size so it's a moot "optimization" for the most part.
There was a problem hiding this comment.
If its consistent with other usage that's fine with me. I would rather see consistency within a code base
There was a problem hiding this comment.
I removed it and I don't think it's problematic inconsistency, and shouldn't save any meaningful execution time so going to leave it cleaner.
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?