Conversation
ea3538f to
38d72b2
Compare
vortex_array ? |
|
It should be exposed as |
38d72b2 to
3b9fcbb
Compare
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
3b9fcbb to
d32581b
Compare
|
ok addressed comments made offline. I am heading out now so won't be able to work on this till next week. |
Merging this PR will degrade performance by 27.1%
Performance Changes
Comparing Footnotes
|
| pub use dtype::*; | ||
|
|
||
| /// A unique identifier for an extension type | ||
| pub type ExtID = ArcRef<str>; |
There was a problem hiding this comment.
Seems kinda pointless to move this under dtype::*, but 🤷
|
|
||
| pub mod vortex_array::dtype::datetime | ||
|
|
||
| pub enum vortex_array::dtype::datetime::TemporalJiff |
Signed-off-by: Nicholas Gates <nick@nickgates.com>
Summary
Tracking Issue: #6618
Since
vortex-dtypeandvortex-scalarhave merged intovortex-array, and since planned extension logic now covers all 3 of dtype, scalar, and array, we need to pull out the existing extension dtype logic so that all 3 of these modules can access it.This PR adds a
vortex::extensionsubmodule tovortexthat contains theExtVTable(which was previously theExtDTypeVTable), and a sub-submoduledtypethat contains things likeExtDTypeRef.We should hope to add other sub-submodules
scalarandarraythat would haveExtScalarValueRefandExtArrayRef.I am wondering if we need to figure out the naming (please read the discussion in #6618) right now. Do we care that much right now or can we just change it later?
API Changes
Adds
vortex::extension(which is pulled fromvortex_array::extension), and moves some logic around extension dtypes and extension vtable into this new submodule.AFAIK nobody is actually using the new extension dtype so I think it should be fine to break?
There is still a concern though if we release this version and someone decides to start implementing
ExtVTable: we need to add a lot more methods to thatTesting
N/A since this is just moving code around.