lidavidm commented on code in PR #34817:
URL: https://github.com/apache/arrow/pull/34817#discussion_r1183585048
##########
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:
I don't think it's intrinsically part of a location: it's a specific way to
transform a URI that makes sense in specific contexts. (Particularly because
the notion of 'URI path' == 'catalog' really _only_ makes sense in Flight SQL,
and it locks out other interpretations of the URI that other real users would
like to have! #34829)
At the very least: the docstring is inadequate and I think the method name
could be more specific as well. I would still prefer this as a static helper in
the arrow::flight or arrow::flight::sql namespace but I'm willing to let that
go if it's clearly documented what is happening here.
--
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]
