feat(flight): add helpers to handle `CommandGetCatalogs`, `CommandGetSchemas`, and `CommandGetTables` requests #4296

roeap · 2023-05-27T14:36:24Z

Which issue does this PR close?

Closes #4295

Rationale for this change

see #4295

What changes are included in this PR?

The main builder implementations were lifted from IOx, but altered to also include sorting the created record batches.

cc @alamb @avantgardnerio

Are there any user-facing changes?

Users can use the added Builders and methods to create responses to CommandGetCatalogs, CommandGetSchemas, and CommandGetTables requests.

alamb · 2023-05-28T12:05:23Z

Thanks @roeap -- I will review this carefully, but I probably won't have a chance until tomorrow or Tuesday

alamb · 2023-05-28T12:06:19Z

FYI @appletreeisyellow

arrow-flight/examples/flight_sql_server.rs

alamb

Thank you @roeap -- I think this looks great!

I had some small API suggestions, but I don't think any are required (or I may propose them as a follow on PR, depending on your feedback).

Really appreciated 🙏

arrow-flight/Cargo.toml

arrow-flight/examples/flight_sql_server.rs

alamb · 2023-05-30T12:57:21Z

arrow-flight/examples/flight_sql_server.rs

        _request: Request<Ticket>,
    ) -> Result<Response<<Self as FlightService>::DoGetStream>, Status> {
-        Err(Status::unimplemented("do_get_tables not implemented"))
+        let tables = TABLES


I wonder if it is worth a comment that this does not implement full SQL style multipart identifier semantics to warn anyone who tries to use this code as a starting point for implementing FlightSQL? Maybe that will be obvious to anyone who uses this code, but we fought for quite a while to get consistent identifier semantics in DataFusion (one needs to parse it via SQL)

alamb · 2023-05-30T13:00:03Z

arrow-flight/examples/flight_sql_server.rs

+            .collect::<HashSet<_>>();
+
+        let mut builder = GetSchemasBuilder::new(query.db_schema_filter_pattern);
+        if let Some(catalog) = query.catalog {


Given that the equality matching on catalog should be the same for all implementations, what do you think about putting it in the builder itself?

The same comment applies to the GetTablesBuilder too

We can do this as a follow on PR too (or never)

Something like:

let mut builder = GetSchemasBuilder::new(query.db_schema_filter_pattern); if let Some(catalog) = query.catalog { builder = builder.with_catalog_filter(catalog) } for (catalog_name, schema_name) in schemas { builder .append(catalog_name, schema_name) .map_err(Status::from)?; }

@alamb, since we are then using all the fields on the command object, should we just convert one into the other?

Someting like given query is CommandGetDbSchemas.

let mut builder: GetSchemasBuilder = query.builder();

implementing from, with maybe the downside, that you would always actually have to cast the type..

let mut builder: GetSchemasBuilder = query.into();

arrow-flight/src/sql/catalogs/db_schemas.rs

alamb · 2023-05-30T13:06:51Z

arrow-flight/src/sql/catalogs/tables.rs

+impl GetTablesBuilder {
+    /// Create a new instance of [`GetTablesBuilder`]
+    ///
+    /// The builder handles filtering by schema and table patterns, the caller


As I mentioned elsewhere I think it might be worth encapsulating all filtering in the Builders to make the API easier to use

arrow-flight/src/sql/catalogs/tables.rs

arrow-flight/src/sql/catalogs/db_schemas.rs

Co-authored-by: Andrew Lamb <[email protected]>

* Improve docs and tests for SqlInfoList * Add an example/ * Update arrow-flight/src/sql/sql_info.rs Co-authored-by: Liang-Chi Hsieh <[email protected]> --------- Co-authored-by: Liang-Chi Hsieh <[email protected]>

alamb

I took the liberty of fixing cargo fmt and merging up from the master branch.

@roeap shall we merge this PR as is, or do you plan to make any changes?

roeap · 2023-05-31T13:26:34Z

I wanted to follow up on your suggestions- integrating catalog filters etc.

I'll leave it up to you if that should be here or in a followup 🙂

alamb · 2023-05-31T14:02:04Z

I'll leave it up to you if that should be here or in a followup 🙂

If you would like to do so, let's do it here so we don't accidentally release an API that is undergoing changes (I expect @tustvold will likely make the next release candidate on Friday, in 2 days. Thus if we merged this PR we would have a self imposed deadline of those 2 days to get subsequent changes in)

roeap · 2023-05-31T14:09:05Z

Should be feasible to get it done by then.

Also have a first draft for the xdbc stuff available- stretch goal .. 🙂

alamb · 2023-05-31T15:45:03Z

Thank you so much @roeap !

roeap

@alamb - Added the additional filtering. This also made it quite trivial to convert the request directly into the builder, which makes for quite a convenient API - I think. So worthwhile eeffort in my opinion :).

roeap · 2023-05-31T20:15:36Z

arrow-flight/src/sql/catalogs/mod.rs

+mod tables;
+
+/// Returns the RecordBatch for
+pub fn get_catalogs_batch(mut catalog_names: Vec<String>) -> Result<RecordBatch> {


Did a quick builder implementation - consistency is always nice ...

roeap · 2023-05-31T20:22:45Z

arrow-flight/src/sql/catalogs/tables.rs

+    /// Append a row
+    pub fn append(
+        &mut self,
+        catalog_name: impl AsRef<str>,


I was not completely sure how this would be expected. The filter distinguishes None which means no filtering vs "" which means empty catalog. So in order to specify an empty catalog, one would pass in "". Thought about making this optional, and then testing for nulls in the filter, but in the end I felt its trivial to default to empty string for the caller, so handling nulls in the catalog array is not necessary.

roeap · 2023-05-31T20:23:28Z

arrow-flight/src/sql/catalogs/tables.rs

+            .collect::<std::result::Result<Vec<_>, _>>()?
+            .into_iter()
+            // We know the arrays are of same length as they are produced fromn the same root array
+            .reduce(|filter, arr| or(&filter, &arr).unwrap());


or should only fail on misaligned arrays, which we should never see here.

roeap · 2023-06-01T07:31:46Z

arrow-flight/examples/flight_sql_server.rs

+        let stream = FlightDataEncoderBuilder::new()
+            .with_schema(get_db_schemas_schema())
+            .build(futures::stream::once(async { batch }))
+            .map_err(Status::from);


Seeing that this code is also always the same, should we add a method build_stream?

roeap · 2023-06-01T19:56:10Z

@alamb - can we make it to get that in by tomorrow? 😄.

alamb

Thanks @roeap -- this is looking great. I am going to merge this as is and try to help with a small follow on with some additional docs.

alamb · 2023-06-01T20:29:07Z

arrow-flight/examples/flight_sql_server.rs

-                    .map_err(Status::from)?;
-            }
-        };
+        let mut builder = query.into_builder();


❤️ that is very nice

alamb · 2023-06-01T20:30:41Z

arrow-flight/src/sql/catalogs/db_schemas.rs

+            )?)
+        }
+
+        if let Some(catalog_filter_name) = catalog_filter {


it doesn't matter here, but this can be implemented more efficiently (in a follow on PR perhaps) by checking equality in append

alamb · 2023-06-01T20:32:28Z

arrow-flight/src/sql/catalogs/tables.rs

+}
+
+impl CommandGetTables {
+    pub fn into_builder(self) -> GetTablesBuilder {


I really like the into_builder() function 👍

roeap added 8 commits May 27, 2023 12:37

feat: add get catalog helpers

223b80a

fix: clippy in arrow_row example

38ab8f1

feat: add db schemas helpers

52e37d5

chore: cleanup db schemas helpers

9f66a22

feat: add table schema hlpers

52b5968

test: add tests and docs

594d31b

docs: add table queries to example server

e01a351

docs: improve builder docs

20f34d7

github-actions bot added arrow Changes to the arrow crate arrow-flight Changes to the arrow-flight crate labels May 27, 2023

fix: docs links

a7f69ee

avantgardnerio approved these changes May 29, 2023

View reviewed changes

arrow-flight/examples/flight_sql_server.rs Outdated Show resolved Hide resolved

alamb approved these changes May 30, 2023

View reviewed changes

roeap and others added 7 commits May 30, 2023 22:33

Apply suggestions from code review

ddfbafe

Co-authored-by: Andrew Lamb <[email protected]>

Improve docs and tests for `SqlInfoList (apache#4293)

1ad6167

* Improve docs and tests for SqlInfoList * Add an example/ * Update arrow-flight/src/sql/sql_info.rs Co-authored-by: Liang-Chi Hsieh <[email protected]> --------- Co-authored-by: Liang-Chi Hsieh <[email protected]>

Improve docs and tests for `SqlInfoList (apache#4293)

f75b846

* Improve docs and tests for SqlInfoList * Add an example/ * Update arrow-flight/src/sql/sql_info.rs Co-authored-by: Liang-Chi Hsieh <[email protected]> --------- Co-authored-by: Liang-Chi Hsieh <[email protected]>

fix: use FlightInfo builders

3fa5bef

chore: clippy

fc9e91d

fmt

2f06792

Merge remote-tracking branch 'apache/master' into sql-catalogs

2255d49

alamb approved these changes May 31, 2023

View reviewed changes

roeap added 3 commits May 31, 2023 21:39

feat: add filters to GetTablesBuilder

0aa8346

fix: clippy

dba0bcd

feat: more consistent builder apis

9ffbfa9

chore: cleanup

3b3da92

roeap commented May 31, 2023

View reviewed changes

roeap commented Jun 1, 2023

View reviewed changes

alamb approved these changes Jun 1, 2023

View reviewed changes

alamb merged commit dde6539 into apache:master Jun 1, 2023

roeap deleted the sql-catalogs branch June 1, 2023 20:55

alamb mentioned this pull request Jun 1, 2023

Update FlightSQL metadata locations, names and docs #4341

Merged

tustvold mentioned this pull request Jun 2, 2023

FLightSQL: add helpers to create CommandGetCatalogs, CommandGetSchemas, and CommandGetTables requests #4295

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(flight): add helpers to handle `CommandGetCatalogs`, `CommandGetSchemas`, and `CommandGetTables` requests #4296

feat(flight): add helpers to handle `CommandGetCatalogs`, `CommandGetSchemas`, and `CommandGetTables` requests #4296

roeap commented May 27, 2023

alamb commented May 28, 2023

alamb commented May 28, 2023

alamb left a comment

alamb May 30, 2023

alamb May 30, 2023

roeap May 31, 2023 •

edited

Loading

alamb May 30, 2023

alamb left a comment

roeap commented May 31, 2023

alamb commented May 31, 2023

roeap commented May 31, 2023

alamb commented May 31, 2023

roeap left a comment

roeap May 31, 2023

roeap May 31, 2023 •

edited

Loading

roeap May 31, 2023

roeap Jun 1, 2023

roeap commented Jun 1, 2023

alamb left a comment

alamb Jun 1, 2023

alamb Jun 1, 2023

alamb Jun 1, 2023

feat(flight): add helpers to handle CommandGetCatalogs, CommandGetSchemas, and CommandGetTables requests #4296

feat(flight): add helpers to handle CommandGetCatalogs, CommandGetSchemas, and CommandGetTables requests #4296

Conversation

roeap commented May 27, 2023

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

alamb commented May 28, 2023

alamb commented May 28, 2023

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roeap May 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

roeap commented May 31, 2023

alamb commented May 31, 2023

roeap commented May 31, 2023

alamb commented May 31, 2023

roeap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roeap May 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roeap commented Jun 1, 2023

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feat(flight): add helpers to handle `CommandGetCatalogs`, `CommandGetSchemas`, and `CommandGetTables` requests #4296

feat(flight): add helpers to handle `CommandGetCatalogs`, `CommandGetSchemas`, and `CommandGetTables` requests #4296

roeap May 31, 2023 •

edited

Loading

roeap May 31, 2023 •

edited

Loading