Extension Scalars #6477
Extension Scalars #6477
Conversation
Signed-off-by: Connor Tsui <[email protected]>
gatesn
left a comment
gatesn
left a comment
There was a problem hiding this comment.
I'll have a think about how we can attach logic to the ExtDTypeVTable such that we don't need to provide the session if we already have a DType
| /// Unlike [`try_downcast`], this borrows rather than consuming `self`. | ||
| /// | ||
| /// [`try_downcast`]: ExtScalarValueRef::try_downcast | ||
| pub fn try_get_vtable<V: ExtScalarVTable>(&self) -> Option<&V> { |
There was a problem hiding this comment.
Would be keen to try and remove functions like this, they're probably superfluous / not called?
| /// # Errors | ||
| /// | ||
| /// Returns an error if the underlying [`fmt::Write`] operation fails. | ||
| pub fn fmt_ext_scalar( |
There was a problem hiding this comment.
Why do we have this function on the public API?
| /// | ||
| /// Note that this is not strictly necessary since [`ExtScalarValueAdapter`] and | ||
| /// [`ExtScalarValueAdapterImpl`] are both private to this module, this is just for hygiene. | ||
| mod sealed { |
There was a problem hiding this comment.
You added sealed for scalars, but removed it for dtypes?
| /// [`ExtScalarValueAdapter`]. | ||
| /// | ||
| /// [`ExtDTypeImpl`]: vortex_dtype::extension | ||
| trait ExtScalarValueAdapterImpl: sealed::Sealed + 'static + Send + Sync { |
There was a problem hiding this comment.
I think DynExtScalarValue is a bit neater than AdapterImpl
| ExtDTypeVTable::id(self) | ||
| } | ||
|
|
||
| fn create_ext_scalar_value_ref( |
There was a problem hiding this comment.
fn build is what we call it in array land?
| mod bool; | ||
| mod decimal; | ||
| mod extension; | ||
| mod ext; |
There was a problem hiding this comment.
Why the name change? Needless API churn
| } | ||
|
|
||
| #[test] | ||
| #[should_panic(expected = "Cannot convert extension scalar")] |
There was a problem hiding this comment.
This should still panic shouldn't it?
| pub fn extension<V: ExtScalarVTable + Default>(metadata: V::Metadata, value: Scalar) -> Self { | ||
| let ext_dtype = ExtDType::<V>::try_new(metadata, value.dtype().clone()) | ||
| .vortex_expect("Failed to create extension dtype"); | ||
| let storage_value = value.into_value(); |
There was a problem hiding this comment.
You can value.into_parts() to avoid the dtype clone above.
Also rename value -> storage
| /// # Errors | ||
| /// | ||
| /// Returns an error if the given [`DType`] and [`ScalarValue`] are incompatible. | ||
| pub fn validate(dtype: &DType, value: Option<&ScalarValue>) -> VortexResult<()> { |
There was a problem hiding this comment.
Curious if this actually needs to be public
| } | ||
|
|
||
| /// Check if the given [`ScalarValue`] is compatible with the given [`DType`]. | ||
| pub fn is_compatible(dtype: &DType, value: Option<&ScalarValue>) -> bool { |
2 participants
TODO
Right now this doesn't compile because it is blocked on a few other things. The tests in vortex-scalar do pass though (when it does compile), so not everything is terrible