*: add aggregate function sum_int #66085
*: add aggregate function sum_int #66085ti-chi-bot[bot] merged 9 commits intopingcap:feature/release-8.5-materialized-viewfrom
sum_int #66085Conversation
|
Hi @windtalker. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feature/release-8.5-materialized-view #66085 +/- ##
==========================================================================
Coverage ? 57.2771%
==========================================================================
Files ? 1787
Lines ? 639677
Branches ? 0
==========================================================================
Hits ? 366389
Misses ? 248704
Partials ? 24584
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This pull request adds a new aggregate function sum_int to TiDB. The new function is designed to sum integer values and return an integer result (TypeLonglong), as opposed to the regular sum function which returns decimal or double results for integer inputs.
Changes:
- Added parser support for the
SUM_INTSQL function - Implemented core aggregation logic for signed and unsigned integers with distinct support
- Added executor-level implementation with sliding window support and spill/serialization capabilities
- Updated aggregation descriptors and metadata to include
sum_intin relevant operations - Explicitly disabled push-down to storage engines for this function
- Added unit and integration tests
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/parser/parser.y | Added grammar rule for SUM_INT function |
| pkg/parser/misc.go | Added token mapping for SUM_INT keyword |
| pkg/parser/ast/functions.go | Added constant for AggFuncSumInt |
| pkg/expression/aggregation/sum_int.go | Core aggregation logic implementation |
| pkg/expression/aggregation/descriptor.go | Updated to handle sum_int in aggregation operations |
| pkg/expression/aggregation/base_func.go | Added type inference and validation for sum_int |
| pkg/expression/aggregation/aggregation.go | Added sum_int to helper functions and disabled push-down |
| pkg/expression/aggregation/BUILD.bazel | Added sum_int.go to build configuration |
| pkg/executor/aggfuncs/func_sum_int.go | Executor implementation with sliding window and spill support |
| pkg/executor/aggfuncs/builder.go | Added builder function for sum_int |
| pkg/executor/aggfuncs/BUILD.bazel | Added func_sum_int.go to build configuration |
| pkg/executor/aggfuncs/spill_serialize_helper.go | Added serialization for sum_int partial results |
| pkg/executor/aggfuncs/spill_deserialize_helper.go | Added deserialization for sum_int partial results |
| pkg/executor/aggfuncs/func_sum_test.go | Added unit tests for sum_int |
| pkg/executor/test/aggregate/aggregate_test.go | Added integration test for sum_int |
| @@ -0,0 +1,88 @@ | |||
| // Copyright 2017 PingCAP, Inc. | |||
There was a problem hiding this comment.
The copyright year should be 2025 for this new file, not 2017. This appears to have been copied from an existing file template without updating the year.
3c71d2f to
4cfcf7d
Compare
| for i := uint64(0); i < shiftEnd; i++ { | ||
| input, isNull, err := e.args[0].EvalInt(sctx, getRow(lastEnd+i)) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if isNull { | ||
| continue | ||
| } | ||
| sum, err := types.AddInt64(p.val, input) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| p.val = sum | ||
| p.notNullRowCount++ | ||
| } | ||
| for i := uint64(0); i < shiftStart; i++ { | ||
| input, isNull, err := e.args[0].EvalInt(sctx, getRow(lastStart+i)) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if isNull { | ||
| continue | ||
| } | ||
| sum, err := types.SubInt64(p.val, input) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| p.val = sum | ||
| p.notNullRowCount-- | ||
| } |
There was a problem hiding this comment.
I think it's better to move the sub before the add to avoid the overflow issue?
There was a problem hiding this comment.
Yes, I've update the code to sub fist
| for i := uint64(0); i < shiftEnd; i++ { | ||
| input, isNull, err := e.args[0].EvalInt(sctx, getRow(lastEnd+i)) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if isNull { | ||
| continue | ||
| } | ||
| sum, err := types.AddUint64(p.val, uint64(input)) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| p.val = sum | ||
| p.notNullRowCount++ | ||
| } | ||
| for i := uint64(0); i < shiftStart; i++ { | ||
| input, isNull, err := e.args[0].EvalInt(sctx, getRow(lastStart+i)) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if isNull { | ||
| continue | ||
| } | ||
| sum, err := types.SubUint64(p.val, uint64(input)) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| p.val = sum | ||
| p.notNullRowCount-- | ||
| } |
|
@gengliqi: adding LGTM is restricted to approvers and reviewers in OWNERS files. DetailsIn response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>
4cfcf7d to
222a32d
Compare
|
/test build |
|
/test unit-test |
|
/test check-dev |
|
/test check-dev2 |
|
@windtalker: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@windtalker: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@windtalker: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@windtalker: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/ok-to-test |
|
/test fast_test_tiprow_for_release |
|
@windtalker: The specified target(s) for The following commands are available to trigger optional jobs: Use DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: gengliqi, wshwsh12, xzhangxian1008 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
[LGTM Timeline notifier]Timeline:
|
|
/retest |
|
@windtalker: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
6ac0a2f
into
pingcap:feature/release-8.5-materialized-view
What problem does this PR solve?
Issue Number: ref #66124
Problem Summary:
What changed and how does it work?
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.