Repository navigation
Remove vortex-dtype dependency from vortex-compute - #6557
connortsui20 wants to merge 2 commits into
Conversation
|
At this point all of vortex-compute is dead code. Should we just delete this? Git always has history |
|
ok, no, filter is being used. Should we move Filter into the vortex-array and delete the rest? |
|
We really should be using more from |
| use arrow_buffer::i256; | ||
| use half::f16; |
There was a problem hiding this comment.
i think we want to keep the wrapper types
There was a problem hiding this comment.
Should I just remove this comparison implementation for this then? We don't actually use it anywhere, and otherwise it would create a circular dependency when we merge vortex-dtype into vortex-array
There was a problem hiding this comment.
Yeah I'm just going to remove it
There was a problem hiding this comment.
maybe we should keep the traits here but keep the impls in vortex-array? do we ever use vortex-compute directly instead of going thru vortex-array?
There was a problem hiding this comment.
or we can keep impls for primitive types here but we need to keep the impls for f16/i256 in vortex-array? idk, i'm also in the camp of not sure why we keep this crate around at all
There was a problem hiding this comment.
as of right now, only vortex-array uses vortex-compute. I think the original idea was that we wanted to have something other stuff (vectors) could call into but now there's probably no reason to keep it.
3d52c95 to
e3ae281
Compare
|
Ok arguably we should completely delete this crate, but I believe until we can say that all of the optimized code we have has been migrated to |
Signed-off-by: Connor Tsui <[email protected]>
Signed-off-by: Connor Tsui <[email protected]>
e3ae281 to
9a85905
Compare
|
Was there a reason why we didn't use this code earlier? Git has the history even if you delete the code |
|
This was when we thought that we were moving towards vectors instead of arrays, so we didn't bother migrating the code to |
|
Discussed offline: we're just going to remove this and I'll create an issue to bring some of these optimizations back. |
Summary
Tracking Issue: #6547
Removes all instances of
vortex-dtypeimports forvortex-compute. This was not super hard since all of the existing imports fromvortex-dtypeare really just re-exports of other third-party crates.Technically there are API changes but these are all generalizations / now the trait bounds on some of the compute is less restrictive than before. So I wouldn't really consider this a breaking change.
The one concern would the SIMD code that was already commented out / not used before. At some point we want to do that (see my failed attempt here: #5722) in general, but I guess today is not that day :(
Testing
N/A