chore: all scalars support cast#1965
Conversation
|
@gatesn curious for your thoughts here. I'm not blocked by this PR, but I keep getting turned around by our types and scalars. There's at least one bug: casting a null BoolScalar to Bool(Nullable) raises the error "not a bool".
|
| assert_eq!(min, Some(null_i32.clone())); | ||
| assert_eq!(max, Some(null_i32)); | ||
| assert_eq!(min, None); | ||
| assert_eq!(max, None); |
There was a problem hiding this comment.
This was just wrong. I'm not really sure why it worked.
| Scalar::null(DType::Bool(Nullable)) | ||
| ); | ||
| assert_eq!(bool_arr.statistics().compute(Stat::Min), None); | ||
| assert_eq!(bool_arr.statistics().compute(Stat::Max), None); |
There was a problem hiding this comment.
These were just wrong, min and max are never null.
| .encoding() | ||
| .compute_statistics(self, stat) | ||
| .ok()? | ||
| .vortex_expect("compute_statistics must not fail") |
There was a problem hiding this comment.
Debatable but it's really annoying to debug compute_statistics errors without this.
|
@gatesn OK, this is finally working as expected. |
| todo!() | ||
| pub(crate) fn cast(&self, dtype: &DType) -> VortexResult<Scalar> { | ||
| if !matches!(dtype, DType::Bool(..)) { | ||
| vortex_bail!("Can't cast bool to {}", dtype) |
|
|
||
| pub fn cast(&self, _dtype: &DType) -> VortexResult<Scalar> { | ||
| todo!() | ||
| pub(crate) fn cast(&self, dtype: &DType) -> VortexResult<Scalar> { |
There was a problem hiding this comment.
Can you cast a string into a number if it parses correctly?
There was a problem hiding this comment.
Parsing a string seems wrong to me because it's so much more work. Casting from string to binary (i.e. forgetting the UTF8-ness) seems correct to me. I guess Binary to any integer type should be fine, erroring if the binary is not the right length. Particularly because if we add FixedWidthBinary, I would think binary -> fixedwidthbinary(4) -> u32 should work.
Should I add the String to Binary case to this PR?
There was a problem hiding this comment.
I think it's okay to add a FLUP issue with various casts we want to support (e.g., bool to int, utf8 to binary, etc)
| use crate::Scalar; | ||
|
|
||
| pub struct ExtScalar<'a> { | ||
| dtype: &'a DType, |
| vortex_bail!("Can't cast null scalar to non-nullable type") | ||
| pub fn cast(&self, target: &DType) -> VortexResult<Self> { | ||
| if let DType::Extension(ext_dtype) = target { | ||
| let storage_scalar = self.cast_to_non_extension(ext_dtype.storage_dtype())?; |
There was a problem hiding this comment.
I think it's theoretically possible right now to have a storage dtype that is an Extension type... we should potentially prohibit that
| if target.is_nullable() { | ||
| return Ok(Scalar::new(target.clone(), self.value.clone())); | ||
| } else { | ||
| vortex_bail!("Can't cast null scalar to non-nullable type") |
There was a problem hiding this comment.
include target in error msg
|
|
||
| pub fn cast(&self, _dtype: &DType) -> VortexResult<Scalar> { | ||
| todo!() | ||
| pub(crate) fn cast(&self, dtype: &DType) -> VortexResult<Scalar> { |
There was a problem hiding this comment.
I think it's okay to add a FLUP issue with various casts we want to support (e.g., bool to int, utf8 to binary, etc)
3 participants
No description provided.