feat: support sort agg #646
Conversation
|
please help review, and give me some more advice. 🤣 |
Kikkon
force-pushed
the
kikkon/sort_agg
branch
2 times, most recently
from
ba9fd53 to
2047a61
Compare
Kikkon
force-pushed
the
kikkon/sort_agg
branch
3 times, most recently
from
6535e4d to
8a6c2f8
Compare
src/executor/sort_agg.rs
Outdated
| } | ||
| last_key = Some(group_key); | ||
| } | ||
| yield finish_agg(&states) |
There was a problem hiding this comment.
Should also be outside the loop just like states?
There was a problem hiding this comment.
So I think adding relevant tests may be better..
|
BTW, since we will squash merge, there's no need to force push everytime to make commit history (too) clean. Leave changes in new commits may also help reviewers to focus on new changes since last review 😄 |
Signed-off-by: kikkon <nian920@outlook.com>
Signed-off-by: kikkon <nian920@outlook.com>
Signed-off-by: kikkon <nian920@outlook.com>
|
@skyzh I compared sort_agg and simple_agg on no rows from child, they both return one row of all null. I added a judgment for last_key, it's right? 🤣 |
|
Let me take a look 🤣 |
src/executor/sort_agg.rs
Outdated
| for col in group_cols.iter() { | ||
| group_key.push(col.get(row_idx)); | ||
| } | ||
| // Check group key & lask key |
| // Create group key | ||
| let mut group_key = HashKey::new(); | ||
| for col in group_cols.iter() { | ||
| group_key.push(col.get(row_idx)); |
There was a problem hiding this comment.
Maybe we can optimize this in the future. We can first check group key is the same as the current group key, and then clone by .get if different. Otherwise we will always need to create new vector for every row of hash key.
close #591