Several of our _Scalar types (e.g., ListScalar and BinaryScalar) are not Copy because they store Option<Arc<>> instead of a Option<&'a Arc<>>. Given that all of these are supposed to be typed views into a Scalar, it doesn't make a lot of sense that these are not copyable like a reference.
#[derive(Debug)] // <- Can only derive `Clone` and cannot derive `Copy`
pub struct ListScalar<'a> {
dtype: &'a DType,
element_dtype: &'a Arc<DType>,
elements: Option<Arc<[ScalarValue]>>,
// ^^^
}
I don't think that there is any reason that we cannot have all of our scalars be similar to StructScalar (which for some reason is not Clone or Copy but it can be):
#[derive(Debug)] // <- CAN derive `Clone` and `Copy`
pub struct StructScalar<'a> {
dtype: &'a DType,
fields: Option<&'a Arc<[ScalarValue]>>,
// ^^^^^^^
}
Unfortunately, making all of these Copy is not as simple as adding a &'a to every Arc, since the constructors require us passing in a reference to ScalarValue instead of an owned value. The constructors are quite inconsistent, and the methods on InnerScalarValue are also quite inconsistent:
impl InnerScalarValue {
pub(crate) fn as_buffer(&self) -> VortexResult<Option<Arc<ByteBuffer>>> {}
pub(crate) fn as_buffer_string(&self) -> VortexResult<Option<Arc<BufferString>>> {}
pub(crate) fn as_list(&self) -> VortexResult<Option<&Arc<[ScalarValue]>>> {}
// ^^^^
}
I think that all of our _Scalar types should be able to #[derive(Clone, Copy)].
I also still think that it doesn't make sense to have each scalar view have an internal null (with Option fields) and the fact that this is inconsistent is more evidence that we should really have Option<ListScalar<'a>> instead of an internal Option<Arc<[ScalarValue]>>, but that is an issue for another day.
Several of our
_Scalartypes (e.g.,ListScalarandBinaryScalar) are notCopybecause they storeOption<Arc<>>instead of aOption<&'a Arc<>>. Given that all of these are supposed to be typed views into aScalar, it doesn't make a lot of sense that these are not copyable like a reference.I don't think that there is any reason that we cannot have all of our scalars be similar to
StructScalar(which for some reason is notCloneorCopybut it can be):Unfortunately, making all of these
Copyis not as simple as adding a&'ato everyArc, since the constructors require us passing in a reference toScalarValueinstead of an owned value. The constructors are quite inconsistent, and the methods onInnerScalarValueare also quite inconsistent:I think that all of our
_Scalartypes should be able to#[derive(Clone, Copy)].I also still think that it doesn't make sense to have each scalar view have an internal null (with
Optionfields) and the fact that this is inconsistent is more evidence that we should really haveOption<ListScalar<'a>>instead of an internalOption<Arc<[ScalarValue]>>, but that is an issue for another day.