Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add Commands enum to decode prost messages to strong type #3887

Merged
merged 17 commits into from
Apr 3, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions arrow-flight/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ arrow-schema = { version = "35.0.0", path = "../arrow-schema" }
base64 = { version = "0.21", default-features = false, features = ["std"] }
tonic = { version = "0.8", default-features = false, features = ["transport", "codegen", "prost"] }
bytes = { version = "1", default-features = false }
paste = { version = "1.0" }
prost = { version = "0.11", default-features = false }
prost-derive = { version = "0.11", default-features = false }
tokio = { version = "1.0", default-features = false, features = ["macros", "rt", "rt-multi-thread"] }
Expand Down
71 changes: 61 additions & 10 deletions arrow-flight/src/sql/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

use arrow_schema::ArrowError;
use bytes::Bytes;
use paste::paste;
use prost::Message;

mod gen {
Expand Down Expand Up @@ -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,)*) => {
alamb marked this conversation as resolved.
Show resolved Hide resolved
paste! {
$(
const [<$name:snake:upper _TYPE_URL>]: &'static str = concat!("type.googleapis.com/arrow.flight.protocol.sql.", stringify!($name));
)*

as_item! {
pub enum Commands {
Copy link
Contributor

@alamb alamb Mar 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a little sparse and I think it would be hard for users to figure out how to use this

Screenshot 2023-03-20 at 4 40 12 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call – do you think adding some docs to the enum and then describing using the TryFrom<Any> trait to decoding them?

$($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> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a TryFrom impl would also be the standard way to expose this function. I will propose one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed – I've removed unpack completely, as Command::try_from or any.try_into() is much better 👍🏻

match any.type_url.as_str() {
$(
[<$name:snake:upper _TYPE_URL>]
=> {
let m: $name = Message::decode(&*any.value).map_err(|err| {
ArrowError::ParseError(format!("Unable to decode Any value: {err}"))
})?;
Ok(Self::$name(m))
}
)*
_ => Err(ArrowError::ParseError(format!("Unable to decode Any value: {}", any.type_url)))
}
}
}
)*
$(
impl ProstMessageExt for $name {
fn type_url() -> &'static str {
[<$name:snake:upper _TYPE_URL>]
}

fn as_any(&self) -> Any {
Any {
type_url: <$name>::type_url().to_string(),
value: self.encode_to_vec().into(),
}
}
}
)*
}
};
}

Expand Down Expand Up @@ -190,4 +229,16 @@ mod tests {
let unpack_query: CommandStatementQuery = any.unpack().unwrap().unwrap();
assert_eq!(query, unpack_query);
}

#[test]
fn test_commands() {
let query = CommandStatementQuery {
query: "select 1".to_string(),
};
let any = Any::pack(&query).unwrap();
assert!(matches!(
Commands::unpack(any).unwrap(),
Commands::CommandStatementQuery(_)
));
}
}