Add TableProviderFactory and test for SQL to register tables dynamically at runtime #892

avantgardnerio · 2022-10-17T21:24:49Z

Description

Use (hopefully soon-to-be-merged) Datafusion functionality to allow dynamic (i.e. SQL-based) registration of deltalake tables at runtime.

Related Issue(s)

closes Support CREATE EXTERNAL TABLE dt STORED AS DELTATABLE syntax with datafusion #891

Documentation

TBD

avantgardnerio · 2022-10-17T21:45:20Z

rust/tests/datafusion_test.rs

+
+    let mut d = PathBuf::from(env!("CARGO_MANIFEST_DIR"));
+    d.push("tests/data/delta-0.8.0-partitioned");
+    let sql = format!("CREATE EXTERNAL TABLE demo STORED AS DELTATABLE LOCATION '{}'", d.to_str().unwrap());


This is the goal of the PR

avantgardnerio · 2022-10-17T21:45:47Z

rust/Cargo.toml

@@ -9,7 +9,7 @@ description = "Native Delta Lake implementation in Rust"
 edition = "2021"

 [dependencies]
-arrow = { version = "22", optional = true }
+arrow = { version = "24", optional = true }


I had to upgrade arrow to match Datafusion master

avantgardnerio · 2022-10-17T21:46:37Z

rust/Cargo.toml

-datafusion = { version = "12", optional = true }
-datafusion-expr = { version = "12", optional = true }
-datafusion-common = { version = "12", optional = true }
+datafusion = { git = "https://github.com/spaceandtimelabs/arrow-datafusion.git", rev = "cb9241982556745939175b0597194ad819bab613", optional = true }


This will point back at apache/arrow-datafusion once that PR (#3867) is merged. Then probably v14.0.0 once it is released.

avantgardnerio · 2022-10-23T18:45:35Z

This is working in Ballista from DataGrip through FlightSQL! 🥳

@alamb that's the end goal for the related datafusion PR

avantgardnerio · 2022-10-24T18:54:06Z

@houqp I was finally able to get Ballista working running deltalake queries. I'd appreciate a review when you get the chance (and a CI kick would be appreciated as well).

avantgardnerio · 2022-10-24T22:29:01Z

I fixed the CI checks I could, and fixed datafusion to not require delta-rs to mess with the build. If someone could fire off CI, I'd appreciate it 🙌

wjones127

This is very cool!

I have an option suggestion and a question on how we are handling storage options.

rust/src/delta_datafusion.rs

wjones127 · 2022-10-25T03:49:21Z

rust/src/delta_datafusion.rs

+    env.register_object_store(
+        url.scheme(),
+        url.host_str().unwrap_or_default(),
+        table.object_store(),
+    );


IIUC, any URI passed with the same scheme and bucket name will match to this object store? And the object stores options will be preserved? I'm worried about the cases where storage options were directly passed into the DeltaTable instance.

the object store url we get from table.storage.object_store_url is just for internal purposes and should not collide with common urls. It uses delta-rs:// as a scheme and does a little bit of pruning required for widows paths.

wjones127 · 2022-10-25T03:54:04Z

rust/src/delta_datafusion.rs

+        let provider = open_table(url).await.unwrap();
+        Ok(Arc::new(provider))


This is perhaps a place where storage_options will get lost, if we care about that. If we only want to support passing credentials via env variables for systems that use this (just distributed, right?), that might be sensible.

we only want to support passing credentials via env variables for systems that use this (just distributed, right?), that might be sensible.

For now, that is all I am attempting to do with this PR. I can possibly see someone wanting to file another PR to extend it in the future, but if they are passing storage_options, I think that means they are registering tables in Rust (not SQL), and therefor already are writing a fork of Ballista, in which case they can write their own serde?

It's a worthwhile thought experiment, but I wouldn't recommend coding for it quite yet...

That's fair. I just wanted to make sure you were aware of that limitation.

roeap · 2022-10-25T07:35:43Z

I just marked #852 as ready for review. That should fix the python builds which are breaking due to the arrow version bump as well as handle the parquet2 build that needed some changes as parquet-format moved into the parquet crate.

roeap

great work! really excited to see this land.

roeap · 2022-10-25T07:44:41Z

rust/src/delta_datafusion.rs

+    env.register_object_store(
+        url.scheme(),
+        url.host_str().unwrap_or_default(),
+        table.object_store(),
+    );


the object store url we get from table.storage.object_store_url is just for internal purposes and should not collide with common urls. It uses delta-rs:// as a scheme and does a little bit of pruning required for widows paths.

rust/src/delta_datafusion.rs

roeap · 2022-10-25T07:52:27Z

rust/src/delta_datafusion.rs

+    fn with_new_children(
+        self: Arc<Self>,
+        children: Vec<Arc<dyn ExecutionPlan>>,
+    ) -> DataFusionResult<Arc<dyn ExecutionPlan>> {
+        ExecutionPlan::with_new_children(self.parquet_scan.clone(), children)
+    }


I may be completely off, but can a scan operation even have children? ParquetScan itself just returns Ok(self) .

can a scan operation even have children?

I would think not, but the goal of this implementation was merely to proxy every call to the wrapped ParquetScan with the one additional behavior of registering the ObjectStore prior to proxying the execute() request... so I just followed that pattern here. I think it's probably best to leave it this way just in case there is some future change where a ParquetScan might have children for some reason - possibly because someone wants to register their own proxy in the same way I just did :)

avantgardnerio · 2022-10-25T17:35:34Z

I did some fixes, so there's a reasonable chance CI should pass now.

avantgardnerio · 2022-10-25T19:24:43Z

It looks like the remaining errors are in src/python, so I assume they will be resolved by #852 .

avantgardnerio · 2022-11-16T18:16:49Z

I see #852 was merged. I think we can merge this now. If someone wouldn't mind kicking off CI, I'd appreciate it!

avantgardnerio · 2022-11-17T17:49:15Z

I don't understand the python test failure... it looks like a timeout, is it an intermittent test? Or should I try to dig into it more?

wjones127 · 2022-11-17T18:09:42Z

Details

Yeah I think that is unrelated. I'll need to look into that.

wjones127

Looks good! Thanks!

avantgardnerio · 2022-11-17T18:14:13Z

Thanks everyone!

roeap · 2022-11-17T18:29:15Z

Thank you!

avantgardnerio commented Oct 17, 2022

View reviewed changes

avantgardnerio mentioned this pull request Oct 20, 2022

Add serde for plans with tables from TableProviderFactorys apache/datafusion#3907

Merged

avantgardnerio force-pushed the bg_register_table branch 2 times, most recently from d96c625 to 3795587 Compare October 24, 2022 18:40

avantgardnerio marked this pull request as ready for review October 24, 2022 18:40

avantgardnerio requested review from houqp, xianwill, wjones127, fvaleye, roeap, rtyler and mosyp as code owners October 24, 2022 18:40

avantgardnerio mentioned this pull request Oct 24, 2022

Don't make dependants install protoc apache/datafusion#3947

Closed

wjones127 reviewed Oct 25, 2022

View reviewed changes

roeap reviewed Oct 25, 2022

View reviewed changes

avantgardnerio mentioned this pull request Oct 25, 2022

Change back to non async interface for try_into_logical_plan apache/datafusion#3955

Closed

wjones127 previously approved these changes Oct 26, 2022

View reviewed changes

avantgardnerio dismissed wjones127’s stale review via 54d6187 October 26, 2022 20:06

avantgardnerio force-pushed the bg_register_table branch from c5c07b4 to 4992277 Compare November 1, 2022 15:48

avantgardnerio mentioned this pull request Nov 15, 2022

refactor how we create listing tables apache/datafusion#4227

Merged

avantgardnerio force-pushed the bg_register_table branch from cb553ea to 92f1960 Compare November 16, 2022 17:54

Squash

6a91f89

avantgardnerio force-pushed the bg_register_table branch from 92f1960 to 6a91f89 Compare November 16, 2022 17:59

Don't drop temp dir until we're done with it

c6aa5f1

Brent Gardner added 3 commits November 16, 2022 11:30

Fix clippy issue

56ffe77

Fix tests when they are run with same features as CI

b41af78

Fix memory store by adding lots of complexity

5b60ae1

wjones127 approved these changes Nov 17, 2022

View reviewed changes

wjones127 merged commit a8f2ff2 into delta-io:main Nov 17, 2022

avantgardnerio deleted the bg_register_table branch November 17, 2022 18:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add TableProviderFactory and test for SQL to register tables dynamically at runtime #892

Add TableProviderFactory and test for SQL to register tables dynamically at runtime #892

avantgardnerio commented Oct 17, 2022

avantgardnerio Oct 17, 2022

avantgardnerio Oct 17, 2022

avantgardnerio Oct 17, 2022

avantgardnerio commented Oct 23, 2022

avantgardnerio commented Oct 24, 2022

avantgardnerio commented Oct 24, 2022

wjones127 left a comment

wjones127 Oct 25, 2022

roeap Oct 25, 2022

wjones127 Oct 25, 2022

avantgardnerio Oct 25, 2022

wjones127 Oct 26, 2022

roeap commented Oct 25, 2022

roeap left a comment

roeap Oct 25, 2022

roeap Oct 25, 2022

avantgardnerio Oct 25, 2022

avantgardnerio commented Oct 25, 2022

avantgardnerio commented Oct 25, 2022

avantgardnerio commented Nov 16, 2022

avantgardnerio commented Nov 17, 2022

wjones127 commented Nov 17, 2022

wjones127 left a comment

avantgardnerio commented Nov 17, 2022

roeap commented Nov 17, 2022

		let provider = open_table(url).await.unwrap();
		Ok(Arc::new(provider))

Add TableProviderFactory and test for SQL to register tables dynamically at runtime #892

Add TableProviderFactory and test for SQL to register tables dynamically at runtime #892

Conversation

avantgardnerio commented Oct 17, 2022

Description

Related Issue(s)

Documentation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avantgardnerio commented Oct 23, 2022

avantgardnerio commented Oct 24, 2022

avantgardnerio commented Oct 24, 2022

wjones127 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

roeap commented Oct 25, 2022

roeap 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

avantgardnerio commented Oct 25, 2022

avantgardnerio commented Oct 25, 2022

avantgardnerio commented Nov 16, 2022

avantgardnerio commented Nov 17, 2022

wjones127 commented Nov 17, 2022

wjones127 left a comment

Choose a reason for hiding this comment

avantgardnerio commented Nov 17, 2022

roeap commented Nov 17, 2022