Skip to content

Commit

Permalink
Remove uses of with_dyn for validity (#1487)
Browse files Browse the repository at this point in the history
  • Loading branch information
gatesn authored Nov 27, 2024
1 parent dbeb724 commit d943392
Show file tree
Hide file tree
Showing 15 changed files with 76 additions and 130 deletions.
3 changes: 2 additions & 1 deletion encodings/alp/src/alp/compute/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use vortex_array::compute::{
filter, scalar_at, slice, take, CompareFn, ComputeVTable, FilterFn, FilterMask, ScalarAtFn,
SliceFn, TakeFn, TakeOptions,
};
use vortex_array::validity::ArrayValidity;
use vortex_array::variants::PrimitiveArrayTrait;
use vortex_array::{ArrayDType, ArrayData, IntoArrayData};
use vortex_error::VortexResult;
Expand Down Expand Up @@ -36,7 +37,7 @@ impl ComputeVTable for ALPEncoding {
impl ScalarAtFn<ALPArray> for ALPEncoding {
fn scalar_at(&self, array: &ALPArray, index: usize) -> VortexResult<Scalar> {
if let Some(patches) = array.patches() {
if patches.with_dyn(|a| a.is_valid(index)) {
if patches.is_valid(index) {
// We need to make sure the value is actually in the patches array
return scalar_at(&patches, index);
}
Expand Down
3 changes: 2 additions & 1 deletion encodings/alp/src/alp_rd/compute/scalar_at.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use vortex_array::compute::{scalar_at, ScalarAtFn};
use vortex_array::validity::ArrayValidity;
use vortex_error::VortexResult;
use vortex_scalar::Scalar;

Expand All @@ -10,7 +11,7 @@ impl ScalarAtFn<ALPRDArray> for ALPRDEncoding {
// The left value can either be a direct value, or an exception.
// The exceptions array represents exception positions with non-null values.
let left: u16 = match array.left_parts_exceptions() {
Some(exceptions) if exceptions.with_dyn(|a| a.is_valid(index)) => {
Some(exceptions) if exceptions.is_valid(index) => {
scalar_at(&exceptions, index)?.try_into()?
}
_ => {
Expand Down
6 changes: 2 additions & 4 deletions encodings/datetime-parts/src/array.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use vortex_array::array::StructArray;
use vortex_array::compute::try_cast;
use vortex_array::encoding::ids;
use vortex_array::stats::{Stat, StatisticsVTable, StatsSet};
use vortex_array::validity::{LogicalValidity, Validity, ValidityVTable};
use vortex_array::validity::{ArrayValidity, LogicalValidity, Validity, ValidityVTable};
use vortex_array::variants::{ArrayVariants, ExtensionArrayTrait};
use vortex_array::visitor::{ArrayVisitor, VisitorVTable};
use vortex_array::{
Expand Down Expand Up @@ -105,9 +105,7 @@ impl DateTimePartsArray {

pub fn validity(&self) -> Validity {
if self.dtype().is_nullable() {
self.days()
.with_dyn(|a| a.logical_validity())
.into_validity()
self.days().logical_validity().into_validity()
} else {
Validity::NonNullable
}
Expand Down
4 changes: 2 additions & 2 deletions encodings/dict/src/array.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use vortex_array::array::BoolArray;
use vortex_array::compute::{scalar_at, take, TakeOptions};
use vortex_array::encoding::ids;
use vortex_array::stats::StatsSet;
use vortex_array::validity::{LogicalValidity, ValidityVTable};
use vortex_array::validity::{ArrayValidity, LogicalValidity, ValidityVTable};
use vortex_array::variants::PrimitiveArrayTrait;
use vortex_array::visitor::{ArrayVisitor, VisitorVTable};
use vortex_array::{
Expand Down Expand Up @@ -91,7 +91,7 @@ impl ValidityVTable<DictArray> for DictEncoding {
.as_ref()
.try_into()
.vortex_expect("Failed to convert dictionary code to usize");
array.values().with_dyn(|a| a.is_valid(values_index))
array.values().is_valid(values_index)
}

fn logical_validity(&self, array: &DictArray) -> LogicalValidity {
Expand Down
3 changes: 2 additions & 1 deletion encodings/fastlanes/src/bitpacking/compute/scalar_at.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use vortex_array::compute::{scalar_at, ScalarAtFn};
use vortex_array::validity::ArrayValidity;
use vortex_array::ArrayDType;
use vortex_error::VortexResult;
use vortex_scalar::Scalar;
Expand All @@ -9,7 +10,7 @@ impl ScalarAtFn<BitPackedArray> for BitPackedEncoding {
fn scalar_at(&self, array: &BitPackedArray, index: usize) -> VortexResult<Scalar> {
if let Some(patches) = array.patches() {
// NB: All non-null values are considered patches
if patches.with_dyn(|a| a.is_valid(index)) {
if patches.is_valid(index) {
return scalar_at(&patches, index)?.cast(array.dtype());
}
}
Expand Down
6 changes: 3 additions & 3 deletions vortex-array/src/array/chunked/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ use crate::iter::{ArrayIterator, ArrayIteratorAdapter};
use crate::stats::ArrayStatistics;
use crate::stream::{ArrayStream, ArrayStreamAdapter};
use crate::validity::Validity::NonNullable;
use crate::validity::{LogicalValidity, Validity, ValidityVTable};
use crate::validity::{ArrayValidity, LogicalValidity, Validity, ValidityVTable};
use crate::visitor::{ArrayVisitor, VisitorVTable};
use crate::{
impl_encoding, ArrayDType, ArrayData, ArrayLen, ArrayTrait, IntoArrayData, IntoCanonical,
Expand Down Expand Up @@ -230,13 +230,13 @@ impl ValidityVTable<ChunkedArray> for ChunkedEncoding {
.unwrap_or_else(|e| {
vortex_panic!(e, "ChunkedArray: is_valid failed to find chunk {}", index)
})
.with_dyn(|a| a.is_valid(offset_in_chunk))
.is_valid(offset_in_chunk)
}

fn logical_validity(&self, array: &ChunkedArray) -> LogicalValidity {
let validity = array
.chunks()
.map(|a| a.with_dyn(|arr| arr.logical_validity()))
.map(|a| a.logical_validity())
.collect::<Validity>();
validity.to_logical(array.len())
}
Expand Down
17 changes: 6 additions & 11 deletions vortex-array/src/array/sparse/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use crate::array::constant::ConstantArray;
use crate::compute::{scalar_at, search_sorted, SearchResult, SearchSortedSide};
use crate::encoding::ids;
use crate::stats::{ArrayStatistics, Stat, StatisticsVTable, StatsSet};
use crate::validity::{LogicalValidity, ValidityVTable};
use crate::validity::{ArrayValidity, LogicalValidity, ValidityVTable};
use crate::variants::PrimitiveArrayTrait;
use crate::visitor::{ArrayVisitor, VisitorVTable};
use crate::{
Expand Down Expand Up @@ -213,7 +213,7 @@ impl ValidityVTable<SparseArray> for SparseEncoding {
fn is_valid(&self, array: &SparseArray, index: usize) -> bool {
match array.search_index(index).map(SearchResult::to_found) {
Ok(None) => !array.fill_scalar().is_null(),
Ok(Some(idx)) => array.values().with_dyn(|a| a.is_valid(idx)),
Ok(Some(idx)) => array.values().is_valid(idx),
Err(e) => vortex_panic!(e, "Error while finding index {} in sparse array", index),
}
}
Expand All @@ -234,9 +234,7 @@ impl ValidityVTable<SparseArray> for SparseEncoding {
// existing values.
SparseArray::try_new_with_offset(
array.indices(),
array
.values()
.with_dyn(|a| a.logical_validity().into_array()),
array.values().logical_validity().into_array(),
array.len(),
array.indices_offset(),
true.into(),
Expand All @@ -257,6 +255,7 @@ mod test {

use crate::array::sparse::SparseArray;
use crate::compute::{scalar_at, slice, try_cast};
use crate::validity::ArrayValidity;
use crate::{ArrayData, IntoArrayData, IntoArrayVariant};

fn nullable_fill() -> Scalar {
Expand Down Expand Up @@ -348,11 +347,7 @@ mod test {
#[test]
pub fn sparse_logical_validity() {
let array = sparse_array(nullable_fill());
let validity = array
.with_dyn(|a| a.logical_validity())
.into_array()
.into_bool()
.unwrap();
let validity = array.logical_validity().into_array().into_bool().unwrap();
assert_eq!(
validity.boolean_buffer().iter().collect_vec(),
[false, false, true, false, false, true, false, false, true, false]
Expand All @@ -365,7 +360,7 @@ mod test {

assert_eq!(
array
.with_dyn(|a| a.logical_validity())
.logical_validity()
.into_array()
.into_bool()
.unwrap()
Expand Down
6 changes: 4 additions & 2 deletions vortex-array/src/compress.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,10 @@ pub fn check_validity_unchanged(arr: &ArrayData, compressed: &ArrayData) {
let _ = compressed;
#[cfg(debug_assertions)]
{
let old_validity = arr.with_dyn(|a| a.logical_validity().len());
let new_validity = compressed.with_dyn(|a| a.logical_validity().len());
use crate::validity::ArrayValidity;

let old_validity = arr.logical_validity().len();
let new_validity = compressed.logical_validity().len();

debug_assert!(
old_validity == new_validity,
Expand Down
3 changes: 2 additions & 1 deletion vortex-array/src/compute/scalar_at.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ use vortex_error::{vortex_bail, vortex_err, VortexError, VortexResult};
use vortex_scalar::Scalar;

use crate::encoding::Encoding;
use crate::validity::ArrayValidity;
use crate::{ArrayDType, ArrayData};

/// Implementation of scalar_at for an encoding.
Expand Down Expand Up @@ -33,7 +34,7 @@ pub fn scalar_at(array: impl AsRef<ArrayData>, index: usize) -> VortexResult<Sca
vortex_bail!(OutOfBounds: index, 0, array.len());
}

if !array.with_dyn(|a| a.is_valid(index)) {
if !array.is_valid(index) {
return Ok(Scalar::null(array.dtype().clone()));
}

Expand Down
41 changes: 15 additions & 26 deletions vortex-array/src/data/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ use crate::encoding::{EncodingId, EncodingRef, EncodingVTable};
use crate::iter::{ArrayIterator, ArrayIteratorAdapter};
use crate::stats::{ArrayStatistics, Stat, Statistics, StatsSet};
use crate::stream::{ArrayStream, ArrayStreamAdapter};
use crate::validity::{ArrayValidity, LogicalValidity};
use crate::validity::{ArrayValidity, LogicalValidity, ValidityVTable};
use crate::{
ArrayChildrenIterator, ArrayDType, ArrayLen, ArrayMetadata, ArrayTrait, Context,
TryDeserializeArrayMetadata,
Expand Down Expand Up @@ -129,36 +129,23 @@ impl ArrayData {
/// Return the array's encoding
pub fn encoding(&self) -> EncodingRef {
match &self.0 {
InnerArrayData::Owned(d) => d.encoding(),
InnerArrayData::Viewed(v) => v.encoding(),
InnerArrayData::Owned(d) => d.encoding,
InnerArrayData::Viewed(v) => v.encoding,
}
}

/// Returns the number of logical elements in the array.
#[allow(clippy::same_name_method)]
pub fn len(&self) -> usize {
match &self.0 {
InnerArrayData::Owned(d) => d.len(),
InnerArrayData::Viewed(v) => v.len(),
InnerArrayData::Owned(d) => d.len,
InnerArrayData::Viewed(v) => v.len,
}
}

/// Check whether the array has any data
pub fn is_empty(&self) -> bool {
match &self.0 {
InnerArrayData::Owned(d) => d.is_empty(),
InnerArrayData::Viewed(v) => v.is_empty(),
}
}

/// Return whether the element at the given index is valid (true) or null (false).
fn is_valid(&self, index: usize) -> bool {
self.encoding().is_valid(self, index)
}

/// Return the logical validity of the array.
fn logical_validity(&self) -> LogicalValidity {
self.encoding().logical_validity(self)
self.len() == 0
}

/// Whether the array is of a canonical encoding.
Expand Down Expand Up @@ -207,7 +194,7 @@ impl ArrayData {
/// Returns a Vec of Arrays with all the array's child arrays.
pub fn children(&self) -> Vec<ArrayData> {
match &self.0 {
InnerArrayData::Owned(d) => d.children().iter().cloned().collect_vec(),
InnerArrayData::Owned(d) => d.children().to_vec(),
InnerArrayData::Viewed(v) => v.children(),
}
}
Expand Down Expand Up @@ -395,8 +382,8 @@ impl Display for ArrayData {
impl<T: AsRef<ArrayData>> ArrayDType for T {
fn dtype(&self) -> &DType {
match &self.as_ref().0 {
InnerArrayData::Owned(d) => d.dtype(),
InnerArrayData::Viewed(v) => v.dtype(),
InnerArrayData::Owned(d) => &d.dtype,
InnerArrayData::Viewed(v) => &v.dtype,
}
}
}
Expand All @@ -412,20 +399,22 @@ impl<T: AsRef<ArrayData>> ArrayLen for T {
}

impl<A: AsRef<ArrayData>> ArrayValidity for A {
/// Return whether the element at the given index is valid (true) or null (false).
fn is_valid(&self, index: usize) -> bool {
self.as_ref().is_valid(index)
ValidityVTable::<ArrayData>::is_valid(self.as_ref().encoding(), self.as_ref(), index)
}

/// Return the logical validity of the array.
fn logical_validity(&self) -> LogicalValidity {
self.as_ref().logical_validity()
ValidityVTable::<ArrayData>::logical_validity(self.as_ref().encoding(), self.as_ref())
}
}

impl<T: AsRef<ArrayData>> ArrayStatistics for T {
fn statistics(&self) -> &(dyn Statistics + '_) {
match &self.as_ref().0 {
InnerArrayData::Owned(d) => d.statistics(),
InnerArrayData::Viewed(v) => v.statistics(),
InnerArrayData::Owned(d) => d,
InnerArrayData::Viewed(v) => v,
}
}

Expand Down
26 changes: 3 additions & 23 deletions vortex-array/src/data/owned.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,22 +22,6 @@ pub(super) struct OwnedArrayData {
}

impl OwnedArrayData {
pub fn encoding(&self) -> EncodingRef {
self.encoding
}

pub fn dtype(&self) -> &DType {
&self.dtype
}

pub fn len(&self) -> usize {
self.len
}

pub fn is_empty(&self) -> bool {
self.len == 0
}

pub fn metadata(&self) -> &Arc<dyn ArrayMetadata> {
&self.metadata
}
Expand All @@ -63,7 +47,7 @@ impl OwnedArrayData {
child.dtype(),
dtype,
"child {index} requested with incorrect dtype for encoding {}",
self.encoding().id().as_ref(),
self.encoding.id().as_ref(),
);
assert_eq!(
child.len(),
Expand All @@ -80,13 +64,9 @@ impl OwnedArrayData {
self.children.len()
}

pub fn children(&self) -> &[ArrayData] {
pub fn children(&self) -> &Arc<[ArrayData]> {
&self.children
}

pub fn statistics(&self) -> &dyn Statistics {
self
}
}

impl Statistics for OwnedArrayData {
Expand Down Expand Up @@ -136,7 +116,7 @@ impl Statistics for OwnedArrayData {
}

let computed = self
.encoding()
.encoding
.compute_statistics(&ArrayData::from(self.clone()), stat)
.ok()?;

Expand Down
Loading

0 comments on commit d943392

Please sign in to comment.