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/06/01 21:07:11 UTC
[GitHub] [arrow-rs] alamb opened a new pull request, #4341: Update FlightSQL metadata locations, names and docs
alamb opened a new pull request, #4341:
URL: https://github.com/apache/arrow-rs/pull/4341
# Which issue does this PR close?
This is a follow on to https://github.com/apache/arrow-rs/pull/4296 from @roeap and related to https://github.com/apache/arrow-rs/issues/4295
# Rationale for this change
Since we haven't released these APIs yet and now that we have several metadata builders I wanted to put them in a unified location in the flight sql implementation
# What changes are included in this PR?
1. Move all the metadata builders into the `sql::metadata` module
2. Reduce the API surface by moving `schema()` to methods on the builders
3. Update and add some more docstrings
<!--
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?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!---
If there are any breaking changes to public APIs, please add the `breaking change` label.
-->
--
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 #4341: Update FlightSQL metadata locations, names and docs
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #4341:
URL: https://github.com/apache/arrow-rs/pull/4341#discussion_r1214217754
##########
arrow-flight/examples/flight_sql_server.rs:
##########
@@ -252,7 +249,7 @@ impl FlightSqlService for FlightSqlServiceImpl {
let endpoint = FlightEndpoint::new().with_ticket(ticket);
let flight_info = FlightInfo::new()
- .try_with_schema(get_catalogs_schema())
+ .try_with_schema(&query.into_builder().schema())
Review Comment:
Turning the query into a builder is so beautiful -- it was a great idea @roeap
##########
arrow-flight/src/sql/metadata/db_schemas.rs:
##########
@@ -33,26 +33,13 @@ use super::lexsort_to_indices;
use crate::error::*;
use crate::sql::CommandGetDbSchemas;
-/// Return the schema of the RecordBatch that will be returned from [`CommandGetDbSchemas`]
+/// A builder for a [`CommandGetDbSchemas`] response.
///
-/// [`CommandGetDbSchemas`]: crate::sql::CommandGetDbSchemas
-pub fn get_db_schemas_schema() -> SchemaRef {
- Arc::clone(&GET_DB_SCHEMAS_SCHEMA)
-}
-
-/// The schema for GetDbSchemas
-static GET_DB_SCHEMAS_SCHEMA: Lazy<SchemaRef> = Lazy::new(|| {
- Arc::new(Schema::new(vec![
- Field::new("catalog_name", DataType::Utf8, false),
- Field::new("db_schema_name", DataType::Utf8, false),
- ]))
-});
-
/// Builds rows like this:
///
/// * catalog_name: utf8,
/// * db_schema_name: utf8,
-pub struct GetSchemasBuilder {
+pub struct GetDbSchemasBuilder {
Review Comment:
renamed to follow the name of the flightSQL request struct (which has a strange `Db` in it 🤷 )
--
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 pull request #4341: Update FlightSQL metadata locations, names and docs
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #4341:
URL: https://github.com/apache/arrow-rs/pull/4341#issuecomment-1573482887
CI appears to be failing for this?
--
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 #4341: Update FlightSQL metadata locations, names and docs
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #4341:
URL: https://github.com/apache/arrow-rs/pull/4341
--
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 #4341: Update FlightSQL metadata locations, names and docs
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #4341:
URL: https://github.com/apache/arrow-rs/pull/4341#issuecomment-1573521185
Fixing...
--
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 #4341: Update FlightSQL metadata locations, names and docs
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #4341:
URL: https://github.com/apache/arrow-rs/pull/4341#issuecomment-1572787605
@tustvold if possible it would be great to get this PR into the release
--
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 #4341: Update FlightSQL metadata locations, names and docs
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #4341:
URL: https://github.com/apache/arrow-rs/pull/4341#discussion_r1213681550
##########
arrow-flight/src/sql/catalogs/mod.rs:
##########
@@ -1,123 +0,0 @@
-// Licensed to the Apache Software Foundation (ASF) under one
Review Comment:
moved to metadata/mod.rs
##########
arrow-flight/src/sql/mod.rs:
##########
@@ -27,7 +27,7 @@
//! 2. Helpers for encoding and decoding FlightSQL messages: [`Any`] and [`Command`]
//! 3. A [`FlightSqlServiceClient`] for interacting with FlightSQL servers.
//! 4. A [`FlightSqlService`] to help building FlightSQL servers from [`FlightService`].
-//! 5. Structures to build responses for FlightSQL metadata APIs: [`SqlInfoList`]
+//! 5. Helpers to build responses for FlightSQL metadata APIs: [`metadata`]
Review Comment:
I think the docs now look more unified and lead people to the `metadata` crate more easily
##########
arrow-flight/src/sql/metadata/mod.rs:
##########
@@ -0,0 +1,55 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Builders and function for building responses to FlightSQL metadata
+//! / information schema requests.
+//!
+//! - [`GetCatalogsBuilder`] for building responses to [`CommandGetCatalogs`] queries.
+//! - [`GetDbSchemasBuilder`] for building responses to [`CommandGetDbSchemas`] queries.
+//! - [`GetTablesBuilder`]for building responses to [`CommandGetTables`] queries.
+//!
+//! [`CommandGetCatalogs`]: crate::sql::CommandGetCatalogs
+//! [`CommandGetDbSchemas`]: crate::sql::CommandGetDbSchemas
+//! [`CommandGetTables`]: crate::sql::CommandGetTables
+
+mod catalogs;
+mod db_schemas;
+mod sql_info;
+mod tables;
+
+pub use catalogs::GetCatalogsBuilder;
+pub use db_schemas::GetDbSchemasBuilder;
+pub use sql_info::SqlInfoList;
Review Comment:
As a follow on it would be awesome to make a `SqlInfoList` builder similarly to the other builders, but I ran out of time today and figured I will do it as a follow on PR
--
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 #4341: Update FlightSQL metadata locations, names and docs
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #4341:
URL: https://github.com/apache/arrow-rs/pull/4341#issuecomment-1573554313
I am going to take the liberty of merging this PR to get it into the release. We can refine the APIs later but I would like to avoid API churn as much as possible (and this is API churn, but on an unreleased API)
--
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