Conversation
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?
There was a problem hiding this comment.
In this case this is now a helper function for the matcher. I'll leave it in for now and we can remove it if it makes more sense
| /// | ||
| /// 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?
There was a problem hiding this comment.
Imo we should probably seal the dtype one as well
| mod bool; | ||
| mod decimal; | ||
| mod extension; | ||
| mod ext; |
There was a problem hiding this comment.
Why the name change? Needless API churn
There was a problem hiding this comment.
It's not public (everything is re-exported) so there's no change. I wanted to make a distinction between this and the extension module that contains all the vtable logic.
| } | ||
|
|
||
| #[test] | ||
| #[should_panic(expected = "Cannot convert extension scalar")] |
There was a problem hiding this comment.
This should still panic shouldn't it?
There was a problem hiding this comment.
This now just doesn't even compile so unless we want to put it in a doctest (which I think it kind of pointless) we can just delete it.
vortex-scalar/src/constructor.rs
Outdated
| 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
There was a problem hiding this comment.
The dtype is the extension dtype, into_parts would give the storage dtype
| /// # 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
There was a problem hiding this comment.
good question 🤔
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
connortsui20
force-pushed
the
ct/ext-scalar
branch
from
3a4c416 to
3c9309a
Compare
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