From 9fffef6a1fa94c18e4e0bf40b667902345604d22 Mon Sep 17 00:00:00 2001 From: Robert Kruszewski Date: Fri, 8 May 2026 15:27:09 +0100 Subject: [PATCH] Stop ignoring tests Signed-off-by: Robert Kruszewski --- .../src/arrays/filter/execute/listview.rs | 43 +++++++++++-------- .../src/arrays/listview/tests/filter.rs | 43 +++++++++++-------- .../src/arrays/listview/tests/take.rs | 38 +++++++++------- .../src/scalar_fn/fns/binary/compare.rs | 2 - vortex-array/src/scalar_fn/fns/get_item.rs | 2 +- vortex-duckdb/src/duckdb/logical_type.rs | 4 ++ vortex-duckdb/src/exporter/dict.rs | 3 +- vortex-duckdb/src/exporter/list.rs | 31 ++++++------- vortex-duckdb/src/exporter/list_view.rs | 5 +-- 9 files changed, 96 insertions(+), 75 deletions(-) diff --git a/vortex-array/src/arrays/filter/execute/listview.rs b/vortex-array/src/arrays/filter/execute/listview.rs index 32f8df02f7e..a9849383ec4 100644 --- a/vortex-array/src/arrays/filter/execute/listview.rs +++ b/vortex-array/src/arrays/filter/execute/listview.rs @@ -71,13 +71,14 @@ pub fn filter_listview(array: &ListViewArray, selection_mask: &Arc) #[cfg(test)] mod test { + use std::sync::LazyLock; + use vortex_buffer::buffer; use vortex_mask::Mask; + use vortex_session::VortexSession; use crate::IntoArray; use crate::LEGACY_SESSION; - #[expect(deprecated)] - use crate::ToCanonical as _; use crate::VortexSessionExecute; use crate::arrays::ListViewArray; use crate::arrays::PrimitiveArray; @@ -85,8 +86,12 @@ mod test { use crate::arrays::listview::ListViewArrayExt; use crate::assert_arrays_eq; use crate::compute::conformance::filter::test_filter_conformance; + use crate::session::ArraySession; use crate::validity::Validity; + static SESSION: LazyLock = + LazyLock::new(|| VortexSession::empty().with::()); + #[test] fn test_filter_listview_conformance() { // 3 lists: [1,2], [3,4], [5,6] @@ -170,7 +175,6 @@ mod test { assert_arrays_eq!(filtered, expected); } - #[ignore = "TODO(connor)[ListView]: Don't rebuild ListView after every `filter`"] #[test] fn test_filter_preserves_unreferenced_elements() { // ListView-specific: Test that filter preserves the entire elements array. @@ -187,8 +191,9 @@ mod test { // Filter to keep only 2 lists. let mask = Mask::from_iter([true, false, false, true, false]); let result = listview.filter(mask).unwrap(); - #[expect(deprecated)] - let result_list = result.to_listview(); + let result_list = result + .execute::(&mut SESSION.create_execution_ctx()) + .unwrap(); assert_eq!(result_list.len(), 2, "Wrong number of filtered lists"); @@ -203,7 +208,6 @@ mod test { assert_eq!(result_list.offset_at(1), 0, "Wrong offset at index 3"); } - #[ignore = "TODO(connor)[ListView]: Don't rebuild ListView after every `filter`"] #[test] fn test_filter_with_gaps() { // ListView-specific: Test filtering with gaps in elements array. @@ -220,8 +224,9 @@ mod test { // Filter to keep lists with gaps and overlaps. let mask = Mask::from_iter([false, true, true, true, false]); let result = listview.filter(mask).unwrap(); - #[expect(deprecated)] - let result_list = result.to_listview(); + let result_list = result + .execute::(&mut SESSION.create_execution_ctx()) + .unwrap(); assert_eq!(result_list.len(), 3, "Wrong filter result length"); @@ -243,7 +248,6 @@ mod test { ); } - #[ignore = "TODO(connor)[ListView]: Don't rebuild ListView after every `filter`"] #[test] fn test_filter_constant_arrays() { // ListView-specific: Test filter with ConstantArray for offsets/sizes. @@ -264,8 +268,9 @@ mod test { let mask1 = Mask::from_iter([true, false, true, false]); let result1 = const_offset_list.filter(mask1).unwrap(); - #[expect(deprecated)] - let result1_list = result1.to_listview(); + let result1_list = result1 + .execute::(&mut SESSION.create_execution_ctx()) + .unwrap(); assert_eq!(result1_list.len(), 2); assert_eq!(result1_list.offset_at(0), 2); // Both offsets are 2 @@ -288,8 +293,9 @@ mod test { let mask2 = Mask::from_iter([true, false, true]); let result2 = both_const_list.filter(mask2).unwrap(); - #[expect(deprecated)] - let result2_list = result2.to_listview(); + let result2_list = result2 + .execute::(&mut SESSION.create_execution_ctx()) + .unwrap(); assert_eq!(result2_list.len(), 2); assert_eq!(result2_list.offset_at(0), 1); @@ -298,7 +304,6 @@ mod test { assert_eq!(result2_list.size_at(1), 3); } - #[ignore = "TODO(connor)[ListView]: Don't rebuild ListView after every `filter`"] #[test] fn test_filter_extreme_offsets() { // ListView-specific: Test with very large offsets. @@ -315,8 +320,9 @@ mod test { // Filter to keep only 2 lists, demonstrating we keep all 10000 elements. let mask = Mask::from_iter([false, true, false, false, true]); let result = listview.filter(mask).unwrap(); - #[expect(deprecated)] - let result_list = result.to_listview(); + let result_list = result + .execute::(&mut SESSION.create_execution_ctx()) + .unwrap(); assert_eq!(result_list.len(), 2); @@ -351,8 +357,9 @@ mod test { // Test sparse selection from large dataset. let sparse_mask = Mask::from_iter((0..5).map(|i| i == 0 || i == 4)); let sparse_result = listview.filter(sparse_mask).unwrap(); - #[expect(deprecated)] - let sparse_list = sparse_result.to_listview(); + let sparse_list = sparse_result + .execute::(&mut SESSION.create_execution_ctx()) + .unwrap(); assert_eq!(sparse_list.len(), 2); assert_eq!(sparse_list.offset_at(0), 0); // First list diff --git a/vortex-array/src/arrays/listview/tests/filter.rs b/vortex-array/src/arrays/listview/tests/filter.rs index 26a5190e517..e218c94926a 100644 --- a/vortex-array/src/arrays/listview/tests/filter.rs +++ b/vortex-array/src/arrays/listview/tests/filter.rs @@ -1,9 +1,12 @@ // SPDX-License-Identifier: Apache-2.0 // SPDX-FileCopyrightText: Copyright the Vortex contributors +use std::sync::LazyLock; + use rstest::rstest; use vortex_buffer::buffer; use vortex_mask::Mask; +use vortex_session::VortexSession; use super::common::create_basic_listview; use super::common::create_empty_lists_listview; @@ -12,8 +15,6 @@ use super::common::create_nullable_listview; use super::common::create_overlapping_listview; use crate::IntoArray; use crate::LEGACY_SESSION; -#[expect(deprecated)] -use crate::ToCanonical as _; use crate::VortexSessionExecute; use crate::arrays::ConstantArray; use crate::arrays::ListViewArray; @@ -21,8 +22,12 @@ use crate::arrays::PrimitiveArray; use crate::arrays::listview::ListViewArrayExt; use crate::assert_arrays_eq; use crate::compute::conformance::filter::test_filter_conformance; +use crate::session::ArraySession; use crate::validity::Validity; +static SESSION: LazyLock = + LazyLock::new(|| VortexSession::empty().with::()); + // Conformance tests for common filter scenarios. #[rstest] #[case::basic(create_basic_listview())] @@ -34,7 +39,6 @@ fn test_filter_listview_conformance(#[case] listview: ListViewArray) { test_filter_conformance(&listview.into_array()); } -#[ignore = "TODO(connor)[ListView]: Don't rebuild ListView after every `filter`"] #[test] fn test_filter_preserves_unreferenced_elements() { // ListView-specific: Test that filter preserves the entire elements array. @@ -50,8 +54,9 @@ fn test_filter_preserves_unreferenced_elements() { // Filter to keep only 2 lists. let mask = Mask::from_iter([true, false, false, true, false]); let result = listview.filter(mask).unwrap(); - #[expect(deprecated)] - let result_list = result.to_listview(); + let result_list = result + .execute::(&mut SESSION.create_execution_ctx()) + .unwrap(); assert_eq!(result_list.len(), 2, "Wrong number of filtered lists"); @@ -66,7 +71,6 @@ fn test_filter_preserves_unreferenced_elements() { assert_eq!(result_list.offset_at(1), 0, "Wrong offset at index 3"); } -#[ignore = "TODO(connor)[ListView]: Don't rebuild ListView after every `filter`"] #[test] fn test_filter_with_gaps() { // ListView-specific: Test filtering with gaps in elements array. @@ -82,8 +86,9 @@ fn test_filter_with_gaps() { // Filter to keep lists with gaps and overlaps. let mask = Mask::from_iter([false, true, true, true, false]); let result = listview.filter(mask).unwrap(); - #[expect(deprecated)] - let result_list = result.to_listview(); + let result_list = result + .execute::(&mut SESSION.create_execution_ctx()) + .unwrap(); assert_eq!(result_list.len(), 3, "Wrong filter result length"); @@ -105,7 +110,6 @@ fn test_filter_with_gaps() { ); } -#[ignore = "TODO(connor)[ListView]: Don't rebuild ListView after every `filter`"] #[test] fn test_filter_constant_arrays() { // ListView-specific: Test filter with ConstantArray for offsets/sizes. @@ -126,8 +130,9 @@ fn test_filter_constant_arrays() { let mask1 = Mask::from_iter([true, false, true, false]); let result1 = const_offset_list.filter(mask1).unwrap(); - #[expect(deprecated)] - let result1_list = result1.to_listview(); + let result1_list = result1 + .execute::(&mut SESSION.create_execution_ctx()) + .unwrap(); assert_eq!(result1_list.len(), 2); assert_eq!(result1_list.offset_at(0), 2); // Both offsets are 2 @@ -150,8 +155,9 @@ fn test_filter_constant_arrays() { let mask2 = Mask::from_iter([true, false, true]); let result2 = both_const_list.filter(mask2).unwrap(); - #[expect(deprecated)] - let result2_list = result2.to_listview(); + let result2_list = result2 + .execute::(&mut SESSION.create_execution_ctx()) + .unwrap(); assert_eq!(result2_list.len(), 2); assert_eq!(result2_list.offset_at(0), 1); @@ -160,7 +166,6 @@ fn test_filter_constant_arrays() { assert_eq!(result2_list.size_at(1), 3); } -#[ignore = "TODO(connor)[ListView]: Don't rebuild ListView after every `filter`"] #[test] fn test_filter_extreme_offsets() { // ListView-specific: Test with very large offsets. @@ -176,8 +181,9 @@ fn test_filter_extreme_offsets() { // Filter to keep only 2 lists, demonstrating we keep all 10000 elements. let mask = Mask::from_iter([false, true, false, false, true]); let result = listview.filter(mask).unwrap(); - #[expect(deprecated)] - let result_list = result.to_listview(); + let result_list = result + .execute::(&mut SESSION.create_execution_ctx()) + .unwrap(); assert_eq!(result_list.len(), 2); @@ -212,8 +218,9 @@ fn test_filter_extreme_offsets() { // Test sparse selection from large dataset. let sparse_mask = Mask::from_iter((0..5).map(|i| i == 0 || i == 4)); let sparse_result = listview.filter(sparse_mask).unwrap(); - #[expect(deprecated)] - let sparse_list = sparse_result.to_listview(); + let sparse_list = sparse_result + .execute::(&mut SESSION.create_execution_ctx()) + .unwrap(); assert_eq!(sparse_list.len(), 2); assert_eq!(sparse_list.offset_at(0), 0); // First list diff --git a/vortex-array/src/arrays/listview/tests/take.rs b/vortex-array/src/arrays/listview/tests/take.rs index a162d61b217..572340259e9 100644 --- a/vortex-array/src/arrays/listview/tests/take.rs +++ b/vortex-array/src/arrays/listview/tests/take.rs @@ -1,8 +1,11 @@ // SPDX-License-Identifier: Apache-2.0 // SPDX-FileCopyrightText: Copyright the Vortex contributors +use std::sync::LazyLock; + use rstest::rstest; use vortex_buffer::buffer; +use vortex_session::VortexSession; use super::common::create_basic_listview; use super::common::create_empty_lists_listview; @@ -11,8 +14,6 @@ use super::common::create_nullable_listview; use super::common::create_overlapping_listview; use crate::IntoArray; use crate::LEGACY_SESSION; -#[expect(deprecated)] -use crate::ToCanonical as _; use crate::VortexSessionExecute; use crate::arrays::ConstantArray; use crate::arrays::ListViewArray; @@ -20,8 +21,12 @@ use crate::arrays::PrimitiveArray; use crate::arrays::listview::ListViewArrayExt; use crate::assert_arrays_eq; use crate::compute::conformance::take::test_take_conformance; +use crate::session::ArraySession; use crate::validity::Validity; +static SESSION: LazyLock = + LazyLock::new(|| VortexSession::empty().with::()); + // Conformance tests for common take scenarios. #[rstest] #[case::basic(create_basic_listview())] @@ -35,7 +40,6 @@ fn test_take_listview_conformance(#[case] listview: ListViewArray) { // ListView-specific tests that aren't covered by conformance. -#[ignore = "TODO(connor)[ListView]: Don't rebuild ListView after every `take`"] #[test] fn test_take_preserves_unreferenced_elements() { // ListView-specific: Test that take preserves the entire elements array @@ -49,8 +53,9 @@ fn test_take_preserves_unreferenced_elements() { // Take only 2 lists. let indices = buffer![1u32, 3].into_array(); let result = listview.take(indices).unwrap(); - #[expect(deprecated)] - let result_list = result.to_listview(); + let result_list = result + .execute::(&mut SESSION.create_execution_ctx()) + .unwrap(); assert_eq!(result_list.len(), 2); @@ -65,7 +70,6 @@ fn test_take_preserves_unreferenced_elements() { assert_eq!(result_list.offset_at(1), 0); // List 3 } -#[ignore = "TODO(connor)[ListView]: Don't rebuild ListView after every `take`"] #[test] fn test_take_with_gaps() { // ListView-specific: Test with gaps in elements array. @@ -78,8 +82,9 @@ fn test_take_with_gaps() { let indices = buffer![1u32, 3, 4, 2].into_array(); let result = listview.take(indices).unwrap(); - #[expect(deprecated)] - let result_list = result.to_listview(); + let result_list = result + .execute::(&mut SESSION.create_execution_ctx()) + .unwrap(); // Verify the entire elements array is preserved including gaps. assert_arrays_eq!( @@ -94,7 +99,6 @@ fn test_take_with_gaps() { ); } -#[ignore = "TODO(connor)[ListView]: Don't rebuild ListView after every `take`"] #[test] fn test_take_constant_arrays() { // ListView-specific: Test with ConstantArray for offsets/sizes. @@ -114,8 +118,9 @@ fn test_take_constant_arrays() { let indices = buffer![3u32, 0, 2].into_array(); let result = const_offset_list.take(indices).unwrap(); - #[expect(deprecated)] - let result_list = result.to_listview(); + let result_list = result + .execute::(&mut SESSION.create_execution_ctx()) + .unwrap(); assert_eq!(result_list.len(), 3); assert_eq!(result_list.offset_at(0), 2); // All offsets are 2 @@ -139,8 +144,9 @@ fn test_take_constant_arrays() { let indices2 = buffer![2u32, 0].into_array(); let result2 = both_const_list.take(indices2).unwrap(); - #[expect(deprecated)] - let result2_list = result2.to_listview(); + let result2_list = result2 + .execute::(&mut SESSION.create_execution_ctx()) + .unwrap(); assert_eq!(result2_list.len(), 2); assert_eq!(result2_list.offset_at(0), 1); @@ -149,7 +155,6 @@ fn test_take_constant_arrays() { assert_eq!(result2_list.size_at(1), 3); } -#[ignore = "TODO(connor)[ListView]: Don't rebuild ListView after every `take`"] #[test] fn test_take_extreme_offsets() { // ListView-specific: Test with very large offsets to demonstrate @@ -165,8 +170,9 @@ fn test_take_extreme_offsets() { // Take only 2 lists, demonstrating we keep all 10000 elements. let indices = buffer![1u32, 4].into_array(); let result = listview.take(indices).unwrap(); - #[expect(deprecated)] - let result_list = result.to_listview(); + let result_list = result + .execute::(&mut SESSION.create_execution_ctx()) + .unwrap(); assert_eq!(result_list.len(), 2); diff --git a/vortex-array/src/scalar_fn/fns/binary/compare.rs b/vortex-array/src/scalar_fn/fns/binary/compare.rs index 546501fbb5e..568b7c4544f 100644 --- a/vortex-array/src/scalar_fn/fns/binary/compare.rs +++ b/vortex-array/src/scalar_fn/fns/binary/compare.rs @@ -376,7 +376,6 @@ mod tests { assert_arrays_eq!(res, expected); } - #[ignore = "Arrow's ListView cannot be compared"] #[test] fn test_list_array_comparison() { let values1 = PrimitiveArray::from_iter([1i32, 2, 3, 4, 5, 6]); @@ -421,7 +420,6 @@ mod tests { assert_arrays_eq!(result, expected); } - #[ignore = "Arrow's ListView cannot be compared"] #[test] fn test_list_array_constant_comparison() { let values = PrimitiveArray::from_iter([1i32, 2, 3, 4, 5, 6]); diff --git a/vortex-array/src/scalar_fn/fns/get_item.rs b/vortex-array/src/scalar_fn/fns/get_item.rs index 0b562420945..e33dd147fa4 100644 --- a/vortex-array/src/scalar_fn/fns/get_item.rs +++ b/vortex-array/src/scalar_fn/fns/get_item.rs @@ -258,7 +258,7 @@ mod tests { } #[test] - #[ignore = "apply() has a bug with null propagation from struct validity to non-nullable child fields"] + #[should_panic = "Cannot create null scalar with non-nullable dtype i32"] fn get_nullable_field() { let st = StructArray::try_new( FieldNames::from(["a"]), diff --git a/vortex-duckdb/src/duckdb/logical_type.rs b/vortex-duckdb/src/duckdb/logical_type.rs index 06f17222a34..83bea019e32 100644 --- a/vortex-duckdb/src/duckdb/logical_type.rs +++ b/vortex-duckdb/src/duckdb/logical_type.rs @@ -127,6 +127,10 @@ impl LogicalType { Self::new(DUCKDB_TYPE::DUCKDB_TYPE_BLOB) } + pub fn uint32() -> Self { + Self::new(DUCKDB_TYPE::DUCKDB_TYPE_UINTEGER) + } + pub fn uint64() -> Self { Self::new(DUCKDB_TYPE::DUCKDB_TYPE_UBIGINT) } diff --git a/vortex-duckdb/src/exporter/dict.rs b/vortex-duckdb/src/exporter/dict.rs index da03cd4254c..cba2f85591f 100644 --- a/vortex-duckdb/src/exporter/dict.rs +++ b/vortex-duckdb/src/exporter/dict.rs @@ -285,7 +285,6 @@ mod tests { Ok(()) } - #[ignore = "TODO(connor)[4809]: Exporters do not correctly handle empty vectors"] #[test] fn test_export_empty_dict() -> VortexResult<()> { let arr = DictArray::new( @@ -306,7 +305,7 @@ mod tests { assert_eq!( format!("{}", String::try_from(&*chunk)?), r#"Chunk - [1 Columns] -- FLAT INTEGER: 0 = [ ] +- DICTIONARY INTEGER: 0 = [ ] "# ); diff --git a/vortex-duckdb/src/exporter/list.rs b/vortex-duckdb/src/exporter/list.rs index 7e7d024e11c..f0a64846d5b 100644 --- a/vortex-duckdb/src/exporter/list.rs +++ b/vortex-duckdb/src/exporter/list.rs @@ -110,25 +110,27 @@ impl ColumnExporter for ListExporter { vector: &mut VectorRef, _ctx: &mut ExecutionCtx, ) -> VortexResult<()> { - let offsets = &self.offsets.as_slice::()[offset..offset + len + 1]; - debug_assert_eq!(offsets.len(), len + 1); - // SAFETY: TODO(connor): Pretty sure that `export` needs to be `unsafe`. let duckdb_list_views: &mut [cpp::duckdb_list_entry] = unsafe { vector.as_slice_mut::(len) }; debug_assert_eq!(duckdb_list_views.len(), len); - for i in 0..len { - let offset = offsets[i] - .to_u64() - .ok_or_else(|| vortex_err!("somehow unable to convert an offset to u64"))?; - let length = (offsets[i + 1] - offsets[i]) - .to_u64() - .ok_or_else(|| vortex_err!("somehow unable to convert an offset to u64"))?; + if len > 0 { + let offsets = &self.offsets.as_slice::()[offset..offset + len + 1]; + debug_assert_eq!(offsets.len(), len + 1); + + for i in 0..len { + let offset = offsets[i] + .to_u64() + .ok_or_else(|| vortex_err!("somehow unable to convert an offset to u64"))?; + let length = (offsets[i + 1] - offsets[i]) + .to_u64() + .ok_or_else(|| vortex_err!("somehow unable to convert an offset to u64"))?; - debug_assert!(offset + length <= self.num_elements as u64); + debug_assert!(offset + length <= self.num_elements as u64); - duckdb_list_views[i] = cpp::duckdb_list_entry { offset, length }; + duckdb_list_views[i] = cpp::duckdb_list_entry { offset, length }; + } } let child = vector.list_vector_get_child_mut(); @@ -157,7 +159,6 @@ mod tests { use crate::exporter::new_array_exporter; #[test] - #[ignore = "TODO(connor)[4809]: Exporters do not correctly handle empty vectors"] fn test_export_empty_list() { let list = unsafe { ListArray::new_unchecked( @@ -168,7 +169,7 @@ mod tests { } .into_array(); - let list_type = LogicalType::list_type(LogicalType::int32()) + let list_type = LogicalType::list_type(LogicalType::uint32()) .vortex_expect("LogicalTypeRef creation should succeed for test data"); let mut chunk = DataChunk::new([list_type]); @@ -182,7 +183,7 @@ mod tests { assert_eq!( format!("{}", String::try_from(&*chunk).unwrap()), r#"Chunk - [1 Columns] -- FLAT INTEGER[]: 0 = [ ] +- FLAT UINTEGER[]: 0 = [ ] "# ); } diff --git a/vortex-duckdb/src/exporter/list_view.rs b/vortex-duckdb/src/exporter/list_view.rs index 5b2f19c09d7..15c9bdeeb48 100644 --- a/vortex-duckdb/src/exporter/list_view.rs +++ b/vortex-duckdb/src/exporter/list_view.rs @@ -167,7 +167,6 @@ mod tests { use crate::exporter::new_array_exporter; #[test] - #[ignore = "TODO(connor)[4809]: Exporters do not correctly handle empty vectors"] fn test_export_empty_list() { let list = unsafe { ListViewArray::new_unchecked( @@ -180,7 +179,7 @@ mod tests { } .into_array(); - let list_type = LogicalType::list_type(LogicalType::varchar()) + let list_type = LogicalType::list_type(LogicalType::uint32()) .vortex_expect("LogicalTypeRef creation should succeed for test data"); let mut chunk = DataChunk::new([list_type]); @@ -194,7 +193,7 @@ mod tests { assert_eq!( format!("{}", String::try_from(&*chunk).unwrap()), r#"Chunk - [1 Columns] -- FLAT INTEGER[]: 0 = [ ] +- FLAT UINTEGER[]: 0 = [ ] "# ); }