feat!: introduce the ability for consumers to add ObjectStore url handlers by rtyler · Pull Request #873 · delta-io/delta-kernel-rs

rtyler · 2025-04-22T15:23:23Z

This provides the ability for downstream crates to insert their own URL handlers, and offers a path for delta-kernel-rs to reduce its dependency on hdfs-native-object-store for all consumers of the crate.

I hacked this out modeling off something similar that I did in delta-rs to allow pluggable storage handlers, which is hugely beneficial to reducing the dependency scope of its core module. In discussing arrow-55 upgrade challenges with Zach I proposed this as a way to help simplify the upgrade path around arrow-55 and object-store-0.12. Side benefit of a slightly smaller dependency footprint for downstream consumers :)

codecov · 2025-04-22T15:26:43Z

Codecov Report

Attention: Patch coverage is 82.75862% with 10 lines in your changes missing coverage. Please review.

Project coverage is 85.11%. Comparing base (79dfea5) to head (d62bd77).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
kernel/src/engine/default/storage.rs	82.75%	7 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #873      +/-   ##
==========================================
+ Coverage   85.05%   85.11%   +0.06%     
==========================================
  Files          85       85              
  Lines       20988    21026      +38     
  Branches    20988    21026      +38     
==========================================
+ Hits        17851    17896      +45     
+ Misses       2242     2233       -9     
- Partials      895      897       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

zachschuermann

dropping a few comments will come back soon!

zachschuermann · 2025-04-22T15:36:59Z

+/// Insert a new URL handler for [parse_url_opts] with the given `scheme`. This allows users to
+/// provide their own custom URL handler to plug new [object_store::ObjectStore] instances into
+/// delta-kernel
+pub fn insert_url_handler(


I wonder if we should add this to the default StorageHandler? Would it be easier to use there instead of just as a standalone? (could still stay a static function but just be invoked like StorageHandler::insert_url_handler?

We don't currently expose the storage handler as an arg in new. So we'd have to modify the trait in the core kernel to include this so that you could get_storage_handler and then insert_url_handler, which seems gross.

I'd say like this is okay for now, and we can refactor to pass the StorageHandler as part of the call to new in the future if we feel it's better.

zachschuermann · 2025-04-22T15:40:32Z

+    if let Ok(mut lock) = registry.write() {
+        (*lock).insert(scheme.as_ref().into(), handler_closure);
+    } else {
+        panic!("Failed to acquire lock for adding a URL handler!");


can we return an error instead of panic!?

I am kind of indifferent here, I think failing to insert a URL handler is a pretty fatal error, nothing will work properly if you were expecting to insert a URL handler and that failed

Agreed, but given that panics are really a pain, especially over ffi, we aim for a "no panic" policy if possible in kernel, and should rather return an error. The engine gets to decide how fatal it is :)

Fatal for Delta queries that need the URL handler perhaps, but surely not the engine as a whole? A panic would bring down the whole engine.

Update: A quick skim of the code base shows that ~all our lock acquires currently panic/unwrap. So I'm ok letting this PR merge as-is (doesn't make the problem worse), but we should track a TODO to fix them all ASAP.

…lers This provides the ability for downstream crates to insert their own URL handlers, and offers a path for delta-kernel-rs to reduce its dependency on hdfs-native-object-store for all consumers of the crate. I hacked this out modeling off something similar that I did in delta-rs to allow pluggable storage handlers, which is hugely beneficial to reducing the dependency scope of its core module. In discussing arrow-55 upgrade challenges with Zach I proposed this as a way to help simplify the upgrade path around arrow-55 and object-store-0.12. Side benefit of a slightly smaller dependency footprint for downstream consumers :) Signed-off-by: R. Tyler Croy <rtyler@brokenco.de>

nicklan

nice, thanks tyler. This looks good, just a couple of nits.

nicklan · 2025-04-22T18:12:25Z

+    if let Ok(mut lock) = registry.write() {
+        (*lock).insert(scheme.as_ref().into(), handler_closure);
+    } else {
+        panic!("Failed to acquire lock for adding a URL handler!");


Agreed, but given that panics are really a pain, especially over ffi, we aim for a "no panic" policy if possible in kernel, and should rather return an error. The engine gets to decide how fatal it is :)

nicklan · 2025-04-22T18:13:39Z

+            return handler(url, options);
+        }
    }
+    parse_url_opts_object_store(url, options)


nit: maybe

Suggested change

parse_url_opts_object_store(url, options)

object_store::parse_url_opts(url, options)

About the same length and we can remove the import.

nicklan · 2025-04-22T18:37:24Z

+/// This type alias makes it easier to reference the handler closure(s)
+///
+/// It uses a HashMap<String, String> which _must_ be converted in our [parse_url_opts] because we
+/// cannot use generics in this scenario.


Can we say why. Specifically I think it's that Into is not "dyn compatible", so we can't just box up the traits because you cannot write:

Box<dyn IntoIterator<Item = dyn AsRef<str>, IntoIter = dyn Into<String>>>

nicklan · 2025-04-22T18:40:12Z

+/// Insert a new URL handler for [parse_url_opts] with the given `scheme`. This allows users to
+/// provide their own custom URL handler to plug new [object_store::ObjectStore] instances into
+/// delta-kernel
+pub fn insert_url_handler(


We don't currently expose the storage handler as an arg in new. So we'd have to modify the trait in the core kernel to include this so that you could get_storage_handler and then insert_url_handler, which seems gross.

I'd say like this is okay for now, and we can refactor to pass the StorageHandler as part of the call to new in the future if we feel it's better.

zachschuermann

LGTM after we handle those few nits/comments!!

scovich · 2025-04-23T11:51:43Z

+        let options: HashMap<String, String> = HashMap::default();
+        // Currently constructing an [HdfsObjectStore] won't work if there isn't an actual HDFS
+        // to connect to, so the only way to really verify that we got the object store we
+        // jxpected is to inspect the `store` on the error v_v


Suggested change

// jxpected is to inspect the `store` on the error v_v

// expected is to inspect the `store` on the error v_v

scovich · 2025-04-23T11:53:51Z

+    if let Ok(mut lock) = registry.write() {
+        (*lock).insert(scheme.as_ref().into(), handler_closure);
+    } else {
+        panic!("Failed to acquire lock for adding a URL handler!");


Update: A quick skim of the code base shows that ~all our lock acquires currently panic/unwrap. So I'm ok letting this PR merge as-is (doesn't make the problem worse), but we should track a TODO to fix them all ASAP.

Kimahriman · 2025-07-07T11:30:22Z

Realized one of the downsides to this is that FFI usages (like duckdb extension) won't get HDFS support anymore because of this, unless they have their own wrapper project. Raised apache/arrow-rs-object-store#424 to hopefully solve all of these issues, but if that doesn't happen might need to add back in a custom feature for adding HDFS support

rzepinskip · 2025-07-10T08:34:34Z

Realized one of the downsides to this is that FFI usages (like duckdb extension) won't get HDFS support anymore because of this, unless they have their own wrapper project. Raised apache/arrow-rs-object-store#424 to hopefully solve all of these issues, but if that doesn't happen might need to add back in a custom feature for adding HDFS support

cc: @samansmink might be of interest to you.

github-actions Bot assigned rtyler Apr 22, 2025

rtyler force-pushed the hdfs-optional branch from a882fe6 to 87e602c Compare April 22, 2025 15:23

rtyler requested review from roeap and zachschuermann April 22, 2025 15:23

github-actions Bot added the breaking-change Public API change that could cause downstream compilation failures. Requires a major version bump. label Apr 22, 2025

rtyler mentioned this pull request Apr 22, 2025

Sketch / MVP: Upgrade to arrow-rs 55.0.0 / parquet 55.0.0 / object_store 0.12.0 #831

Closed

rtyler marked this pull request as ready for review April 22, 2025 15:35

zachschuermann reviewed Apr 22, 2025

View reviewed changes

rtyler force-pushed the hdfs-optional branch from 87e602c to ca95c91 Compare April 22, 2025 16:19

nicklan reviewed Apr 22, 2025

View reviewed changes

zachschuermann approved these changes Apr 22, 2025

View reviewed changes

scovich approved these changes Apr 23, 2025

View reviewed changes

zachschuermann changed the title ~~feat: introduce the ability for consumers to add ObjectStore url handlers~~ feat!: introduce the ability for consumers to add ObjectStore url handlers Apr 23, 2025

rtyler added 2 commits April 23, 2025 14:30

Merge branch 'main' into hdfs-optional

386c95c

Merge branch 'main' into hdfs-optional

d62bd77

rtyler merged commit c036bed into delta-io:main Apr 23, 2025
20 of 21 checks passed

Kimahriman mentioned this pull request Jul 7, 2025

"All DataNodes failed" transient error Kimahriman/hdfs-native#227

Open

	parse_url_opts_object_store(url, options)
	object_store::parse_url_opts(url, options)

	// jxpected is to inspect the `store` on the error v_v
	// expected is to inspect the `store` on the error v_v

Conversation

rtyler commented Apr 22, 2025

Uh oh!

codecov Bot commented Apr 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

zachschuermann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nicklan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zachschuermann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Kimahriman commented Jul 7, 2025

Uh oh!

rzepinskip commented Jul 10, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov Bot commented Apr 22, 2025 •

edited

Loading