use enum_dispatch #615
use enum_dispatch #615
Conversation
|
do you consider/evaluate enum_dispatch? which do you prefer(dispatch vs downcast)? |
yeah I think enum_dispatch also works since we know there will be only fixed number of variants. and i assume enum_diospatch will be more performant than downcast? |
|
|
||
| /// Struct for handling Parquet types dynamically. | ||
| #[derive(Debug)] | ||
| pub struct ParquetTypeStructs { |
There was a problem hiding this comment.
Is this struct ParquetTypeStructs needed if it only contains 1 field: data? Maybe we can use Box<dyn ParquetTypeTrait> directly.
| @@ -100,6 +61,10 @@ pub fn create_new_writer(schema: Arc<Type>) -> anyhow::Result<SerializedFileWrit | |||
| SerializedFileWriter::new(Vec::new(), schema, props_arc).context("Failed to create new writer") | |||
| } | |||
|
|
|||
| fn determine_parquet_type(buffer: &ParquetTypeStructs) -> ParquetTypeEnum { | |||
There was a problem hiding this comment.
if you Box<dyn trait> and need the type still, I assume dispatch is better?
There was a problem hiding this comment.
agreed, I think enum dispatch is the better choice here
…with having too many enums
yuunlimm
force-pushed
the
11-22-parquet-structs
branch
from
5b9608c to
9422589
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #615 +/- ##
=======================================
+ Coverage 48.7% 48.8% +0.1%
=======================================
Files 186 186
Lines 24904 24890 -14
=======================================
+ Hits 12131 12151 +20
+ Misses 12773 12739 -34 ☔ View full report in Codecov by Sentry. |
yuunlimm
changed the title
yuunlimm
changed the title
There was a problem hiding this comment.
spoke offline; using enum_dispatch here for better performance and worth the extensibility trade-off. This crate also gives some nice code dedupe. See enum_dispatch docs for more info.
@yuunlimm can you update the PR desc please?
| @@ -146,3 +146,4 @@ serde_canonical_json = "1.0.0" | |||
| allocative = "0.3.3" | |||
| allocative_derive = "0.3.3" | |||
| mockall = "0.12.1" | |||
| downcast-rs = "1.2.1" | |||
Description
we were initially considering using
dyn traitto address the concerns around having too many match statements. however, since dyn trait has a performance concern, we decided to go withenum_dispatch, which is 10x performantTest Plan