Move `DynamicFileCatalog` back to core #10745

goldmedal · 2024-05-31T16:36:19Z

Which issue does this PR close?

No specific issue but mentioned here #4850 (comment)

Rationale for this change

Discussed with @alamb here. #10619 (comment)

What changes are included in this PR?

Move DynamicFileCatalog and object storage-related features from CLI to the DataFusion core.

Are these changes tested?

No, just move original tests to core.

Are there any user-facing changes?

No

goldmedal · 2024-05-31T17:43:55Z

datafusion/core/Cargo.toml

+aws-config = "0.101.0"
+aws-credential-types = "0.101.0"


Upgrade aws package to fix the audit failed message:

Crate: rustls Version: 0.20.9 error: 1 vulnerability found! Title: `rustls::ConnectionCommon::complete_io` could fall into an infinite loop based on network input Date: 2024-04-19 ID: RUSTSEC-2024-0336 URL: https://rustsec.org/advisories/RUSTSEC-2024-0336 Severity: 7.5 (high) Solution: Upgrade to >=0.23.5 OR >=0.22.4, <0.23.0 OR >=0.21.[11](https://github.com/apache/datafusion/actions/runs/9321626823/job/25661037574?pr=10745#step:4:12), <0.22.0 Dependency tree: rustls 0.20.9 ├── tokio-rustls 0.23.4 │ └── hyper-rustls 0.23.2 │ └── aws-smithy-client 0.55.3 │ ├── aws-types 0.55.3 │ │ ├── aws-sig-auth 0.55.3 │ │ │ ├── aws-sdk-sts 0.28.0 │ │ │ │ └── aws-config 0.55.3 │ │ │ │ └── datafusion 38.0.0 │ │ │ │ ├── datafusion-wasmtest 38.0.0 │ │ │ │ ├── datafusion-substrait 38.0.0 │ │ │ │ ├── datafusion-sqllogictest 38.0.0 │ │ │ │ ├── datafusion-proto 38.0.0 │ │ │ │ │ └── datafusion-benchmarks 38.0.0 │ │ │ │ ├── datafusion-examples 38.0.0 │ │ │ │ ├── datafusion-docs-tests 38.0.0 │ │ │ │ └── datafusion-benchmarks 38.0.0 │ │ │ └── aws-sdk-sso 0.28.0 │ │ │ └── aws-config 0.55.3 │ │ ├── aws-sdk-sts 0.28.0 │ │ ├── aws-sdk-sso 0.28.0 │ │ ├── aws-http 0.55.3 │ │ │ ├── aws-sdk-sts 0.28.0 │ │ │ ├── aws-sdk-sso 0.28.0 │ │ │ └── aws-config 0.55.3 │ │ ├── aws-endpoint 0.55.3 │ │ │ ├── aws-sdk-sts 0.28.0 │ │ │ └── aws-sdk-sso 0.28.0 │ │ └── aws-config 0.55.3 │ ├── aws-sdk-sts 0.28.0 │ ├── aws-sdk-sso 0.28.0 │ └── aws-config 0.55.3 ├── hyper-rustls 0.[23](https://github.com/apache/datafusion/actions/runs/9321626823/job/25661037574?pr=10745#step:4:24).2 └── aws-smithy-client 0.55.3

I'm not sure which version is better. Just pick up the latest version for version 0.xxx.xx.

goldmedal · 2024-06-01T05:37:53Z

Hi @alamb, I encountered a dependency issue with wasm-build.
https://github.com/apache/datafusion/actions/runs/9327817555/job/25678224652?pr=10745

After some research, I found that some people encountered a similar error message, and someone mentioned it could be an issue with Tokio's limited support for WASM. I found that the failed crate, mio, is used by tokio and aws-config. datafusion-cli enables more of Tokio's features than core. I think I might have moved too many objects to core. I might need to keep aws, gcp, or other object-store-related things in cli. I'll keep trying.

However, this also means that this feature will have limited support for WASM.

Do you have any ideas about this issue? Thanks.

goldmedal · 2024-06-02T15:56:59Z

To solve the WASM building issue, I disabled the object_store related feature for WASM. There is one remaining issue with macOS that I don't know how to fix.

GitHub Actions Log:

libunwind: stepWithCompactEncoding - invalid compact unwind encoding
error: test failed, to rerun pass `-p datafusion --lib`

Caused by:
  process didn't exit successfully: `/Users/runner/work/datafusion/datafusion/target/debug/deps/datafusion-e14cd1eb6c13d6be` (signal: 6, SIGABRT: process abort signal)

After some research, it seems to be a known issue with Xcode 15. Refer to:

I'm not sure if it's related to the macos-latest GitHub runner. I can't reproduce it in my local.

I have tried the following:

Upgrading Rust to version 1.78.
Upgrading AWS-related crates to the latest version.
Rolling back AWS-related crates to version 0.55.3.
Upgrade dirs to the latest version.

None of these solutions have worked.

goldmedal · 2024-06-03T16:24:24Z

datafusion/core/Cargo.toml

+aws-config = "0.101.0"
+aws-credential-types = "0.101.0"
+# the default features will cause libunwind error when testing on macos paltform.
+object_store = { version = "0.9.1", default-features = false, features = ["aws", "gcp", "http"] }


Actually, I don't know which default feature causes the error on macOS. I just used trial and error many times, and eventually, it worked. I noticed that we disabled the default features in the root Cargo.toml by #8095, but I can't find the reason. Could someone explain this?

This is going to be problematic -- I don't think we can add these dependencies to the core datafusion crate without massively increasing the dependency footprint.

The idea is that the core of datafusion can be used from many places (including wasm) that don't want the (very large) additional dependency of the aws sdk. Having the dependencies in the DataFusion CLI is fine as it is the standalone app (rather than a library)

goldmedal · 2024-06-03T16:30:40Z

I'm not sure if it's related to the macos-latest GitHub runner. I can't reproduce it in my local.

I have checked that the macos-latest GitHub runner has been updated to Xcode 15.4 after actions/runner-images#9842. So, it may not be an Xcode issue.

alamb

Thank you @goldmedal -- it is very impressive you got this to build 🙏

I think in general, I don't think we should add these new dependencies into the core, I think we should split out the features of the dynamic file catalog (which knows how to treat strings as table names and look them up as tables) and the registering / interpreting of URLs

I took a look around in the code, and it seems like one challenge is that the catalog traits (like CatalogProvider are still in the core crate and thus any new catalog implementation would have to be there.

I think it may be time to break out the catalog code into its own crate and enforce the boundaries a bit more. I wrote up this idea here: #10782

Let me know what you think

alamb · 2024-06-03T19:06:43Z

datafusion/core/Cargo.toml

+aws-config = "0.101.0"
+aws-credential-types = "0.101.0"
+# the default features will cause libunwind error when testing on macos paltform.
+object_store = { version = "0.9.1", default-features = false, features = ["aws", "gcp", "http"] }


This is going to be problematic -- I don't think we can add these dependencies to the core datafusion crate without massively increasing the dependency footprint.

The idea is that the core of datafusion can be used from many places (including wasm) that don't want the (very large) additional dependency of the aws sdk. Having the dependencies in the DataFusion CLI is fine as it is the standalone app (rather than a library)

goldmedal · 2024-06-04T03:30:01Z

Thanks @alamb
I agree with this approach. The dependency of the core should be simpler. Therefore, I think I can wait for the discussion in #10782 to plan the next steps for this issue.

alamb · 2024-06-06T20:52:49Z

Converting to draft as we work on separating out the catalog a bit more

goldmedal · 2024-06-16T14:35:33Z

Hi @alamb, is there any update on this topic? Or maybe I can help by splitting out some traits into the new crate datafusion-catalog first?

alamb · 2024-06-18T10:58:08Z

Hi @alamb, is there any update on this topic? Or maybe I can help by splitting out some traits into the new crate datafusion-catalog first?

Hi @goldmedal -- sorry for the lack of update. From my perspective, it is not a good idea to add any more dependencies (even optional) to the core datafusion crate.

After thinking about this some more, I think there is a difference between:

The logic that interprets table names as potential object store locations
The logic that handles urls like s3://... by automatically registering object store factories

Putting the first in the datafusion core makes sense. I think the second comes with many dependencies and thus doesn't make sense.

Thus, what do you think about somehow splitting the two pieces of functionalty apart? Maybe via a trait like

struct DynamicTableProvider {
...
  /// A callback function that is 
  url_lookup: Arc<dyn UrlLookup>
}

/// Trait for looking up the correct object store instance based on url
pub trait UrlLookup{
  fn lookup(&self, url: &Url) -> Result<Arc<dyn ObjectStore>>;
}

Then we could put DynamicTableProvider in the core (with a default handler that knew how to look up file:// urls, for example) but leave the s3 specicific stuff in datafusion-cli

Thank you for working on this PR / IDE

goldmedal · 2024-06-18T12:47:50Z

Then we could put DynamicTableProvider in the core (with a default handler that knew how to look up file:// urls, for example) but leave the s3 specicific stuff in datafusion-cli

I think it makes sense to me. As far as I know, DuckDB also only supports reading local files by default. You need to install an additional extension, httpfs, to enable the feature for object storages. I'm not sure if there is any similar feature (installing an extension or table function, then query dynamically) in DataFusion, but I can implement DynamicTableProvider first.

goldmedal · 2024-06-18T15:52:30Z

I think we should close this PR to keep the discussions. Then, I'll create another PR for DynamicTableProvider implementation. WDYT @alamb?

alamb · 2024-06-18T16:06:34Z

. I'm not sure if there is any similar feature (installing an extension or table function, then query dynamically) in DataFusion, but I can implement DynamicTableProvider first.

I think this is what https://docs.rs/datafusion/latest/datafusion/execution/context/struct.SessionContext.html#method.register_object_store is used for

And access to the remote store is hanlded via and ObjectStore

alamb · 2024-06-18T16:07:31Z

I think we should close this PR to keep the discussions. Then, I'll create another PR for DynamicTableProvider implementation. WDYT @alamb?

I think this is a good idea

Given how this project has grown, I think it might also be valuable to file a ticket describing the high level idea too -- are you able to do so? I can do so too, but likely won't have a chance until tomorrow

goldmedal · 2024-06-18T16:11:54Z

Given how this project has grown, I think it might also be valuable to file a ticket describing the high level idea too -- are you able to do so? I can do so too, but likely won't have a chance until tomorrow

Sure, I can file a ticket for it.

goldmedal · 2024-06-18T16:48:19Z

I have filed #10986. Let's move the follow up discussion to it.

github-actions bot added the core Core DataFusion crate label May 31, 2024

goldmedal commented May 31, 2024

View reviewed changes

goldmedal marked this pull request as draft May 31, 2024 17:49

goldmedal added 11 commits June 1, 2024 12:00

move object_storage and dynamic_file_catalog_provider to core

fe45558

modified cargo toml and update lock

30f4d71

rollback some unnecessary chaged

fc79e91

cargo fmt

236248c

rollback the struct position

dac401e

switch the position

ebb777a

fix cargo fmt check

937d1b0

rollback the lock

6c93f6f

fix cargo audit

f64f257

fix warning: remove deprecated method

ea05807

fix test

a3fb390

goldmedal force-pushed the chore/move-dyfile-to-core branch from 8c76b92 to a3fb390 Compare June 1, 2024 05:10

goldmedal added 6 commits June 2, 2024 19:14

exclude aws, gcp, http feature for wasm

c2ce056

fix audit

16b2c9e

fix wasm-build

d67f8fe

update cargo lock

256adbd

fix test

c32ccd0

fix wasm build

4654397

github-actions bot added the substrait label Jun 2, 2024

goldmedal force-pushed the chore/move-dyfile-to-core branch from 0417f89 to 9811979 Compare June 2, 2024 15:07

github-actions bot removed the substrait label Jun 2, 2024

goldmedal force-pushed the chore/move-dyfile-to-core branch from e492aa3 to 4654397 Compare June 2, 2024 15:21

fix typo

d1a9368

fix typo

efa9bac

goldmedal force-pushed the chore/move-dyfile-to-core branch from 090558d to efa9bac Compare June 2, 2024 16:23

goldmedal marked this pull request as ready for review June 2, 2024 16:55

alamb mentioned this pull request Jun 3, 2024

DataFusion weekly project plan (Andrew Lamb) - June 3, 2024 #10779

Closed

8 tasks

goldmedal marked this pull request as draft June 3, 2024 15:33

goldmedal added 2 commits June 3, 2024 23:37

try to disable default-feature

6146d08

format toml file and add comments

679935b

goldmedal commented Jun 3, 2024

View reviewed changes

goldmedal marked this pull request as ready for review June 3, 2024 16:25

alamb mentioned this pull request Jun 3, 2024

[Epic] Extract catalog functionality from the core to make it more modular #10782

Open

alamb requested changes Jun 3, 2024

View reviewed changes

alamb marked this pull request as draft June 6, 2024 20:52

alamb mentioned this pull request Jun 18, 2024

Support select .. from 'data.parquet' files in SQL from any SessionContext (optionally) #4850

Closed

goldmedal mentioned this pull request Jun 18, 2024

Implement DynamicTableProvider in DataFusion Core #10986

Closed

goldmedal closed this Jun 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move `DynamicFileCatalog` back to core #10745

Move `DynamicFileCatalog` back to core #10745

goldmedal commented May 31, 2024

goldmedal May 31, 2024

goldmedal commented Jun 1, 2024

goldmedal commented Jun 2, 2024 •

edited

Loading

goldmedal Jun 3, 2024 •

edited

Loading

alamb Jun 3, 2024

goldmedal commented Jun 3, 2024 •

edited

Loading

alamb left a comment

alamb Jun 3, 2024

goldmedal commented Jun 4, 2024

alamb commented Jun 6, 2024

goldmedal commented Jun 16, 2024

alamb commented Jun 18, 2024 •

edited

Loading

goldmedal commented Jun 18, 2024

goldmedal commented Jun 18, 2024

alamb commented Jun 18, 2024

alamb commented Jun 18, 2024

goldmedal commented Jun 18, 2024

goldmedal commented Jun 18, 2024

Move DynamicFileCatalog back to core #10745

Move DynamicFileCatalog back to core #10745

Conversation

goldmedal commented May 31, 2024

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

goldmedal May 31, 2024

Choose a reason for hiding this comment

goldmedal commented Jun 1, 2024

goldmedal commented Jun 2, 2024 • edited Loading

goldmedal Jun 3, 2024 • edited Loading

Choose a reason for hiding this comment

alamb Jun 3, 2024

Choose a reason for hiding this comment

goldmedal commented Jun 3, 2024 • edited Loading

alamb left a comment

Choose a reason for hiding this comment

alamb Jun 3, 2024

Choose a reason for hiding this comment

goldmedal commented Jun 4, 2024

alamb commented Jun 6, 2024

goldmedal commented Jun 16, 2024

alamb commented Jun 18, 2024 • edited Loading

goldmedal commented Jun 18, 2024

goldmedal commented Jun 18, 2024

alamb commented Jun 18, 2024

alamb commented Jun 18, 2024

goldmedal commented Jun 18, 2024

goldmedal commented Jun 18, 2024

Move `DynamicFileCatalog` back to core #10745

Move `DynamicFileCatalog` back to core #10745

goldmedal commented Jun 2, 2024 •

edited

Loading

goldmedal Jun 3, 2024 •

edited

Loading

goldmedal commented Jun 3, 2024 •

edited

Loading

alamb commented Jun 18, 2024 •

edited

Loading