fix!: arrow and panic fixes (#903)

## What changes are proposed in this pull request?
1. README fixes to clarify new arrow semantics
2. default to arrow-55 in all cases except when user passes ONLY
`arrow-54`
3. `insert_url_handler` returns `delta_kernel::Error` instead of
`object_store::Error` and we return err instead of panic.

### This PR affects the following public APIs
`insert_url_handler` returns `delta_kernel::Error` instead of
`object_store::Error`


## How was this change tested?
existing. `cargo tree` to check deps and manually verified that our code
blows up if we remove `arrow-54` from `arrow_compat` and specify
`arrow-54` flag (better testing coming later).

---------

Co-authored-by: Ryan Johnson <[email protected]>

Author: zachschuermann

README.md

@@ -79,13 +79,17 @@ flags:
 
 - `arrow-54`: Use arrow version 54
 - `arrow-55`: Use arrow version 55
+- `arrow`: Use the latest arrow version. Note that this is an _unstable_ flag: we will bump this to
+  the latest arrow version at every arrow version release. Only removing old arrow versions will
+  cause a breaking change for kernel. If you require a specific version N of arrow, you should
+  specify it directly with `arrow-N`, e.g. `arrow-55`.
 
-Note that if more than one `arrow-x` feature is enabled, kernel will default to the _lowest_
-specified flag. This also means that if you use `--all-features` you will get the lowest version of
+Note that if more than one `arrow-x` feature is enabled, kernel will use the _highest_ (latest)
+specified flag. This also means that if you use `--all-features` you will get the latest version of
 arrow that kernel supports.
 
 If you enable at least one of `default-engine`, `sync-engine`, `arrow-conversion`, or
-`arrow-expression`, you must enable either `arrow` (we pick default version) or `arrow-54` or
+`arrow-expression`, you must enable either `arrow` (latest arrow version) or `arrow-54` or
 `arrow-55`.
 
 ### Object Store

kernel/Cargo.toml

@@ -104,19 +104,20 @@ internal-api = []
 integration-test = ["hdfs-native-object-store/integration-test"]
 
 # The default versions for arrow/parquet/object_store
-arrow = ["arrow-55"]
+arrow = ["arrow-55"] # latest arrow version
+need-arrow = [] # need-arrow is a marker that the feature needs arrow dep
 arrow-54 = ["dep:arrow_54", "dep:parquet_54", "dep:object_store_54"]
 arrow-55 = ["dep:arrow_55", "dep:parquet_55", "dep:object_store_55"]
-arrow-conversion = ["arrow"]
-arrow-expression = ["arrow"]
+arrow-conversion = ["need-arrow"]
+arrow-expression = ["need-arrow"]
 
 # this is an 'internal' feature flag which has all the shared bits from default-engine and
 # default-engine-rustls
 default-engine-base = [
   "arrow-conversion",
   "arrow-expression",
   "futures",
-  "arrow",
+  "need-arrow",
   "tokio",
 ]
 # the default-engine use the reqwest crate with default features which uses native-tls. if you want
@@ -128,7 +129,7 @@ default-engine-rustls = [
   "reqwest/http2",
 ]
 sync-engine = [
-  "arrow",
+  "need-arrow",
   "tempfile",
 ]
 

kernel/src/arrow_compat.rs

@@ -1,13 +1,6 @@
 //! This module re-exports the different versions of arrow, parquet, and object_store we support.
 
-#[cfg(all(feature = "arrow-55", not(feature = "arrow-54")))]
-mod arrow_compat_shims {
-    pub use arrow_55 as arrow;
-    pub use object_store_55 as object_store;
-    pub use parquet_55 as parquet;
-}
-
-#[cfg(all(feature = "arrow-55", feature = "arrow-54"))]
+#[cfg(feature = "arrow-55")]
 mod arrow_compat_shims {
     pub use arrow_55 as arrow;
     pub use object_store_55 as object_store;
@@ -24,7 +17,7 @@ mod arrow_compat_shims {
 // if nothing is enabled but we need arrow because of some other feature flag, throw compile-time
 // error
 #[cfg(all(
-    feature = "arrow",
+    feature = "need-arrow",
     not(feature = "arrow-54"),
     not(feature = "arrow-55")
 ))]

kernel/src/engine/default/storage.rs

@@ -3,6 +3,7 @@ use crate::object_store::path::Path;
 use crate::object_store::{Error, ObjectStore};
 use url::Url;
 
+use crate::Error as DeltaError;
 use std::collections::HashMap;
 use std::sync::{Arc, LazyLock, RwLock};
 
@@ -25,12 +26,13 @@ static URL_REGISTRY: LazyLock<RwLock<Handlers>> = LazyLock::new(|| RwLock::new(H
 pub fn insert_url_handler(
     scheme: impl AsRef<str>,
     handler_closure: HandlerClosure,
-) -> Result<(), Error> {
-    if let Ok(mut lock) = URL_REGISTRY.write() {
-        (*lock).insert(scheme.as_ref().into(), handler_closure);
-    } else {
-        panic!("Failed to acquire lock for adding a URL handler!");
-    }
+) -> Result<(), DeltaError> {
+    let Ok(mut registry) = URL_REGISTRY.write() else {
+        return Err(DeltaError::generic(
+            "failed to acquire lock for adding a URL handler!",
+        ));
+    };
+    registry.insert(scheme.as_ref().into(), handler_closure);
     Ok(())
 }