Skip to content

Commit

Permalink
Remove AsContiguousFn (#346)
Browse files Browse the repository at this point in the history
Per the comments on #341 , this PR removes the AsContiguousFn trait and
as_contiguous function entirely, and instead pushes the relevant details
into the implementation of `ArrayFlatten::flatten` for the
`ChunkedArray` type.

A few questions I've bumped into while doing this refactor:

* Can DType::List only reference primitive types? I see ListScalar, but
it’s not clear to me how Vortex would encode e.g. `List<Struct>` or
`List<List>`
* **Answer**: Deferring for now as List DTypes not fully support yet
anyway
* How should this work for ExtensionArray?
* **Answer**: ExtensionArray can contain a ChunkedArray for its internal
storage Array
  • Loading branch information
a10y authored Jun 11, 2024
1 parent 3308c6c commit e8e5557
Show file tree
Hide file tree
Showing 34 changed files with 398 additions and 570 deletions.
2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -107,5 +107,5 @@ zigzag = "0.1.0"
warnings = "deny"

[workspace.lints.clippy]
all = "deny"
all = { level = "deny", priority = -1 }
or_fun_call = "deny"
4 changes: 2 additions & 2 deletions bench-vortex/benches/compress_benchmark.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use criterion::{black_box, criterion_group, criterion_main, Criterion};

fn vortex_compress_taxi(c: &mut Criterion) {
taxi_data_parquet();
let mut group = c.benchmark_group("end to end");
let mut group = c.benchmark_group("end to end - taxi");
group.sample_size(10);
group.bench_function("compress", |b| b.iter(|| black_box(compress_taxi_data())));
group.finish()
Expand All @@ -16,7 +16,7 @@ fn vortex_compress_taxi(c: &mut Criterion) {
fn vortex_compress_medicare1(c: &mut Criterion) {
let dataset = BenchmarkDatasets::PBI(Medicare1);
dataset.as_uncompressed();
let mut group = c.benchmark_group("end to end");
let mut group = c.benchmark_group("end to end - medicare");
group.sample_size(10);
group.bench_function("compress", |b| {
b.iter(|| black_box(dataset.compress_to_vortex()))
Expand Down
39 changes: 1 addition & 38 deletions vortex-alp/src/compute.rs
Original file line number Diff line number Diff line change
@@ -1,16 +1,13 @@
use vortex::compute::as_contiguous::AsContiguousFn;
use vortex::compute::scalar_at::{scalar_at, ScalarAtFn};
use vortex::compute::slice::{slice, SliceFn};
use vortex::compute::take::{take, TakeFn};
use vortex::compute::ArrayCompute;
use vortex::{impl_default_as_contiguous_fn, Array, ArrayDType, IntoArray};
use vortex::{Array, ArrayDType, IntoArray};
use vortex_error::VortexResult;
use vortex_scalar::Scalar;

use crate::{match_each_alp_float_ptype, ALPArray};

impl_default_as_contiguous_fn!(ALPArray);

impl ArrayCompute for ALPArray {
fn scalar_at(&self) -> Option<&dyn ScalarAtFn> {
Some(self)
Expand All @@ -23,10 +20,6 @@ impl ArrayCompute for ALPArray {
fn take(&self) -> Option<&dyn TakeFn> {
Some(self)
}

fn as_contiguous(&self) -> Option<&dyn AsContiguousFn> {
Some(self)
}
}

impl ScalarAtFn for ALPArray {
Expand Down Expand Up @@ -68,33 +61,3 @@ impl SliceFn for ALPArray {
.into_array())
}
}

#[cfg(test)]
mod test {
use vortex::array::primitive::PrimitiveArray;
use vortex::compute::as_contiguous::AsContiguousFn;
use vortex::compute::scalar_at::scalar_at;
use vortex::validity::Validity;
use vortex::IntoArray;

use crate::ALPArray;

#[test]
fn test_as_contiguous() {
let values = vec![1.0, 2.0, 3.0];
let primitives = PrimitiveArray::from_vec(values, Validity::NonNullable);
let encoded = ALPArray::encode(primitives.into_array()).unwrap();
let alp = ALPArray::try_from(&encoded).unwrap();

let flat = alp.as_contiguous(&[encoded]).unwrap();

let a: f64 = scalar_at(&flat, 0).unwrap().try_into().unwrap();
let b: f64 = scalar_at(&flat, 1).unwrap().try_into().unwrap();

let c: f64 = scalar_at(&flat, 2).unwrap().try_into().unwrap();

assert_eq!(a, 1.0);
assert_eq!(b, 2.0);
assert_eq!(c, 3.0);
}
}
27 changes: 0 additions & 27 deletions vortex-array/src/array/bool/compute/as_contiguous.rs

This file was deleted.

6 changes: 0 additions & 6 deletions vortex-array/src/array/bool/compute/mod.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
use crate::array::bool::BoolArray;
use crate::compute::as_arrow::AsArrowArray;
use crate::compute::as_contiguous::AsContiguousFn;
use crate::compute::compare::CompareFn;
use crate::compute::fill::FillForwardFn;
use crate::compute::scalar_at::ScalarAtFn;
Expand All @@ -9,7 +8,6 @@ use crate::compute::take::TakeFn;
use crate::compute::ArrayCompute;

mod as_arrow;
mod as_contiguous;
mod compare;
mod fill;
mod flatten;
Expand All @@ -22,10 +20,6 @@ impl ArrayCompute for BoolArray {
Some(self)
}

fn as_contiguous(&self) -> Option<&dyn AsContiguousFn> {
Some(self)
}

fn compare(&self) -> Option<&dyn CompareFn> {
Some(self)
}
Expand Down
23 changes: 2 additions & 21 deletions vortex-array/src/array/chunked/compute/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,23 +2,21 @@ use vortex_error::VortexResult;
use vortex_scalar::Scalar;

use crate::array::chunked::ChunkedArray;
use crate::compute::as_contiguous::{as_contiguous, AsContiguousFn};
use crate::compute::scalar_at::{scalar_at, ScalarAtFn};
use crate::compute::scalar_subtract::SubtractScalarFn;
use crate::compute::slice::SliceFn;
use crate::compute::take::TakeFn;
use crate::compute::ArrayCompute;
use crate::Array;

mod slice;
mod take;

impl ArrayCompute for ChunkedArray {
fn as_contiguous(&self) -> Option<&dyn AsContiguousFn> {
fn scalar_at(&self) -> Option<&dyn ScalarAtFn> {
Some(self)
}

fn scalar_at(&self) -> Option<&dyn ScalarAtFn> {
fn subtract_scalar(&self) -> Option<&dyn SubtractScalarFn> {
Some(self)
}

Expand All @@ -29,23 +27,6 @@ impl ArrayCompute for ChunkedArray {
fn take(&self) -> Option<&dyn TakeFn> {
Some(self)
}

fn subtract_scalar(&self) -> Option<&dyn SubtractScalarFn> {
Some(self)
}
}

impl AsContiguousFn for ChunkedArray {
fn as_contiguous(&self, arrays: &[Array]) -> VortexResult<Array> {
// Combine all the chunks into one, then call as_contiguous again.
let mut chunks = Vec::with_capacity(self.nchunks());
for array in arrays {
for chunk in Self::try_from(array).unwrap().chunks() {
chunks.push(chunk);
}
}
as_contiguous(&chunks)
}
}

impl ScalarAtFn for ChunkedArray {
Expand Down
16 changes: 5 additions & 11 deletions vortex-array/src/array/chunked/compute/take.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,10 +52,7 @@ impl TakeFn for ChunkedArray {

#[cfg(test)]
mod test {
use itertools::Itertools;

use crate::array::chunked::ChunkedArray;
use crate::compute::as_contiguous::as_contiguous;
use crate::compute::take::take;
use crate::{ArrayDType, ArrayTrait, AsArray, IntoArray};

Expand All @@ -68,14 +65,11 @@ mod test {
assert_eq!(arr.len(), 9);
let indices = vec![0, 0, 6, 4].into_array();

let result = as_contiguous(
&ChunkedArray::try_from(take(arr.as_array_ref(), &indices).unwrap())
.unwrap()
.chunks()
.collect_vec(),
)
.unwrap()
.into_primitive();
let result = &ChunkedArray::try_from(take(arr.as_array_ref(), &indices).unwrap())
.unwrap()
.into_array()
.flatten_primitive()
.unwrap();
assert_eq!(result.typed_data::<i32>(), &[1, 1, 1, 2]);
}
}
Loading

0 comments on commit e8e5557

Please sign in to comment.