You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "roeap (via GitHub)" <gi...@apache.org> on 2023/05/21 13:00:03 UTC
[GitHub] [arrow-rs] roeap opened a new pull request, #4250: feat: update flight-sql to latest specs
roeap opened a new pull request, #4250:
URL: https://github.com/apache/arrow-rs/pull/4250
# Which issue does this PR close?
Closes #4249
# Rationale for this change
arrow-rs should keep up-to-date with the reference specs from apache/arrow.
# 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?
New actions and statements are added. The flight sql server trait has corresponding handers added.
--
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] roeap commented on a diff in pull request #4250: feat: update flight-sql to latest specs
Posted by "roeap (via GitHub)" <gi...@apache.org>.
roeap commented on code in PR #4250:
URL: https://github.com/apache/arrow-rs/pull/4250#discussion_r1200670099
##########
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:
There is a status macro defined, which also reports line numbers.
```rs
macro_rules! status {
($desc:expr, $err:expr) => {
Status::internal(format!("{}: {} at {}:{}", $desc, $err, file!(), line!()))
};
}
```
Maybe we could add another macro that uses `Status::unimplemented` to communicate the right status and have the line info available?
--
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] roeap commented on a diff in pull request #4250: feat: update flight-sql to latest specs
Posted by "roeap (via GitHub)" <gi...@apache.org>.
roeap commented on code in PR #4250:
URL: https://github.com/apache/arrow-rs/pull/4250#discussion_r1200670099
##########
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:
There is a status macro defined, which also reports line numbers.
```rs
macro_rules! status {
($desc:expr, $err:expr) => {
Status::internal(format!("{}: {} at {}:{}", $desc, $err, file!(), line!()))
};
}
```
Maybe we could add another macro that uses `Status::unimplemented` to communicate the right status and have the line info available? Something along the lines of
```rs
macro_rules! status {
($method:expr) => {
Status::unimplemented(format!("not implemented: {} at {}:{}", $method, file!(), line!()))
};
}
```
--
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] roeap commented on pull request #4250: feat: update flight-sql to latest specs
Posted by "roeap (via GitHub)" <gi...@apache.org>.
roeap commented on PR #4250:
URL: https://github.com/apache/arrow-rs/pull/4250#issuecomment-1557428156
> There are several other things that could be hoisted upstream (like the basic implementations for GetTables and GetSchemas both of which need basic filtering). Do you think those are worth pulling upstream too?
Definitely - should they however go here, or would they need to be in the datafusion example server and include extracting those from the session context?
--
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] roeap commented on pull request #4250: feat: update flight-sql to latest specs
Posted by "roeap (via GitHub)" <gi...@apache.org>.
roeap commented on PR #4250:
URL: https://github.com/apache/arrow-rs/pull/4250#issuecomment-1556724021
@alamb - playing around with implementing a flight sql server I read a bit through the iOx implementation. Over there I found that the structs around sql infos (`SqlInfoValue`, `SqlInfoList`, `SqlInfoUnionBuilder`) are quite useful and probably non trivial to figure out for users. Do you think this is something that could be hoisted upstream?
--
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] roeap commented on a diff in pull request #4250: feat: update flight-sql to latest specs
Posted by "roeap (via GitHub)" <gi...@apache.org>.
roeap commented on code in PR #4250:
URL: https://github.com/apache/arrow-rs/pull/4250#discussion_r1200464625
##########
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:
updated to `eles if` ... i guess since we return in every branch, it's not too different though?
--
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 #4250: feat: update flight-sql to latest specs
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #4250:
URL: https://github.com/apache/arrow-rs/pull/4250
--
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] avantgardnerio commented on a diff in pull request #4250: feat: update flight-sql to latest specs
Posted by "avantgardnerio (via GitHub)" <gi...@apache.org>.
avantgardnerio commented on code in PR #4250:
URL: https://github.com/apache/arrow-rs/pull/4250#discussion_r1200634008
##########
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:
The nice thing about `unimplemented!("xxx")` is that it contains information both readable by code (unimplemented) and humans ("Implement do_action_create_prepared_substrait_plan"). This information only exists to get serialized via FlightSQL and sent back to the client. So when someone runs a query in say DataGrip (or whatever their favorite tool is) they get an error message telling them specifically which one of these many methods failed, which makes it much easier to debug.
The alternative is that they get a basic error "unimplemented" and they have to drill into the library files in their IDE, and add breakpoints to each one of these methods in order to track down what isn't implemented.
So my argument for retaining this is that more information is better than losing information.
--
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 #4250: feat: update flight-sql to latest specs
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #4250:
URL: https://github.com/apache/arrow-rs/pull/4250#issuecomment-1557220231
I will review this PR carefully later today or tomorrow. Thank you @roeap
--
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] roeap commented on a diff in pull request #4250: feat: update flight-sql to latest specs
Posted by "roeap (via GitHub)" <gi...@apache.org>.
roeap commented on code in PR #4250:
URL: https://github.com/apache/arrow-rs/pull/4250#discussion_r1200670099
##########
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:
There is a status macro defined, which also reports line numbers.
```rs
macro_rules! status {
($desc:expr, $err:expr) => {
Status::internal(format!("{}: {} at {}:{}", $desc, $err, file!(), line!()))
};
}
```
Maybe we could add another macro that uses `Status::unimplemented` to communicate the right status and have the line info available? Something along the lines of
```rs
macro_rules! status_unimplemented {
($method:expr) => {
Status::unimplemented(format!("not implemented: {} at {}:{}", $method, file!(), line!()))
};
}
```
--
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] roeap commented on a diff in pull request #4250: feat: update flight-sql to latest specs
Posted by "roeap (via GitHub)" <gi...@apache.org>.
roeap commented on code in PR #4250:
URL: https://github.com/apache/arrow-rs/pull/4250#discussion_r1200423796
##########
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:
just matched what was there ... but yes, adding some `else`s makes much more sene :)
--
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] tustvold commented on a diff in pull request #4250: feat: update flight-sql to latest specs
Posted by "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
[GitHub] [arrow-rs] alamb commented on pull request #4250: feat: update flight-sql to latest specs
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #4250:
URL: https://github.com/apache/arrow-rs/pull/4250#issuecomment-1557893430
> Definitely - should they however go here, or would they need to be in the datafusion example server and include extracting those from the session context?
I think they can be implemented directly with the arrow kernels without datafusion -- the comparisons needed are pretty simple (string matching) and we did this in IOx to avoid the overhead of creating a query plan.
> Also, I'd be happy to take on these issues and move the code to arrow-flight.
Thank you @roeap -- that is awesome. ❤️ I will plan to file the other tickets tomorrow and would love to help design / review the PRs.
--
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 #4250: feat: update flight-sql to latest specs
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #4250:
URL: https://github.com/apache/arrow-rs/pull/4250#issuecomment-1557210650
> @alamb - playing around with implementing a flight sql server I read a bit through the iOx implementation. Over there I found that the structs around sql infos (SqlInfoValue, SqlInfoList, SqlInfoUnionBuilder) are quite useful and probably non trivial to figure out for users. Do you think this is something that could be hoisted upstream?
Yes, absolutely we could hoist this upstream.
I filed these tickets to do so:
* `CommandGetSqlInfo`: https://github.com/apache/arrow-rs/issues/4256
* `CommandGetXdbcTypeInfo`: https://github.com/apache/arrow-rs/issues/4257
There are several other things that could be hoisted upstream (like the basic implementations for `GetTables` and `GetSchemas` both of which need basic filtering). Do you think those are worth pulling upstream too?
--
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