Conversation
Signed-off-by: Nicholas Gates <nick@nickgates.com>
Signed-off-by: Nicholas Gates <nick@nickgates.com>
Signed-off-by: Nicholas Gates <nick@nickgates.com>
Signed-off-by: Nicholas Gates <nick@nickgates.com>
Signed-off-by: Nicholas Gates <nick@nickgates.com>
Signed-off-by: Nicholas Gates <nick@nickgates.com>
Signed-off-by: Nicholas Gates <nick@nickgates.com>
Deploying vortex-bench with
|
| Latest commit: |
1978f79
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://a8b69026.vortex-93b.pages.dev |
| Branch Preview URL: | https://ngates-ext-dtype-vtable.vortex-93b.pages.dev |
Signed-off-by: Nicholas Gates <nick@nickgates.com>
Signed-off-by: Nicholas Gates <nick@nickgates.com>
Signed-off-by: Nicholas Gates <nick@nickgates.com>
Signed-off-by: Nicholas Gates <nick@nickgates.com>
Signed-off-by: Nicholas Gates <nick@nickgates.com>
Signed-off-by: Nicholas Gates <nick@nickgates.com>
Signed-off-by: Nicholas Gates <nick@nickgates.com>
Signed-off-by: Nicholas Gates <nick@nickgates.com>
Signed-off-by: Nicholas Gates <nick@nickgates.com>
Signed-off-by: Nicholas Gates <nick@nickgates.com>
Signed-off-by: Nicholas Gates <nick@nickgates.com>
vortex-dtype/src/extension/vtable.rs
Outdated
Comment on lines
16
to
19
| // FIXME(ngates): are VTables ZSTs or not? | ||
| // * If yes, then we can create &'static dyn DynVTable references easily. | ||
| // * If no, then we need to manage their lifetimes some other way. And we likely need to hold | ||
| // instances of them on the object itself. |
Contributor
Author
There was a problem hiding this comment.
Ah, no they're not. But they almost always are. The cases where they're not are FFI / PyVortex / language bindings. For Rust implementations they are ZST and get optimized away to nothing.
Contributor
There was a problem hiding this comment.
Why not have that with a struct that impls the dtype?
Contributor
Benchmarks: Random AccessSummary
|
Contributor
Benchmarks: TPC-H SF=1 on NVMESummary
Detailed Results Table
|
Contributor
Benchmarks: FineWeb NVMeSummary
Detailed Results Table
|
Contributor
Benchmarks: TPC-H SF=1 on S3Summary
Detailed Results Table
|
Contributor
Benchmarks: TPC-DS SF=1 on NVMESummary
Detailed Results Table
|
Comment on lines
-280
to
-313
| use std::sync::Arc; | ||
|
|
||
| use super::*; | ||
| use crate::ExtDType; | ||
| use crate::ExtMetadata; | ||
| use crate::PType; | ||
|
|
||
| #[test] | ||
| fn test_roundtrip_metadata() { | ||
| let meta: ExtMetadata = | ||
| TemporalMetadata::Timestamp(TimeUnit::Milliseconds, Some("UTC".to_string())).into(); | ||
|
|
||
| assert_eq!( | ||
| meta.as_ref(), | ||
| vec![ | ||
| 2u8, // Tag for TimeUnit::Ms | ||
| 0x3u8, 0x0u8, // u16 length | ||
| b'U', b'T', b'C', | ||
| ] | ||
| .as_slice() | ||
| ); | ||
|
|
||
| let temporal_metadata = TemporalMetadata::try_from(&ExtDType::new( | ||
| TIMESTAMP_ID.clone(), | ||
| Arc::new(PType::I64.into()), | ||
| Some(meta), | ||
| )) | ||
| .unwrap(); | ||
|
|
||
| assert_eq!( | ||
| temporal_metadata, | ||
| TemporalMetadata::Timestamp(TimeUnit::Milliseconds, Some("UTC".to_string())) | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
shall we keep these?
Contributor
Benchmarks: TPC-H SF=10 on NVMESummary
Detailed Results Table
|
vortex-dtype/src/extension/vtable.rs
Outdated
Comment on lines
53
to
54
| // TODO(ngates): add conversion vtable for Arrow extension types. | ||
| // type ArrowConversion: ArrowConversion<Self>; |
Contributor
There was a problem hiding this comment.
this lives here? surely not
Contributor
Benchmarks: FineWeb S3Summary
Detailed Results Table
|
Contributor
Benchmarks: Statistical and Population GeneticsSummary
Detailed Results Table
|
Contributor
Benchmarks: TPC-H SF=10 on S3Summary
Detailed Results Table
|
Contributor
Benchmarks: Clickbench on NVMESummary
Detailed Results Table
|
| fn deserialize( | ||
| &self, | ||
| _metadata: &[u8], | ||
| _session: &VortexSession, |
Contributor
There was a problem hiding this comment.
why is this session?
Contributor
|
Why did we include Expression and DType changes in the same PR |
Contributor
Benchmarks: CompressionSummary
Detailed Results Table
|
joseph-isaacs
approved these changes
Jan 29, 2026
Contributor
connortsui20
left a comment
connortsui20
left a comment
There was a problem hiding this comment.
In general this looks good to me. Quite a few nits related to naming though, otherwise this is a good change!
vortex-dtype/src/extension/mod.rs
Outdated
| } | ||
|
|
||
| /// Downcast to the concrete options type. | ||
| pub fn try_options<M: Matcher>(&self) -> Option<M::Match<'_>> { |
Contributor
There was a problem hiding this comment.
Maybe call this try_as_ext_options? This was kind of hard to grok
Contributor
There was a problem hiding this comment.
Or _ext_metadata if that changes
vortex-dtype/src/extension/mod.rs
Outdated
| } | ||
|
|
||
| /// Downcast to the concrete options type. | ||
| pub fn options<M: Matcher>(&self) -> M::Match<'_> { |
Contributor
There was a problem hiding this comment.
And then this as_ext_options?
vortex-dtype/src/extension/vtable.rs
Outdated
| /// The public API for defining new extension DTypes. | ||
| pub trait ExtDTypeVTable: 'static + Sized + Send + Sync + Clone + Debug { | ||
| /// Associated type containing the deserialized metadata for this extension type | ||
| type Options: 'static + Send + Sync + Clone + Debug + Display + Eq + Hash; |
Contributor
There was a problem hiding this comment.
I fell like we might as well just call this Metadata since that is exactly what this is
vortex-dtype/src/datetime/mod.rs
Outdated
| mod temporal; | ||
| mod date; | ||
| mod matcher; | ||
| // mod temporal; |
| /// Assert that the size of DType is 16 bytes. | ||
| #[cfg(not(target_arch = "wasm32"))] | ||
| const_assert_eq!(size_of::<DType>(), 16); | ||
| const _: [(); size_of::<DType>()] = [(); 24]; // FIXME(ngates): should we keep this at 16? |
Contributor
There was a problem hiding this comment.
Is there a reason this is different?
AdamGS
pushed a commit
that referenced
this pull request
Feb 2, 2026
Signed-off-by: Nicholas Gates <nick@nickgates.com>
danking
pushed a commit
that referenced
this pull request
Feb 6, 2026
Signed-off-by: Nicholas Gates <nick@nickgates.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Time::new(...)etc.ext_dtype.metadata::<Time>()andext_dtype.metadata::<AnyTemporal>()