DO NOT LAND Add Rust back #420
Conversation
| fn from_transaction_info( | ||
| info: &TransactionInfo, | ||
| txn_version: i64, | ||
| epoch: i64, | ||
| block_height: i64, | ||
| ) -> Self { | ||
| Self { | ||
| txn_version, | ||
| block_height, |
There was a problem hiding this comment.
Not 100% sure the use case but if possible, it'd be good to just take in the owned object, it'd be more efficient. No worries if it doesn't make sense for this use case.
There was a problem hiding this comment.
there are usages in more upstreams, which requires cloning. so I would leave as it is for now.
| } | ||
| } | ||
|
|
||
| impl Default for Transaction { |
There was a problem hiding this comment.
How come we have to implement Default? There is no "default transaction" so I'm not sure it makes sense to implement Default for Transaction. Same comment for many of these. If some trait / library is forcing us to implement Default, we should at least use unreachable so we never use it, or if something does use it, for the fields we should use default, like txn_version: Default::default(), event_root_hash: Default::default(), etc.
Member
Author
@yuunlimm yuunlimm 3 days ago •
there is a usecase, where we have derive the parquet schema from the instance, and the we need to know this schema in advance. please take a look at hasParquetSchema trait in the generic_parquet_processor.rs. and we use this schema() to construct parquetHandler for each struct type. that's the only usecase of default but I am not too sure if there is any other workaround.
| } | ||
| } | ||
|
|
||
| pub fn get_outer_type_from_resource(write_resource: &WriteResource) -> String { |
There was a problem hiding this comment.
This method and convert_move_struct_tag don't have anything to do with MoveResource. Can you move them to the relevant struct or just make them freestanding?
As a general rule, if a method doesn't take self as a param or return Self there is no reason for it to a method on a struct.
There was a problem hiding this comment.
get_outer_type_from_resource isn't being used anywhere, will remove. and made convert_move_struct_tag freestadanding
| let mut processor_tasks = vec![fetcher_task]; | ||
| for task_index in 0..concurrent_tasks { | ||
| let join_handle: JoinHandle<()> = self | ||
| .launch_processor_task( |
There was a problem hiding this comment.
I see this function takes two Options, one for the default sender and one for the parquet sender. I see we always have exactly one Some and one Option. Feels like launch_processor_task could be made more safe by encoding this in the type system, e.g. by using an enum:
enum GapDetectorSender {
Normal(AsyncSender),
Parquet(AsyncSender),
}
| txn_version: i64, | ||
| block_height: i64, | ||
| block_timestamp: chrono::NaiveDateTime, | ||
| ) -> Self { |
There was a problem hiding this comment.
) -> Self {
let parsed_data = Self::convert_move_struct_tag(
delete_resource
.r#type
.as_ref()
.expect("MoveStructTag Not Exists."),
);
PR hacking for the purpose of auditing the Parquet code.