-
Notifications
You must be signed in to change notification settings - Fork 7
WIP draft refactoring SelectQueryBuilder #53
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| I: Iterator<Item = #row_type>, | ||
| { | ||
| fn execute(self) -> Result<Vec<#row_type>, WorkTableError> { | ||
| let mut vals: Vec<#row_type> = self.iter.collect(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why..... The idea was to modify iterator and to not collect it on start allocating memory for full non-limited select.
|
Not sure this implementation is ideal, DoubleEndedIterator doesn't allow to use take and skip because of lazy iteration and we don't have ExactSizeIterator (or I did something wrong and didn't find a way to pass it) Also we cannot sort Iterators without collect to Vec Also now we can use multiple where_by to specify ranges of different columns. I moved Order to simple record instead VecDeque (actually I cannot image a logic how to multiple sorting should work without braking previuos sort) I don't like offset and limit implementation but it's my best try, possibly later I will find better solution Please feel free to feedback or note on the changes Much appreciate |
|
Also there are we have different return types for select_all (SelectQueryBuilder) and for select_by_non_index(Result(SelectQueryBuilder), not better to move to one return type? |
I maybe already implemented this before. It can be done, but in separate task. You will need to create proper For example we have some struct: struct Test {
a: u64,
b: u64,
c: String,
}And we have some records like For this we can create cmp function like let cmp: |left: Test, right: Test| = {
let b_cmp = left.b.partial_cmp(right.b);
if let Some(Ordering::Equal) = b_cmp {
// if b is same, we compare a and return it's result
let a_cmp = right.a.partial_cmp(rleft.a);
} else {
b_cmp
}
}and sort our result vec with this cmp fn |
We can't skip or limit before ordering, so there are no problems |
Yeah, it's better to unify this API (in this task I think) |
|
Also, for order by there should be one optimisation. For just select if we use order by and field is indexed we should use index to avoid sort (I think also in separate task) |
Refactoring SelectQueryBuilder
Returns SelectQueryBuilder
Accepts chain of params
And for non-uniq indecies