You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "alamb (via GitHub)" <gi...@apache.org> on 2023/04/03 19:10:58 UTC
[GitHub] [arrow-rs] alamb opened a new pull request, #4012: Alamb/update docsss
alamb opened a new pull request, #4012:
URL: https://github.com/apache/arrow-rs/pull/4012
# Which issue does this PR close?
re https://github.com/apache/arrow-rs/pull/3887
# Rationale for this change
While waiting for CI to run on https://github.com/apache/arrow-rs/pull/3887 I spent some time writing docs to make it easier to understand what is going on / what we have in this crate.
# What changes are included in this PR?
1. Add a breadcrumb to `arrow-flight` to point at the `sql` feature.
2. Add module documentation to arrow-flight sql with some orienting text and pointers
# Are there any user-facing changes?
more docs
--
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 #4012: Add FlightSQL module docs and links to `arrow-flight` crates
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #4012:
URL: https://github.com/apache/arrow-rs/pull/4012#discussion_r1157781070
##########
arrow-flight/src/lib.rs:
##########
@@ -30,6 +30,11 @@
//!
//! 2. Low level [tonic] generated [`flight_service_client`] and
//! [`flight_service_server`].
+//!
+//! 3. Experimental support for [Flight SQL] in [`sql`]. Requires the
Review Comment:
> backwards compatibility fairly
We break Rust API compatibility for sure, protocol compatibility is a different beast imo, as providing an incremental migration story becomes very important for upgrades to actually be possible
--
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 #4012: Add FlightSQL module docs and links to `arrow-flight` crates
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #4012:
URL: 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 #4012: Add FlightSQL module docs and links to `arrow-flight` crates
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #4012:
URL: https://github.com/apache/arrow-rs/pull/4012#discussion_r1157727782
##########
arrow-flight/src/lib.rs:
##########
@@ -30,6 +30,11 @@
//!
//! 2. Low level [tonic] generated [`flight_service_client`] and
//! [`flight_service_server`].
+//!
+//! 3. Experimental support for [Flight SQL] in [`sql`]. Requires the
Review Comment:
> The experimental flag allows us to "fix" such things without needing to preserve backwards compatibility
Well, I guess I was thinking we break backwards compatibility fairly regularly (as in there are several API changes per release)
So it isn't like I think the flight sql feature is fully ready and the API won't change -- more like I wonder if we should treat it specially from the rest of arrow-flight
--
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 #4012: Add FlightSQL module docs and links to `arrow-flight` crates
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #4012:
URL: https://github.com/apache/arrow-rs/pull/4012#discussion_r1156939479
##########
arrow-flight/src/sql/mod.rs:
##########
@@ -15,6 +15,26 @@
// specific language governing permissions and limitations
// under the License.
+//! Support for execute SQL queries using [Apache Arrow] [Flight SQL].
+//!
+//! [Flight SQL] is built on top of Arrow Flight RPC framework, by
+//! defining specific message, encoded using the protobuf format, sent
Review Comment:
```suggestion
//! defining specific messages, encoded using the protobuf format, sent
```
##########
arrow-flight/src/sql/mod.rs:
##########
@@ -15,6 +15,26 @@
// specific language governing permissions and limitations
// under the License.
+//! Support for execute SQL queries using [Apache Arrow] [Flight SQL].
+//!
+//! [Flight SQL] is built on top of Arrow Flight RPC framework, by
Review Comment:
This might be easier to read if the link anchors were moved to the end
--
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 #4012: Add FlightSQL module docs and links to `arrow-flight` crates
Posted by "avantgardnerio (via GitHub)" <gi...@apache.org>.
avantgardnerio commented on code in PR #4012:
URL: https://github.com/apache/arrow-rs/pull/4012#discussion_r1156427003
##########
arrow-flight/src/lib.rs:
##########
@@ -30,6 +30,11 @@
//!
//! 2. Low level [tonic] generated [`flight_service_client`] and
//! [`flight_service_server`].
+//!
+//! 3. Experimental support for [Flight SQL] in [`sql`]. Requires the
Review Comment:
I'd love to make this not experimental BTW. It's just a struct and impl in a library AFAIK, so it shouldn't hurt anyone.
--
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 #4012: Add FlightSQL module docs and links to `arrow-flight` crates
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #4012:
URL: https://github.com/apache/arrow-rs/pull/4012#discussion_r1158380319
##########
arrow-flight/src/lib.rs:
##########
@@ -30,6 +30,11 @@
//!
//! 2. Low level [tonic] generated [`flight_service_client`] and
//! [`flight_service_server`].
+//!
+//! 3. Experimental support for [Flight SQL] in [`sql`]. Requires the
Review Comment:
I see -- you are perhaps imagining that the FlightSQL protocol itself may change, not just the rust implementation. The spec is marked as experimental in many places. 🤷
https://github.com/apache/arrow/blob/ad115be1214b13ce393537bdb9c34ae919e4997f/format/FlightSql.proto#LL46
--
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 #4012: Add FlightSQL module docs and links to `arrow-flight` crates
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #4012:
URL: https://github.com/apache/arrow-rs/pull/4012#discussion_r1157710824
##########
arrow-flight/src/lib.rs:
##########
@@ -30,6 +30,11 @@
//!
//! 2. Low level [tonic] generated [`flight_service_client`] and
//! [`flight_service_server`].
+//!
+//! 3. Experimental support for [Flight SQL] in [`sql`]. Requires the
Review Comment:
Where did we get to with an integration test of FlightSQL? I think that would help justify graduating this functionality perhaps?
I'm somewhat apprehensive there may still be areas we have interpreted the specification differently, one was fixed last week. The experimental flag allows us to "fix" such things without needing to preserve backwards compatibility
--
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 #4012: Add FlightSQL module docs and links to `arrow-flight` crates
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #4012:
URL: https://github.com/apache/arrow-rs/pull/4012#discussion_r1157661289
##########
arrow-flight/src/lib.rs:
##########
@@ -30,6 +30,11 @@
//!
//! 2. Low level [tonic] generated [`flight_service_client`] and
//! [`flight_service_server`].
+//!
+//! 3. Experimental support for [Flight SQL] in [`sql`]. Requires the
Review Comment:
Yeah, I think we were just trying to give people the heads up that we would likely be making substantial changes to the API -- but then we do that with other parts of arrow, so maybe the extra "experimental" part is unecessary.
What do you think @tustvold / @viirya -- should we rename the feature flag from experimental (and maybe turn it on by default?)
##########
arrow-flight/src/sql/mod.rs:
##########
@@ -15,6 +15,26 @@
// specific language governing permissions and limitations
// under the License.
+//! Support for execute SQL queries using [Apache Arrow] [Flight SQL].
+//!
+//! [Flight SQL] is built on top of Arrow Flight RPC framework, by
Review Comment:
Good idea -- done in 7c8f3e172
--
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