[GitHub] [arrow-rs] tustvold commented on a diff in pull request #4250: feat: update flight-sql to latest specs

["tustvold (via GitHub)" <gi...@apache.org>]

tustvold commented on code in PR #4250:
URL: https://github.com/apache/arrow-rs/pull/4250#discussion_r1200410702


##########
arrow-flight/examples/flight_sql_server.rs:
##########
@@ -486,10 +512,58 @@ impl FlightSqlService for FlightSqlServiceImpl {
         &self,
         _query: ActionClosePreparedStatementRequest,
         _request: Request<Action>,
-    ) {
+    ) -> Result<(), Status> {
         unimplemented!("Implement do_action_close_prepared_statement")
     }
 
+    async fn do_action_create_prepared_substrait_plan(
+        &self,
+        _query: ActionCreatePreparedSubstraitPlanRequest,
+        _request: Request<Action>,
+    ) -> Result<ActionCreatePreparedStatementResult, Status> {
+        unimplemented!("Implement do_action_create_prepared_substrait_plan")

Review Comment:
   I think it would be better to consistently return Err(Status::unimplemented)



##########
arrow-flight/src/sql/server.rs:
##########
@@ -529,9 +656,108 @@ where
                         "Unable to unpack ActionClosePreparedStatementRequest.",
                     )
                 })?;
-            self.do_action_close_prepared_statement(cmd, request).await;
+            self.do_action_close_prepared_statement(cmd, request)
+                .await?;
             return Ok(Response::new(Box::pin(futures::stream::empty())));
         }
+        if request.get_ref().r#type == CREATE_PREPARED_SUBSTRAIT_PLAN {

Review Comment:
   I'm curious why this isn't an else if chain :thinking: 



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