Skip to content

Commit 5411fde

Browse files
connortsui20connortsui20
committed
final touches
Signed-off-by: Connor Tsui <[email protected]>
1 parent b04fd04 commit 5411fde

11 files changed

Lines changed: 53 additions & 39 deletions

File tree

‎Cargo.lock‎

Lines changed: 0 additions & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎encodings/alp/src/alp/compute/compare.rs‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ use vortex_array::register_kernel;
1515
use vortex_dtype::NativePType;
1616
use vortex_error::VortexResult;
1717
use vortex_error::vortex_bail;
18+
use vortex_error::vortex_err;
1819
use vortex_scalar::Scalar;
1920

2021
use crate::ALPArray;
@@ -41,7 +42,13 @@ impl CompareKernel for ALPVTable {
4142
}
4243

4344
if let Some(const_scalar) = rhs.as_constant() {
44-
let pscalar = const_scalar.as_primitive();
45+
let pscalar = const_scalar.as_primitive_opt().ok_or_else(|| {
46+
vortex_err!(
47+
"ALP Compare RHS had the wrong type {}, expected {}",
48+
const_scalar,
49+
const_scalar.dtype()
50+
)
51+
})?;
4552

4653
match_each_alp_float_ptype!(pscalar.ptype(), |T| {
4754
match pscalar.typed_value::<T>() {

‎encodings/alp/src/alp_rd/compute/take.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ impl TakeExecute for ALPRDVTable {
3535
.transpose()?;
3636
let right_parts = fill_null(
3737
&array.right_parts().take(indices.to_array())?,
38-
&Scalar::zero_value(&array.right_parts().dtype().clone()),
38+
&Scalar::zero_value(array.right_parts().dtype()),
3939
)?;
4040

4141
Ok(Some(

‎encodings/datetime-parts/src/compute/rules.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -171,7 +171,7 @@ fn try_extract_days_constant(array: &ArrayRef) -> Option<i64> {
171171
fn is_constant_zero(array: &ArrayRef) -> bool {
172172
array
173173
.as_opt::<ConstantVTable>()
174-
.is_some_and(|c| c.scalar().value().is_some_and(|value| value.is_zero()))
174+
.is_some_and(|c| c.scalar().is_zero() == Some(true))
175175
}
176176

177177
#[cfg(test)]

‎vortex-datafusion/src/persistent/format.rs‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -392,6 +392,8 @@ impl FileFormat for VortexFormat {
392392
.zip(column_size)
393393
.map(|(acc, size)| acc + size);
394394

395+
// TODO(connor): There's a lot that can go wrong here, should probably handle this
396+
// more gracefully...
395397
// Find the min statistic.
396398
let min = stats_set.get(Stat::Min).and_then(|pstat_val| {
397399
pstat_val

‎vortex-error/Cargo.toml‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@ flatbuffers = ["dep:flatbuffers"]
2121
serde = ["dep:serde_json"]
2222

2323
[dependencies]
24-
arbitrary = { workspace = true }
2524
arrow-schema = { workspace = true }
2625
flatbuffers = { workspace = true, optional = true }
2726
jiff = { workspace = true }

‎vortex-file/src/footer/mod.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ mod serializer;
2020
pub use serializer::*;
2121
mod deserializer;
2222
pub use deserializer::*;
23-
pub(crate) use file_statistics::*;
23+
pub use file_statistics::FileStatistics;
2424
use flatbuffers::root;
2525
use itertools::Itertools;
2626
pub use segment::*;

‎vortex-scalar/src/convert/primitive.rs‎

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -154,13 +154,11 @@ impl TryFrom<&ScalarValue> for usize {
154154
type Error = VortexError;
155155

156156
fn try_from(value: &ScalarValue) -> VortexResult<Self> {
157-
// TODO(connor): This could probably be written in a better way...
158-
usize::try_from(
159-
value.as_primitive().cast_opt::<u64>().ok_or_else(|| {
160-
vortex_err!("Unable to convert the `ScalarValue` to the target type")
161-
})?,
162-
)
163-
.map_err(|err| vortex_err!("Cannot convert into usize: {err}"))
157+
let val = value
158+
.as_primitive()
159+
.cast_opt::<u64>()
160+
.ok_or_else(|| vortex_err!("Unable to convert the `ScalarValue` to the target type"))?;
161+
Ok(usize::try_from(val)?)
164162
}
165163
}
166164

‎vortex-scalar/src/scalar.rs‎

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ use vortex_dtype::PType;
1313
use vortex_error::VortexResult;
1414
use vortex_error::vortex_ensure;
1515
use vortex_error::vortex_ensure_eq;
16+
use vortex_error::vortex_panic;
1617

1718
use crate::PValue;
1819
use crate::ScalarValue;
@@ -127,10 +128,6 @@ impl Scalar {
127128
Self { dtype, value }
128129
}
129130

130-
// TODO(connor): Add an `is_zero` method to `Scalar`. Note that it is **not** as simple as
131-
// calling `is_zero` on the `ScalarValue` since each variant can represent multiple things
132-
// (notably `Struct` and `FixedSizeList` are represented as non-empty `List`s).
133-
134131
/// Returns a default value for the given [`DType`].
135132
///
136133
/// For nullable types, this returns a null scalar. For non-nullable and non-nested types, this
@@ -275,7 +272,22 @@ impl Scalar {
275272
/// Returns `None` if the scalar is null, otherwise returns `Some(true)` if the value is zero
276273
/// and `Some(false)` otherwise.
277274
pub fn is_zero(&self) -> Option<bool> {
278-
self.value.as_ref().map(|v| v.is_zero())
275+
let value = self.value()?;
276+
277+
let is_zero = match self.dtype() {
278+
DType::Null => vortex_panic!("non-null value somehow had `DType::Null`"),
279+
DType::Bool(_) => !value.as_bool(),
280+
DType::Primitive(..) => value.as_primitive().is_zero(),
281+
DType::Decimal(..) => value.as_decimal().is_zero(),
282+
DType::Utf8(_) => value.as_utf8().is_empty(),
283+
DType::Binary(_) => value.as_binary().is_empty(),
284+
DType::List(..) => value.as_list().is_empty(),
285+
DType::FixedSizeList(_, list_size, _) => value.as_list().len() == *list_size as usize,
286+
DType::Struct(struct_fields, _) => value.as_list().len() == struct_fields.nfields(),
287+
DType::Extension(_) => self.as_extension().storage().is_zero()?,
288+
};
289+
290+
Some(is_zero)
279291
}
280292

281293
/// Reinterprets the bytes of this scalar as a different primitive type.

‎vortex-scalar/src/scalar_value.rs‎

Lines changed: 18 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -43,24 +43,24 @@ impl ScalarValue {
4343
// TODO(connor): There is an inconsistency here w.r.t. `FixedSizeList` and `Struct` types, since
4444
// we say that the zero value for those are **not** empty lists. But here we say that a list is
4545
// a "zero" value if it is empty. So depending on the dtype this might just be incorrect!
46-
/// Returns true if the scalar represents the zero / identity value for its [`DType`].
47-
///
48-
/// Returns false if the scalar is null.
49-
///
50-
/// See [`Scalar::zero_value()`] for more details about "zero" values.
51-
///
52-
/// [`Scalar::zero_value()`]: crate::Scalar::zero_value
53-
pub fn is_zero(&self) -> bool {
54-
// TODO(connor): Is it better to just do == Self::zero_value()?
55-
match self {
56-
ScalarValue::Bool(b) => !*b,
57-
ScalarValue::Primitive(p) => p.is_zero(),
58-
ScalarValue::Decimal(d) => d.is_zero(),
59-
ScalarValue::Utf8(s) => s.is_empty(),
60-
ScalarValue::Binary(b) => b.is_empty(),
61-
ScalarValue::List(elems) => elems.is_empty(),
62-
}
63-
}
46+
// /// Returns true if the scalar represents the zero / identity value for its [`DType`].
47+
// ///
48+
// /// Returns false if the scalar is null.
49+
// ///
50+
// /// See [`Scalar::zero_value()`] for more details about "zero" values.
51+
// ///
52+
// /// [`Scalar::zero_value()`]: crate::Scalar::zero_value
53+
// pub fn is_zero(&self) -> bool {
54+
// // TODO(connor): Is it better to just do == Self::zero_value()?
55+
// match self {
56+
// ScalarValue::Bool(b) => !*b,
57+
// ScalarValue::Primitive(p) => p.is_zero(),
58+
// ScalarValue::Decimal(d) => d.is_zero(),
59+
// ScalarValue::Utf8(s) => s.is_empty(),
60+
// ScalarValue::Binary(b) => b.is_empty(),
61+
// ScalarValue::List(elems) => elems.is_empty(),
62+
// }
63+
// }
6464

6565
/// Returns the zero / identity value for the given [`DType`].
6666
///

0 commit comments

Comments
 (0)