-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
executor: refine StreamAggExec when child is empty #7002
Conversation
…gg_defaultval Conflicts: executor/aggregate.go
add a test that uses the new framework? |
/run-all-tests |
LGTM |
PTAL @winoros |
@XuHuaiyu sqllogical test failed. |
/run-common-test |
return errors.Trace(err) | ||
} | ||
if !e.isChildReturnEmpty { | ||
err = e.appendResult2Chunk(chk) |
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 don't check the value of err
here? If you have any particular reason to do so, is it better you add it here?
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.
err is checked in line 892
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.
got it.
@XuHuaiyu any update? |
…gg_defaultval :q!
… into streamagg_defaultval :q!
/run-all-tests |
1 similar comment
/run-all-tests |
/run-all-tests |
expression/aggregation/descriptor.go
Outdated
// According to MySQL, DefaultValue of the aggregation function can be tested as the following sql: | ||
// | ||
// e.g. | ||
// mysql> create table t(a int); |
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.
Can we simplify the comment? This format is not very formal 😂
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.
ok, I'll have a try
… into streamagg_defaultval :Q!
expression/aggregation/descriptor.go
Outdated
// +------+--------+--------+----------+------------+-----------+----------------------+--------+--------+-----------------+ | ||
// | NULL | NULL | NULL | 0 | 0 | 0 | 18446744073709551615 | NULL | NULL | NULL | | ||
// +------+--------+--------+----------+------------+-----------+----------------------+--------+--------+-----------------+ | ||
func (a *AggFuncDesc) GetDefaultValue() (v types.Datum) { |
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.
Can this function be moved to the Aggregation
interface, it seems like belonging to the execution layer.
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.
LGTM
PTAL @winoros |
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.
LGTM
What have you changed? (mandatory)
This PR refines the StreamAgg process when ChildExec returns empty.
This processing logic can be used both in the old and new evaluation framework.
What are the type of the changes (mandatory)?
The currently defined types are listed below, please pick one of the types for this PR by removing the others:
How has this PR been tested (mandatory)?
the existing tests
Does this PR affect documentation (docs/docs-cn) update? (optional)
no
Refer to a related PR or issue link (optional)
#6952
Benchmark result if necessary (optional)
Add a few positive/negative examples (optional)