From e7686c04d568ee7800b8de9c994545633c015c69 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 20 Oct 2026 13:19:19 +0000 Subject: [PATCH 1/4] Remove legacy_session and the deprecated APIs that rely on it With every other user migrated, delete the hidden global `legacy_session()`, its clippy ban, and the deprecated wrappers that still used it, migrating their callers to the explicit-context versions: - `ArrayRef::{scalar_at, into_canonical, to_canonical}` and `Array::::{scalar_at, to_canonical}` - the `ToCanonical` trait and its clippy bans - `Validity::{is_valid, is_null}` - `vortex_arrow::IntoArrowArray` and `from_arrow_array_with_len` `native_geometry_scalar_from_wkb` now takes a `VortexSession` instead of an `ArrowSession`, so it can read the decoded scalar with an explicit context. Signed-off-by: Claude Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Y1zHpE1K1Y9HvyU4kt24Tq --- STYLE.md | 4 +- clippy.toml | 10 -- docs/developer-guide/benchmarking.md | 12 +- encodings/fastlanes/src/delta/compute/cast.rs | 7 +- vortex-array/src/array/erased.rs | 27 ---- vortex-array/src/array/typed.rs | 19 --- .../src/arrays/primitive/compute/cast.rs | 19 ++- vortex-array/src/canonical.rs | 117 ------------------ vortex-array/src/lib.rs | 13 -- vortex-array/src/normalize.rs | 4 +- vortex-array/src/validity.rs | 18 --- vortex-arrow/src/datum.rs | 44 ------- vortex-arrow/src/lib.rs | 32 ----- vortex-duckdb/src/convert/expr.rs | 3 +- vortex-jni/src/expression.rs | 5 +- vortex-spatial/src/extension/mod.rs | 30 ++--- vortex-spatial/src/scalar_fn/contains.rs | 3 +- vortex-spatial/src/scalar_fn/intersects.rs | 3 +- 18 files changed, 51 insertions(+), 319 deletions(-) diff --git a/STYLE.md b/STYLE.md index b75876e0e6e..b65a1de164f 100644 --- a/STYLE.md +++ b/STYLE.md @@ -114,8 +114,8 @@ Watch for these accessors inside `for i in 0..n { ... }`: | Per-element accessor | Hidden cost | Bulk replacement | | --- | --- | --- | -| `Validity::is_valid(i)` / `is_null(i)` | Array-backed validity allocates an `ExecutionCtx` and runs a scalar lookup per call. | Call `validity.execute_mask(len, ctx)?` once, then read the materialized mask. | -| `array.scalar_at(i)` / `array.execute_scalar(i, ctx)` | Executes through the compute stack per element. | Canonicalize once with `execute::` or `as_slice`, then index. | +| `Validity::execute_is_valid(i, ctx)` / `execute_is_null(i, ctx)` | Array-backed validity runs a scalar lookup per call. | Call `validity.execute_mask(len, ctx)?` once, then read the materialized mask. | +| `array.execute_scalar(i, ctx)` | Executes through the compute stack per element. | Canonicalize once with `execute::` or `as_slice`, then index. | | `BitBuffer::value(i)` / `Mask::value(i)` accumulated into a count | Recomputes the byte address and defeats popcount. | Use `true_count()`, `BitBuffer::count_range(start, end)`, or `set_indices()`. | | `BitIterator::next()` accumulated into a rank or prefix count | Processes one bit at a time. | Use `count_range` over each gap. | | Re-deriving a value such as `self.validity()?` | Repeats the derivation for every element. | Hoist the derivation above the loop. | diff --git a/clippy.toml b/clippy.toml index 73f74ca35f5..9f6d5bd21b8 100644 --- a/clippy.toml +++ b/clippy.toml @@ -17,14 +17,4 @@ disallowed-methods = [ { path = "std::thread::available_parallelism", reason = "This function might do an unbounded amount of work, use `vortex_utils::parallelism::get_available_parallelism instead" }, { path = "vortex_session::registry::Id::new", reason = "Interning a static id on every call grabs the interner lock (#8380). Use a `CachedId` static for static ids; for a dynamic string, annotate the call with `#[expect(clippy::disallowed_methods)]`.", allow-invalid = true }, { path = "vortex_session::registry::Id::new_static", reason = "Interning a static id on every call grabs the interner lock; use a `CachedId` static instead (#8380).", allow-invalid = true }, - { path = "vortex_array::legacy_session", reason = "Relies on the hidden global session; thread an explicit `VortexSession`/`ExecutionCtx` through instead" }, - { path = "vortex_array::ToCanonical::to_null", reason = "This function doesn't take the context, use `array.execute::(ctx)` instead", allow-invalid = true }, - { path = "vortex_array::ToCanonical::to_bool", reason = "This function doesn't take the context, use `array.execute::(ctx)` instead", allow-invalid = true }, - { path = "vortex_array::ToCanonical::to_primitive", reason = "This function doesn't take the context, use `array.execute::(ctx)` instead", allow-invalid = true }, - { path = "vortex_array::ToCanonical::to_decimal", reason = "This function doesn't take the context, use `array.execute::(ctx)` instead", allow-invalid = true }, - { path = "vortex_array::ToCanonical::to_struct", reason = "This function doesn't take the context, use `array.execute::(ctx)` instead", allow-invalid = true }, - { path = "vortex_array::ToCanonical::to_listview", reason = "This function doesn't take the context, use `array.execute::(ctx)` instead", allow-invalid = true }, - { path = "vortex_array::ToCanonical::to_fixed_size_list", reason = "This function doesn't take the context, use `array.execute::(ctx)` instead", allow-invalid = true }, - { path = "vortex_array::ToCanonical::to_varbinview", reason = "This function doesn't take the context, use `array.execute::(ctx)` instead", allow-invalid = true }, - { path = "vortex_array::ToCanonical::to_extension", reason = "This function doesn't take the context, use `array.execute::(ctx)` instead", allow-invalid = true }, ] diff --git a/docs/developer-guide/benchmarking.md b/docs/developer-guide/benchmarking.md index 9c2f096f897..a747a29bf3b 100644 --- a/docs/developer-guide/benchmarking.md +++ b/docs/developer-guide/benchmarking.md @@ -92,11 +92,13 @@ Return values are automatically black-boxed. You only need explicit `black_box` for side-effect-free results inside manual loops: ```rust -bencher.with_inputs(|| &array).bench_refs(|array| { - for idx in 0..len { - divan::black_box(array.scalar_at(idx).unwrap()); - } -}); +bencher + .with_inputs(|| (&array, SESSION.create_execution_ctx())) + .bench_refs(|(array, ctx)| { + for idx in 0..len { + divan::black_box(array.execute_scalar(idx, ctx).unwrap()); + } + }); ``` ### Use deterministic, seeded RNG diff --git a/encodings/fastlanes/src/delta/compute/cast.rs b/encodings/fastlanes/src/delta/compute/cast.rs index d2beafe62ff..658d3ddb2e0 100644 --- a/encodings/fastlanes/src/delta/compute/cast.rs +++ b/encodings/fastlanes/src/delta/compute/cast.rs @@ -53,6 +53,7 @@ mod tests { use std::sync::LazyLock; use rstest::rstest; + use vortex_array::Canonical; use vortex_array::IntoArray; use vortex_array::VortexSessionExecute; use vortex_array::arrays::PrimitiveArray; @@ -173,11 +174,13 @@ mod tests { let array = Delta::try_from_primitive_array(&values, &mut SESSION.create_execution_ctx())?; // The cast is lazy; the nullability check fires when it is executed. - #[expect(deprecated)] let result = array .into_array() .cast(DType::Primitive(PType::U32, Nullability::NonNullable)) - .and_then(|a| a.to_canonical().map(|c| c.into_array())); + .and_then(|a| { + a.execute::(&mut SESSION.create_execution_ctx()) + .map(|c| c.into_array()) + }); assert!( result.is_err(), diff --git a/vortex-array/src/array/erased.rs b/vortex-array/src/array/erased.rs index f0559b576bb..7f804c6c865 100644 --- a/vortex-array/src/array/erased.rs +++ b/vortex-array/src/array/erased.rs @@ -29,7 +29,6 @@ use crate::ExecutionCtx; use crate::ExecutionResult; use crate::IntoArray; use crate::VTable; -use crate::VortexSessionExecute; use crate::aggregate_fn::fns::sum::sum; use crate::array::ArrayData; use crate::array::ArrayId; @@ -48,7 +47,6 @@ use crate::dtype::DType; use crate::expr::stats::Precision; use crate::expr::stats::Stat; use crate::expr::stats::StatsProviderExt; -use crate::legacy_session; use crate::matcher::Matcher; use crate::optimizer::ArrayOptimizer; use crate::scalar::Scalar; @@ -266,16 +264,6 @@ impl ArrayRef { .optimize() } - /// Fetch the scalar at the given index. - #[deprecated( - note = "Use `execute_scalar` instead, which allows passing an execution context for more \ - efficient execution when fetching multiple scalars from the same array." - )] - #[allow(clippy::disallowed_methods)] - pub fn scalar_at(&self, index: usize) -> VortexResult { - self.execute_scalar(index, &mut legacy_session().create_execution_ctx()) - } - /// Execute the array to extract a scalar at the given index. /// /// A one-off read; the same as `self.probe().execute_scalar(index, ctx)`. @@ -399,21 +387,6 @@ impl ArrayRef { self.0.data.validity(self) } - /// Returns the canonical representation of the array. - #[deprecated(note = "use `array.execute::(ctx)` instead")] - #[allow(clippy::disallowed_methods)] - pub fn into_canonical(self) -> VortexResult { - self.execute(&mut legacy_session().create_execution_ctx()) - } - - /// Returns the canonical representation of the array. - #[deprecated(note = "use `array.execute::(ctx)` instead")] - pub fn to_canonical(&self) -> VortexResult { - #[expect(deprecated)] - let result = self.clone().into_canonical(); - result - } - /// Writes the array into the canonical builder. pub fn append_to_builder( &self, diff --git a/vortex-array/src/array/typed.rs b/vortex-array/src/array/typed.rs index 90f07a0bd38..1446afc217c 100644 --- a/vortex-array/src/array/typed.rs +++ b/vortex-array/src/array/typed.rs @@ -24,12 +24,10 @@ use crate::ArrayRef; use crate::ArraySlots; use crate::ExecutionCtx; use crate::IntoArray; -use crate::VortexSessionExecute; use crate::array::ArrayId; use crate::array::ArrayView; use crate::array::VTable; use crate::dtype::DType; -use crate::legacy_session; use crate::stats::ArrayStats; use crate::stats::StatsSet; use crate::stats::StatsSetRef; @@ -413,16 +411,6 @@ impl Array { self.inner.slice(range) } - #[deprecated( - note = "Use `execute_scalar` instead, which allows passing an execution context for more \ - efficient execution when fetching multiple scalars from the same array." - )] - #[allow(clippy::disallowed_methods)] - pub fn scalar_at(&self, index: usize) -> VortexResult { - self.inner - .execute_scalar(index, &mut legacy_session().create_execution_ctx()) - } - /// Execute the array to extract a scalar at the given index. pub fn execute_scalar( &self, @@ -467,13 +455,6 @@ impl Array { self.inner.all_invalid(ctx) } - #[deprecated(note = "Use Array::::execute::() instead")] - pub fn to_canonical(&self) -> VortexResult { - #[expect(deprecated)] - let result = self.inner.to_canonical(); - result - } - /// Returns the estimated physical bytes owned or referenced by this array tree. pub fn nbytes(&self) -> u64 { self.inner.nbytes() diff --git a/vortex-array/src/arrays/primitive/compute/cast.rs b/vortex-array/src/arrays/primitive/compute/cast.rs index deb4915fb23..3eb19b4b6f9 100644 --- a/vortex-array/src/arrays/primitive/compute/cast.rs +++ b/vortex-array/src/arrays/primitive/compute/cast.rs @@ -667,6 +667,7 @@ mod test { use vortex_mask::Mask; use crate::ArrayRef; + use crate::Canonical; use crate::IntoArray; use crate::VortexSessionExecute; use crate::array_session; @@ -1081,10 +1082,12 @@ mod test { #[test] fn cast_i32_u32() { let arr = buffer![-1i32].into_array(); - #[expect(deprecated)] let error = arr .cast(PType::U32.into()) - .and_then(|a| a.to_canonical().map(|c| c.into_array())) + .and_then(|a| { + a.execute::(&mut array_session().create_execution_ctx()) + .map(|c| c.into_array()) + }) .unwrap_err(); assert!(matches!(error, VortexError::Compute(..))); assert!(error.to_string().contains("values exceed target range")); @@ -1093,11 +1096,13 @@ mod test { #[test] fn cast_array_with_nulls_to_nonnullable() { let arr = PrimitiveArray::from_option_iter([Some(-1i32), None, Some(10)]); - #[expect(deprecated)] let err = arr .into_array() .cast(PType::I32.into()) - .and_then(|a| a.to_canonical().map(|c| c.into_array())) + .and_then(|a| { + a.execute::(&mut array_session().create_execution_ctx()) + .map(|c| c.into_array()) + }) .unwrap_err(); assert!(matches!(err, VortexError::InvalidArgument(..))); @@ -1157,10 +1162,12 @@ mod test { #[test] fn cast_same_width_int_out_of_range_errors() { let arr = buffer![u32::MAX].into_array(); - #[expect(deprecated)] let err = arr .cast(PType::I32.into()) - .and_then(|a| a.to_canonical().map(|c| c.into_array())) + .and_then(|a| { + a.execute::(&mut array_session().create_execution_ctx()) + .map(|c| c.into_array()) + }) .unwrap_err(); assert!(matches!(err, VortexError::Compute(..))); } diff --git a/vortex-array/src/canonical.rs b/vortex-array/src/canonical.rs index 97e36001fd4..eb92d6300b8 100644 --- a/vortex-array/src/canonical.rs +++ b/vortex-array/src/canonical.rs @@ -493,123 +493,6 @@ impl IntoArray for Canonical { } } -/// Trait for types that can be converted from an owned type into an owned array variant. -/// -/// # Canonicalization -/// -/// This trait has a blanket implementation for all types implementing [ToCanonical]. -#[deprecated(note = "use `array.execute::(ctx)` instead")] -pub trait ToCanonical { - /// Canonicalize into a [`NullArray`] if the target is [`Null`](DType::Null) typed. - #[deprecated(note = "use `array.execute::(ctx)` instead")] - fn to_null(&self) -> NullArray; - - /// Canonicalize into a [`BoolArray`] if the target is [`Bool`](DType::Bool) typed. - #[deprecated(note = "use `array.execute::(ctx)` instead")] - fn to_bool(&self) -> BoolArray; - - /// Canonicalize into a [`PrimitiveArray`] if the target is [`Primitive`](DType::Primitive) - /// typed. - #[deprecated(note = "use `array.execute::(ctx)` instead")] - fn to_primitive(&self) -> PrimitiveArray; - - /// Canonicalize into a [`DecimalArray`] if the target is [`Decimal`](DType::Decimal) - /// typed. - #[deprecated(note = "use `array.execute::(ctx)` instead")] - fn to_decimal(&self) -> DecimalArray; - - /// Canonicalize into a [`StructArray`] if the target is [`Struct`](DType::Struct) typed. - #[deprecated(note = "use `array.execute::(ctx)` instead")] - fn to_struct(&self) -> StructArray; - - /// Canonicalize into a [`ListViewArray`] if the target is [`List`](DType::List) typed. - #[deprecated(note = "use `array.execute::(ctx)` instead")] - fn to_listview(&self) -> ListViewArray; - - /// Canonicalize into a [`MapArray`] if the target is [`Map`](DType::Map) typed. - #[deprecated(note = "use `array.execute::(ctx)` instead")] - fn to_map(&self) -> MapArray; - - /// Canonicalize into a [`FixedSizeListArray`] if the target is [`List`](DType::FixedSizeList) - /// typed. - #[deprecated(note = "use `array.execute::(ctx)` instead")] - fn to_fixed_size_list(&self) -> FixedSizeListArray; - - /// Canonicalize into a [`VarBinViewArray`] if the target is [`Utf8`](DType::Utf8) - /// or [`Binary`](DType::Binary) typed. - #[deprecated(note = "use `array.execute::(ctx)` instead")] - fn to_varbinview(&self) -> VarBinViewArray; - - /// Canonicalize into an [`ExtensionArray`] if the array is [`Extension`](DType::Extension) - /// typed. - #[deprecated(note = "use `array.execute::(ctx)` instead")] - fn to_extension(&self) -> ExtensionArray; -} - -// Blanket impl for all Array encodings. -#[expect(deprecated)] -impl ToCanonical for ArrayRef { - fn to_null(&self) -> NullArray { - #[expect(deprecated)] - let result = self.to_canonical().vortex_expect("to_canonical failed"); - result.into_null() - } - - fn to_bool(&self) -> BoolArray { - #[expect(deprecated)] - let result = self.to_canonical().vortex_expect("to_canonical failed"); - result.into_bool() - } - - fn to_primitive(&self) -> PrimitiveArray { - #[expect(deprecated)] - let result = self.to_canonical().vortex_expect("to_canonical failed"); - result.into_primitive() - } - - fn to_decimal(&self) -> DecimalArray { - #[expect(deprecated)] - let result = self.to_canonical().vortex_expect("to_canonical failed"); - result.into_decimal() - } - - fn to_struct(&self) -> StructArray { - #[expect(deprecated)] - let result = self.to_canonical().vortex_expect("to_canonical failed"); - result.into_struct() - } - - fn to_listview(&self) -> ListViewArray { - #[expect(deprecated)] - let result = self.to_canonical().vortex_expect("to_canonical failed"); - result.into_listview() - } - - fn to_map(&self) -> MapArray { - #[expect(deprecated)] - let result = self.to_canonical().vortex_expect("to_canonical failed"); - result.into_map() - } - - fn to_fixed_size_list(&self) -> FixedSizeListArray { - #[expect(deprecated)] - let result = self.to_canonical().vortex_expect("to_canonical failed"); - result.into_fixed_size_list() - } - - fn to_varbinview(&self) -> VarBinViewArray { - #[expect(deprecated)] - let result = self.to_canonical().vortex_expect("to_canonical failed"); - result.into_varbinview() - } - - fn to_extension(&self) -> ExtensionArray { - #[expect(deprecated)] - let result = self.to_canonical().vortex_expect("to_canonical failed"); - result.into_extension() - } -} - impl From for ArrayRef { fn from(value: Canonical) -> Self { match_each_canonical!(value, |arr| arr.into_array()) diff --git a/vortex-array/src/lib.rs b/vortex-array/src/lib.rs index fc64f01de52..4eb2620fe82 100644 --- a/vortex-array/src/lib.rs +++ b/vortex-array/src/lib.rs @@ -79,8 +79,6 @@ extern crate self as vortex_array; -use std::sync::LazyLock; - pub use array::*; pub use canonical::*; pub use columnar::*; @@ -177,15 +175,4 @@ pub fn array_session() -> VortexSession { .with::() } -// TODO(ngates): canonicalize doesn't currently take a session, therefore we cannot invoke execute -// from the new array encodings to support back-compat for legacy encodings. So we hold a session -// here... -/// Returns the hidden global [`VortexSession`] used as a back-compat shim for code paths that -/// cannot yet thread an explicit session through. -#[inline] -pub fn legacy_session() -> &'static VortexSession { - static LEGACY_SESSION: LazyLock = LazyLock::new(array_session); - &LEGACY_SESSION -} - pub type ArrayContext = Interner; diff --git a/vortex-array/src/normalize.rs b/vortex-array/src/normalize.rs index 76c3ba23742..33f4de740ad 100644 --- a/vortex-array/src/normalize.rs +++ b/vortex-array/src/normalize.rs @@ -105,6 +105,7 @@ mod tests { use super::NormalizeOptions; use super::Operation; use crate::ArrayRef; + use crate::Canonical; use crate::IntoArray; use crate::VortexSessionExecute; use crate::array::VTable; @@ -229,8 +230,7 @@ mod tests { assert_eq!(normalized.len(), 3); // Verify the data: codes [1,0,1] -> values [20, 10, 20] - #[expect(deprecated)] - let normalized_canonical = normalized.to_canonical()?; + let normalized_canonical = normalized.execute::(&mut ctx)?; assert_arrays_eq!( normalized_canonical, PrimitiveArray::from_iter(vec![20i32, 10, 20]), diff --git a/vortex-array/src/validity.rs b/vortex-array/src/validity.rs index 4442d378e03..d1acdf650df 100644 --- a/vortex-array/src/validity.rs +++ b/vortex-array/src/validity.rs @@ -24,14 +24,12 @@ use crate::ArrayRef; use crate::Canonical; use crate::ExecutionCtx; use crate::IntoArray; -use crate::VortexSessionExecute; use crate::arrays::BoolArray; use crate::arrays::ChunkedArray; use crate::arrays::ConstantArray; use crate::builtins::ArrayBuiltins; use crate::dtype::DType; use crate::dtype::Nullability; -use crate::legacy_session; use crate::optimizer::ArrayOptimizer; use crate::patches::Patches; use crate::scalar::Scalar; @@ -184,22 +182,6 @@ impl Validity { Ok(!self.execute_is_valid(index, ctx)?) } - /// Returns whether the `index` item is valid. - #[deprecated(note = "use `execute_is_valid` with an explicit `ExecutionCtx`")] - #[inline] - #[allow(clippy::disallowed_methods)] - pub fn is_valid(&self, index: usize) -> VortexResult { - self.execute_is_valid(index, &mut legacy_session().create_execution_ctx()) - } - - /// Returns whether the `index` item is null. - #[deprecated(note = "use `execute_is_null` with an explicit `ExecutionCtx`")] - #[inline] - #[allow(clippy::disallowed_methods)] - pub fn is_null(&self, index: usize) -> VortexResult { - self.execute_is_null(index, &mut legacy_session().create_execution_ctx()) - } - #[inline] pub fn slice(&self, range: Range) -> VortexResult { match self { diff --git a/vortex-arrow/src/datum.rs b/vortex-arrow/src/datum.rs index 953a9d8d714..d501ff1d920 100644 --- a/vortex-arrow/src/datum.rs +++ b/vortex-arrow/src/datum.rs @@ -9,17 +9,12 @@ use arrow_schema::Field; use vortex_array::ArrayRef; use vortex_array::ExecutionCtx; use vortex_array::IntoArray; -use vortex_array::VortexSessionExecute; use vortex_array::arrays::Constant; use vortex_array::arrays::ConstantArray; -use vortex_array::legacy_session; -use vortex_error::VortexExpect; use vortex_error::VortexResult; use vortex_error::vortex_panic; use crate::ArrowSessionExt; -#[allow(deprecated)] -use crate::FromArrowArray; /// A wrapper around a generic Arrow array that can be used as a Datum in Arrow compute. #[derive(Debug)] @@ -99,45 +94,6 @@ impl ArrowDatum for Datum { } } -/// Convert an Arrow array to an Array with a specific length. -/// This is useful for compute functions that delegate to Arrow using [Datum], -/// which will return a scalar (length 1 Arrow array) if the input array is constant. -/// -/// # Error -/// -/// The provided array must have length -#[deprecated( - note = "Relies on the hidden global `legacy_session()`; use `from_arrow_columnar` with an explicit `ExecutionCtx` instead" -)] -#[allow(clippy::disallowed_methods)] -#[allow(deprecated)] -pub fn from_arrow_array_with_len(array: A, len: usize, nullable: bool) -> VortexResult -where - ArrayRef: FromArrowArray, -{ - let array = ArrayRef::from_arrow(array, nullable)?; - if array.len() == len { - return Ok(array); - } - - if array.len() != 1 { - vortex_panic!( - "Array length mismatch, expected {} got {} for encoding {}", - len, - array.len(), - array.encoding_id() - ); - } - - Ok(ConstantArray::new( - array - .execute_scalar(0, &mut legacy_session().create_execution_ctx()) - .vortex_expect("array of length 1 must support execute_scalar(0)"), - len, - ) - .into_array()) -} - /// Convert an Arrow array to an Array with a specific length, using the provided /// [`ExecutionCtx`]. /// diff --git a/vortex-arrow/src/lib.rs b/vortex-arrow/src/lib.rs index 7b08d5e7c6f..e69e2106ea8 100644 --- a/vortex-arrow/src/lib.rs +++ b/vortex-arrow/src/lib.rs @@ -16,11 +16,6 @@ //! The [`ArrowSession`] session variable is created lazily on first use of //! [`ArrowSessionExt::arrow`], so no explicit registration is required for plain conversions. -use arrow_array::ArrayRef as ArrowArrayRef; -use arrow_schema::DataType; -use vortex_array::ArrayRef; -use vortex_array::VortexSessionExecute; -use vortex_array::legacy_session; use vortex_error::VortexResult; use vortex_session::VortexSession; @@ -89,30 +84,3 @@ pub trait FromArrowArray { where Self: Sized; } - -#[deprecated(note = "Use `execute_arrow(None, ctx)` or `execute_arrow(Some(dt), ctx)` instead")] -pub trait IntoArrowArray { - #[deprecated(note = "Use `execute_arrow(None, ctx)` instead")] - fn into_arrow_preferred(self) -> VortexResult; - - #[deprecated(note = "Use `execute_arrow(Some(data_type), ctx)` instead")] - fn into_arrow(self, data_type: &DataType) -> VortexResult; -} - -#[allow(deprecated)] -impl IntoArrowArray for ArrayRef { - /// Convert this [`vortex_array::ArrayRef`] into an Arrow [`ArrowArrayRef`] by using the array's - /// preferred (cheapest) Arrow [`DataType`]. - #[allow(clippy::disallowed_methods)] - fn into_arrow_preferred(self) -> VortexResult { - self.execute_arrow(None, &mut legacy_session().create_execution_ctx()) - } - - #[allow(clippy::disallowed_methods)] - fn into_arrow(self, data_type: &DataType) -> VortexResult { - self.execute_arrow( - Some(data_type), - &mut legacy_session().create_execution_ctx(), - ) - } -} diff --git a/vortex-duckdb/src/convert/expr.rs b/vortex-duckdb/src/convert/expr.rs index dff2b674d5b..7d7cf0e6aa8 100644 --- a/vortex-duckdb/src/convert/expr.rs +++ b/vortex-duckdb/src/convert/expr.rs @@ -17,7 +17,6 @@ use vortex::aggregate_fn::fns::max::Max; use vortex::aggregate_fn::fns::mean::Mean; use vortex::aggregate_fn::fns::min::Min; use vortex::aggregate_fn::fns::sum_v2::SumV2; -use vortex::arrow::ArrowSessionExt; use vortex::dtype::DType; use vortex::dtype::Nullability; use vortex::dtype::PType; @@ -158,7 +157,7 @@ fn spatial_operand(value: &ExpressionRef, ctx: ConvertCtx<'_>) -> VortexResult diff --git a/vortex-jni/src/expression.rs b/vortex-jni/src/expression.rs index a766bf3d6c3..afcefe990c4 100644 --- a/vortex-jni/src/expression.rs +++ b/vortex-jni/src/expression.rs @@ -24,6 +24,7 @@ use jni::sys::jfloat; use jni::sys::jint; use jni::sys::jlong; use jni::sys::jshort; +use vortex::VortexSessionDefault; use vortex::dtype::BigCast; use vortex::dtype::DType; use vortex::dtype::DecimalDType; @@ -71,7 +72,7 @@ use vortex::scalar_fn::fns::list_contains::ListContainsOptions; use vortex::scalar_fn::fns::literal::Literal; use vortex::scalar_fn::fns::merge::DuplicateHandling; use vortex::scalar_fn::fns::operators::Operator; -use vortex_arrow::ArrowSession; +use vortex::session::VortexSession; use vortex_spatial::extension::native_geometry_scalar_from_wkb; use vortex_spatial::scalar_fn::area::SpatialArea; use vortex_spatial::scalar_fn::collect::SpatialCollect; @@ -748,7 +749,7 @@ pub extern "system" fn Java_dev_vortex_jni_NativeExpression_literalGeometry( throw_runtime!("geometry literal WKB bytes must not be null"); } let bytes = env.convert_byte_array(&wkb)?; - let scalar = native_geometry_scalar_from_wkb(&bytes, &ArrowSession::default())? + let scalar = native_geometry_scalar_from_wkb(&bytes, &VortexSession::default())? .ok_or_else(|| vortex_err!("unsupported WKB geometry type for a geometry literal"))?; Ok(into_raw(lit(scalar))) }) diff --git a/vortex-spatial/src/extension/mod.rs b/vortex-spatial/src/extension/mod.rs index e662c479bb6..4da9cd74571 100644 --- a/vortex-spatial/src/extension/mod.rs +++ b/vortex-spatial/src/extension/mod.rs @@ -42,6 +42,7 @@ pub use rect::*; use vortex_array::ArrayRef; use vortex_array::ExecutionCtx; use vortex_array::IntoArray; +use vortex_array::VortexSessionExecute; use vortex_array::arrays::ConstantArray; use vortex_array::arrays::ExtensionArray; use vortex_array::arrays::ListViewArray; @@ -58,10 +59,12 @@ use vortex_array::dtype::extension::ExtDType; use vortex_array::dtype::extension::ExtVTable; use vortex_array::scalar::Scalar; use vortex_arrow::ArrowSession; +use vortex_arrow::ArrowSessionExt; use vortex_buffer::Buffer; use vortex_error::VortexResult; use vortex_error::vortex_bail; use vortex_error::vortex_err; +use vortex_session::VortexSession; pub use wkb::*; /// Whether `dtype` is one of the native geometry extension types the spatial kernels operate on. @@ -194,7 +197,7 @@ pub(crate) fn single_geometry( /// `Point`/`Polygon`/`MultiPolygon` scalar. `None` for unsupported types. Plan-time, one value only. pub fn native_geometry_scalar_from_wkb( bytes: &[u8], - session: &ArrowSession, + session: &VortexSession, ) -> VortexResult> { let metadata = geoarrow_metadata(&SpatialMetadata::default()); let binary = BinaryArray::from(vec![Some(bytes)]); @@ -208,7 +211,9 @@ pub fn native_geometry_scalar_from_wkb( let to_storage = |target: &GeoArrowType| -> VortexResult { let native = cast(&wkb, target).map_err(|e| vortex_err!("failed to cast WKB literal: {e}"))?; - session.from_arrow_array(native.to_array_ref(), false) + session + .arrow() + .from_arrow_array(native.to_array_ref(), false) }; let scalar = match Wkb::try_from_bytes(bytes)?.geometry_type() { @@ -216,39 +221,39 @@ pub fn native_geometry_scalar_from_wkb( let target = GeoArrowType::Point( PointType::new(Dimension::XY, metadata).with_coord_type(CoordType::Separated), ); - spatial_ext_scalar(Point, to_storage(&target)?)? + spatial_ext_scalar(Point, to_storage(&target)?, session)? } GeometryType::LineString => { let target = GeoArrowType::LineString( LineStringType::new(Dimension::XY, metadata).with_coord_type(CoordType::Separated), ); - spatial_ext_scalar(LineString, to_storage(&target)?)? + spatial_ext_scalar(LineString, to_storage(&target)?, session)? } GeometryType::Polygon => { let target = GeoArrowType::Polygon( PolygonType::new(Dimension::XY, metadata).with_coord_type(CoordType::Separated), ); - spatial_ext_scalar(Polygon, to_storage(&target)?)? + spatial_ext_scalar(Polygon, to_storage(&target)?, session)? } GeometryType::MultiPoint => { let target = GeoArrowType::MultiPoint( MultiPointType::new(Dimension::XY, metadata).with_coord_type(CoordType::Separated), ); - spatial_ext_scalar(MultiPoint, to_storage(&target)?)? + spatial_ext_scalar(MultiPoint, to_storage(&target)?, session)? } GeometryType::MultiLineString => { let target = GeoArrowType::MultiLineString( MultiLineStringType::new(Dimension::XY, metadata) .with_coord_type(CoordType::Separated), ); - spatial_ext_scalar(MultiLineString, to_storage(&target)?)? + spatial_ext_scalar(MultiLineString, to_storage(&target)?, session)? } GeometryType::MultiPolygon => { let target = GeoArrowType::MultiPolygon( MultiPolygonType::new(Dimension::XY, metadata) .with_coord_type(CoordType::Separated), ); - spatial_ext_scalar(MultiPolygon, to_storage(&target)?)? + spatial_ext_scalar(MultiPolygon, to_storage(&target)?, session)? } _ => return Ok(None), }; @@ -256,18 +261,17 @@ pub fn native_geometry_scalar_from_wkb( } /// Wrap cast-from-WKB `storage` in its `vtable` extension type and pull out the single scalar. -// `scalar_at` is deprecated for `execute_scalar`, but there is no execution context at plan time. -#[allow(deprecated)] fn spatial_ext_scalar>( vtable: V, storage: ArrayRef, + session: &VortexSession, ) -> VortexResult { let ext = ExtDType::try_with_vtable(vtable, SpatialMetadata::default(), storage.dtype().clone())? .erased(); ExtensionArray::try_new(ext, storage)? .into_array() - .scalar_at(0) + .execute_scalar(0, &mut session.create_execution_ctx()) } /// Extension metadata that is common to all the spatial extension types. @@ -334,7 +338,6 @@ mod tests { use prost::Message; use vortex_array::VortexSessionExecute; use vortex_array::dtype::DType; - use vortex_arrow::ArrowSessionExt; use vortex_error::VortexResult; use vortex_error::vortex_err; @@ -349,8 +352,7 @@ mod tests { fn native_geometry_scalar_from_wkb( bytes: &[u8], ) -> VortexResult> { - let session = vortex_array::array_session(); - super::native_geometry_scalar_from_wkb(bytes, &session.arrow()) + super::native_geometry_scalar_from_wkb(bytes, &vortex_array::array_session()) } #[test] diff --git a/vortex-spatial/src/scalar_fn/contains.rs b/vortex-spatial/src/scalar_fn/contains.rs index 796e283f63f..9093f7055c2 100644 --- a/vortex-spatial/src/scalar_fn/contains.rs +++ b/vortex-spatial/src/scalar_fn/contains.rs @@ -93,7 +93,6 @@ mod tests { use vortex_array::scalar_fn::EmptyOptions; use vortex_array::scalar_fn::ScalarFnVTable; use vortex_array::validity::Validity; - use vortex_arrow::ArrowSessionExt; use vortex_buffer::BitBuffer; use vortex_error::VortexResult; use vortex_error::vortex_err; @@ -118,7 +117,7 @@ mod tests { wkb::writer::write_geometry(&mut buf, geometry, &WriteOptions::default()) .map_err(|e| vortex_err!("writing WKB failed: {e}"))?; let session = vortex_array::array_session(); - let scalar = crate::extension::native_geometry_scalar_from_wkb(&buf, &session.arrow())? + let scalar = crate::extension::native_geometry_scalar_from_wkb(&buf, &session)? .ok_or_else(|| vortex_err!("unsupported geometry type"))?; Ok(ConstantArray::new(scalar, len).into_array()) } diff --git a/vortex-spatial/src/scalar_fn/intersects.rs b/vortex-spatial/src/scalar_fn/intersects.rs index bf913ec507c..3a6593b2ba2 100644 --- a/vortex-spatial/src/scalar_fn/intersects.rs +++ b/vortex-spatial/src/scalar_fn/intersects.rs @@ -92,7 +92,6 @@ mod tests { use vortex_array::scalar_fn::EmptyOptions; use vortex_array::scalar_fn::ScalarFnVTable; use vortex_array::validity::Validity; - use vortex_arrow::ArrowSessionExt; use vortex_buffer::BitBuffer; use vortex_error::VortexResult; use vortex_error::vortex_err; @@ -139,7 +138,7 @@ mod tests { wkb::writer::write_geometry(&mut buf, geometry, &WriteOptions::default()) .map_err(|e| vortex_err!("writing WKB failed: {e}"))?; let session = vortex_array::array_session(); - let scalar = crate::extension::native_geometry_scalar_from_wkb(&buf, &session.arrow())? + let scalar = crate::extension::native_geometry_scalar_from_wkb(&buf, &session)? .ok_or_else(|| vortex_err!("unsupported geometry type"))?; Ok(ConstantArray::new(scalar, len).into_array()) } From f84ec378c5791f314287faef63fdc4083c21c0e4 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 20 Oct 2026 13:40:16 +0000 Subject: [PATCH 2/4] Remove the deprecated FromArrowArray trait Its impls were thin shims over the canonical from_arrow_* conversions, and ArrowSession is the plugin-aware replacement. Tests now call the conversion functions directly. Signed-off-by: Claude Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Y1zHpE1K1Y9HvyU4kt24Tq --- vortex-arrow/src/convert.rs | 267 ++++++----------------------- vortex-arrow/src/executor/map.rs | 13 +- vortex-arrow/src/lib.rs | 32 ---- vortex-arrow/src/run_end_import.rs | 6 +- vortex-arrow/src/session.rs | 3 +- 5 files changed, 59 insertions(+), 262 deletions(-) diff --git a/vortex-arrow/src/convert.rs b/vortex-arrow/src/convert.rs index 20976c71434..f85982f50fb 100644 --- a/vortex-arrow/src/convert.rs +++ b/vortex-arrow/src/convert.rs @@ -6,9 +6,7 @@ //! Each `from_arrow_*` function converts one Arrow array shape; [`from_arrow_dyn`] dispatches on //! the Arrow [`DataType`]. These functions have no awareness of Arrow extension types — the //! plugin-aware entry points are the [`ArrowSession`](crate::ArrowSession) methods, which fall -//! back to these conversions for non-extension data. The deprecated [`FromArrowArray`] impls are -//! thin shims over these functions and will eventually be removed. -#![allow(deprecated)] +//! back to these conversions for non-extension data. use std::ops::Range; use std::sync::Arc; @@ -107,7 +105,6 @@ use vortex_error::vortex_ensure_eq; use vortex_error::vortex_err; use vortex_error::vortex_panic; -use crate::FromArrowArray; use crate::dtype::from_arrow_time_unit; /// Zero-copy conversion of Arrow buffers into non-nullable Vortex arrays. @@ -179,28 +176,6 @@ where Ok(PrimitiveArray::new(buffer, validity).into_array()) } -macro_rules! impl_from_arrow_primitive { - ($T:path) => { - impl FromArrowArray<&ArrowPrimitiveArray<$T>> for ArrayRef { - fn from_arrow(value: &ArrowPrimitiveArray<$T>, nullable: bool) -> VortexResult { - from_arrow_primitive(value, nullable) - } - } - }; -} - -impl_from_arrow_primitive!(Int8Type); -impl_from_arrow_primitive!(Int16Type); -impl_from_arrow_primitive!(Int32Type); -impl_from_arrow_primitive!(Int64Type); -impl_from_arrow_primitive!(UInt8Type); -impl_from_arrow_primitive!(UInt16Type); -impl_from_arrow_primitive!(UInt32Type); -impl_from_arrow_primitive!(UInt64Type); -impl_from_arrow_primitive!(Float16Type); -impl_from_arrow_primitive!(Float32Type); -impl_from_arrow_primitive!(Float64Type); - /// Zero-copy conversion of an Arrow `Decimal32` array into a Vortex decimal array. pub fn from_arrow_decimal32( array: &ArrowPrimitiveArray, @@ -212,15 +187,6 @@ pub fn from_arrow_decimal32( Ok(DecimalArray::new(buffer, decimal_type, validity).into_array()) } -impl FromArrowArray<&ArrowPrimitiveArray> for ArrayRef { - fn from_arrow( - array: &ArrowPrimitiveArray, - nullable: bool, - ) -> VortexResult { - from_arrow_decimal32(array, nullable) - } -} - /// Zero-copy conversion of an Arrow `Decimal64` array into a Vortex decimal array. pub fn from_arrow_decimal64( array: &ArrowPrimitiveArray, @@ -232,15 +198,6 @@ pub fn from_arrow_decimal64( Ok(DecimalArray::new(buffer, decimal_type, validity).into_array()) } -impl FromArrowArray<&ArrowPrimitiveArray> for ArrayRef { - fn from_arrow( - array: &ArrowPrimitiveArray, - nullable: bool, - ) -> VortexResult { - from_arrow_decimal64(array, nullable) - } -} - /// Zero-copy conversion of an Arrow `Decimal128` array into a Vortex decimal array. pub fn from_arrow_decimal128( array: &ArrowPrimitiveArray, @@ -252,15 +209,6 @@ pub fn from_arrow_decimal128( Ok(DecimalArray::new(buffer, decimal_type, validity).into_array()) } -impl FromArrowArray<&ArrowPrimitiveArray> for ArrayRef { - fn from_arrow( - array: &ArrowPrimitiveArray, - nullable: bool, - ) -> VortexResult { - from_arrow_decimal128(array, nullable) - } -} - /// Zero-copy conversion of an Arrow `Decimal256` array into a Vortex decimal array. pub fn from_arrow_decimal256( array: &ArrowPrimitiveArray, @@ -276,43 +224,11 @@ pub fn from_arrow_decimal256( Ok(DecimalArray::new(buffer, decimal_type, validity).into_array()) } -impl FromArrowArray<&ArrowPrimitiveArray> for ArrayRef { - fn from_arrow( - array: &ArrowPrimitiveArray, - nullable: bool, - ) -> VortexResult { - from_arrow_decimal256(array, nullable) - } -} - -macro_rules! impl_from_arrow_temporal { - ($T:path) => { - impl FromArrowArray<&ArrowPrimitiveArray<$T>> for ArrayRef { - fn from_arrow( - value: &ArrowPrimitiveArray<$T>, - nullable: bool, - ) -> vortex_error::VortexResult { - from_arrow_temporal(value, nullable) - } - } - }; -} - // timestamp -impl_from_arrow_temporal!(TimestampSecondType); -impl_from_arrow_temporal!(TimestampMillisecondType); -impl_from_arrow_temporal!(TimestampMicrosecondType); -impl_from_arrow_temporal!(TimestampNanosecondType); // time -impl_from_arrow_temporal!(Time32SecondType); -impl_from_arrow_temporal!(Time32MillisecondType); -impl_from_arrow_temporal!(Time64MicrosecondType); -impl_from_arrow_temporal!(Time64NanosecondType); // date -impl_from_arrow_temporal!(Date32Type); -impl_from_arrow_temporal!(Date64Type); /// Conversion of an Arrow temporal array (timestamp/date/time) into a Vortex temporal /// extension array. @@ -394,15 +310,6 @@ where .into_array()) } -impl FromArrowArray<&GenericByteArray> for ArrayRef -where - ::Offset: IntegerPType, -{ - fn from_arrow(value: &GenericByteArray, nullable: bool) -> VortexResult { - from_arrow_bytes(value, nullable) - } -} - /// Zero-copy conversion of an Arrow string/binary view array into a Vortex `VarBinView` array. pub fn from_arrow_byte_view( value: &GenericByteViewArray, @@ -437,12 +344,6 @@ pub fn from_arrow_byte_view( }) } -impl FromArrowArray<&GenericByteViewArray> for ArrayRef { - fn from_arrow(value: &GenericByteViewArray, nullable: bool) -> VortexResult { - from_arrow_byte_view(value, nullable) - } -} - /// Zero-copy conversion of an Arrow boolean array into a Vortex `Bool` array. pub fn from_arrow_boolean(value: &ArrowBooleanArray, nullable: bool) -> VortexResult { Ok(BoolArray::new( @@ -452,12 +353,6 @@ pub fn from_arrow_boolean(value: &ArrowBooleanArray, nullable: bool) -> VortexRe .into_array()) } -impl FromArrowArray<&ArrowBooleanArray> for ArrayRef { - fn from_arrow(value: &ArrowBooleanArray, nullable: bool) -> VortexResult { - from_arrow_boolean(value, nullable) - } -} - /// Strip out the nulls from this array and return a new array without nulls. pub(crate) fn remove_nulls(data: arrow_data::ArrayData) -> VortexResult { if data.null_count() == 0 { @@ -531,12 +426,6 @@ pub fn from_arrow_struct(value: &ArrowStructArray, nullable: bool) -> VortexResu .into_array()) } -impl FromArrowArray<&ArrowStructArray> for ArrayRef { - fn from_arrow(value: &ArrowStructArray, nullable: bool) -> VortexResult { - from_arrow_struct(value, nullable) - } -} - /// Conversion of an Arrow (large) list array into a Vortex `List` array. pub fn from_arrow_list( value: &GenericListArray, @@ -565,12 +454,6 @@ pub fn from_arrow_list( Ok(unsafe { ListArray::new_unchecked(elements, offsets.into_array(), nulls) }.into_array()) } -impl FromArrowArray<&GenericListArray> for ArrayRef { - fn from_arrow(value: &GenericListArray, nullable: bool) -> VortexResult { - from_arrow_list(value, nullable) - } -} - /// Conversion of an Arrow (large) list-view array into a Vortex `ListView` array. pub fn from_arrow_list_view( array: &GenericListViewArray, @@ -595,12 +478,6 @@ pub fn from_arrow_list_view( Ok(unsafe { ListViewArray::new_unchecked(elements, offsets, sizes, nulls) }.into_array()) } -impl FromArrowArray<&GenericListViewArray> for ArrayRef { - fn from_arrow(array: &GenericListViewArray, nullable: bool) -> VortexResult { - from_arrow_list_view(array, nullable) - } -} - /// Conversion of an Arrow fixed-size list array into a Vortex `FixedSizeList` array. pub fn from_arrow_fixed_size_list( array: &ArrowFixedSizeListArray, @@ -619,12 +496,6 @@ pub fn from_arrow_fixed_size_list( .into_array()) } -impl FromArrowArray<&ArrowFixedSizeListArray> for ArrayRef { - fn from_arrow(array: &ArrowFixedSizeListArray, nullable: bool) -> VortexResult { - from_arrow_fixed_size_list(array, nullable) - } -} - pub(crate) fn map_from_arrow_parts( entries: ArrayRef, offsets: &OffsetBuffer, @@ -698,12 +569,6 @@ pub fn from_arrow_map(array: &ArrowMapArray, nullable: bool) -> VortexResult for ArrayRef { - fn from_arrow(array: &ArrowMapArray, nullable: bool) -> VortexResult { - from_arrow_map(array, nullable) - } -} - /// Conversion of an Arrow null array into a Vortex `Null` array. pub fn from_arrow_null(value: &ArrowNullArray, nullable: bool) -> VortexResult { vortex_ensure!( @@ -713,12 +578,6 @@ pub fn from_arrow_null(value: &ArrowNullArray, nullable: bool) -> VortexResult for ArrayRef { - fn from_arrow(value: &ArrowNullArray, nullable: bool) -> VortexResult { - from_arrow_null(value, nullable) - } -} - /// Conversion of an Arrow dictionary array into a Vortex `Dict` array. pub fn from_arrow_dictionary( array: &DictionaryArray, @@ -731,12 +590,6 @@ pub fn from_arrow_dictionary( Ok(unsafe { DictArray::new_unchecked(keys, values) }) } -impl FromArrowArray<&DictionaryArray> for DictArray { - fn from_arrow(array: &DictionaryArray, nullable: bool) -> VortexResult { - from_arrow_dictionary(array, nullable) - } -} - /// Convert an Arrow [`NullBuffer`] into a Vortex [`Validity`] for an array of the given /// nullability. /// @@ -890,29 +743,11 @@ pub fn from_arrow_dyn(array: &dyn ArrowArray, nullable: bool) -> VortexResult for ArrayRef { - fn from_arrow(array: &dyn ArrowArray, nullable: bool) -> VortexResult { - from_arrow_dyn(array, nullable) - } -} - /// Canonical conversion of an Arrow [`RecordBatch`] into a Vortex struct array. pub fn from_arrow_batch(batch: &RecordBatch, nullable: bool) -> VortexResult { from_arrow_struct(&arrow_array::StructArray::from(batch.clone()), nullable) } -impl FromArrowArray for ArrayRef { - fn from_arrow(array: RecordBatch, nullable: bool) -> VortexResult { - from_arrow_batch(&array, nullable) - } -} - -impl FromArrowArray<&RecordBatch> for ArrayRef { - fn from_arrow(array: &RecordBatch, nullable: bool) -> VortexResult { - from_arrow_batch(array, nullable) - } -} - #[cfg(test)] mod tests { use std::ops::Range; @@ -971,7 +806,6 @@ mod tests { use arrow_schema::Fields; use arrow_schema::Schema; use rstest::rstest; - use vortex_array::ArrayRef; use vortex_array::IntoArray; use vortex_array::VortexSessionExecute as _; use vortex_array::array_session; @@ -1010,7 +844,6 @@ mod tests { use vortex_buffer::buffer; use vortex_error::VortexResult; - use crate::FromArrowArray as _; use crate::IntoVortexArray as _; use crate::convert::from_arrow_batch; use crate::convert::from_arrow_dyn; @@ -1071,8 +904,8 @@ mod tests { #[case] non_nullable: Arc, #[case] expected_ptype: PType, ) { - let v_null = ArrayRef::from_arrow(nullable.as_ref(), true).unwrap(); - let v_non_null = ArrayRef::from_arrow(non_nullable.as_ref(), false).unwrap(); + let v_null = from_arrow_dyn(nullable.as_ref(), true).unwrap(); + let v_non_null = from_arrow_dyn(non_nullable.as_ref(), false).unwrap(); assert_eq!(v_null.len(), 4); assert_eq!(v_non_null.len(), 4); assert_eq!(v_null.as_::().ptype(), expected_ptype); @@ -1140,7 +973,7 @@ mod tests { Some(::Native::from_f32(3.5)), ]; let arrow_array = arrow_array::PrimitiveArray::::from(values); - let vortex_array = ArrayRef::from_arrow(&arrow_array, true).unwrap(); + let vortex_array = from_arrow_dyn(&arrow_array, true).unwrap(); let non_null_values = vec![ ::Native::from_f32(1.5), @@ -1148,7 +981,7 @@ mod tests { ]; let arrow_array_non_null = arrow_array::PrimitiveArray::::from(non_null_values); - let vortex_array_non_null = ArrayRef::from_arrow(&arrow_array_non_null, false).unwrap(); + let vortex_array_non_null = from_arrow_dyn(&arrow_array_non_null, false).unwrap(); assert_eq!(vortex_array.len(), 3); assert_eq!(vortex_array_non_null.len(), 2); @@ -1188,7 +1021,7 @@ mod tests { builder.append_value(11111); let decimal_array = builder.finish().with_precision_and_scale(10, 2).unwrap(); - let vortex_array = ArrayRef::from_arrow(&decimal_array, true).unwrap(); + let vortex_array = from_arrow_dyn(&decimal_array, true).unwrap(); assert_eq!(vortex_array.len(), 4); let mut builder_non_null = Decimal128Builder::with_capacity(3); @@ -1200,7 +1033,7 @@ mod tests { .with_precision_and_scale(10, 2) .unwrap(); - let vortex_array_non_null = ArrayRef::from_arrow(&decimal_array_non_null, false).unwrap(); + let vortex_array_non_null = from_arrow_dyn(&decimal_array_non_null, false).unwrap(); assert_eq!(vortex_array_non_null.len(), 3); // Verify metadata - should be DecimalArray with correct precision and scale @@ -1238,7 +1071,7 @@ mod tests { builder.append_value(arrow_buffer::i256::from_i128(11111)); let decimal_array = builder.finish().with_precision_and_scale(38, 10).unwrap(); - let vortex_array = ArrayRef::from_arrow(&decimal_array, true).unwrap(); + let vortex_array = from_arrow_dyn(&decimal_array, true).unwrap(); assert_eq!(vortex_array.len(), 4); let mut builder_non_null = Decimal256Builder::with_capacity(3); @@ -1250,7 +1083,7 @@ mod tests { .with_precision_and_scale(38, 10) .unwrap(); - let vortex_array_non_null = ArrayRef::from_arrow(&decimal_array_non_null, false).unwrap(); + let vortex_array_non_null = from_arrow_dyn(&decimal_array_non_null, false).unwrap(); assert_eq!(vortex_array_non_null.len(), 3); // Verify metadata - should be DecimalArray with correct precision and scale @@ -1339,8 +1172,8 @@ mod tests { #[case] nullable: Arc, #[case] non_nullable: Arc, ) { - let v_null = ArrayRef::from_arrow(nullable.as_ref(), true).unwrap(); - let v_non_null = ArrayRef::from_arrow(non_nullable.as_ref(), false).unwrap(); + let v_null = from_arrow_dyn(nullable.as_ref(), true).unwrap(); + let v_non_null = from_arrow_dyn(non_nullable.as_ref(), false).unwrap(); assert_eq!(v_null.len(), 4); assert_eq!(v_non_null.len(), 4); let expected = |nullable: bool| { @@ -1433,11 +1266,11 @@ mod tests { let arrow_array = TimestampMicrosecondArray::from(vec![Some(1000), None, Some(3000), Some(4000)]) .with_timezone("UTC"); - let vortex_array = ArrayRef::from_arrow(&arrow_array, true).unwrap(); + let vortex_array = from_arrow_dyn(&arrow_array, true).unwrap(); let arrow_array_non_null = TimestampMicrosecondArray::from(vec![1000_i64, 2000, 3000, 4000]).with_timezone("UTC"); - let vortex_array_non_null = ArrayRef::from_arrow(&arrow_array_non_null, false).unwrap(); + let vortex_array_non_null = from_arrow_dyn(&arrow_array_non_null, false).unwrap(); assert_eq!(vortex_array.len(), 4); assert_eq!( @@ -1519,8 +1352,8 @@ mod tests { #[case] non_nullable: Arc, #[case] expected_non_nullable_dtype: DType, ) { - let v_null = ArrayRef::from_arrow(nullable.as_ref(), true).unwrap(); - let v_non_null = ArrayRef::from_arrow(non_nullable.as_ref(), false).unwrap(); + let v_null = from_arrow_dyn(nullable.as_ref(), true).unwrap(); + let v_non_null = from_arrow_dyn(non_nullable.as_ref(), false).unwrap(); assert_eq!(v_null.len(), 4); assert_eq!(v_non_null.len(), 4); assert_eq!(v_null.dtype(), &expected_non_nullable_dtype.as_nullable()); @@ -1551,7 +1384,7 @@ mod tests { builder.append_value("world"); builder.append_value("test"); let arrow_array = builder.finish(); - let vortex_array = ArrayRef::from_arrow(&arrow_array, true).unwrap(); + let vortex_array = from_arrow_dyn(&arrow_array, true).unwrap(); let mut builder_non_null = StringViewBuilder::new(); builder_non_null.append_value("hello"); @@ -1559,7 +1392,7 @@ mod tests { builder_non_null.append_value("test"); builder_non_null.append_value("vortex"); let arrow_array_non_null = builder_non_null.finish(); - let vortex_array_non_null = ArrayRef::from_arrow(&arrow_array_non_null, false).unwrap(); + let vortex_array_non_null = from_arrow_dyn(&arrow_array_non_null, false).unwrap(); assert_eq!(vortex_array.len(), 4); assert_eq!(vortex_array_non_null.len(), 4); @@ -1607,7 +1440,7 @@ mod tests { builder.append_value(b"world"); builder.append_value(b"test"); let arrow_array = builder.finish(); - let vortex_array = ArrayRef::from_arrow(&arrow_array, true).unwrap(); + let vortex_array = from_arrow_dyn(&arrow_array, true).unwrap(); let mut builder_non_null = BinaryViewBuilder::new(); builder_non_null.append_value(b"hello"); @@ -1615,7 +1448,7 @@ mod tests { builder_non_null.append_value(b"test"); builder_non_null.append_value(b"vortex"); let arrow_array_non_null = builder_non_null.finish(); - let vortex_array_non_null = ArrayRef::from_arrow(&arrow_array_non_null, false).unwrap(); + let vortex_array_non_null = from_arrow_dyn(&arrow_array_non_null, false).unwrap(); assert_eq!(vortex_array.len(), 4); assert_eq!(vortex_array_non_null.len(), 4); @@ -1659,10 +1492,10 @@ mod tests { #[test] fn test_boolean_array_conversion() { let arrow_array = BooleanArray::from(vec![Some(true), None, Some(false), Some(true)]); - let vortex_array = ArrayRef::from_arrow(&arrow_array, true).unwrap(); + let vortex_array = from_arrow_dyn(&arrow_array, true).unwrap(); let arrow_array_non_null = BooleanArray::from(vec![true, false, true, false]); - let vortex_array_non_null = ArrayRef::from_arrow(&arrow_array_non_null, false).unwrap(); + let vortex_array_non_null = from_arrow_dyn(&arrow_array_non_null, false).unwrap(); assert_eq!(vortex_array.len(), 4); assert_eq!(vortex_array_non_null.len(), 4); @@ -1696,7 +1529,7 @@ mod tests { None, ); - let vortex_array = ArrayRef::from_arrow(&arrow_array, false).unwrap(); + let vortex_array = from_arrow_dyn(&arrow_array, false).unwrap(); assert_eq!(vortex_array.len(), 3); // Verify metadata - should be StructArray with correct field names @@ -1717,7 +1550,7 @@ mod tests { ]))), ); - let vortex_nullable_array = ArrayRef::from_arrow(&nullable_array, true).unwrap(); + let vortex_nullable_array = from_arrow_dyn(&nullable_array, true).unwrap(); assert_eq!(vortex_nullable_array.len(), 3); // Verify metadata for nullable struct @@ -1762,7 +1595,7 @@ mod tests { builder.append_value([Some(4), Some(5)]); let arrow_array = builder.finish(); - let vortex_array = ArrayRef::from_arrow(&arrow_array, true).unwrap(); + let vortex_array = from_arrow_dyn(&arrow_array, true).unwrap(); assert_eq!(vortex_array.len(), 3); // Verify metadata - should be ListArray with correct offsets @@ -1777,7 +1610,7 @@ mod tests { builder_non_null.append_value([Some(4), Some(5)]); let arrow_array_non_null = builder_non_null.finish(); - let vortex_array_non_null = ArrayRef::from_arrow(&arrow_array_non_null, false).unwrap(); + let vortex_array_non_null = from_arrow_dyn(&arrow_array_non_null, false).unwrap(); assert_eq!(vortex_array_non_null.len(), 2); // Verify metadata for non-nullable list @@ -1828,8 +1661,8 @@ mod tests { let mut ctx = array_session().create_execution_ctx(); // A slice keeps its parent's whole child; the import must not. - let sliced = ArrayRef::from_arrow(&strings(0..1000).slice(500, 2), false)?; - let fresh = ArrayRef::from_arrow(&strings(500..502), false)?; + let sliced = from_arrow_dyn(&strings(0..1000).slice(500, 2), false)?; + let fresh = from_arrow_dyn(&strings(500..502), false)?; assert_eq!(sliced.nbytes(), fresh.nbytes()); let expected = VarBinArray::from_iter_nonnull( ["value-500", "value-501"], @@ -1838,8 +1671,8 @@ mod tests { assert_arrays_eq!(sliced, expected, &mut ctx); assert_arrays_eq!(fresh, expected, &mut ctx); - let sliced = ArrayRef::from_arrow(&lists(0..1000).slice(500, 2), false)?; - let fresh = ArrayRef::from_arrow(&lists(500..502), false)?; + let sliced = from_arrow_dyn(&lists(0..1000).slice(500, 2), false)?; + let fresh = from_arrow_dyn(&lists(500..502), false)?; assert_eq!(sliced.nbytes(), fresh.nbytes()); let expected = ListArray::new( PrimitiveArray::from_option_iter([Some(500i64), Some(501), Some(501), Some(502)]) @@ -1862,7 +1695,7 @@ mod tests { builder.append_value([Some(4), Some(5)]); let arrow_array = builder.finish(); - let vortex_array = ArrayRef::from_arrow(&arrow_array, true).unwrap(); + let vortex_array = from_arrow_dyn(&arrow_array, true).unwrap(); assert_eq!(vortex_array.len(), 3); // Verify metadata - should be ListArray with correct offsets (I64 for large lists) @@ -1877,7 +1710,7 @@ mod tests { builder_non_null.append_value([Some(4), Some(5)]); let arrow_array_non_null = builder_non_null.finish(); - let vortex_array_non_null = ArrayRef::from_arrow(&arrow_array_non_null, false).unwrap(); + let vortex_array_non_null = from_arrow_dyn(&arrow_array_non_null, false).unwrap(); assert_eq!(vortex_array_non_null.len(), 2); // Verify metadata for non-nullable large list @@ -1936,7 +1769,7 @@ mod tests { let arrow_array = ArrowFixedSizeListArray::try_new(Arc::clone(&field), 3, Arc::new(values), None) .unwrap(); - let vortex_array = ArrayRef::from_arrow(&arrow_array, false).unwrap(); + let vortex_array = from_arrow_dyn(&arrow_array, false).unwrap(); assert_eq!(vortex_array.len(), 4); @@ -1969,7 +1802,7 @@ mod tests { Some(null_buffer), ) .unwrap(); - let vortex_array_nullable = ArrayRef::from_arrow(&arrow_array_nullable, true).unwrap(); + let vortex_array_nullable = from_arrow_dyn(&arrow_array_nullable, true).unwrap(); assert_eq!(vortex_array_nullable.len(), 3); @@ -2055,7 +1888,7 @@ mod tests { ) .unwrap(); - let vortex_array = ArrayRef::from_arrow(&arrow_array, false).unwrap(); + let vortex_array = from_arrow_dyn(&arrow_array, false).unwrap(); assert_eq!(vortex_array.len(), 4); // Verify metadata - should be ListViewArray with correct offsets and sizes @@ -2081,7 +1914,7 @@ mod tests { ) .unwrap(); - let vortex_array_nullable = ArrayRef::from_arrow(&arrow_array_nullable, true).unwrap(); + let vortex_array_nullable = from_arrow_dyn(&arrow_array_nullable, true).unwrap(); assert_eq!(vortex_array_nullable.len(), 4); // Test LargeListView (i64 offsets and sizes) @@ -2097,7 +1930,7 @@ mod tests { ) .unwrap(); - let large_vortex_array = ArrayRef::from_arrow(&large_arrow_array, false).unwrap(); + let large_vortex_array = from_arrow_dyn(&large_arrow_array, false).unwrap(); assert_eq!(large_vortex_array.len(), 4); // Verify metadata for large ListView @@ -2139,7 +1972,7 @@ mod tests { #[test] fn test_null_array_conversion() { let arrow_array = NullArray::new(5); - let vortex_array = ArrayRef::from_arrow(&arrow_array, true).unwrap(); + let vortex_array = from_arrow_dyn(&arrow_array, true).unwrap(); assert_eq!(vortex_array.len(), 5); assert_arrays_eq!( vortex_array, @@ -2214,7 +2047,7 @@ mod tests { let record_batch = RecordBatch::try_new(schema, vec![field1_data, field2_data]).unwrap(); - let vortex_array = ArrayRef::from_arrow(record_batch, false).unwrap(); + let vortex_array = from_arrow_batch(&record_batch, false).unwrap(); let expected = VortexStructArray::new( ["field1", "field2"].into(), vec![ @@ -2245,7 +2078,7 @@ mod tests { let record_batch = RecordBatch::try_new(schema, vec![field1_data, field2_data]).unwrap(); - let vortex_array = ArrayRef::from_arrow(&record_batch, false).unwrap(); + let vortex_array = from_arrow_batch(&record_batch, false).unwrap(); let expected = VortexStructArray::new( ["field1", "field2"].into(), vec![ @@ -2271,7 +2104,7 @@ mod tests { fn test_dyn_array_conversion() { let int_array = Int32Array::from(vec![1, 2, 3, 4]); let dyn_array: &dyn ArrowArray = &int_array; - let vortex_array = ArrayRef::from_arrow(dyn_array, false).unwrap(); + let vortex_array = from_arrow_dyn(dyn_array, false).unwrap(); assert_arrays_eq!( vortex_array, buffer![1i32, 2, 3, 4].into_array(), @@ -2280,7 +2113,7 @@ mod tests { let string_array = StringArray::from(vec!["a", "b", "c"]); let dyn_array: &dyn ArrowArray = &string_array; - let vortex_array = ArrayRef::from_arrow(dyn_array, false).unwrap(); + let vortex_array = from_arrow_dyn(dyn_array, false).unwrap(); assert_arrays_eq!( vortex_array, VarBinArray::from_iter_nonnull(["a", "b", "c"], DType::Utf8(Nullability::NonNullable)), @@ -2289,7 +2122,7 @@ mod tests { let bool_array = BooleanArray::from(vec![true, false, true]); let dyn_array: &dyn ArrowArray = &bool_array; - let vortex_array = ArrayRef::from_arrow(dyn_array, false).unwrap(); + let vortex_array = from_arrow_dyn(dyn_array, false).unwrap(); assert_arrays_eq!( vortex_array, BoolArray::from_iter([true, false, true]), @@ -2309,7 +2142,7 @@ mod tests { 1, ); let actual = - ArrayRef::from_arrow(null_struct_array_with_non_nullable_field.as_ref(), true).unwrap(); + from_arrow_dyn(null_struct_array_with_non_nullable_field.as_ref(), true).unwrap(); assert_arrays_eq!( actual, VortexStructArray::new( @@ -2337,7 +2170,7 @@ mod tests { 1, ); let actual = - ArrayRef::from_arrow(null_struct_array_with_non_nullable_field.as_ref(), true).unwrap(); + from_arrow_dyn(null_struct_array_with_non_nullable_field.as_ref(), true).unwrap(); assert_arrays_eq!( actual, VortexStructArray::new( @@ -2414,7 +2247,7 @@ mod tests { // Requesting `nullable = false` on an Arrow array that physically contains nulls is a // contradiction and must surface as an error, not a panic. let arrow_array = Int32Array::from(vec![Some(1), None, Some(3)]); - assert!(ArrayRef::from_arrow(&arrow_array, false).is_err()); + assert!(from_arrow_dyn(&arrow_array, false).is_err()); } #[test] @@ -2422,7 +2255,7 @@ mod tests { // An Arrow NullArray is entirely null, so it cannot be converted to a non-nullable // Vortex array. let arrow_array = NullArray::new(5); - assert!(ArrayRef::from_arrow(&arrow_array, false).is_err()); + assert!(from_arrow_dyn(&arrow_array, false).is_err()); } #[test] @@ -2433,7 +2266,7 @@ mod tests { &DataType::Struct(Fields::from(vec![Field::new("a", DataType::Int32, true)])), 3, ); - assert!(ArrayRef::from_arrow(struct_array.as_ref(), false).is_err()); + assert!(from_arrow_dyn(struct_array.as_ref(), false).is_err()); } #[test] @@ -2444,7 +2277,7 @@ mod tests { builder.append_value([Some(1), Some(2)]); builder.append_null(); let list = builder.finish(); - assert!(ArrayRef::from_arrow(&list, false).is_err()); + assert!(from_arrow_dyn(&list, false).is_err()); } #[test] @@ -2461,8 +2294,6 @@ mod tests { 1, ); - assert!( - ArrayRef::from_arrow(null_struct_array_with_non_nullable_field.as_ref(), true).is_err() - ); + assert!(from_arrow_dyn(null_struct_array_with_non_nullable_field.as_ref(), true).is_err()); } } diff --git a/vortex-arrow/src/executor/map.rs b/vortex-arrow/src/executor/map.rs index 810c74a6a1d..f93b5a34179 100644 --- a/vortex-arrow/src/executor/map.rs +++ b/vortex-arrow/src/executor/map.rs @@ -86,7 +86,6 @@ mod tests { use arrow_schema::DataType; use arrow_schema::Field; use arrow_schema::Fields; - use vortex_array::ArrayRef; use vortex_array::IntoArray; use vortex_array::VortexSessionExecute; use vortex_array::array_session; @@ -105,7 +104,7 @@ mod tests { use vortex_error::VortexResult; use vortex_mask::Mask; - use crate::FromArrowArray as _; + use crate::convert::from_arrow_map; use crate::session::ArrowSessionExt as _; fn i32_utf8_map_field(keys_sorted: bool, nullable: bool) -> Field { @@ -202,7 +201,7 @@ mod tests { } #[test] - fn legacy_from_arrow_map_imports_canonical_map() -> VortexResult<()> { + fn from_arrow_map_imports_canonical_map() -> VortexResult<()> { let arrow = i32_utf8_map_array( vec![0, 1, 2], vec![1, 2], @@ -210,7 +209,7 @@ mod tests { None, false, )?; - let vortex = ArrayRef::from_arrow(&arrow, false)?; + let vortex = from_arrow_map(&arrow, false)?; assert!(vortex.is::()); assert_eq!( @@ -274,13 +273,13 @@ mod tests { false, Nullable, )?; - let legacy = ArrayRef::from_arrow(&arrow, true)?; - assert_eq!(legacy.dtype(), &expected_dtype); + let canonical = from_arrow_map(&arrow, true)?; + assert_eq!(canonical.dtype(), &expected_dtype); let field = Field::new("maps", arrow.data_type().clone(), true); let vortex = session.from_arrow_array(Arc::new(arrow), &field)?; assert_eq!(vortex.dtype(), &expected_dtype); - for imported in [&legacy, &vortex] { + for imported in [&canonical, &vortex] { assert_eq!(imported.len(), 4); let expected_rows = [ Some(vec![("joe", 1i32)]), diff --git a/vortex-arrow/src/lib.rs b/vortex-arrow/src/lib.rs index e69e2106ea8..25f47180e4c 100644 --- a/vortex-arrow/src/lib.rs +++ b/vortex-arrow/src/lib.rs @@ -16,7 +16,6 @@ //! The [`ArrowSession`] session variable is created lazily on first use of //! [`ArrowSessionExt::arrow`], so no explicit registration is required for plain conversions. -use vortex_error::VortexResult; use vortex_session::VortexSession; pub mod convert; @@ -53,34 +52,3 @@ pub use session::*; pub fn initialize(session: &VortexSession) { session.register(ArrowSession::default()); } - -/// Construct a Vortex array from an Arrow array (or other Arrow container) of type `A`. -/// -/// Implementations reuse the underlying Arrow buffers without copying wherever the Arrow and -/// Vortex memory layouts allow it. -#[deprecated(note = "Use `ArrowSession` (`from_arrow_array`, `from_arrow_record_batch`) instead")] -pub trait FromArrowArray { - /// Convert `array` into a Vortex array whose [`DType`](vortex_array::dtype::DType) has the requested - /// `nullable` [`Nullability`](vortex_array::dtype::Nullability). - /// - /// An Arrow array can carry a validity (null) buffer regardless of whether its schema declares - /// the field nullable, so the desired nullability is supplied separately by the caller - /// (typically from the corresponding Arrow `Field`'s `is_nullable`). This flag is reconciled - /// with the array's physical nulls as follows: - /// - /// - `nullable == true`: the resulting validity is derived from the array's null buffer, or - /// all-valid when the array has none. - /// - `nullable == false`: the array must contain no nulls, and the result is non-nullable. - /// - /// # Errors - /// - /// Returns an error if `nullable` is `false` but `array` physically contains one or more nulls - /// (including an Arrow `NullArray`, which is entirely null), or if the Arrow data type is not - /// supported. - #[deprecated( - note = "Use `ArrowSession` (`from_arrow_array`, `from_arrow_record_batch`) instead" - )] - fn from_arrow(array: A, nullable: bool) -> VortexResult - where - Self: Sized; -} diff --git a/vortex-arrow/src/run_end_import.rs b/vortex-arrow/src/run_end_import.rs index d18b6bbecd0..d92c6b6c5a8 100644 --- a/vortex-arrow/src/run_end_import.rs +++ b/vortex-arrow/src/run_end_import.rs @@ -18,9 +18,9 @@ use vortex_runend::RunEnd; /// Build a Vortex run-end array from an Arrow [`RunArray`] and its already-converted `values`. /// -/// The caller converts `values` separately (rather than this function calling -/// [`crate::FromArrowArray`] on `array.values()`) so that extension metadata on the Arrow values -/// field dispatches through the session's registered import plugins. +/// The caller converts `values` separately (rather than this function converting +/// `array.values()` itself) so that extension metadata on the Arrow values field dispatches +/// through the session's registered import plugins. pub(crate) fn run_end_from_arrow( array: &RunArray, values: ArrayRef, diff --git a/vortex-arrow/src/session.rs b/vortex-arrow/src/session.rs index c830ef16bb0..9212def5b85 100644 --- a/vortex-arrow/src/session.rs +++ b/vortex-arrow/src/session.rs @@ -213,8 +213,7 @@ pub trait ArrowImportVTable: 'static + Send + Sync + Debug { /// /// `session` is provided so plugins can convert storage or nested arrays through the /// session (e.g. [`ArrowSession::from_arrow_array`], passing a nullability rather than a - /// [`Field`] so the plugin is not dispatched again) instead of the deprecated - /// `FromArrowArray` trait. + /// [`Field`] so the plugin is not dispatched again). #[allow(clippy::wrong_self_convention)] fn from_arrow_array( &self, From a7389c6ab85a77d8068850fb955da088820fb189 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 20 Oct 2026 13:41:56 +0000 Subject: [PATCH 3/4] Drop leftover section comments in Arrow conversions Signed-off-by: Claude Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Y1zHpE1K1Y9HvyU4kt24Tq --- vortex-arrow/src/convert.rs | 6 ------ 1 file changed, 6 deletions(-) diff --git a/vortex-arrow/src/convert.rs b/vortex-arrow/src/convert.rs index f85982f50fb..bc25b1a9406 100644 --- a/vortex-arrow/src/convert.rs +++ b/vortex-arrow/src/convert.rs @@ -224,12 +224,6 @@ pub fn from_arrow_decimal256( Ok(DecimalArray::new(buffer, decimal_type, validity).into_array()) } -// timestamp - -// time - -// date - /// Conversion of an Arrow temporal array (timestamp/date/time) into a Vortex temporal /// extension array. pub fn from_arrow_temporal( From 7231e0bce286749dc9e14be69fa82e83cb5d69d6 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 20 Oct 2026 13:45:15 +0000 Subject: [PATCH 4/4] Remove the deprecated FromArrowType, TryFromArrowType and ToArrowType traits ArrowSession's from_arrow_* and to_arrow_* methods replace them. The tests call the naive conversion functions or the session directly, and the bench of the deprecated to_arrow_dtype path is dropped. The naive schema and decimal helpers only backed these traits and are removed with them, along with two tests of the naive schema export's own behaviour. Signed-off-by: Claude Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Y1zHpE1K1Y9HvyU4kt24Tq --- vortex-arrow/benches/to_arrow.rs | 10 - vortex-arrow/src/dtype.rs | 328 +++---------------------------- vortex-arrow/src/lib.rs | 6 - 3 files changed, 31 insertions(+), 313 deletions(-) diff --git a/vortex-arrow/benches/to_arrow.rs b/vortex-arrow/benches/to_arrow.rs index e0e037ae372..9833bcc64d0 100644 --- a/vortex-arrow/benches/to_arrow.rs +++ b/vortex-arrow/benches/to_arrow.rs @@ -40,8 +40,6 @@ use vortex_array::session::ArraySessionExt; )] use vortex_arrow::ArrowArrayExecutor; use vortex_arrow::ArrowSessionExt; -#[allow(deprecated)] -use vortex_arrow::dtype::ToArrowType as _; use vortex_fsst::fsst_compress; use vortex_fsst::fsst_train_compressor; use vortex_onpair::DEFAULT_CONFIG; @@ -110,14 +108,6 @@ fn array() -> ArrayRef { .into_array() } -#[divan::bench] -fn to_arrow_dtype(bencher: Bencher) { - bencher.with_inputs(schema).bench_values(|dtype| { - #[expect(deprecated, reason = "benchmarking deprecated code path")] - dtype.to_arrow_dtype().unwrap() - }); -} - #[allow(non_snake_case)] #[divan::bench] fn ArrowExportVTable_to_arrow_field(bencher: Bencher) { diff --git a/vortex-arrow/src/dtype.rs b/vortex-arrow/src/dtype.rs index 860d56113da..9fb33b8ebba 100644 --- a/vortex-arrow/src/dtype.rs +++ b/vortex-arrow/src/dtype.rs @@ -9,9 +9,8 @@ //! The conversions in this module are "naive": every logical type is encoded in its simplest //! corresponding Arrow type, and Arrow extension types (other than the builtin temporal types and //! Parquet Variant) are not understood. The authoritative, plugin-aware conversion entry point is -//! [`ArrowSession`](crate::ArrowSession) — prefer its `to_arrow_schema` / `to_arrow_datatype` / -//! `from_arrow_schema` / `from_arrow_field` / `from_arrow_datatype` methods over the deprecated -//! traits in this module. +//! [`ArrowSession`](crate::ArrowSession), through its `to_arrow_schema` / `to_arrow_datatype` / +//! `from_arrow_schema` / `from_arrow_field` / `from_arrow_datatype` methods. //! //! For this reason, it's recommended to do as much computation as possible within Vortex, and then //! materialize an Arrow ArrayRef at the very end of the processing chain. @@ -22,9 +21,6 @@ use arrow_schema::DataType; use arrow_schema::Field; use arrow_schema::FieldRef; use arrow_schema::Fields; -use arrow_schema::Schema; -use arrow_schema::SchemaBuilder; -use arrow_schema::SchemaRef; use arrow_schema::TimeUnit as ArrowTimeUnit; use vortex_array::dtype::DType; use vortex_array::dtype::DecimalDType; @@ -38,7 +34,6 @@ use vortex_array::extension::datetime::TemporalMetadata; use vortex_array::extension::datetime::Time; use vortex_array::extension::datetime::TimeUnit; use vortex_array::extension::datetime::Timestamp; -use vortex_error::VortexExpect; use vortex_error::VortexResult; use vortex_error::vortex_bail; use vortex_error::vortex_ensure; @@ -46,54 +41,6 @@ use vortex_error::vortex_ensure_eq; use vortex_error::vortex_err; use vortex_error::vortex_panic; -/// Trait for converting Arrow types to Vortex types. -#[deprecated( - note = "Use `ArrowSession` (`from_arrow_schema`, `from_arrow_field`, `from_arrow_datatype`) instead" -)] -pub trait FromArrowType: Sized { - /// Convert the Arrow type to a Vortex type. - #[deprecated( - note = "Use `ArrowSession` (`from_arrow_schema`, `from_arrow_field`, `from_arrow_datatype`) instead" - )] - fn from_arrow(value: T) -> Self; -} - -/// Trait for converting Arrow types to Vortex types. -#[deprecated( - note = "Use `ArrowSession` (`from_arrow_schema`, `from_arrow_field`, `from_arrow_datatype`) instead" -)] -pub trait TryFromArrowType: Sized { - /// Convert the Arrow type to a Vortex type. - #[deprecated( - note = "Use `ArrowSession` (`from_arrow_schema`, `from_arrow_field`, `from_arrow_datatype`) instead" - )] - fn try_from_arrow(value: T) -> VortexResult; -} - -/// Extension trait converting Vortex [`DType`]s into Arrow schemas and data types. -/// -/// This mirrors inherent methods that lived on [`DType`] before Arrow interoperability moved -/// into this crate. -#[deprecated(note = "Use `ArrowSession::to_arrow_schema` / `to_arrow_datatype` instead")] -pub trait ToArrowType { - /// Convert a Vortex [`DType`] into an Arrow [`Schema`]. - /// - /// This method is not plugin-aware and strips any `ARROW:extension:name` metadata for - /// non-builtin extensions (only `arrow.parquet.variant` is special-cased here). Use the - /// session method when you need round-trippable extension metadata. - #[deprecated(note = "Use `ArrowSession::to_arrow_schema` instead")] - fn to_arrow_schema(&self) -> VortexResult; - - /// Returns the Arrow [`DataType`] that best corresponds to this Vortex [`DType`]. - /// - /// This method has no awareness of registered Arrow extension plugins, so any - /// [`DType::Extension`] outside the builtin temporal set will fail or silently lose its - /// `ARROW:extension:name` metadata. The session methods recurse through containers - /// and dispatch plugins at every extension node. - #[deprecated(note = "Use `ArrowSession::to_arrow_datatype` instead")] - fn to_arrow_dtype(&self) -> VortexResult; -} - /// Convert an Arrow [`ArrowTimeUnit`] to a Vortex [`TimeUnit`]. pub(crate) fn from_arrow_time_unit(value: ArrowTimeUnit) -> TimeUnit { match value { @@ -140,21 +87,6 @@ pub(crate) fn ptype_from_arrow(value: &DataType) -> VortexResult { } } -/// Naive conversion of an Arrow decimal [`DataType`] to a [`DecimalDType`]. -pub(crate) fn decimal_dtype_from_arrow(value: &DataType) -> VortexResult { - match value { - DataType::Decimal32(precision, scale) - | DataType::Decimal64(precision, scale) - | DataType::Decimal128(precision, scale) - | DataType::Decimal256(precision, scale) => DecimalDType::try_new(*precision, *scale), - - _ => Err(vortex_err!( - "Arrow datatype {:?} cannot be converted to DecimalDType", - value - )), - } -} - /// Naive conversion of an Arrow [`DataType`] to a Vortex [`DType`], with no awareness of Arrow /// extension plugins. pub(crate) fn from_arrow_data_type( @@ -269,49 +201,6 @@ pub(crate) fn from_arrow_fields_naive(fields: &Fields) -> VortexResult>() } -/// Naive conversion of an Arrow [`Schema`] to a Vortex top-level non-nullable struct [`DType`]. -pub(crate) fn from_arrow_schema_naive(schema: &Schema) -> VortexResult { - Ok(DType::Struct( - from_arrow_fields_naive(schema.fields())?, - Nullability::NonNullable, // Must match From for Array - )) -} - -/// Naive conversion of a Vortex top-level struct [`DType`] to an Arrow [`Schema`], with the -/// builtin `arrow.parquet.variant` special-case. -fn to_arrow_schema_naive(dtype: &DType) -> VortexResult { - let DType::Struct(struct_dtype, nullable) = dtype else { - vortex_bail!("only DType::Struct can be converted to arrow schema"); - }; - - if *nullable != Nullability::NonNullable { - vortex_bail!("top-level struct in Schema must be NonNullable"); - } - - let mut builder = SchemaBuilder::with_capacity(struct_dtype.names().len()); - for (field_name, field_dtype) in struct_dtype.names().iter().zip(struct_dtype.fields()) { - let field = if field_dtype.is_variant() { - let storage = DataType::Struct(variant_storage_fields_minimal()); - Field::new(field_name.as_ref(), storage, field_dtype.is_nullable()).with_metadata( - [( - "ARROW:extension:name".to_owned(), - "arrow.parquet.variant".to_owned(), - )] - .into(), - ) - } else { - Field::new( - field_name.as_ref(), - to_data_type_naive(&field_dtype)?, - field_dtype.is_nullable(), - ) - }; - builder.push(field); - } - - Ok(builder.finish()) -} - /// Naive conversion from a Vortex `DType` to the nearest Arrow physical data type. pub(crate) fn to_data_type_naive(dtype: &DType) -> VortexResult { Ok(match dtype { @@ -423,118 +312,8 @@ pub(crate) fn to_data_type_naive(dtype: &DType) -> VortexResult { }) } -fn variant_storage_fields_minimal() -> Fields { - Fields::from(vec![ - Field::new("metadata", DataType::Binary, false), - Field::new("value", DataType::Binary, true), - ]) -} - -/// Impls of the deprecated conversion traits, delegating to the naive free functions above. -#[allow(deprecated)] -mod deprecated_impls { - use super::*; - - impl TryFromArrowType<&DataType> for PType { - fn try_from_arrow(value: &DataType) -> VortexResult { - ptype_from_arrow(value) - } - } - - impl TryFromArrowType<&DataType> for DecimalDType { - fn try_from_arrow(value: &DataType) -> VortexResult { - decimal_dtype_from_arrow(value) - } - } - - impl FromArrowType<&ArrowTimeUnit> for TimeUnit { - fn from_arrow(value: &ArrowTimeUnit) -> Self { - from_arrow_time_unit(*value) - } - } - - impl FromArrowType for TimeUnit { - fn from_arrow(value: ArrowTimeUnit) -> Self { - from_arrow_time_unit(value) - } - } - - impl FromArrowType for DType { - fn from_arrow(value: SchemaRef) -> Self { - Self::from_arrow(value.as_ref()) - } - } - - impl TryFromArrowType for DType { - fn try_from_arrow(value: SchemaRef) -> VortexResult { - from_arrow_schema_naive(value.as_ref()) - } - } - - impl FromArrowType<&Schema> for DType { - fn from_arrow(value: &Schema) -> Self { - from_arrow_schema_naive(value).vortex_expect("arrow schema to dtype") - } - } - - impl TryFromArrowType<&Schema> for DType { - fn try_from_arrow(value: &Schema) -> VortexResult { - from_arrow_schema_naive(value) - } - } - - impl FromArrowType<&Fields> for StructFields { - fn from_arrow(value: &Fields) -> Self { - from_arrow_fields_naive(value).vortex_expect("arrow fields to struct fields") - } - } - - impl TryFromArrowType<&Fields> for StructFields { - fn try_from_arrow(value: &Fields) -> VortexResult { - from_arrow_fields_naive(value) - } - } - - impl FromArrowType<(&DataType, Nullability)> for DType { - fn from_arrow(value: (&DataType, Nullability)) -> Self { - from_arrow_data_type(value.0, value.1).vortex_expect("arrow data type to dtype") - } - } - - impl TryFromArrowType<(&DataType, Nullability)> for DType { - fn try_from_arrow( - (data_type, nullability): (&DataType, Nullability), - ) -> VortexResult { - from_arrow_data_type(data_type, nullability) - } - } - - impl FromArrowType<&Field> for DType { - fn from_arrow(field: &Field) -> Self { - from_arrow_field_naive(field).vortex_expect("arrow field to dtype") - } - } - - impl TryFromArrowType<&Field> for DType { - fn try_from_arrow(field: &Field) -> VortexResult { - from_arrow_field_naive(field) - } - } - - impl ToArrowType for DType { - fn to_arrow_schema(&self) -> VortexResult { - to_arrow_schema_naive(self) - } - - fn to_arrow_dtype(&self) -> VortexResult { - to_data_type_naive(self) - } - } -} - #[cfg(test)] mod test { - #![expect(deprecated, reason = "tests for deprecated traits and methods")] use arrow_schema::DataType; use arrow_schema::Field; use arrow_schema::FieldRef; @@ -550,48 +329,40 @@ mod test { use vortex_array::dtype::StructFields; use super::*; + use crate::ArrowSession; #[test] fn test_dtype_conversion_success() { - assert_eq!(DType::Null.to_arrow_dtype().unwrap(), DataType::Null); + assert_eq!(to_data_type_naive(&DType::Null).unwrap(), DataType::Null); assert_eq!( - DType::Bool(Nullability::NonNullable) - .to_arrow_dtype() - .unwrap(), + to_data_type_naive(&DType::Bool(Nullability::NonNullable)).unwrap(), DataType::Boolean ); assert_eq!( - DType::Primitive(PType::U64, Nullability::NonNullable) - .to_arrow_dtype() - .unwrap(), + to_data_type_naive(&DType::Primitive(PType::U64, Nullability::NonNullable)).unwrap(), DataType::UInt64 ); assert_eq!( - DType::Utf8(Nullability::NonNullable) - .to_arrow_dtype() - .unwrap(), + to_data_type_naive(&DType::Utf8(Nullability::NonNullable)).unwrap(), DataType::Utf8View ); assert_eq!( - DType::Binary(Nullability::NonNullable) - .to_arrow_dtype() - .unwrap(), + to_data_type_naive(&DType::Binary(Nullability::NonNullable)).unwrap(), DataType::BinaryView ); assert_eq!( - DType::struct_( + to_data_type_naive(&DType::struct_( [ ("field_a", DType::Bool(false.into())), ("field_b", DType::Utf8(true.into())) ], Nullability::NonNullable, - ) - .to_arrow_dtype() + )) .unwrap(), DataType::Struct(Fields::from(vec![ FieldRef::from(Field::new("field_a", DataType::Boolean, false)), @@ -609,7 +380,7 @@ mod test { use vortex_array::dtype::DecimalDType; let dtype = DType::Decimal(DecimalDType::new(precision, 0), Nullability::NonNullable); - assert_eq!(dtype.to_arrow_dtype().unwrap(), expected); + assert_eq!(to_data_type_naive(&dtype).unwrap(), expected); } #[rstest] @@ -625,8 +396,7 @@ mod test { #[test] fn test_variant_dtype_to_arrow_dtype_errors() { - let err = DType::Variant(Nullability::NonNullable) - .to_arrow_dtype() + let err = to_data_type_naive(&DType::Variant(Nullability::NonNullable)) .unwrap_err() .to_string(); assert!(err.contains("Variant")); @@ -639,13 +409,13 @@ mod test { Nullability::Nullable, ); - let arrow_list_non_nullable = list_non_nullable.to_arrow_dtype().unwrap(); + let arrow_list_non_nullable = to_data_type_naive(&list_non_nullable).unwrap(); let list_nullable = DType::List( Arc::new(DType::Primitive(PType::I64, Nullability::Nullable)), Nullability::Nullable, ); - let arrow_list_nullable = list_nullable.to_arrow_dtype().unwrap(); + let arrow_list_nullable = to_data_type_naive(&list_nullable).unwrap(); assert_ne!(arrow_list_non_nullable, arrow_list_nullable); assert_eq!( @@ -679,7 +449,9 @@ mod test { let schema_nonnull = DType::Struct(the_struct, Nullability::NonNullable); assert_eq!( - schema_nonnull.to_arrow_schema().unwrap(), + ArrowSession::default() + .to_arrow_schema(&schema_nonnull) + .unwrap(), Schema::new(Fields::from(vec![ Field::new("field_a", DataType::Boolean, false), Field::new("field_b", DataType::Utf8View, false), @@ -688,43 +460,6 @@ mod test { ); } - #[test] - fn test_schema_variant_field_metadata() { - let dtype = DType::struct_( - [("v", DType::Variant(Nullability::NonNullable))], - Nullability::NonNullable, - ); - let schema = dtype.to_arrow_schema().unwrap(); - let field = schema.field(0); - assert_eq!( - field - .metadata() - .get("ARROW:extension:name") - .map(|s| s.as_str()), - Some("arrow.parquet.variant") - ); - assert_eq!(field.name(), "v"); - assert_eq!( - field.data_type(), - &DataType::Struct(Fields::from(vec![ - Field::new("metadata", DataType::Binary, false), - Field::new("value", DataType::Binary, true) - ])) - ); - assert!(!field.is_nullable()); - } - - #[rstest] - fn test_schema_conversion_rejects_nullable_struct(the_struct: StructFields) { - let schema_null = DType::Struct(the_struct, Nullability::Nullable); - let error = schema_null.to_arrow_schema().unwrap_err(); - assert!( - error - .to_string() - .contains("top-level struct in Schema must be NonNullable") - ); - } - #[test] fn test_unicode_field_names_roundtrip() { // Regression test for https://github.com/vortex-data/vortex/issues/5979. @@ -740,7 +475,7 @@ mod test { Nullability::NonNullable, ); - let arrow_dtype = original_dtype.to_arrow_dtype().unwrap(); + let arrow_dtype = to_data_type_naive(&original_dtype).unwrap(); assert_eq!( arrow_dtype, DataType::Struct(Fields::from(vec![Field::new( @@ -749,7 +484,8 @@ mod test { true )])) ); - let roundtripped_dtype = DType::from_arrow((&arrow_dtype, Nullability::NonNullable)); + let roundtripped_dtype = + from_arrow_data_type(&arrow_dtype, Nullability::NonNullable).unwrap(); assert_eq!(original_dtype, roundtripped_dtype); } @@ -769,7 +505,7 @@ mod test { let original_dtype = DType::struct_([("\u{7}=outer", inner_struct)], Nullability::NonNullable); - let arrow_dtype = original_dtype.to_arrow_dtype().unwrap(); + let arrow_dtype = to_data_type_naive(&original_dtype).unwrap(); assert_eq!( arrow_dtype, DataType::Struct(Fields::from(vec![Field::new( @@ -782,7 +518,8 @@ mod test { true )])) ); - let roundtripped_dtype = DType::from_arrow((&arrow_dtype, Nullability::NonNullable)); + let roundtripped_dtype = + from_arrow_data_type(&arrow_dtype, Nullability::NonNullable).unwrap(); assert_eq!(original_dtype, roundtripped_dtype); } @@ -797,7 +534,7 @@ mod test { Nullability::Nullable, )?; - let arrow = dtype.to_arrow_dtype()?; + let arrow = to_data_type_naive(&dtype)?; let DataType::Map(entries, keys_sorted) = &arrow else { panic!("expected Map, got {arrow:?}"); }; @@ -811,10 +548,7 @@ mod test { assert!(!fields[0].is_nullable()); assert_eq!(fields[1].name(), "value"); assert!(fields[1].is_nullable()); - assert_eq!( - DType::try_from_arrow((&arrow, Nullability::Nullable))?, - dtype - ); + assert_eq!(from_arrow_data_type(&arrow, Nullability::Nullable)?, dtype); let positional = DataType::Map( Arc::new(Field::new_struct( @@ -828,7 +562,7 @@ mod test { false, ); assert_eq!( - DType::try_from_arrow((&positional, Nullability::NonNullable))?, + from_arrow_data_type(&positional, Nullability::NonNullable)?, DType::map( DType::Primitive(PType::I32, Nullability::NonNullable), DType::Utf8(Nullability::Nullable), @@ -869,7 +603,7 @@ mod test { for entries in invalid_entries { let data_type = DataType::Map(Arc::new(entries), false); - assert!(DType::try_from_arrow((&data_type, Nullability::NonNullable)).is_err()); + assert!(from_arrow_data_type(&data_type, Nullability::NonNullable).is_err()); } } @@ -880,21 +614,21 @@ mod test { #[case::interval(DataType::Interval(arrow_schema::IntervalUnit::DayTime))] #[case::fixed_size_binary(DataType::FixedSizeBinary(3))] fn test_try_from_arrow_unsupported_type_errors(#[case] data_type: DataType) { - let err = DType::try_from_arrow((&data_type, Nullability::NonNullable)) + let err = from_arrow_data_type(&data_type, Nullability::NonNullable) .expect_err("unsupported Arrow type should not convert") .to_string(); assert!(err.contains("not supported"), "unexpected error: {err}"); // The same unsupported type nested in a field or schema must also error cleanly. let field = Field::new("c0", data_type, true); - assert!(DType::try_from_arrow(&field).is_err()); + assert!(from_arrow_field_naive(&field).is_err()); let schema = Schema::new(vec![field]); - assert!(DType::try_from_arrow(&schema).is_err()); + assert!(ArrowSession::default().from_arrow_schema(&schema).is_err()); } #[test] fn test_try_from_arrow_supported_type_succeeds() -> VortexResult<()> { - let dtype = DType::try_from_arrow((&DataType::Int32, Nullability::Nullable))?; + let dtype = from_arrow_data_type(&DataType::Int32, Nullability::Nullable)?; assert_eq!(dtype, DType::Primitive(PType::I32, Nullability::Nullable)); Ok(()) } diff --git a/vortex-arrow/src/lib.rs b/vortex-arrow/src/lib.rs index 25f47180e4c..b0388ff176f 100644 --- a/vortex-arrow/src/lib.rs +++ b/vortex-arrow/src/lib.rs @@ -32,12 +32,6 @@ mod session; pub use convert::IntoVortexArray; pub use convert::nulls; pub use datum::*; -#[allow(deprecated)] -pub use dtype::FromArrowType; -#[allow(deprecated)] -pub use dtype::ToArrowType; -#[allow(deprecated)] -pub use dtype::TryFromArrowType; pub use executor::*; pub use iter::*; pub use null_buffer::to_arrow_null_buffer;