feat: specify the storage_dtype in ExtDType #1007
Conversation
| let storage_dtype = storage_chunks | ||
| .first() | ||
| .ok_or_else(|| vortex_err!("Expected at least one chunk in ChunkedArray"))? | ||
| .dtype() | ||
| .clone(); | ||
| let storage_dtype = ext_dtype.scalars_dtype().clone(); |
| let scalar_ext = ExtScalar::try_new(self.dtype(), self.scalar_value()) | ||
| .vortex_expect("Expected an extension scalar"); | ||
|
|
||
| // FIXME(ngates): there's not enough information to get the storage array. | ||
| let n = self.dtype().nullability(); | ||
| let storage_dtype = match scalar_ext.value() { | ||
| ScalarValue::Bool(_) => DType::Binary(n), | ||
| ScalarValue::Primitive(pvalue) => DType::Primitive(pvalue.ptype(), n), | ||
| ScalarValue::Buffer(_) => DType::Binary(n), | ||
| ScalarValue::BufferString(_) => DType::Utf8(n), | ||
| ScalarValue::List(_) => vortex_panic!("List not supported"), | ||
| ScalarValue::Null => DType::Null, | ||
| }; | ||
|
|
||
| let storage_dtype = self.ext_dtype().scalars_dtype().clone(); |
vortex-proto/Cargo.toml
Outdated
| #[lints] | ||
| #workspace = true |
There was a problem hiding this comment.
I couldn't figure out a better way to kill the clippy errors on the generated code.
I tried both #[allow(clippy)] and #[allow(clippy::all)] on the module declarations and that didn't do it 🤷
There was a problem hiding this comment.
Clippy::all doesn’t mean “all” it means the things that are not pedantic or nursery or something like that
| @@ -1,22 +1,18 @@ | |||
| // This file is @generated by prost-build. | |||
| #[allow(clippy::derive_partial_eq_without_eq)] | |||
There was a problem hiding this comment.
this was changed in tokio-rs/prost#1115 so we now need to allow(clippy::nursery) in the module level
a10y
force-pushed
the
aduffy/ext-types
branch
from
9d3bc32
to
87cb364
Compare
a10y
changed the title
a10y
changed the title
| @@ -14,7 +14,7 @@ use vortex_error::vortex_panic; | |||
| use vortex_scalar::{PValue, Scalar, ScalarValue}; | |||
|
|
|||
| pub fn scalar_into_py(py: Python, x: Scalar, copy_into_python: bool) -> PyResult<PyObject> { | |||
| let (value, dtype) = x.into_parts(); | |||
| let (dtype, value) = x.into_parts(); | |||
There was a problem hiding this comment.
lol i guess dan wrote the same method at the same time as me we just wrote the args in a different order
There was a problem hiding this comment.
i just sort of yolo resolved the conflict and this was the outcome
| ext.metadata() | ||
| .map(|m| format!(", {:?}", m)) | ||
| .unwrap_or_else(|| "".to_string()), | ||
| n | ||
| ext.storage_dtype().nullability(), |
There was a problem hiding this comment.
the nullability split thing here might read weird (it will say NonNullable above... and then maybe-Nullable here?)
Adds
scalars_dtypetoExtDType.This PR adds
scalars_dtype(alternative name options:canonical_dtype,storage_dtype) toExtDType.This is desirable for a few reasons
To avoid duplicating the nullability, we remove top-level
nullabilityfrom theDType::Extensionvariant, instead nullability is accessed through the inner ExtDType.This has the unfortunate effect of bringing
size_of::<DType>()from 40 -> 48 bytes, which obviously makes every array's metadata 8 bytes larger.