Skip to content
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

fix: make header correct when using group by #624

Merged
merged 6 commits into from
Apr 17, 2022
Merged

fix: make header correct when using group by #624

merged 6 commits into from
Apr 17, 2022

Conversation

shmiwy
Copy link
Contributor

@shmiwy shmiwy commented Apr 13, 2022

close #538
Signed-off-by: Shmiwy wyf000219@gamil.com

@@ -133,6 +133,26 @@ impl BoundExpr {
visitor.visit_expr(self);
visitor.0
}

pub fn format_name(&self, child_schema: &Vec<ColumnDesc>) -> String {
println!("{:?}", self);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't print extra things to the screen. Also, would you please add unit tests for this function?

self.group_keys
.iter()
.for_each(|expr| expr.resolve_input_ref(&mut input_refs));
self.group_keys.iter().for_each(|expr| {
Copy link
Member

@skyzh skyzh Apr 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to have input_refs vec here. Just map group_keys to ColumnDesc, and chain it with agg_call is enough.

Copy link
Member

@skyzh skyzh Apr 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The resolve_input_ref is useful in somewhere else, but not here 🤣 I forgot why we wrote such thing here.

@skyzh
Copy link
Member

skyzh commented Apr 13, 2022

btw, you'll need to sign DCO 🤣 git commit --amend -s && git push -f.

@shmiwy shmiwy changed the title fix: make header correct when using group by(#538) fix: make header correct when using group by Apr 13, 2022
@shmiwy
Copy link
Contributor Author

shmiwy commented Apr 13, 2022

btw, you'll need to sign DCO git commit --amend -s && git push -f.

it seems that something terrible when I press this command(Lots of commits come up)🤣🤣🤣🤣

@skyzh
Copy link
Member

skyzh commented Apr 13, 2022

I've rebase updated for you :) You might need to git fetch && git reset --hard origin/bug_fix to make your local branch up-to-date with this remote one.

Copy link
Member

@skyzh skyzh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rest LGTM, good work!

.enumerate()
.map(|(index, expr)| {
ColumnDesc::new(
child_schema[index].datatype().clone(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still doesn't seem correct? we should use expression's return type instead of child schema's type.

@@ -133,6 +133,25 @@ impl BoundExpr {
visitor.visit_expr(self);
visitor.0
}

pub fn format_name(&self, child_schema: &Vec<ColumnDesc>) -> String {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add unit tests for this function :)

JayiceZ and others added 6 commits April 17, 2022 16:24
* use DataChunkbuilder to refactor ValuesExecutor
Signed-off-by: Jayice <1185430411@qq.com>

* code format
Signed-off-by: Jayice <1185430411@qq.com>

* code style
Signed-off-by: Jayice <1185430411@qq.com>
Signed-off-by: Shmiwy <wyf000219@126.com>
Signed-off-by: Shmiwy <wyf000219@126.com>
Signed-off-by: Shmiwy wyf000219@gamil.com
Signed-off-by: Shmiwy <wyf000219@126.com>
Signed-off-by: Shmiwy wyf000219@gamil.com
Signed-off-by: Shmiwy <wyf000219@126.com>
Signed-off-by: TennyZhuang <zty0826@gmail.com>
Signed-off-by: Shmiwy <wyf000219@126.com>
@skyzh skyzh merged commit 8546d0a into risinglightdb:main Apr 17, 2022
@shmiwy shmiwy deleted the bug_fix branch April 18, 2022 05:45
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.

The header of group by is incorrect
4 participants