[CHORE]: Move image kernel out of daft-core #2804
Conversation
CodSpeed Performance ReportMerging #2804 will improve performances by ×2.8Comparing Summary
Benchmarks breakdown
|
| use daft_core::datatypes::ExtensionArray; | ||
| use daft_core::{prelude::DataType, series::Series}; | ||
|
|
||
| pub fn html_value(s: &Series, idx: usize) -> String { |
There was a problem hiding this comment.
#2806 moves some of the display functionality out, we should reconcile with that!
There was a problem hiding this comment.
A benefit of that is that we can preserve html rendering for a series
There was a problem hiding this comment.
Initially, i was trying to make a HTMLValue trait but was running into some issues that I think were addressed in #2806. Will take another look!
There was a problem hiding this comment.
@samster25 I took another look, and I don't think this is possible with our current architecture. We would have to fully move ImageArray out of core and into daft-image which comes with it's own host of problems, and is a SIGNIFICANTLY larger refactor.
the problem is that the html_value implementation relies on the image crate, which is the main thing we want to pull out of core.
I'm not super familiar with the inventory crate, but I don't see how it is supposed to solve the problem.
If the goal is to remove the daft-image -> daft-table dependency, we could simple move the repr_html logic into python, which needs all dependencies anyways.
| } | ||
|
|
||
| pub fn data_array(&self) -> &ListArray { | ||
| const IMAGE_DATA_IDX: usize = 0; |
There was a problem hiding this comment.
WDYT about baking these const in the ImageArray itself since it's spanning across crates?
| @@ -125,10 +125,6 @@ macro_rules! impl_series_like_for_logical_array { | |||
| self.0.str_value(idx) | |||
| } | |||
|
|
|||
| fn html_value(&self, idx: usize) -> String { | |||
There was a problem hiding this comment.
how are we ensuring that we still have html_value if we drop this?
There was a problem hiding this comment.
We can't ensure that we have html_value on Series, instead we have to call the html_value function defined in daft-table. The issue is that the ImageArray definition is still in core, but the html_value implementation needed to be moved to the daft-image crate, so this logic needs to now happen further up in a crate that has access to both core and daft-image.
| use daft_core::datatypes::ExtensionArray; | ||
| use daft_core::{prelude::DataType, series::Series}; | ||
|
|
||
| pub fn html_value(s: &Series, idx: usize) -> String { |
| use daft_core::datatypes::ExtensionArray; | ||
| use daft_core::{prelude::DataType, series::Series}; | ||
|
|
||
| pub fn html_value(s: &Series, idx: usize) -> String { |
There was a problem hiding this comment.
A benefit of that is that we can preserve html rendering for a series
| @@ -4,14 +4,21 @@ comfy-table = {workspace = true} | |||
| common-error = {path = "../common/error", default-features = false} | |||
| daft-core = {path = "../daft-core", default-features = false} | |||
| daft-dsl = {path = "../daft-dsl", default-features = false} | |||
| daft-image = {path = "../daft-image", default-features = false} | |||
There was a problem hiding this comment.
we can likely drop this, if we perform the trait + register pattern discussed above
No description provided.