executor: support firstrow under new aggregation evaluation framework #7057
Conversation
XuHuaiyu
added
type/enhancement
| input, isNull, err := e.args[0].EvalInt(sctx, row) | ||
| if err != nil { | ||
| return errors.Trace(err) | ||
| } |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
executor/aggfuncs/func_first_row.go
Outdated
| p.isNull = true | ||
| break | ||
| } | ||
| p.val = input |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
PTAL @zz-jason @crazycs520 |
executor/aggfuncs/func_first_row.go
Outdated
| return nil | ||
| } | ||
|
|
||
| type firstRow4Uint struct { |
There was a problem hiding this comment.
Do we need this implementation? Can we directly use firstRow4Int?
executor/aggfuncs/func_first_row.go
Outdated
| break | ||
| } | ||
| // We should copy the input value here to avoid the origin value be covered. | ||
| p.val = fmt.Sprintf("%s", input) |
There was a problem hiding this comment.
this way is not efficient to copy a string, we should find another better way.
There was a problem hiding this comment.
https://gist.github.com/lysu/39d4e114632d1790af171ed98ab7d8cb I try to find it too..
executor/aggfuncs/func_first_row.go
Outdated
| package aggfuncs | ||
|
|
||
| import ( | ||
| "fmt" |
There was a problem hiding this comment.
imported and not used: "fmt"
|
Please fix CI first. |
|
/run-all-tests |
|
/run-common-test tidb-test=pr/588 |
|
This PR should be merged with https://github.com/pingcap/tidb-test/pull/588 together |
|
PTAL @zz-jason @crazycs520 |
executor/aggfuncs/func_first_row.go
Outdated
| return errors.Trace(err) | ||
| } | ||
| p.gotFirstRow = true | ||
| if isNull { |
There was a problem hiding this comment.
how about:
p.isNull = isNull
p.val = input
break
crazycs520
added
status/LGT2
|
/run-all-tests |
|
/run-common-test tidb-test=pr/588 |
|
Merge if it's ready @XuHuaiyu |
|
/run-all-tests |
|
/run-common-test tidb-test=pr/588 |
| @@ -36,6 +36,7 @@ var ( | |||
|
|
|||
| // All the AggFunc implementations for "SUM" are listed here. | |||
| // All the AggFunc implementations for "FIRSTROW" are listed here. | |||
|
|
|||
There was a problem hiding this comment.
we should put all the implementations for first_row in this code block 😂
What have you changed? (mandatory)
support first_row under new aggregation evaluation framework
What is the type of the changes? (mandatory)
How has this PR been tested? (mandatory)
The existing test cases.
Does this PR affect documentation (docs/docs-cn) update? (mandatory)
no
Does this PR affect tidb-ansible update? (mandatory)
no
Does this PR need to be added to the release notes? (mandatory)
no
Refer to a related PR or issue link (optional)
#6952
Benchmark result if necessary (optional)
Add a few positive/negative examples (optional)