Extract Listing URI logic into ListingTableUri structure #2578

tustvold · 2022-05-19T12:42:54Z

Which issue does this PR close?

Closes #2562
Part of #2489

Rationale for this change

See tickets, the previous logic was inconsistent, and varied based on call-site.

What changes are included in this PR?

This extracts a ListingTableUrl that handles all the logic of translating from the user-provided string, to a canonical representation that can be used by the rest of DataFusion.

~~I'm having some difficulty with some of the ballista tests, likely related to apache/datafusion-ballista#481, and would appreciate some help with this. Perhaps @thinkharderdev ?~~

Are there any user-facing changes?

Yes, this makes changes both to the public API, and also the interpretation of certain strings. In particular:

If passed a string without a scheme, this must exist on the local filesystem or parsing will return an error
Globbing will only be applied to strings without a scheme, as glob expressions aren't part of the specification

FYI @timvw

ballista/rust/core/src/serde/physical_plan/mod.rs

tustvold · 2022-05-19T12:45:10Z

datafusion/core/src/catalog/schema.rs

-        &self,
-        uri: &'a str,
-    ) -> Result<(Arc<dyn ObjectStore>, &'a str)> {
+    pub fn object_store(&self, uri: &ListingTableUrl) -> Result<Arc<dyn ObjectStore>> {


The scheme stripping logic is now part of ListingTableUrl

tustvold · 2022-05-19T12:46:06Z

datafusion/core/src/datasource/listing/path.rs

+    /// and lacks a leading `/`
+    ///
+    /// TODO: Handle paths consistently (#2489)
+    fn prefix(&self) -> &str {


As part of #2489 I hope to bring some consistency to what the paths passed to the ObjectStore actually are. It's currently a bit confused

tustvold · 2022-05-19T12:47:07Z

datafusion/core/src/datasource/listing/path.rs

+    }
+
+    /// List all files identified by this [`ListingTableUrl`] for the provided `file_extension`
+    pub(crate) fn list_all_files<'a>(


This combines the various different ObjectStore listing methods into a single method. This is clearer and more consistent

tustvold · 2022-05-19T12:49:45Z

datafusion/core/src/datasource/listing/table.rs

@@ -504,7 +504,7 @@ mod tests {
                "bucket/key-prefix/file3",
                "bucket/key-prefix/file4",
            ],
-            "bucket/key-prefix/",
+            "file:///bucket/key-prefix/",


These files don't actually exist, so we need to fully qualify the URLs

tustvold · 2022-05-19T12:50:21Z

datafusion/core/src/lib.rs

-//!     "| 1 | 2                        |",
-//!     "+---+--------------------------+"
+//!     "+---+----------------+",
+//!     "| a | MIN(?table?.b) |",


I'm not entirely sure what change caused this... I wonder if we aren't running doctests in CI 🤔

Edit: it is because of https://github.com/apache/arrow-datafusion/pull/2578/files#diff-e9c2d69b5793675f76c48aee263e29006065f1b28c50f056ccc6efe7d062aa77L611, this makes CSV consistent with other formats, which is probably a good thing

it seems ok to me. I am not sure if the use the filename as table name function of csv was intentional (like maybe it is some sort of dataframe compatibility)?

cc @andygrove

datafusion/core/src/test_util.rs

tustvold · 2022-05-19T12:51:28Z

datafusion/data-access/src/object_store/mod.rs

-    }
-
-    #[tokio::test]
-    async fn test_find_longest_search_path_without_glob_pattern() -> Result<()> {


This test is reformulated and moved to ListingTableUrl

tustvold · 2022-05-19T12:51:40Z

datafusion/data-access/src/object_store/mod.rs

@@ -78,54 +74,6 @@ pub trait ObjectStore: Sync + Send + Debug {
    /// Returns all the files in path `prefix`
    async fn list_file(&self, prefix: &str) -> Result<FileMetaStream>;

-    /// Calls `list_file` with a suffix filter


This logic is now encapsulated in ListingTableUrl

thinkharderdev · 2022-05-19T13:37:54Z

Aside from the serialization issue we discussed above, I think this looks great.

timvw · 2022-05-20T11:58:12Z

Looks good.. (I am under the impression that support for globbing (almost as an edge-case) does not belong in here but should be implemented on a more outward/user-facing api instead where it a proper part of the spec..)

tustvold · 2022-05-20T15:58:36Z

I am under the impression that support for globbing (almost as an edge-case) does not belong in here

I don't disagree with this, but aside from not supporting globbing, I couldn't see another way to support it whilst also treating the argument as a URI, which is critical for object store support. Alternative suggestions welcome...

tustvold · 2022-05-27T11:29:02Z

datafusion/core/tests/sql/explain_analyze.rs

@@ -642,7 +642,7 @@ order by
 #[tokio::test]
 async fn test_physical_plan_display_indent() {
    // Hard code target_partitions as it appears in the RepartitionExec output
-    let config = SessionConfig::new().with_target_partitions(3);
+    let config = SessionConfig::new().with_target_partitions(9000);


This was causing an amusing issue, as the mac runner happened to have 3 CPUs, it would replace the 3 with NUM_CPUS 🤦

tustvold · 2022-05-27T12:29:52Z

I'm working on fixing the windows issues, will then do the ballista dance, I think this should be ready for review though

tustvold · 2022-05-27T13:10:25Z

datafusion/core/Cargo.toml

@@ -67,7 +67,9 @@ datafusion-physical-expr = { path = "../physical-expr", version = "8.0.0" }
 datafusion-row = { path = "../row", version = "8.0.0" }
 datafusion-sql = { path = "../sql", version = "8.0.0" }
 futures = "0.3"
+glob = "0.3.0"


This is moved from data_access

tustvold · 2022-05-27T13:11:20Z

datafusion/core/Cargo.toml

 hashbrown = { version = "0.12", features = ["raw"] }
+itertools = "0.10"


This is technically a new dependency of this crate, but it is so ubiquitous and is a dependency of prost, etc.. so this is probably ok

But datafusion doesn't depend on prost, right? If we could avoid another dependency that would be goo in my opinion

Well once we bring in the new object_store we will depend on it...

Ultimately it is such a fundamental crate, like bytes, parking_lot, etc... I'm inclined to think it isn't really a problem...

ok, I still cringe a little, but I vote

tustvold · 2022-05-27T13:11:57Z

datafusion/core/Cargo.toml

@@ -85,6 +87,7 @@ sqlparser = "0.17"
 tempfile = "3"
 tokio = { version = "1.0", features = ["macros", "rt", "rt-multi-thread", "sync", "fs", "parking_lot"] }
 tokio-stream = "0.1"
+url = "2.2"


This is a new dependency, it is brought in by hyper and friends and so again is pretty ubiquitous

tustvold · 2022-05-29T17:35:42Z

#2639 seems to have introduced logical conflicts... Fixing...

tustvold · 2022-05-29T23:07:59Z

datafusion/core/src/physical_plan/file_format/mod.rs

@@ -68,6 +70,8 @@ lazy_static! {
 pub struct FileScanConfig {
    /// Store from which the `files` should be fetched
    pub object_store: Arc<dyn ObjectStore>,
+    /// Object store URL
+    pub object_store_url: ObjectStoreUrl,


This was the solution I came up with to allow ballista to round-trip the physical plan, store a canonical ObjectStoreUrl instead of relying on the files to contain the scheme.

I think perhaps in a subsequent PR I'll try to remove the object_store member which is now redundant.

FYI @thinkharderdev

worth a tracking ticket?

tustvold · 2022-05-30T11:14:05Z

datafusion/core/src/datasource/object_store.rs

+}
+
+/// Object store registry
+pub struct ObjectStoreRegistry {


This is moved from object_store_registry

tustvold · 2022-05-30T11:14:20Z

datafusion/core/src/datasource/object_store_registry.rs

-use std::sync::Arc;
-
-/// Object store registry
-pub struct ObjectStoreRegistry {


Moved to object_store module

alamb

I went over this PR quite carefully. I think it is a significant step forward. Nice work @tustvold

Other than the windows test removal in datafusion/core/src/datasource/listing/helpers.rs it all looked pretty great to me 🏆

alamb · 2022-05-30T11:51:48Z

benchmarks/src/bin/tpch.rs

@@ -50,6 +50,7 @@ use datafusion::{

 use datafusion::datasource::file_format::csv::DEFAULT_CSV_EXTENSION;
 use datafusion::datasource::file_format::parquet::DEFAULT_PARQUET_EXTENSION;
+use datafusion::datasource::listing::ListingTableUrl;


I wonder if it is time to move datasource to its own crate, if possible 🤔

alamb · 2022-05-30T11:53:39Z

datafusion/core/src/datasource/listing/path.rs

@@ -0,0 +1,304 @@
+// Licensed to the Apache Software Foundation (ASF) under one


minor comment: maybe this file should be called "url.rs" for consistency

Heh, this was my trick to avoid a name collision with the url crate 😆

alamb · 2022-05-30T11:53:58Z

datafusion/core/src/datasource/listing/path.rs

+    ///
+    /// # Paths without a Scheme
+    ///
+    /// If no scheme is provided, or the string is an absolute filesystem path


👍 this is great. Thank you for this writeup. I think it makes sense to me

@yjshen is this behavior ok with you?

datafusion/core/src/datasource/listing/path.rs

alamb · 2022-05-30T11:58:59Z

datafusion/core/src/datasource/listing/path.rs

+    }
+
+    /// Creates a new [`ListingTableUrl`] interpreting `s` as a filesystem path
+    fn parse_path(s: &str) -> Result<Self> {


Is there any use case for this being pub? Like to allow users to force the parsing to treat it like a path

I think I'd rather keep it private until such a use-case comes along. There's enough funky here, that I'm loathe to introduce more potential for strangeness 😅

datafusion/core/src/datasource/object_store.rs

alamb · 2022-05-30T12:23:08Z

datafusion/core/src/lib.rs

-//!     "| 1 | 2                        |",
-//!     "+---+--------------------------+"
+//!     "+---+----------------+",
+//!     "| a | MIN(?table?.b) |",


it seems ok to me. I am not sure if the use the filename as table name function of csv was intentional (like maybe it is some sort of dataframe compatibility)?

cc @andygrove

alamb · 2022-05-30T12:26:33Z

datafusion/core/src/physical_plan/file_format/mod.rs

@@ -68,6 +70,8 @@ lazy_static! {
 pub struct FileScanConfig {
    /// Store from which the `files` should be fetched
    pub object_store: Arc<dyn ObjectStore>,
+    /// Object store URL
+    pub object_store_url: ObjectStoreUrl,


worth a tracking ticket?

alamb · 2022-05-30T12:28:15Z

datafusion/core/tests/sql/mod.rs

-    // to ARROW_TEST_DATA/csv/aggregate_test_100.csv
-    let data_path = datafusion::test_util::arrow_test_data();
-    let s = s.replace(&data_path, "ARROW_TEST_DATA");
+pub struct ExplainNormalizer {


* Update to apache/datafusion#2578 * Fix standalone build * Update datafusion pin

github-actions bot added ballista datafusion Changes in the datafusion crate labels May 19, 2022

tustvold commented May 19, 2022

View reviewed changes

ballista/rust/core/src/serde/physical_plan/mod.rs Outdated Show resolved Hide resolved

tustvold commented May 19, 2022

View reviewed changes

tustvold force-pushed the url-refactor branch from 9096fe7 to b382bb7 Compare May 19, 2022 12:53

Extract Listing URI logic into ListingTableUri structure

4314a3b

tustvold force-pushed the url-refactor branch from b382bb7 to 4314a3b Compare May 24, 2022 16:34

tustvold added the api change Changes the API exposed to users of the crate label May 24, 2022

tustvold added 2 commits May 27, 2022 11:40

Merge remote-tracking branch 'upstream/master' into url-refactor

7dc7eef

Fix explain test CPU count sensitivity

18ceed5

github-actions bot added the core Core DataFusion crate label May 27, 2022

tustvold commented May 27, 2022

View reviewed changes

tustvold marked this pull request as ready for review May 27, 2022 11:29

tustvold commented May 27, 2022

View reviewed changes

Windows compatibility

9c8b546

tustvold force-pushed the url-refactor branch from b618afd to 9c8b546 Compare May 27, 2022 14:50

tustvold added 3 commits May 27, 2022 16:47

More windows pacification

230e42b

Even more windows pacification

deef0b6

Fix windows doctest

6c311ba

Merge remote-tracking branch 'upstream/master' into url-refactor

9c9b4b2

tustvold added a commit to tustvold/arrow-ballista that referenced this pull request May 29, 2022

Update to apache/datafusion#2578

bc14102

tustvold mentioned this pull request May 29, 2022

Update to apache/arrow-datafusion#2578 apache/datafusion-ballista#48

Merged

tustvold added a commit to tustvold/arrow-ballista that referenced this pull request May 29, 2022

Update to apache/datafusion#2578

d8a4cfa

Add ObjectStoreUrl

63df81c

tustvold added a commit to tustvold/arrow-ballista that referenced this pull request May 29, 2022

Update to apache/datafusion#2578

86fe200

Update ballista pin

938e916

github-actions bot added the development-process Related to development process of DataFusion label May 29, 2022

tustvold commented May 29, 2022

View reviewed changes

tustvold added 3 commits May 30, 2022 00:10

Format

c1cf2da

Clippy

c32e27e

Consistent naming

61d67cf

tustvold commented May 30, 2022

View reviewed changes

alamb approved these changes May 30, 2022

View reviewed changes

tustvold added 2 commits May 30, 2022 13:53

Review feedback

a504fe8

Fix windows test

0ec7433

tustvold merged commit fdb8fec into apache:master May 30, 2022

alamb pushed a commit to apache/datafusion-ballista that referenced this pull request May 31, 2022

Update to apache/datafusion#2578 (#48)

311fa53

* Update to apache/datafusion#2578 * Fix standalone build * Update datafusion pin

tustvold mentioned this pull request May 31, 2022

Remove ObjectStore from FileScanConfig and ListingTableConfig #2668

Merged

This was referenced Aug 3, 2022

[object store] Parsing of well-known uri formats apache/arrow-rs#2304

Closed

Are the Azure paths inconsitent with other remote store paths? delta-io/delta-rs#721

Closed

timvw mentioned this pull request Aug 25, 2022

(Re-)add support for glob patterns in ListingTableUrl #3261

Closed

tustvold mentioned this pull request Oct 20, 2022

Local object store accepts file:/// as base path, but LocalStore returns meta without the prefix. #1923

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract Listing URI logic into ListingTableUri structure #2578

Extract Listing URI logic into ListingTableUri structure #2578

tustvold commented May 19, 2022 •

edited

Loading

tustvold May 19, 2022

tustvold May 19, 2022

tustvold May 19, 2022

tustvold May 19, 2022

tustvold May 19, 2022 •

edited

Loading

alamb May 30, 2022

tustvold May 19, 2022

tustvold May 19, 2022

thinkharderdev commented May 19, 2022

timvw commented May 20, 2022

tustvold commented May 20, 2022

tustvold May 27, 2022

tustvold commented May 27, 2022

tustvold May 27, 2022

tustvold May 27, 2022

alamb May 30, 2022

tustvold May 30, 2022

alamb May 30, 2022

tustvold May 27, 2022

tustvold commented May 29, 2022

tustvold May 29, 2022

alamb May 30, 2022

tustvold May 30, 2022

tustvold May 30, 2022

alamb left a comment

alamb May 30, 2022

alamb May 30, 2022

tustvold May 30, 2022

alamb May 30, 2022

alamb May 30, 2022

tustvold May 30, 2022

alamb May 30, 2022

alamb May 30, 2022

alamb May 30, 2022

		hashbrown = { version = "0.12", features = ["raw"] }
		itertools = "0.10"

		@@ -0,0 +1,304 @@
		// Licensed to the Apache Software Foundation (ASF) under one

Extract Listing URI logic into ListingTableUri structure #2578

Extract Listing URI logic into ListingTableUri structure #2578

Conversation

tustvold commented May 19, 2022 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

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

tustvold May 19, 2022 • edited Loading

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

thinkharderdev commented May 19, 2022

timvw commented May 20, 2022

tustvold commented May 20, 2022

Choose a reason for hiding this comment

tustvold commented May 27, 2022

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

tustvold commented May 29, 2022

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tustvold commented May 19, 2022 •

edited

Loading

tustvold May 19, 2022 •

edited

Loading