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

stuartcarnie · 2023-03-19T23:51:10Z

Which issue does this PR close?

Rationale for this change

Per the issue, this PR introduces a Commands enum to allow client to match on all supported messages.

The following minor improvements were made to the prost_message_ext macro:

1. Generate a constant for the type URLs:

const ACTION_CLOSE_PREPARED_STATEMENT_REQUEST_TYPE_URL: &'static str = concat!( "type.googleapis.com/arrow.flight.protocol.sql.", stringify!( ActionClosePreparedStatementRequest ) );
const ACTION_CREATE_PREPARED_STATEMENT_REQUEST_TYPE_URL: &'static str = concat!( "type.googleapis.com/arrow.flight.protocol.sql.", stringify!( ActionCreatePreparedStatementRequest ) );
// Remainder omitted for brevity

2. `Commands` enum to unpack any of the supported FlightSQL commands:

pub enum Commands {
    ActionClosePreparedStatementRequest(ActionClosePreparedStatementRequest),
    ActionCreatePreparedStatementRequest(ActionCreatePreparedStatementRequest),
    // Remainder omitted for brevity
}

with an associated unpack function:

impl Commands {
    pub fn unpack(any: Any) -> Result<Commands, ArrowError> {
        match any.type_url.as_str() {
            ACTION_CLOSE_PREPARED_STATEMENT_REQUEST_TYPE_URL => {
                let m: ActionClosePreparedStatementRequest = Message::decode(&*any.value).map_err(|err| {
                    ArrowError::ParseError({
                        let res = ::alloc::fmt::format(format_args!("Unable to decode Any value: {err}", err = err));
                        res
                    })
                })?;
                Ok(Self::ActionClosePreparedStatementRequest(m))
            }
            ACTION_CREATE_PREPARED_STATEMENT_REQUEST_TYPE_URL => {
                let m: ActionCreatePreparedStatementRequest = Message::decode(&*any.value).map_err(|err| {
                    ArrowError::ParseError({
                        let res = ::alloc::fmt::format(format_args!("Unable to decode Any value: {err}", err = err));
                        res
                    })
                })?;
                Ok(Self::ActionCreatePreparedStatementRequest(m))
            }

            // Remainder omitted for brevity

            _ => Err(ArrowError::ParseError({
                let res = ::alloc::fmt::format(IntellijRustDollarCrate::__export::format_args!("Unable to decode Any value: {}", any.type_url));
                res
            }))
        }
    }
}

3. Update other generated code to use the shared constant for the type URL:

impl ProstMessageExt for ActionClosePreparedStatementRequest {
    fn type_url() -> &'static str {
        ACTION_CLOSE_PREPARED_STATEMENT_REQUEST_TYPE_URL
    }

    fn as_any(&self) -> Any {
        Any {
            type_url: <ActionClosePreparedStatementRequest>::type_url().to_string(),
            value: self.encode_to_vec().into(),
        }
    }
}
impl ProstMessageExt for ActionCreatePreparedStatementRequest {
    fn type_url() -> &'static str {
        ACTION_CREATE_PREPARED_STATEMENT_REQUEST_TYPE_URL
    }

    fn as_any(&self) -> Any {
        Any {
            type_url: <ActionCreatePreparedStatementRequest>::type_url().to_string(),
            value: self.encode_to_vec().into(),
        }
    }
}
// Remainder omitted for brevity

What changes are included in this PR?

Are there any user-facing changes?

API improvements for users of the FlightSQL module.

alamb · 2023-03-20T19:29:35Z

I plan to review this shortly

alamb

Thank you @stuartcarnie -- this is very helpful ❤️ -- I really like it

Some things I noticed while playing with this:

In order to make this code more "discoverable" I think it would be great to add some additional documentation / examples and update the examples. I am working on a PR to do so
. It would be nice if the FlightSQL implementation used FlightError rather than ArrowError (not caused by this PR). I will file a follow on PR to track this

alamb · 2023-03-20T20:58:23Z

arrow-flight/src/sql/mod.rs

+            )*
+
+            as_item! {
+                pub enum Commands {


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

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

alamb · 2023-03-20T20:58:57Z

arrow-flight/src/sql/mod.rs

-                        type_url: <$name>::type_url().to_string(),
-                        value: self.encode_to_vec().into(),
+            impl Commands {
+                pub fn unpack(any: Any) -> Result<Commands, ArrowError> {


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

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

arrow-flight/src/sql/mod.rs

alamb · 2023-03-20T21:41:41Z

Here are some suggested improvements: stuartcarnie#2 (docs + revamp some of the server.rs code to use the new dispatch logic 👍 )

Add docs and use Commands

alamb

Thanks @stuartcarnie -- I think this is looking great

One thing I would really like to do is to translate

    async fn do_get(

in arrow-flight/src/sql/server.rs to use this new pattern too (mostly as an exercise in trying out the API). I ran out of time last night. I think we could do it in a follow on PR too

alamb · 2023-03-21T10:03:02Z

arrow-flight/src/sql/mod.rs

-                    Any {
-                        type_url: <$name>::type_url().to_string(),
-                        value: self.encode_to_vec().into(),
+                /// Get the URL for the command.


👍 this is great

alamb · 2023-03-21T10:03:42Z

arrow-flight/src/sql/server.rs

+            | Command::TicketStatementQuery(_)
+            | Command::ActionCreatePreparedStatementResult(_) => {
+                Err(Status::unimplemented(format!(
+                    "get_flight_info: The defined request is invalid: {command:?}",


Suggested change

"get_flight_info: The defined request is invalid: {command:?}",

"get_flight_info: The defined request is invalid: {}", command.type_url(),

alamb · 2023-03-21T15:15:26Z

I pushed a few minor doc updates directly to this branch.

alamb · 2023-03-21T15:26:31Z

I found an issue with this approach while trying to use this Command structure: stuartcarnie#3 would love to know your thoughts @stuartcarnie

* Updated `do_get` and `do_put` functions to use `Command` enum * Added test for Unknown variant

arrow-flight/src/sql/server.rs

stuartcarnie · 2023-03-21T23:00:09Z

@alamb see ea18fa9 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 ✅

stuartcarnie · 2023-03-21T23:09:57Z

arrow-flight/src/sql/server.rs

+            cmd => Err(Status::unimplemented(format!(
+                "get_flight_info: The defined request is invalid: {}",
+                cmd.type_url()
+            ))),


@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.

alamb · 2023-03-23T17:41:12Z

Thanks -- I plan to come back to this PR over the weekend but got overloaded with other things this week

# Conflicts: # arrow-flight/src/sql/server.rs

alamb · 2023-04-03T15:55:47Z

I am sorry for the delay -- I will get this done / merged this week

alamb

Thanks @stuartcarnie -- I reviewed this again and I think it looks great. I merged up from main to resolve a conflict and plan to merge this PR in when it passes CI

alamb · 2023-04-03T18:32:22Z

arrow-flight/src/sql/mod.rs

+
+        // Unknown variant
+
+        let any = Any {


alamb · 2023-04-03T19:05:31Z

Github seems to have some issue -- restarting

alamb · 2023-04-03T19:16:42Z

I propose some additional documentation for this feature here #4012

feat: Add Commands enum to decode known messages to strong type

b3b29c2

github-actions bot added arrow Changes to the arrow crate arrow-flight Changes to the arrow-flight crate labels Mar 19, 2023

stuartcarnie mentioned this pull request Mar 19, 2023

Make it easier to match FlightSQL messages #3874

Closed

chore: paste needs to be a dependency

727e420

stuartcarnie force-pushed the sgc/issue/prost_3874 branch from 8c0521a to 727e420 Compare March 20, 2023 00:03

chore: rustfmt

84af4a3

alamb reviewed Mar 20, 2023

View reviewed changes

Add docs and use Commands

8de0d7b

alamb reviewed Mar 20, 2023

View reviewed changes

alamb mentioned this pull request Mar 20, 2023

Add docs and use Commands stuartcarnie/arrow-rs#2

Merged

stuartcarnie added 3 commits March 21, 2023 16:13

Merge pull request #2 from alamb/alamb/docs_and_updates

5128652

Add docs and use Commands

chore: Rename to Command; impl TryFrom<Any>

fbb1e9e

chore: Add into_any and type_url API

08f9192

alamb approved these changes Mar 21, 2023

View reviewed changes

alamb added 3 commits March 21, 2023 10:37

Tweak documentation

a7fc658

fixup

cda233b

Merge remote-tracking branch 'apache/master' into sgc/issue/prost_3874

df463c5

clippy

ff8e212

alamb mentioned this pull request Mar 21, 2023

Demonstrate errors decoding Command in FlightSQL stuartcarnie/arrow-rs#3

Open

feat: Add Command::Unknown(Any) variant

ea18fa9

* Updated `do_get` and `do_put` functions to use `Command` enum * Added test for Unknown variant

stuartcarnie commented Mar 21, 2023

View reviewed changes

arrow-flight/src/sql/server.rs Outdated Show resolved Hide resolved

stuartcarnie added 2 commits March 22, 2023 10:03

chore: placate clippy

37f79cc

chore: combine errors

f616aca

chore: don't change error code

5926eda

stuartcarnie commented Mar 21, 2023

View reviewed changes

Merge branch 'master' into sgc/issue/prost_3874

ef456eb

# Conflicts: # arrow-flight/src/sql/server.rs

Merge remote-tracking branch 'apache/master' into sgc/issue/prost_3874

2fa3542

alamb approved these changes Apr 3, 2023

View reviewed changes

arrow-flight/src/sql/mod.rs

// Unknown variant

let any = Any {

Copy link

Contributor

alamb Apr 3, 2023

Choose a reason for hiding this comment

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

👍

alamb merged commit e3f212c into apache:master Apr 3, 2023

alamb mentioned this pull request Apr 3, 2023

Add FlightSQL module docs and links to arrow-flight crates #4012

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

stuartcarnie commented Mar 19, 2023 •

edited

Loading

alamb commented Mar 20, 2023

alamb left a comment

alamb Mar 20, 2023 •

edited

Loading

stuartcarnie Mar 20, 2023

alamb Mar 20, 2023

stuartcarnie Mar 21, 2023

alamb commented Mar 20, 2023

alamb left a comment •

edited

Loading

alamb Mar 21, 2023

alamb Mar 21, 2023

alamb commented Mar 21, 2023

alamb commented Mar 21, 2023

stuartcarnie commented Mar 21, 2023

stuartcarnie Mar 21, 2023

alamb commented Mar 23, 2023

alamb commented Apr 3, 2023

alamb left a comment

alamb Apr 3, 2023

alamb commented Apr 3, 2023

alamb commented Apr 3, 2023

	"get_flight_info: The defined request is invalid: {command:?}",
	"get_flight_info: The defined request is invalid: {}", command.type_url(),

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

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

Conversation

stuartcarnie commented Mar 19, 2023 • edited Loading

Which issue does this PR close?

Rationale for this change

1. Generate a constant for the type URLs:

2. Commands enum to unpack any of the supported FlightSQL commands:

3. Update other generated code to use the shared constant for the type URL:

What changes are included in this PR?

Are there any user-facing changes?

alamb commented Mar 20, 2023

alamb left a comment

Choose a reason for hiding this comment

alamb Mar 20, 2023 • edited Loading

Choose a reason for hiding this comment

stuartcarnie Mar 20, 2023

Choose a reason for hiding this comment

alamb Mar 20, 2023

Choose a reason for hiding this comment

stuartcarnie Mar 21, 2023

Choose a reason for hiding this comment

alamb commented Mar 20, 2023

alamb left a comment • edited Loading

Choose a reason for hiding this comment

alamb Mar 21, 2023

Choose a reason for hiding this comment

alamb Mar 21, 2023

Choose a reason for hiding this comment

alamb commented Mar 21, 2023

alamb commented Mar 21, 2023

stuartcarnie commented Mar 21, 2023

stuartcarnie Mar 21, 2023

Choose a reason for hiding this comment

alamb commented Mar 23, 2023

alamb commented Apr 3, 2023

alamb left a comment

Choose a reason for hiding this comment

alamb Apr 3, 2023

Choose a reason for hiding this comment

alamb commented Apr 3, 2023

alamb commented Apr 3, 2023

stuartcarnie commented Mar 19, 2023 •

edited

Loading

2. `Commands` enum to unpack any of the supported FlightSQL commands:

alamb Mar 20, 2023 •

edited

Loading

alamb left a comment •

edited

Loading