Make struct cast implementation pluggable#7684
Conversation
Signed-off-by: Robert Kruszewski <github@robertk.io>
Merging this PR will improve performance by 31.26%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | Simulation | take_search[(0.005, 0.05)] |
167.5 µs | 131 µs | +27.83% |
| ⚡ | Simulation | take_search[(0.005, 0.1)] |
319.6 µs | 246.8 µs | +29.53% |
| ⚡ | Simulation | take_search[(0.005, 1.0)] |
3.1 ms | 2.3 ms | +31.26% |
| ⚡ | Simulation | take_search[(0.005, 0.5)] |
1.5 ms | 1.2 ms | +31.03% |
| ⚡ | Simulation | take_search[(0.01, 0.05)] |
178.6 µs | 142.1 µs | +25.65% |
| ⚡ | Simulation | take_search[(0.01, 0.5)] |
1.6 ms | 1.3 ms | +28.58% |
| ⚡ | Simulation | take_search[(0.1, 0.05)] |
248.3 µs | 211.9 µs | +17.21% |
| ⚡ | Simulation | take_search[(0.1, 1.0)] |
4.3 ms | 3.5 ms | +20.62% |
| ⚡ | Simulation | take_search[(0.01, 1.0)] |
3.3 ms | 2.5 ms | +28.77% |
| ⚡ | Simulation | take_search_chunked[(0.01, 0.05)] |
212.6 µs | 181.8 µs | +16.97% |
| ⚡ | Simulation | take_search_chunked[(0.005, 1.0)] |
3.7 ms | 3.1 ms | +20.16% |
| ⚡ | Simulation | take_search_chunked[(0.005, 0.5)] |
1.9 ms | 1.5 ms | +20.05% |
| ⚡ | Simulation | take_search[(0.1, 0.1)] |
458 µs | 385.1 µs | +18.93% |
| ⚡ | Simulation | take_search_chunked[(0.005, 0.1)] |
383.3 µs | 321.5 µs | +19.23% |
| ⚡ | Simulation | take_search[(0.1, 0.5)] |
2.2 ms | 1.8 ms | +20.39% |
| ⚡ | Simulation | take_search_chunked[(0.005, 0.05)] |
199.9 µs | 169 µs | +18.27% |
| ⚡ | Simulation | take_search[(0.01, 0.1)] |
340.5 µs | 267.7 µs | +27.22% |
| ⚡ | Simulation | take_search_chunked[(0.01, 0.5)] |
2 ms | 1.7 ms | +18.61% |
| ⚡ | Simulation | take_search_chunked[(0.1, 0.1)] |
518.1 µs | 456.3 µs | +13.56% |
| ⚡ | Simulation | take_search_chunked[(0.1, 0.05)] |
278.5 µs | 247.6 µs | +12.48% |
| ... | ... | ... | ... | ... | ... |
ℹ️ Only the first 20 benchmarks are displayed. Go to the app to view all benchmarks.
Comparing rk/movecasttoplugin (0dd1ebc) with develop (f9067c7)
| child: &ArrayRef, | ||
| ) -> Option<Arc<[ExecuteParentFn]>> { | ||
| ctx.session() | ||
| .get_opt::<ArrayKernels>() |
There was a problem hiding this comment.
Hold this and pass it into the loop, so you avoid the repeated fetch
| /// functions. | ||
| pub fn find_execute_parent(&self, parent: Id, child: Id) -> Option<Arc<[ExecuteParentFn]>> { | ||
| let id = hash_fn_id(parent, child); | ||
| self.execute_parent.load().get(&id).cloned() |
There was a problem hiding this comment.
would be nice to used ArcSwap::cached here. It might change the API slightly
| reduce_parent: ArcSwap<HashMap<u64, Arc<[ReduceParentFn]>>>, | ||
| execute_parent: ArcSwap<HashMap<u64, Arc<[ExecuteParentFn]>>>, |
There was a problem hiding this comment.
can we have the u64 as a newtype or at least type alias
There was a problem hiding this comment.
Shall we clean up the deprecated here?
There was a problem hiding this comment.
I noticed there's more refactoring to do here
| } | ||
|
|
||
| let source_sdtype = array.struct_fields(); | ||
| pub(crate) fn struct_cast_execute_parent( |
There was a problem hiding this comment.
why is this an execute if _ctx?
There was a problem hiding this comment.
This should be just a reduce rule apart from the case where you want to convert validity from nullable into non nullable which MIGHT require compute. We need to refactor validity casting
|
Is this related to #6900? |
|
It is related. We need to acknowledge that different systems have different requirements and we need to be able to override the behaviour |
|
as I was refactoring this logic I made #7710 to cleanup validity casting |
3 participants
This pr moves Struct Cast implementation to be overridable by VortexSession.
The default implementation stays the same, i.e. it resolves the fields by name.
In follow up we will add back the positional cast and add both implementations
to the fuzzer
Signed-off-by: Robert Kruszewski github@robertk.io