Custom / Dynamic table provider factories #3311

avantgardnerio · 2022-08-31T20:44:27Z

Which issue does this PR close?

Closes #3310.

Rationale for this change

I would like to allow users to register custom table types at runtime.

What changes are included in this PR?

A new concept of a TableProviderFactory
A new FileType::Custom(String) to allow the parser to pass information about these tables
A HashMap of TableProviderFactorys on the SessionContext to allow them to be registered programmatically but used dynamically

Are there any user-facing changes?

They can now register custom tables.

avantgardnerio · 2022-08-31T20:46:57Z

@andygrove @alamb @houqp This was a quick and dirty implementation, but I thought I would throw it out there to collect your thoughts on it. If something like this is deemed acceptable and merged, we'd extend Ballista to use it as well, then either:

make a fork of Ballista with delta-rs included
make our own server that depends on Ballista as a library but also depends on delta-rs and has it's own void main()
Add a plugin architecture to Ballista to allow it to load delta-rs dynamically as an .so file

avantgardnerio · 2022-08-31T20:48:52Z

datafusion/sql/src/parser.rs

-            "expect one of PARQUET, AVRO, NDJSON, or CSV, found: {}",
-            other
-        ))),
+        name => Ok(FileType::Custom(name.to_string()))


Eventually I'd like to un-hard-code the built-in tables and just have a ListingTableFactory in the map so all these behave the same.

Also, I'd love to use the postgres with options syntax to pass an arbitrary hashmap for credentials, host & port, etc.

https://www.postgresql.org/docs/current/sql-createforeigntable.html

CREATE FOREIGN TABLE [ IF NOT EXISTS ] table_name ( [ { column_name data_type [ OPTIONS ( option 'value' [, ... ] ) ]

On second thought, credentials could be controversial... forget I said that :)

andygrove · 2022-08-31T21:02:35Z

I think this looks like a great start. Thanks @avantgardnerio

avantgardnerio · 2022-08-31T21:04:53Z

Oh, also related to #2025

avantgardnerio · 2022-08-31T21:51:54Z

I guess I should mention @matthewmturner since I linked that ticket.

matthewmturner · 2022-08-31T22:26:07Z

@avantgardnerio This is very cool. Unfortunately I haven't been able to work on datafusion lately but I could definitely see integrating this into datafusion tui again when I start again.

alamb

I also really like where this PR is heading

datafusion/core/src/datasource/datasource.rs

datafusion/core/src/execution/context.rs

alamb · 2022-09-01T13:34:43Z

datafusion/core/tests/sql/create_drop.rs

+    ctx.register_table_factory("DELTATABLE", Arc::new(TestTableFactory {}));
+
+    let sql = "CREATE EXTERNAL TABLE dt STORED AS DELTATABLE LOCATION 's3://bucket/schema/table';";
+    ctx.sql(sql).await.unwrap();
+


This API is very cool -- I like it a lot 💯

Cool, affirmation of general direction was what I was looking for, I'll clean it up and get it submitted, ty!

avantgardnerio · 2022-09-02T17:10:44Z

@ianmcook I should probably start CCing you on these PRs as well.

Sorry I meant @iajoiner

codecov-commenter · 2022-09-02T18:00:11Z

Codecov Report

Merging #3311 (761f384) into master (b175f9a) will increase coverage by 0.02%.
The diff coverage is 70.75%.

@@            Coverage Diff             @@
##           master    #3311      +/-   ##
==========================================
+ Coverage   85.48%   85.51%   +0.02%     
==========================================
  Files         294      294              
  Lines       54115    54120       +5     
==========================================
+ Hits        46259    46279      +20     
+ Misses       7856     7841      -15

Impacted Files	Coverage Δ
datafusion/core/src/datasource/datasource.rs	`100.00% <ø> (ø)`
datafusion/core/tests/sql/timestamp.rs	`99.65% <ø> (-0.01%)`	⬇️
datafusion/expr/src/logical_plan/plan.rs	`77.77% <ø> (+0.33%)`	⬆️
datafusion/proto/src/lib.rs	`93.52% <ø> (ø)`
datafusion/proto/src/logical_plan.rs	`17.81% <0.00%> (+0.49%)`	⬆️
datafusion/core/src/execution/context.rs	`78.96% <78.72%> (+0.20%)`	⬆️
datafusion/core/tests/sql/create_drop.rs	`91.44% <80.00%> (-2.26%)`	⬇️
datafusion/sql/src/parser.rs	`83.52% <100.00%> (-0.45%)`	⬇️
datafusion/sql/src/planner.rs	`80.55% <100.00%> (+0.21%)`	⬆️
... and 2 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

alamb

I think this looks great -- thank you @avantgardnerio

It does contain the changes from #3333 as well, so I think we should probably merge #3333 first and then rebase this one.

I had several small comments / style suggestions, but overall I think this PR is basically ready to go. I didn't "approve" it as it has the #3333 changes in it as well and wanted to give you a chance to respond to suggestions

Something I suggest as well is a datafusion-example in https://github.com/apache/arrow-datafusion/tree/master/datafusion-examples/examples

The rationale for doing so is:

Serves as end-to-end test (that uses DataFusion's public API only) so we don't inadvertently break something if code is moved around (we did that in the past when we moved something and it became non-pub 🤦 ) as the scoping / visibility rules are different in crate / out of crate.
Serves as a documentation / starting point for others to actually use it.

Such an end to end can be done as a follow on PR (or we could just file a ticket to track the work)

alamb · 2022-09-04T11:05:50Z

.github/workflows/rust.yml

@@ -277,7 +277,9 @@ jobs:
          rustup default stable
          rustup component add rustfmt
      - name: Run
-        run: ci/scripts/rust_fmt.sh
+        run: |
+          echo '' > datafusion/proto/src/generated/datafusion.rs


I think this is a left over from #3333 -- but I also don't think it hurts to leave it in this PR

datafusion/core/src/datasource/datasource.rs

datafusion/core/src/execution/context.rs

datafusion/core/tests/sql/create_drop.rs

alamb · 2022-09-04T11:18:06Z

datafusion/proto/src/logical_plan.rs

-                let pb_file_type: protobuf::FileType =
-                    create_extern_table.file_type.try_into()?;
+                match create_extern_table.file_type.as_str() {
+                    "CSV" | "JSON" | "PARQUET" | "AVRO" => {}


It might be nice to avoid having these four strings hardcoded in several places, so that if we add another format, we don't have to make changes all over the place.

Maybe we could put it into a struct like BuiltInTableProviders or something

I 100% agree, but I was hoping to do it in a follow-on PR, as there seems to be a lot of action going on with TableProviders right now, and I was hoping to not keep this one long-standing.

alamb · 2022-09-04T11:20:22Z

datafusion/expr/src/logical_plan/plan.rs

@@ -1214,19 +1214,6 @@ pub struct CreateView {
    pub definition: Option<String>,
 }

-/// Types of files to parse as DataFrames


Another potential way to write this (and maybe reduce the diff) would be to add a new enum variant like "Dynamic":

pub enum FileType { /// Newline-delimited JSON NdJson, ... /// Custom factory Dynamic(String), }

This strategy might be more "idiomatic" as it encodes more information in the type system rather than a String

I don't feel strongly about this, I just wanted to offer it as an option

I started with that solution, but I think it makes more sense to go:

enum (as it was)

string (with hard-coded values)

everything-as-a-TableProviderFactory (string with no hard-coded values)

If this PR is accepted, I will move immediately onto step #3.

datafusion/sql/src/parser.rs

alamb

I believe this will cause a conflict with #3279 from @psvri

alamb · 2022-09-05T10:32:57Z

Thanks @avantgardnerio

thinkharderdev · 2022-09-05T10:34:31Z

datafusion/core/src/datasource/datasource.rs

+///
+/// For example, this can be used to create a table "on the fly"
+/// from a directory of files only when that name is referenced.  
+pub trait TableProviderFactory: Sync + Send {


Should create be async?

The TableProvider will have to know the Schema which will most likely have to be inferred by reading from the ObjectStore (or involve a network call otherwise if there is some sort of metastore involved).

ursabot · 2022-09-05T10:41:50Z

Benchmark runs are scheduled for baseline = b175f9a and contender = bb08d31. bb08d31 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

github-actions bot added core Core datafusion crate logical-expr Logical plan and expressions sql labels Aug 31, 2022

avantgardnerio commented Aug 31, 2022

View reviewed changes

alamb reviewed Sep 1, 2022

View reviewed changes

avantgardnerio mentioned this pull request Sep 1, 2022

[Ballista] Support to access remote object store, like HDFS, S3, etc apache/datafusion-ballista#6

Open

avantgardnerio marked this pull request as ready for review September 2, 2022 17:16

avantgardnerio requested a review from alamb September 2, 2022 17:16

alamb changed the title ~~Custom table provider factories~~ Custom / Dynamic table provider factories Sep 4, 2022

alamb reviewed Sep 4, 2022

View reviewed changes

avantgardnerio mentioned this pull request Sep 4, 2022

Support for custom ParquetFileReaderFactory apache/datafusion-ballista#186

Open

Brent Gardner added 2 commits September 4, 2022 19:09

Custom table provider factories

a40d4e4

Rebase

761f384

avantgardnerio force-pushed the bg_table_factory_pr branch from 319619b to 761f384 Compare September 5, 2022 01:30

alamb approved these changes Sep 5, 2022

View reviewed changes

alamb merged commit bb08d31 into apache:master Sep 5, 2022

thinkharderdev reviewed Sep 5, 2022

View reviewed changes

This was referenced Sep 5, 2022

Move codegen into src/ folder #3333

Closed

Allow IDEs to recognize generated code #3332

Closed

avantgardnerio deleted the bg_table_factory_pr branch September 5, 2022 16:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Custom / Dynamic table provider factories #3311

Custom / Dynamic table provider factories #3311

avantgardnerio commented Aug 31, 2022 •

edited

avantgardnerio commented Aug 31, 2022

avantgardnerio Aug 31, 2022

avantgardnerio Aug 31, 2022 •

edited

andygrove commented Aug 31, 2022

avantgardnerio commented Aug 31, 2022

avantgardnerio commented Aug 31, 2022

matthewmturner commented Aug 31, 2022

alamb left a comment

alamb Sep 1, 2022

avantgardnerio Sep 1, 2022

avantgardnerio commented Sep 2, 2022 •

edited

codecov-commenter commented Sep 2, 2022 •

edited

alamb left a comment

alamb Sep 4, 2022

alamb Sep 4, 2022

avantgardnerio Sep 5, 2022

alamb Sep 4, 2022

avantgardnerio Sep 5, 2022

alamb left a comment

alamb commented Sep 5, 2022

thinkharderdev Sep 5, 2022

ursabot commented Sep 5, 2022

Custom / Dynamic table provider factories #3311

Custom / Dynamic table provider factories #3311

Conversation

avantgardnerio commented Aug 31, 2022 • edited

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

avantgardnerio commented Aug 31, 2022

Choose a reason for hiding this comment

avantgardnerio Aug 31, 2022 • edited

Choose a reason for hiding this comment

andygrove commented Aug 31, 2022

avantgardnerio commented Aug 31, 2022

avantgardnerio commented Aug 31, 2022

matthewmturner commented Aug 31, 2022

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

avantgardnerio commented Sep 2, 2022 • edited

codecov-commenter commented Sep 2, 2022 • edited

Codecov Report

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

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

alamb commented Sep 5, 2022

Choose a reason for hiding this comment

ursabot commented Sep 5, 2022

avantgardnerio commented Aug 31, 2022 •

edited

avantgardnerio Aug 31, 2022 •

edited

avantgardnerio commented Sep 2, 2022 •

edited

codecov-commenter commented Sep 2, 2022 •

edited