Conversation
connortsui20
left a comment
connortsui20
left a comment
There was a problem hiding this comment.
Is it possible to follow the template a bit more closely? It's less about following the exact template and more that it's a bit hard to understand what it being proposed and the different tradeoffs (alternatives) without some of the sections in the template. Basically, when reading through this I'm trying to figure out the "why" via the "what", and it should be the other way around.
Additionally, there is quite a lot of code here that is paired with high-level description. I think it would be helpful if some of the boilerplate-y code was removed so we could focus on things that are not obvious. That would help in making this easier to read as well.
Also as an aside: I'm not an FFI expert but I feel like there is a lack of ownership semantics described here. Shouldn't that be one of the main things we have to worry about with interop between Rust and C?
| typedef struct vx_scan_selection { | ||
| const size_t* idx; | ||
| size_t idx_len; | ||
| } vx_scan_selection; |
There was a problem hiding this comment.
is this a remnant of a revision (this exists above)
| ```c | ||
| ``` |
| However, as the future of such approach is unclear, a async-unfriendly option is | ||
| described below |
There was a problem hiding this comment.
I think it would be valuable to actually think through what an async API looks like in practice, because it is almost certainly the case that the devil is in the details.
Otherwise, this should be in an unresolved questions section and we should acknowledge we are not going to work on this now.
| void* (*cache_init)(vx_error* err); | ||
| void (*cache_free)(void* cache, vx_error* err); | ||
| void (*cache_get)(void* cache, const char* key, void** value, vx_error* err); | ||
| void (*cache_put)(void* cache, const char* key, void* value, vx_error* err); | ||
| void (*cache_delete)(void* cache, const char* key, vx_error* err); |
There was a problem hiding this comment.
Is this the way we want to expose a cache?
I feel like for Vortex arrays specifically this is way too generic to be useful. What exactly would we be storing in the cache? Just vortex arrays, or things like stats? Partitions? Array trees? And if we are passing around void * values then shouldn't there also be a size parameter somewhere? Also things related to ownership and eviction semantics, etc are missing.
2 participants
Implementation https://github.com/vortex-data/vortex/tree/myrrc/scan-api-duckdb