You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "stuartcarnie (via GitHub)" <gi...@apache.org> on 2023/03/19 23:51:12 UTC
[GitHub] [arrow-rs] stuartcarnie opened a new pull request, #3887: feat: Add Commands enum to decode prost messages to strong type
stuartcarnie opened a new pull request, #3887:
URL: https://github.com/apache/arrow-rs/pull/3887
# Which issue does this PR close?
Closes #3874
# Rationale for this change
Per the issue, this PR introduces a `Commands` enum to allow client to match on all supported messages.
# What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
# Are there any user-facing changes?
API improvements for users of the FlightSQL module.
--
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
[GitHub] [arrow-rs] alamb commented on a diff in pull request #3887: feat: Add Commands enum to decode prost messages to strong type
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #3887:
URL: https://github.com/apache/arrow-rs/pull/3887#discussion_r1142671108
##########
arrow-flight/src/sql/mod.rs:
##########
@@ -71,22 +72,60 @@ pub trait ProstMessageExt: prost::Message + Default {
fn as_any(&self) -> Any;
}
+/// Macro to coerce a token to an item, specifically
+/// to build the `Commands` enum.
+///
+/// See: <https://danielkeep.github.io/tlborm/book/blk-ast-coercion.html>
+macro_rules! as_item {
+ ($i:item) => {
+ $i
+ };
+}
+
macro_rules! prost_message_ext {
- ($($name:ty,)*) => {
- $(
- impl ProstMessageExt for $name {
- fn type_url() -> &'static str {
- concat!("type.googleapis.com/arrow.flight.protocol.sql.", stringify!($name))
+ ($($name:tt,)*) => {
+ paste! {
+ $(
+ const [<$name:snake:upper _TYPE_URL>]: &'static str = concat!("type.googleapis.com/arrow.flight.protocol.sql.", stringify!($name));
+ )*
+
+ as_item! {
+ pub enum Commands {
+ $($name($name),)*
}
+ }
- fn as_any(&self) -> Any {
- Any {
- type_url: <$name>::type_url().to_string(),
- value: self.encode_to_vec().into(),
+ impl Commands {
+ pub fn unpack(any: Any) -> Result<Commands, ArrowError> {
Review Comment:
I think a `TryFrom` impl would also be the standard way to expose this function. I will propose one.
##########
arrow-flight/src/sql/mod.rs:
##########
@@ -71,22 +72,60 @@ pub trait ProstMessageExt: prost::Message + Default {
fn as_any(&self) -> Any;
}
+/// Macro to coerce a token to an item, specifically
+/// to build the `Commands` enum.
+///
+/// See: <https://danielkeep.github.io/tlborm/book/blk-ast-coercion.html>
+macro_rules! as_item {
+ ($i:item) => {
+ $i
+ };
+}
+
macro_rules! prost_message_ext {
- ($($name:ty,)*) => {
- $(
- impl ProstMessageExt for $name {
- fn type_url() -> &'static str {
- concat!("type.googleapis.com/arrow.flight.protocol.sql.", stringify!($name))
+ ($($name:tt,)*) => {
Review Comment:
What do you think about calling this `Command` to be more idiomatic (enums normally aren't plural) or `FlightSqlCommand` (if we want to be more explicit)
--
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
[GitHub] [arrow-rs] stuartcarnie commented on a diff in pull request #3887: feat: Add Commands enum to decode prost messages to strong type
Posted by "stuartcarnie (via GitHub)" <gi...@apache.org>.
stuartcarnie commented on code in PR #3887:
URL: https://github.com/apache/arrow-rs/pull/3887#discussion_r1144075851
##########
arrow-flight/src/sql/server.rs:
##########
@@ -315,90 +315,57 @@ where
let message =
Any::decode(&*request.get_ref().cmd).map_err(decode_error_to_status)?;
- if message.is::<CommandStatementQuery>() {
- let token = message
- .unpack()
- .map_err(arrow_error_to_status)?
- .expect("unreachable");
- return self.get_flight_info_statement(token, request).await;
- }
- if message.is::<CommandPreparedStatementQuery>() {
- let handle = message
- .unpack()
- .map_err(arrow_error_to_status)?
- .expect("unreachable");
- return self
- .get_flight_info_prepared_statement(handle, request)
- .await;
- }
- if message.is::<CommandGetCatalogs>() {
- let token = message
- .unpack()
- .map_err(arrow_error_to_status)?
- .expect("unreachable");
- return self.get_flight_info_catalogs(token, request).await;
- }
- if message.is::<CommandGetDbSchemas>() {
- let token = message
- .unpack()
- .map_err(arrow_error_to_status)?
- .expect("unreachable");
- return self.get_flight_info_schemas(token, request).await;
- }
- if message.is::<CommandGetTables>() {
- let token = message
- .unpack()
- .map_err(arrow_error_to_status)?
- .expect("unreachable");
- return self.get_flight_info_tables(token, request).await;
- }
- if message.is::<CommandGetTableTypes>() {
- let token = message
- .unpack()
- .map_err(arrow_error_to_status)?
- .expect("unreachable");
- return self.get_flight_info_table_types(token, request).await;
+ match Command::try_from(message).map_err(arrow_error_to_status)? {
+ Command::CommandStatementQuery(token) => {
+ self.get_flight_info_statement(token, request).await
+ }
+ Command::CommandPreparedStatementQuery(handle) => {
+ self.get_flight_info_prepared_statement(handle, request)
+ .await
+ }
+ Command::CommandGetCatalogs(token) => {
+ self.get_flight_info_catalogs(token, request).await
+ }
+ Command::CommandGetDbSchemas(token) => {
+ return self.get_flight_info_schemas(token, request).await
+ }
+ Command::CommandGetTables(token) => {
+ self.get_flight_info_tables(token, request).await
+ }
+ Command::CommandGetTableTypes(token) => {
+ self.get_flight_info_table_types(token, request).await
+ }
+ Command::CommandGetSqlInfo(token) => {
+ self.get_flight_info_sql_info(token, request).await
+ }
+ Command::CommandGetPrimaryKeys(token) => {
+ self.get_flight_info_primary_keys(token, request).await
+ }
+ Command::CommandGetExportedKeys(token) => {
+ self.get_flight_info_exported_keys(token, request).await
+ }
+ Command::CommandGetImportedKeys(token) => {
+ self.get_flight_info_imported_keys(token, request).await
+ }
+ Command::CommandGetCrossReference(token) => {
+ self.get_flight_info_cross_reference(token, request).await
+ }
+ Command::ActionClosePreparedStatementRequest(_)
+ | Command::ActionCreatePreparedStatementRequest(_)
+ | Command::CommandPreparedStatementUpdate(_)
+ | Command::CommandStatementUpdate(_)
+ | Command::DoPutUpdateResult(_)
+ | Command::TicketStatementQuery(_)
+ | Command::ActionCreatePreparedStatementResult(_) => {
+ Err(Status::unimplemented(format!(
+ "get_flight_info: The defined request is invalid: {}",
+ command.type_url()
+ )))
+ }
+ Command::Unknown(Any { ref type_url, .. }) => Err(Status::invalid_argument(
+ format!("get_flight_info: The defined request is invalid: {type_url}"),
+ )),
Review Comment:
Disregard, as I've combined remaining cases.
--
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
[GitHub] [arrow-rs] alamb commented on pull request #3887: feat: Add Commands enum to decode prost messages to strong type
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #3887:
URL: https://github.com/apache/arrow-rs/pull/3887#issuecomment-1494578837
I am sorry for the delay -- I will get this done / merged this week
--
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
[GitHub] [arrow-rs] stuartcarnie commented on a diff in pull request #3887: feat: Add Commands enum to decode prost messages to strong type
Posted by "stuartcarnie (via GitHub)" <gi...@apache.org>.
stuartcarnie commented on code in PR #3887:
URL: https://github.com/apache/arrow-rs/pull/3887#discussion_r1142905576
##########
arrow-flight/src/sql/mod.rs:
##########
@@ -71,22 +72,60 @@ pub trait ProstMessageExt: prost::Message + Default {
fn as_any(&self) -> Any;
}
+/// Macro to coerce a token to an item, specifically
+/// to build the `Commands` enum.
+///
+/// See: <https://danielkeep.github.io/tlborm/book/blk-ast-coercion.html>
+macro_rules! as_item {
+ ($i:item) => {
+ $i
+ };
+}
+
macro_rules! prost_message_ext {
- ($($name:ty,)*) => {
- $(
- impl ProstMessageExt for $name {
- fn type_url() -> &'static str {
- concat!("type.googleapis.com/arrow.flight.protocol.sql.", stringify!($name))
+ ($($name:tt,)*) => {
Review Comment:
Done – I've renamed it to `Command`
--
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
[GitHub] [arrow-rs] alamb commented on a diff in pull request #3887: feat: Add Commands enum to decode prost messages to strong type
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #3887:
URL: https://github.com/apache/arrow-rs/pull/3887#discussion_r1156318103
##########
arrow-flight/src/sql/mod.rs:
##########
@@ -190,4 +279,27 @@ mod tests {
let unpack_query: CommandStatementQuery = any.unpack().unwrap().unwrap();
assert_eq!(query, unpack_query);
}
+
+ #[test]
+ fn test_command() {
+ let query = CommandStatementQuery {
+ query: "select 1".to_string(),
+ };
+ let any = Any::pack(&query).unwrap();
+ let cmd: Command = any.try_into().unwrap();
+
+ assert!(matches!(cmd, Command::CommandStatementQuery(_)));
+ assert_eq!(cmd.type_url(), COMMAND_STATEMENT_QUERY_TYPE_URL);
+
+ // Unknown variant
+
+ let any = Any {
Review Comment:
👍
--
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
[GitHub] [arrow-rs] stuartcarnie commented on a diff in pull request #3887: feat: Add Commands enum to decode prost messages to strong type
Posted by "stuartcarnie (via GitHub)" <gi...@apache.org>.
stuartcarnie commented on code in PR #3887:
URL: https://github.com/apache/arrow-rs/pull/3887#discussion_r1142905798
##########
arrow-flight/src/sql/mod.rs:
##########
@@ -71,22 +72,60 @@ pub trait ProstMessageExt: prost::Message + Default {
fn as_any(&self) -> Any;
}
+/// Macro to coerce a token to an item, specifically
+/// to build the `Commands` enum.
+///
+/// See: <https://danielkeep.github.io/tlborm/book/blk-ast-coercion.html>
+macro_rules! as_item {
+ ($i:item) => {
+ $i
+ };
+}
+
macro_rules! prost_message_ext {
- ($($name:ty,)*) => {
- $(
- impl ProstMessageExt for $name {
- fn type_url() -> &'static str {
- concat!("type.googleapis.com/arrow.flight.protocol.sql.", stringify!($name))
+ ($($name:tt,)*) => {
+ paste! {
+ $(
+ const [<$name:snake:upper _TYPE_URL>]: &'static str = concat!("type.googleapis.com/arrow.flight.protocol.sql.", stringify!($name));
+ )*
+
+ as_item! {
+ pub enum Commands {
+ $($name($name),)*
}
+ }
- fn as_any(&self) -> Any {
- Any {
- type_url: <$name>::type_url().to_string(),
- value: self.encode_to_vec().into(),
+ impl Commands {
+ pub fn unpack(any: Any) -> Result<Commands, ArrowError> {
Review Comment:
Agreed – I've removed unpack completely, as `Command::try_from` or `any.try_into()` is much better 👍🏻
--
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
[GitHub] [arrow-rs] alamb commented on pull request #3887: feat: Add Commands enum to decode prost messages to strong type
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #3887:
URL: https://github.com/apache/arrow-rs/pull/3887#issuecomment-1476818823
I plan to review this shortly
--
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
[GitHub] [arrow-rs] stuartcarnie commented on pull request #3887: feat: Add Commands enum to decode prost messages to strong type
Posted by "stuartcarnie (via GitHub)" <gi...@apache.org>.
stuartcarnie commented on PR #3887:
URL: https://github.com/apache/arrow-rs/pull/3887#issuecomment-1478704107
@alamb see ea18fa9697d1face7945e40dc6542b59c3890614 for the addition of the `Unknown(Any)` variant, which was a great suggestion. I've also updated `do_get` and `do_put`, and the simple test now passes ✅
--
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
[GitHub] [arrow-rs] alamb commented on a diff in pull request #3887: feat: Add Commands enum to decode prost messages to strong type
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #3887:
URL: https://github.com/apache/arrow-rs/pull/3887#discussion_r1142670585
##########
arrow-flight/src/sql/mod.rs:
##########
@@ -71,22 +72,60 @@ pub trait ProstMessageExt: prost::Message + Default {
fn as_any(&self) -> Any;
}
+/// Macro to coerce a token to an item, specifically
+/// to build the `Commands` enum.
+///
+/// See: <https://danielkeep.github.io/tlborm/book/blk-ast-coercion.html>
+macro_rules! as_item {
+ ($i:item) => {
+ $i
+ };
+}
+
macro_rules! prost_message_ext {
- ($($name:ty,)*) => {
- $(
- impl ProstMessageExt for $name {
- fn type_url() -> &'static str {
- concat!("type.googleapis.com/arrow.flight.protocol.sql.", stringify!($name))
+ ($($name:tt,)*) => {
+ paste! {
+ $(
+ const [<$name:snake:upper _TYPE_URL>]: &'static str = concat!("type.googleapis.com/arrow.flight.protocol.sql.", stringify!($name));
+ )*
+
+ as_item! {
+ pub enum Commands {
Review Comment:
This is a little sparse and I think it would be hard for users to figure out how to use this
<img width="1062" alt="Screenshot 2023-03-20 at 4 40 12 PM" src="https://user-images.githubusercontent.com/490673/226461015-a410f5a1-f216-4cb9-9ba7-4c2c48cf403f.png">
--
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
[GitHub] [arrow-rs] alamb commented on pull request #3887: feat: Add Commands enum to decode prost messages to strong type
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #3887:
URL: https://github.com/apache/arrow-rs/pull/3887#issuecomment-1494846175
I propose some additional documentation for this feature here https://github.com/apache/arrow-rs/pull/4012
--
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
[GitHub] [arrow-rs] alamb commented on a diff in pull request #3887: feat: Add Commands enum to decode prost messages to strong type
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #3887:
URL: https://github.com/apache/arrow-rs/pull/3887#discussion_r1143137588
##########
arrow-flight/src/sql/server.rs:
##########
@@ -315,90 +315,56 @@ where
let message =
Any::decode(&*request.get_ref().cmd).map_err(decode_error_to_status)?;
- if message.is::<CommandStatementQuery>() {
- let token = message
- .unpack()
- .map_err(arrow_error_to_status)?
- .expect("unreachable");
- return self.get_flight_info_statement(token, request).await;
- }
- if message.is::<CommandPreparedStatementQuery>() {
- let handle = message
- .unpack()
- .map_err(arrow_error_to_status)?
- .expect("unreachable");
- return self
- .get_flight_info_prepared_statement(handle, request)
- .await;
- }
- if message.is::<CommandGetCatalogs>() {
- let token = message
- .unpack()
- .map_err(arrow_error_to_status)?
- .expect("unreachable");
- return self.get_flight_info_catalogs(token, request).await;
- }
- if message.is::<CommandGetDbSchemas>() {
- let token = message
- .unpack()
- .map_err(arrow_error_to_status)?
- .expect("unreachable");
- return self.get_flight_info_schemas(token, request).await;
- }
- if message.is::<CommandGetTables>() {
- let token = message
- .unpack()
- .map_err(arrow_error_to_status)?
- .expect("unreachable");
- return self.get_flight_info_tables(token, request).await;
- }
- if message.is::<CommandGetTableTypes>() {
- let token = message
- .unpack()
- .map_err(arrow_error_to_status)?
- .expect("unreachable");
- return self.get_flight_info_table_types(token, request).await;
+ let command = Command::try_from(message).map_err(arrow_error_to_status)?;
+
+ match command {
+ Command::CommandStatementQuery(token) => {
+ self.get_flight_info_statement(token, request).await
+ }
+ Command::CommandPreparedStatementQuery(handle) => {
+ self.get_flight_info_prepared_statement(handle, request)
+ .await
+ }
+ Command::CommandGetCatalogs(token) => {
+ self.get_flight_info_catalogs(token, request).await
+ }
+ Command::CommandGetDbSchemas(token) => {
+ return self.get_flight_info_schemas(token, request).await
+ }
+ Command::CommandGetTables(token) => {
+ self.get_flight_info_tables(token, request).await
+ }
+ Command::CommandGetTableTypes(token) => {
+ self.get_flight_info_table_types(token, request).await
+ }
+
+ Command::CommandGetSqlInfo(token) => {
+ self.get_flight_info_sql_info(token, request).await
+ }
+ Command::CommandGetPrimaryKeys(token) => {
+ self.get_flight_info_primary_keys(token, request).await
+ }
+ Command::CommandGetExportedKeys(token) => {
+ self.get_flight_info_exported_keys(token, request).await
+ }
+ Command::CommandGetImportedKeys(token) => {
+ self.get_flight_info_imported_keys(token, request).await
+ }
+ Command::CommandGetCrossReference(token) => {
+ self.get_flight_info_cross_reference(token, request).await
+ }
+ Command::ActionClosePreparedStatementRequest(_)
+ | Command::ActionCreatePreparedStatementRequest(_)
+ | Command::CommandPreparedStatementUpdate(_)
+ | Command::CommandStatementUpdate(_)
+ | Command::DoPutUpdateResult(_)
+ | Command::TicketStatementQuery(_)
+ | Command::ActionCreatePreparedStatementResult(_) => {
+ Err(Status::unimplemented(format!(
+ "get_flight_info: The defined request is invalid: {command:?}",
Review Comment:
```suggestion
"get_flight_info: The defined request is invalid: {}", command.type_url(),
```
##########
arrow-flight/src/sql/mod.rs:
##########
@@ -71,22 +72,104 @@ pub trait ProstMessageExt: prost::Message + Default {
fn as_any(&self) -> Any;
}
+/// Macro to coerce a token to an item, specifically
+/// to build the `Commands` enum.
+///
+/// See: <https://danielkeep.github.io/tlborm/book/blk-ast-coercion.html>
+macro_rules! as_item {
+ ($i:item) => {
+ $i
+ };
+}
+
macro_rules! prost_message_ext {
- ($($name:ty,)*) => {
- $(
- impl ProstMessageExt for $name {
- fn type_url() -> &'static str {
- concat!("type.googleapis.com/arrow.flight.protocol.sql.", stringify!($name))
+ ($($name:tt,)*) => {
+ paste! {
+ $(
+ const [<$name:snake:upper _TYPE_URL>]: &'static str = concat!("type.googleapis.com/arrow.flight.protocol.sql.", stringify!($name));
+ )*
+
+ as_item! {
+ /// Helper to convert to/from protobuf [`Any`] to a strongly typed enum.
+ ///
+ /// # Example
+ /// ```rust
+ /// # use arrow_flight::sql::{Any, CommandStatementQuery, Command};
+ /// let flightsql_message = CommandStatementQuery {
+ /// query: "SELECT * FROM foo".to_string(),
+ /// };
+ ///
+ /// // Given a packed FlightSQL Any message
+ /// let any_message = Any::pack(&flightsql_message).unwrap();
+ ///
+ /// // decode it to Commands:
+ /// match Command::try_from(any_message).unwrap() {
+ /// Command::CommandStatementQuery(decoded) => {
+ /// assert_eq!(flightsql_message, decoded);
+ /// }
+ /// _ => panic!("Unexpected decoded message"),
+ /// }
+ /// ```
+ #[derive(Clone, Debug, PartialEq)]
+ pub enum Command {
+ $($name($name),)*
+ }
+ }
+
+ impl Command {
+ /// Convert the command to [`Any`].
+ pub fn into_any(self) -> Any {
+ match self {
+ $(
+ Self::$name(cmd) => cmd.as_any(),
+ )*
+ }
}
- fn as_any(&self) -> Any {
- Any {
- type_url: <$name>::type_url().to_string(),
- value: self.encode_to_vec().into(),
+ /// Get the URL for the command.
Review Comment:
👍 this is great
--
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
[GitHub] [arrow-rs] alamb commented on pull request #3887: feat: Add Commands enum to decode prost messages to strong type
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #3887:
URL: https://github.com/apache/arrow-rs/pull/3887#issuecomment-1476974001
Here are some suggested improvements: https://github.com/stuartcarnie/arrow-rs/pull/2# (docs + revamp some of the server.rs code to use the new dispatch logic 👍 )
--
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
[GitHub] [arrow-rs] alamb commented on a diff in pull request #3887: feat: Add Commands enum to decode prost messages to strong type
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #3887:
URL: https://github.com/apache/arrow-rs/pull/3887#discussion_r1142670585
##########
arrow-flight/src/sql/mod.rs:
##########
@@ -71,22 +72,60 @@ pub trait ProstMessageExt: prost::Message + Default {
fn as_any(&self) -> Any;
}
+/// Macro to coerce a token to an item, specifically
+/// to build the `Commands` enum.
+///
+/// See: <https://danielkeep.github.io/tlborm/book/blk-ast-coercion.html>
+macro_rules! as_item {
+ ($i:item) => {
+ $i
+ };
+}
+
macro_rules! prost_message_ext {
- ($($name:ty,)*) => {
- $(
- impl ProstMessageExt for $name {
- fn type_url() -> &'static str {
- concat!("type.googleapis.com/arrow.flight.protocol.sql.", stringify!($name))
+ ($($name:tt,)*) => {
+ paste! {
+ $(
+ const [<$name:snake:upper _TYPE_URL>]: &'static str = concat!("type.googleapis.com/arrow.flight.protocol.sql.", stringify!($name));
+ )*
+
+ as_item! {
+ pub enum Commands {
Review Comment:
<img width="1062" alt="Screenshot 2023-03-20 at 4 40 12 PM" src="https://user-images.githubusercontent.com/490673/226461015-a410f5a1-f216-4cb9-9ba7-4c2c48cf403f.png">
--
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
[GitHub] [arrow-rs] alamb commented on pull request #3887: feat: Add Commands enum to decode prost messages to strong type
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #3887:
URL: https://github.com/apache/arrow-rs/pull/3887#issuecomment-1494830918
Github seems to have some issue -- restarting
--
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
[GitHub] [arrow-rs] alamb commented on pull request #3887: feat: Add Commands enum to decode prost messages to strong type
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #3887:
URL: https://github.com/apache/arrow-rs/pull/3887#issuecomment-1478021286
I pushed a few minor doc updates directly to this branch.
--
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
[GitHub] [arrow-rs] stuartcarnie commented on a diff in pull request #3887: feat: Add Commands enum to decode prost messages to strong type
Posted by "stuartcarnie (via GitHub)" <gi...@apache.org>.
stuartcarnie commented on code in PR #3887:
URL: https://github.com/apache/arrow-rs/pull/3887#discussion_r1144069539
##########
arrow-flight/src/sql/server.rs:
##########
@@ -315,90 +315,57 @@ where
let message =
Any::decode(&*request.get_ref().cmd).map_err(decode_error_to_status)?;
- if message.is::<CommandStatementQuery>() {
- let token = message
- .unpack()
- .map_err(arrow_error_to_status)?
- .expect("unreachable");
- return self.get_flight_info_statement(token, request).await;
- }
- if message.is::<CommandPreparedStatementQuery>() {
- let handle = message
- .unpack()
- .map_err(arrow_error_to_status)?
- .expect("unreachable");
- return self
- .get_flight_info_prepared_statement(handle, request)
- .await;
- }
- if message.is::<CommandGetCatalogs>() {
- let token = message
- .unpack()
- .map_err(arrow_error_to_status)?
- .expect("unreachable");
- return self.get_flight_info_catalogs(token, request).await;
- }
- if message.is::<CommandGetDbSchemas>() {
- let token = message
- .unpack()
- .map_err(arrow_error_to_status)?
- .expect("unreachable");
- return self.get_flight_info_schemas(token, request).await;
- }
- if message.is::<CommandGetTables>() {
- let token = message
- .unpack()
- .map_err(arrow_error_to_status)?
- .expect("unreachable");
- return self.get_flight_info_tables(token, request).await;
- }
- if message.is::<CommandGetTableTypes>() {
- let token = message
- .unpack()
- .map_err(arrow_error_to_status)?
- .expect("unreachable");
- return self.get_flight_info_table_types(token, request).await;
+ match Command::try_from(message).map_err(arrow_error_to_status)? {
+ Command::CommandStatementQuery(token) => {
+ self.get_flight_info_statement(token, request).await
+ }
+ Command::CommandPreparedStatementQuery(handle) => {
+ self.get_flight_info_prepared_statement(handle, request)
+ .await
+ }
+ Command::CommandGetCatalogs(token) => {
+ self.get_flight_info_catalogs(token, request).await
+ }
+ Command::CommandGetDbSchemas(token) => {
+ return self.get_flight_info_schemas(token, request).await
+ }
+ Command::CommandGetTables(token) => {
+ self.get_flight_info_tables(token, request).await
+ }
+ Command::CommandGetTableTypes(token) => {
+ self.get_flight_info_table_types(token, request).await
+ }
+ Command::CommandGetSqlInfo(token) => {
+ self.get_flight_info_sql_info(token, request).await
+ }
+ Command::CommandGetPrimaryKeys(token) => {
+ self.get_flight_info_primary_keys(token, request).await
+ }
+ Command::CommandGetExportedKeys(token) => {
+ self.get_flight_info_exported_keys(token, request).await
+ }
+ Command::CommandGetImportedKeys(token) => {
+ self.get_flight_info_imported_keys(token, request).await
+ }
+ Command::CommandGetCrossReference(token) => {
+ self.get_flight_info_cross_reference(token, request).await
+ }
+ Command::ActionClosePreparedStatementRequest(_)
+ | Command::ActionCreatePreparedStatementRequest(_)
+ | Command::CommandPreparedStatementUpdate(_)
+ | Command::CommandStatementUpdate(_)
+ | Command::DoPutUpdateResult(_)
+ | Command::TicketStatementQuery(_)
+ | Command::ActionCreatePreparedStatementResult(_) => {
+ Err(Status::unimplemented(format!(
+ "get_flight_info: The defined request is invalid: {}",
+ command.type_url()
+ )))
+ }
+ Command::Unknown(Any { ref type_url, .. }) => Err(Status::invalid_argument(
+ format!("get_flight_info: The defined request is invalid: {type_url}"),
+ )),
Review Comment:
@alamb I wasn't sure if this was deliberately an `unimplemented` error, so I did not combine the `Unknown` variant, which returns an `invalid_argument` status.
--
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
[GitHub] [arrow-rs] alamb commented on pull request #3887: feat: Add Commands enum to decode prost messages to strong type
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #3887:
URL: https://github.com/apache/arrow-rs/pull/3887#issuecomment-1481616892
Thanks -- I plan to come back to this PR over the weekend but got overloaded with other things this week
--
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
[GitHub] [arrow-rs] stuartcarnie commented on a diff in pull request #3887: feat: Add Commands enum to decode prost messages to strong type
Posted by "stuartcarnie (via GitHub)" <gi...@apache.org>.
stuartcarnie commented on code in PR #3887:
URL: https://github.com/apache/arrow-rs/pull/3887#discussion_r1144075458
##########
arrow-flight/src/sql/server.rs:
##########
@@ -315,90 +315,46 @@ where
let message =
Any::decode(&*request.get_ref().cmd).map_err(decode_error_to_status)?;
- if message.is::<CommandStatementQuery>() {
- let token = message
- .unpack()
- .map_err(arrow_error_to_status)?
- .expect("unreachable");
- return self.get_flight_info_statement(token, request).await;
- }
- if message.is::<CommandPreparedStatementQuery>() {
- let handle = message
- .unpack()
- .map_err(arrow_error_to_status)?
- .expect("unreachable");
- return self
- .get_flight_info_prepared_statement(handle, request)
- .await;
- }
- if message.is::<CommandGetCatalogs>() {
- let token = message
- .unpack()
- .map_err(arrow_error_to_status)?
- .expect("unreachable");
- return self.get_flight_info_catalogs(token, request).await;
- }
- if message.is::<CommandGetDbSchemas>() {
- let token = message
- .unpack()
- .map_err(arrow_error_to_status)?
- .expect("unreachable");
- return self.get_flight_info_schemas(token, request).await;
- }
- if message.is::<CommandGetTables>() {
- let token = message
- .unpack()
- .map_err(arrow_error_to_status)?
- .expect("unreachable");
- return self.get_flight_info_tables(token, request).await;
- }
- if message.is::<CommandGetTableTypes>() {
- let token = message
- .unpack()
- .map_err(arrow_error_to_status)?
- .expect("unreachable");
- return self.get_flight_info_table_types(token, request).await;
+ match Command::try_from(message).map_err(arrow_error_to_status)? {
+ Command::CommandStatementQuery(token) => {
+ self.get_flight_info_statement(token, request).await
+ }
+ Command::CommandPreparedStatementQuery(handle) => {
+ self.get_flight_info_prepared_statement(handle, request)
+ .await
+ }
+ Command::CommandGetCatalogs(token) => {
+ self.get_flight_info_catalogs(token, request).await
+ }
+ Command::CommandGetDbSchemas(token) => {
+ return self.get_flight_info_schemas(token, request).await
+ }
+ Command::CommandGetTables(token) => {
+ self.get_flight_info_tables(token, request).await
+ }
+ Command::CommandGetTableTypes(token) => {
+ self.get_flight_info_table_types(token, request).await
+ }
+ Command::CommandGetSqlInfo(token) => {
+ self.get_flight_info_sql_info(token, request).await
+ }
+ Command::CommandGetPrimaryKeys(token) => {
+ self.get_flight_info_primary_keys(token, request).await
+ }
+ Command::CommandGetExportedKeys(token) => {
+ self.get_flight_info_exported_keys(token, request).await
+ }
+ Command::CommandGetImportedKeys(token) => {
+ self.get_flight_info_imported_keys(token, request).await
+ }
+ Command::CommandGetCrossReference(token) => {
+ self.get_flight_info_cross_reference(token, request).await
+ }
+ cmd => Err(Status::unimplemented(format!(
+ "get_flight_info: The defined request is invalid: {}",
+ cmd.type_url()
+ ))),
Review Comment:
@alamb I combined all remaining variants into a single error. I left it as `unimplemented`, as it is unclear to me if there are additional commands that may be implemented in the future. If not, perhaps `invalid_argument` is a more appropriate response, however, given that is a change that could break downstream clients, I've left it.
--
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
[GitHub] [arrow-rs] alamb commented on pull request #3887: feat: Add Commands enum to decode prost messages to strong type
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #3887:
URL: https://github.com/apache/arrow-rs/pull/3887#issuecomment-1478040037
I found an issue with this approach while trying to use this `Command` structure: https://github.com/stuartcarnie/arrow-rs/pull/3 would love to know your thoughts @stuartcarnie
--
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
[GitHub] [arrow-rs] stuartcarnie commented on a diff in pull request #3887: feat: Add Commands enum to decode prost messages to strong type
Posted by "stuartcarnie (via GitHub)" <gi...@apache.org>.
stuartcarnie commented on code in PR #3887:
URL: https://github.com/apache/arrow-rs/pull/3887#discussion_r1142749508
##########
arrow-flight/src/sql/mod.rs:
##########
@@ -71,22 +72,60 @@ pub trait ProstMessageExt: prost::Message + Default {
fn as_any(&self) -> Any;
}
+/// Macro to coerce a token to an item, specifically
+/// to build the `Commands` enum.
+///
+/// See: <https://danielkeep.github.io/tlborm/book/blk-ast-coercion.html>
+macro_rules! as_item {
+ ($i:item) => {
+ $i
+ };
+}
+
macro_rules! prost_message_ext {
- ($($name:ty,)*) => {
- $(
- impl ProstMessageExt for $name {
- fn type_url() -> &'static str {
- concat!("type.googleapis.com/arrow.flight.protocol.sql.", stringify!($name))
+ ($($name:tt,)*) => {
+ paste! {
+ $(
+ const [<$name:snake:upper _TYPE_URL>]: &'static str = concat!("type.googleapis.com/arrow.flight.protocol.sql.", stringify!($name));
+ )*
+
+ as_item! {
+ pub enum Commands {
Review Comment:
Good call – do you think adding some docs to the enum and then describing using the `TryFrom<Any>` trait to decoding them?
--
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
[GitHub] [arrow-rs] alamb merged pull request #3887: feat: Add Commands enum to decode prost messages to strong type
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #3887:
URL: https://github.com/apache/arrow-rs/pull/3887
--
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