Skip to content

Conversation

@the2pizza
Copy link

@the2pizza the2pizza commented Mar 7, 2025

Refactoring SelectQueryBuilder

Returns SelectQueryBuilder

Accepts chain of params

        .select_all()
        .where_by(0..=3i16, "attr2")
        .order_by(Order::Asc, "attr2")
        .offset(1)
        .limit(10)
        .execute();

And for non-uniq indecies

        .select_by_attr2(2)
        .expect("msg")
        .where_by(2..=5u8, "test")
        .order_by(Order::Asc, "attr2")
        .offset(0)
        .limit(10)
        .execute();

I: Iterator<Item = #row_type>,
{
fn execute(self) -> Result<Vec<#row_type>, WorkTableError> {
let mut vals: Vec<#row_type> = self.iter.collect();
Copy link
Collaborator

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.

@the2pizza
Copy link
Author

@Handy-caT

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

@the2pizza
Copy link
Author

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?

@the2pizza the2pizza marked this pull request as ready for review March 11, 2025 15:25
@Handy-caT
Copy link
Collaborator

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 maybe already implemented this before. It can be done, but in separate task. You will need to create proper cmp function for orders you need.

For example we have some struct:

struct Test {
  a: u64,
  b: u64,
  c: String,
}

And we have some records like Test { 1, 0, "a"} Test { 2, 0, "b"}, Test { 3, -1, "c"}, Test { 4, -1, "d"}
We want to call .order_by("b", Asc).order_by("a", Desc). It will lead to this situation: first we need to sort records by b and records where a is same we need to sort by b.

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

@Handy-caT
Copy link
Collaborator

Handy-caT commented Mar 12, 2025

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)

We can't skip or limit before ordering, so there are no problems

@Handy-caT
Copy link
Collaborator

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?

Yeah, it's better to unify this API (in this task I think)

@Handy-caT
Copy link
Collaborator

Handy-caT commented Mar 12, 2025

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)

@Handy-caT Handy-caT merged commit 8e23a53 into main Mar 14, 2025
4 checks passed
@Handy-caT Handy-caT deleted the select-executor branch March 14, 2025 11:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants