Automatically register tables if ObjectStore root is configured #4095

avantgardnerio · 2022-11-03T04:14:21Z

Which issue does this PR close?

Closes #4094.

Rationale for this change

Described in issue.

What changes are included in this PR?

a ListingSchemaProvider is defined
if appropriate config_options are set, the ListingSchemaProvider is automatically registered
if the ListingSchemaProvider is registered, it scans the ObjectStore and registers the appropriate tables

Are there any user-facing changes?

When they connect, tables are there as they would expect.

avantgardnerio · 2022-11-03T04:15:21Z

@andygrove and @alamb as usual, I'd appreciate a review as well as adding anyone else who is appropriate. Thanks guys 🙌

alamb · 2022-11-03T18:44:33Z

Until the fix for #4100 is merged, clippy will likely fail on this PR as well

alamb

This looks great @avantgardnerio -- I think the only thing I think is missing is some sort of test (mostly so we don't accidentally break this feature when it is refactored, etc)

datafusion/core/src/catalog/listing_schema.rs

alamb · 2022-11-03T19:05:58Z

datafusion/core/src/catalog/listing_schema.rs

+ }
+ }
+
+ /// Reload table information from ObjectStore


I recommend documenting here when refresh() should be called -- like it has to be called explicitly after construction for example

That is the big open question with this PR 😄 . Presently, in flight_sql.rs in Ballista, when FlightSql clients enumerate the catalogs/schemas/tables, I am doing a checked downcast_ref to see if it is a ListingSchemaProvider and if so calling this method. Ultimately, I think we should probably extend the SchemaProvider API? The downcast trick certainly doesn't seem elegant.

Unfortunately this is a state synchronization problem, and I'm not sure that the ObjectStore has APIs for file system listeners, so we will need to figure out the best times to try to synchronize the state. Every time we run a query perhaps? My worry is that this could get expensive, I can imagine (or have heard about) each delta table containing 1000 parquet files, and there could probably be 100s of tables, which means a lot of files to scan.

Also unfortunately, it looks like the ObjectStore doesn't let us list only children of a folder - it lists all the files in the entire bucket, thus the weird recursive .parent() stuff.

I thought I would file this PR to get the discussion going.

I agree that there is no policy that will work well for all implementations / usecases so the refresh policy will need to be decided by whatever the upstream system is (e.g. ballista or iox, or whatever)

Adding a refresh() method to SchemaProvider seems fine, though to be honest I think using downcast_ref() also seems fine to me

Would definitely be interested in hearing other opinions on this

datafusion/core/src/catalog/listing_schema.rs

avantgardnerio · 2022-11-03T19:23:31Z

missing is some sort of test

For sure, working on it...

datafusion/core/src/execution/context.rs

benchmarks/Cargo.toml

avantgardnerio · 2022-11-03T22:26:43Z

I don't understand why the hash collision test is failing, and I can't get it to happen locally :(

avantgardnerio · 2022-11-03T22:35:02Z

If it looks anything like local, we should see

Looking for data in /home/bgardner/workspace/ballista/arrow-datafusion/datafusion/core/tests/tpch-csv exists=true

avantgardnerio · 2022-11-03T23:01:06Z

So the directory exists, so it seems like:

either the files in it were deleted
or, the ObjectStore has some problem accessing it

Looking for data in /__w/arrow-datafusion/arrow-datafusion/datafusion/core/tests/tpch-csv exists=true
thread 'execution::context::tests::with_listing_schema_provider' panicked at 'assertion failed: `(left == right)`
  left: `0`,
 right: `8`', datafusion/core/src/execution/context.rs:2288:9

avantgardnerio · 2022-11-03T23:02:30Z

Oh, could the underscores be messing with it?

avantgardnerio · 2022-11-03T23:04:28Z

Yup, a bug in ObjectStore:

Looking for data in /home/bgardner/workspace/ballista/arrow-datafusion/datafusion/core/tests/tpch_csv exists=true


Left:  0
Right: 8
<Click to see difference>

avantgardnerio · 2022-11-03T23:24:00Z

Yup, a bug in ObjectStore:

Nope, it was my faulty test combined with hashing inconsistency that happened to get triggered by underscores in this case 🤦 .

avantgardnerio · 2022-11-04T00:36:56Z

Oh wow. The UI went nuts. Sorry for all the review requests.

datafusion/core/src/execution/context.rs

andygrove · 2022-11-04T13:50:46Z

I really like this feature. It will save me time as a user once we plumb this through to the CLI and Python 👍

alamb

I think this looks good @avantgardnerio -- thank you!

I had some minor naming suggestions on the parameters, but I would also be fine with this being merged as is

datafusion/core/src/catalog/listing_schema.rs

datafusion/core/src/config.rs

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

andygrove

LGTM. Thanks @avantgardnerio

ursabot · 2022-11-04T18:32:39Z

Benchmark runs are scheduled for baseline = fc669d5 and contender = 4a67d0d. 4a67d0d 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

…he#4095) * squash * Debug test in CI :'( * Hashing inconsistency * Address Andy's concerns * Docs * Docs * fmt * treat empty string like None :( * clippy * PR feedback * Update datafusion/core/src/config.rs Co-authored-by: Andrew Lamb <[email protected]> * Update datafusion/core/src/config.rs Co-authored-by: Andrew Lamb <[email protected]> Co-authored-by: Andrew Lamb <[email protected]>

github-actions bot added the core Core DataFusion crate label Nov 3, 2022

alamb changed the title ~~Bg listing catalog~~ Automatically register tables if ObjectStore root is configured Nov 3, 2022

alamb reviewed Nov 3, 2022

View reviewed changes

avantgardnerio commented Nov 3, 2022

View reviewed changes

datafusion/core/src/execution/context.rs Show resolved Hide resolved

avantgardnerio requested a review from alamb November 3, 2022 20:21

avantgardnerio commented Nov 3, 2022

View reviewed changes

datafusion/core/src/execution/context.rs Show resolved Hide resolved

squash

9f75172

avantgardnerio force-pushed the bg_listing_catalog branch from ee992ad to 9f75172 Compare November 3, 2022 20:24

andygrove reviewed Nov 3, 2022

View reviewed changes

benchmarks/Cargo.toml Outdated Show resolved Hide resolved

Debug test in CI :'(

97177e8

Hashing inconsistency

ed0c2e1

Address Andy's concerns

e5fe0d4

avantgardnerio requested review from andygrove and alamb and removed request for alamb and andygrove November 4, 2022 00:16

avantgardnerio requested review from alamb and andygrove and removed request for alamb and andygrove November 4, 2022 00:17

andygrove reviewed Nov 4, 2022

View reviewed changes

datafusion/core/src/execution/context.rs Outdated Show resolved Hide resolved

Brent Gardner added 5 commits November 4, 2022 08:31

Docs

77d4887

Docs

64154e8

fmt

577ae6f

treat empty string like None :(

e76efce

clippy

b2ec207

alamb approved these changes Nov 4, 2022

View reviewed changes

datafusion/core/src/catalog/listing_schema.rs Show resolved Hide resolved

datafusion/core/src/config.rs Outdated Show resolved Hide resolved

datafusion/core/src/config.rs Outdated Show resolved Hide resolved

datafusion/core/src/config.rs Outdated Show resolved Hide resolved

Brent Gardner and others added 3 commits November 4, 2022 10:32

PR feedback

656b361

Update datafusion/core/src/config.rs

355506c

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

Update datafusion/core/src/config.rs

c1317f8

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

avantgardnerio requested a review from andygrove November 4, 2022 17:24

andygrove approved these changes Nov 4, 2022

View reviewed changes

andygrove merged commit 4a67d0d into apache:master Nov 4, 2022

avantgardnerio deleted the bg_listing_catalog branch November 4, 2022 18:25

avantgardnerio mentioned this pull request Nov 4, 2022

Allow for automatic registration of ListingTables #4111

Closed

alamb mentioned this pull request Nov 10, 2022

An asynchronous version of CatalogList/CatalogProvider/SchemaProvider #3777

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Automatically register tables if ObjectStore root is configured #4095

Automatically register tables if ObjectStore root is configured #4095

avantgardnerio commented Nov 3, 2022 •

edited

Loading

avantgardnerio commented Nov 3, 2022

alamb commented Nov 3, 2022

alamb left a comment

alamb Nov 3, 2022

avantgardnerio Nov 3, 2022

alamb Nov 4, 2022

avantgardnerio commented Nov 3, 2022

avantgardnerio commented Nov 3, 2022

avantgardnerio commented Nov 3, 2022

avantgardnerio commented Nov 3, 2022

avantgardnerio commented Nov 3, 2022

avantgardnerio commented Nov 3, 2022

avantgardnerio commented Nov 3, 2022

avantgardnerio commented Nov 4, 2022

andygrove commented Nov 4, 2022

alamb left a comment

andygrove left a comment

ursabot commented Nov 4, 2022

Automatically register tables if ObjectStore root is configured #4095

Automatically register tables if ObjectStore root is configured #4095

Conversation

avantgardnerio commented Nov 3, 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?

avantgardnerio commented Nov 3, 2022

alamb commented Nov 3, 2022

alamb left a comment

Choose a reason for hiding this comment

alamb Nov 3, 2022

Choose a reason for hiding this comment

avantgardnerio Nov 3, 2022

Choose a reason for hiding this comment

alamb Nov 4, 2022

Choose a reason for hiding this comment

avantgardnerio commented Nov 3, 2022

avantgardnerio commented Nov 3, 2022

avantgardnerio commented Nov 3, 2022

avantgardnerio commented Nov 3, 2022

avantgardnerio commented Nov 3, 2022

avantgardnerio commented Nov 3, 2022

avantgardnerio commented Nov 3, 2022

avantgardnerio commented Nov 4, 2022

andygrove commented Nov 4, 2022

alamb left a comment

Choose a reason for hiding this comment

andygrove left a comment

Choose a reason for hiding this comment

ursabot commented Nov 4, 2022

avantgardnerio commented Nov 3, 2022 •

edited

Loading