Adding find_with_linked #1728
Adding find_with_linked #1728
Conversation
src/query/combine.rs
Outdated
| pub(crate) fn new(query: SelectStatement) -> Self { | ||
| Self::new_without_prepare(query) | ||
| .prepare_select() | ||
| .prepare_order_by() | ||
| } | ||
|
|
||
| pub(crate) fn new_without_prepare(query: SelectStatement) -> Self { | ||
| Self { | ||
| query, | ||
| entity: PhantomData, | ||
| } | ||
| .prepare_select() | ||
| .prepare_order_by() | ||
| } |
There was a problem hiding this comment.
Did we invoke prepare_order_by() method twice? When we call SelectTwoMany::new() it will invoke prepare_order_by() method twice.
|
Just to clarify, the implementation should involve using a hash map, instead of relying on order by (which is theoretically wrong) to group elements. The algorithm can be referenced in Line 208 in 4905dc5 Since now Value is already Hashable via SeaQL/sea-query#598, we should change the implementation of loader as well. Instead of We should also consider chaning the existing find_with_related impl as well, but may be, not in the same PR. Proceed with caution: I would rate this task ★★★★☆ |
src/executor/select.rs
Outdated
| @@ -997,6 +998,27 @@ where | |||
| L: EntityTrait, | |||
| R: EntityTrait, | |||
| { | |||
| // #[cfg(feature = "hashable-value")] | |||
There was a problem hiding this comment.
I think sea_query::Value is not hashable by default and a specified feature is needed
but then the tests would need to also have the feature on in the configuration
my thought for solution is to make sea_query::Value to be hashable by default
There was a problem hiding this comment.
Yes the 'hashable-value' feature is intended to be added by default.
src/executor/select.rs
Outdated
| let last_val = last_l.get(col); | ||
| if !val.eq(&last_val) { | ||
| same_l = false; | ||
| { |
There was a problem hiding this comment.
Why do we need a nested scope here?
src/executor/select.rs
Outdated
| } | ||
| acc | ||
|
|
||
| // let mut acc: Vec<(L::Model, Vec<R::Model>)> = Vec::new(); |
There was a problem hiding this comment.
if you want to keep the old code for reference, we can put it in another function consolidate_query_result_of_ordered_rows
|
@darkmmon I believe you liked performance algorithms, so may be we can optimize the implementation, here we want to minimize allocation, copy and (to some extend) move. We observe that So the intermediate hashmap can simply be In the final step, we can re-assemble everything together. Something like: rows.into_iter().filter_map(|l_model, _| {
let l_pk = pk_of(l_model);
let r_models = hashmap.remove(l_pk);
if let Some(r_models) = r_models {
Some((l_model, r_models))
} else {
// it means we have seen this l_model already
None
}
}).collect()The key points here:
Anything more might be unnecessary. In Rust, objects are heavy as they are not pointers, so cloning and even moving them (to/from the heap) is somewhat expensive. |
|
GitHub somehow did not allow me to merge into a different branch. So I did manually. |
🎉 Released In 0.12.1 🎉Thank you everyone for the contribution! |
PR Info
New Features
Bug Fixes
Breaking Changes
Changes