*: add aggregate functions min_count and max_count#69642
Conversation
ti-chi-bot
Bot
added
release-note
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds ChangesMAX_COUNT/MIN_COUNT aggregate function implementation
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Parser
participant Aggregation
participant Executor
participant Planner
Client->>Parser: Parse MAX_COUNT/MIN_COUNT
Parser->>Aggregation: Build aggregate descriptor
Aggregation->>Planner: Check pushdown and MPP properties
Planner->>Executor: Select aggregation implementation
Executor-->>Client: Return extrema occurrence count
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #69642 +/- ##
================================================
- Coverage 76.3194% 74.0279% -2.2915%
================================================
Files 2041 2056 +15
Lines 560092 578379 +18287
================================================
+ Hits 427459 428162 +703
- Misses 131732 149867 +18135
+ Partials 901 350 -551
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/expression/aggregation/descriptor.go (1)
202-217: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHandle
max_count/min_countas two-argument partial aggregates inpkg/expression/aggregation/descriptor.go
maxMinCountFunction.GetPartialResultreturns[count, value], butSplit()only wires oneColumnin the default branch. In parallel HashAgg, the final phase then treats partial rows as single values and loses the real count. Add a dedicated 2-arg branch likeAVGforAggFuncMaxCount/AggFuncMinCount.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/expression/aggregation/descriptor.go` around lines 202 - 217, The default branch in descriptor assembly is wiring only one Column for max_count/min_count, but maxMinCountFunction.GetPartialResult produces two partial fields ([count, value]); update the Split logic to add a dedicated two-argument partial aggregate path for ast.AggFuncMaxCount and ast.AggFuncMinCount, similar to the AVG handling, and make sure finalAggDesc.Args preserves both partial inputs instead of collapsing them into a single value.
🧹 Nitpick comments (5)
pkg/expression/aggregation/base_func.go (1)
128-133: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: merge case with
AggFuncCount.
ast.AggFuncMaxCount, ast.AggFuncMinCountcould be merged into the samecaseasast.AggFuncCounton line 129 since both calla.typeInfer4Count()with no other logic difference.♻️ Proposed simplification
- case ast.AggFuncCount: - a.typeInfer4Count() - case ast.AggFuncMaxCount, ast.AggFuncMinCount: - a.typeInfer4Count() + case ast.AggFuncCount, ast.AggFuncMaxCount, ast.AggFuncMinCount: + a.typeInfer4Count()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/expression/aggregation/base_func.go` around lines 128 - 133, Merge the redundant aggregation switch branches in baseFunc’s type inference logic: in the switch over a.Name inside the type inference method, combine ast.AggFuncMaxCount and ast.AggFuncMinCount with ast.AggFuncCount since they all call a.typeInfer4Count() and have no unique behavior, keeping the existing ast.AggFuncApproxCountDistinct branch unchanged.pkg/planner/core/plan_test.go (1)
682-721: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding a MIN_COUNT case for symmetry.
Test only exercises
ast.AggFuncMaxCount;ast.AggFuncMinCountshares the same code path inBuildFinalModeAggregation(line 791 checks both), so this isn't strictly required, but a quick parallel case would strengthen coverage of the min branch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/planner/core/plan_test.go` around lines 682 - 721, The test only covers ast.AggFuncMaxCount, but BuildFinalModeAggregation handles ast.AggFuncMinCount on the same path as well. Extend TestBuildFinalModeAggregationMaxMinCountSchema with a parallel MinCount case using the same setup and assertions, so both max and min branches are covered with matching schema and argument type checks.pkg/executor/aggfuncs/spill_helper_test.go (1)
476-524: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winTest doesn't cover the zero-value/zero-size branch or buffer-capacity growth.
Two gaps in this new round-trip test:
- Neither
expectDataentry has a truly zero-valueVectorFloat32(SerializedSize() == 0), so the special-case branch added inSerializeVectorFloat32(pkg/util/serialization/serialization_util.go) is never exercised here.- Unlike the JSON test above it (which asserts
bufSizeChecker.checkBufferCapacity(serializeHelper)after each serialize call), this test never validates buffer capacity — despite using a large 1024-float vector seemingly intended to stress buffer growth.🧪 Suggested additions
expectData := []partialResult4MaxMinVectorFloat32{ {val: types.MustCreateVectorFloat32([]float32{1, 2, 3}), isNull: false}, {val: types.MustCreateVectorFloat32(largeVecData), isNull: true}, + {val: types.VectorFloat32{}, isNull: true}, } ... for _, pr := range serializedPartialResults { serializedData := serializeHelper.serializePartialResult4MaxMinVectorFloat32(*(*partialResult4MaxMinVectorFloat32)(pr)) chk.AppendBytes(0, serializedData) + require.True(t, bufSizeChecker.checkBufferCapacity(serializeHelper)) }Do you want me to draft the full patch, including initializing
bufSizeCheckerconsistently with the JSON test above?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/executor/aggfuncs/spill_helper_test.go` around lines 476 - 524, The round-trip test in TestPartialResult4MaxMinVectorFloat32 is missing coverage for the zero-size VectorFloat32 serialization path and for serialize buffer growth. Update the test data to include a truly zero-value partialResult4MaxMinVectorFloat32 so SerializeVectorFloat32’s SerializedSize() == 0 branch is exercised, and add the same buffer-capacity assertions used by the nearby JSON test by initializing bufSizeChecker and checking it after each serializePartialResult4MaxMinVectorFloat32 call in SerializeHelper.pkg/util/serialization/serialization_util.go (1)
133-139: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the zero-value serialization branch.
VectorFloat32{}has nildata, soZeroCopySerialize()would emit an empty slice; substitutingtypes.ZeroVectorFloat32preserves the canonical 4-byte empty-vector encoding.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/util/serialization/serialization_util.go` around lines 133 - 139, Document and preserve the zero-value branch in SerializeVectorFloat32: when value.SerializedSize() is 0, keep using types.ZeroVectorFloat32 with serializeBuffer instead of value.ZeroCopySerialize(), since VectorFloat32{} has nil data and must serialize to the canonical empty-vector encoding. Make the intent explicit in the SerializeVectorFloat32 function so future changes don’t remove this special case.Source: Coding guidelines
pkg/executor/aggfuncs/func_max_min_count.go (1)
82-152: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftSliding-window deque boxes every value into
any, causing per-row heap allocations.
maxMinCountDequeItem.itemis typedany, andenqueueis called once per row inUpdatePartialResult/Slidefor every sliding-window variant (int64, uint64, float32/64,MyDecimal,types.Time,types.Duration). Boxing these values into an interface allocates on the heap for most values, adding GC pressure on what is effectively a per-row hot path for window function evaluation over large windows.A generic
minMaxCountDeque[T any](or per-type typed slices) would avoid the boxing while keeping the same eviction logic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/executor/aggfuncs/func_max_min_count.go` around lines 82 - 152, The deque hot path in minMaxCountDeque is boxing every value into any, which causes avoidable per-row heap allocations during sliding-window updates. Refactor maxMinCountDequeItem and minMaxCountDeque into a generic typed deque (or equivalent per-type storage) so enqueue, dequeue, and frontCount operate on concrete value types instead of interface values. Keep the same eviction/index tracking behavior, and update the callers in UpdatePartialResult/Slide to use the typed deque for the max/min/count variants.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@pkg/expression/aggregation/descriptor.go`:
- Around line 202-217: The default branch in descriptor assembly is wiring only
one Column for max_count/min_count, but maxMinCountFunction.GetPartialResult
produces two partial fields ([count, value]); update the Split logic to add a
dedicated two-argument partial aggregate path for ast.AggFuncMaxCount and
ast.AggFuncMinCount, similar to the AVG handling, and make sure
finalAggDesc.Args preserves both partial inputs instead of collapsing them into
a single value.
---
Nitpick comments:
In `@pkg/executor/aggfuncs/func_max_min_count.go`:
- Around line 82-152: The deque hot path in minMaxCountDeque is boxing every
value into any, which causes avoidable per-row heap allocations during
sliding-window updates. Refactor maxMinCountDequeItem and minMaxCountDeque into
a generic typed deque (or equivalent per-type storage) so enqueue, dequeue, and
frontCount operate on concrete value types instead of interface values. Keep the
same eviction/index tracking behavior, and update the callers in
UpdatePartialResult/Slide to use the typed deque for the max/min/count variants.
In `@pkg/executor/aggfuncs/spill_helper_test.go`:
- Around line 476-524: The round-trip test in
TestPartialResult4MaxMinVectorFloat32 is missing coverage for the zero-size
VectorFloat32 serialization path and for serialize buffer growth. Update the
test data to include a truly zero-value partialResult4MaxMinVectorFloat32 so
SerializeVectorFloat32’s SerializedSize() == 0 branch is exercised, and add the
same buffer-capacity assertions used by the nearby JSON test by initializing
bufSizeChecker and checking it after each
serializePartialResult4MaxMinVectorFloat32 call in SerializeHelper.
In `@pkg/expression/aggregation/base_func.go`:
- Around line 128-133: Merge the redundant aggregation switch branches in
baseFunc’s type inference logic: in the switch over a.Name inside the type
inference method, combine ast.AggFuncMaxCount and ast.AggFuncMinCount with
ast.AggFuncCount since they all call a.typeInfer4Count() and have no unique
behavior, keeping the existing ast.AggFuncApproxCountDistinct branch unchanged.
In `@pkg/planner/core/plan_test.go`:
- Around line 682-721: The test only covers ast.AggFuncMaxCount, but
BuildFinalModeAggregation handles ast.AggFuncMinCount on the same path as well.
Extend TestBuildFinalModeAggregationMaxMinCountSchema with a parallel MinCount
case using the same setup and assertions, so both max and min branches are
covered with matching schema and argument type checks.
In `@pkg/util/serialization/serialization_util.go`:
- Around line 133-139: Document and preserve the zero-value branch in
SerializeVectorFloat32: when value.SerializedSize() is 0, keep using
types.ZeroVectorFloat32 with serializeBuffer instead of
value.ZeroCopySerialize(), since VectorFloat32{} has nil data and must serialize
to the canonical empty-vector encoding. Make the intent explicit in the
SerializeVectorFloat32 function so future changes don’t remove this special
case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1d9907ac-ae77-427d-94ec-68ed04d508d6
📒 Files selected for processing (27)
pkg/executor/aggfuncs/BUILD.bazelpkg/executor/aggfuncs/aggfuncs.gopkg/executor/aggfuncs/builder.gopkg/executor/aggfuncs/func_max_min.gopkg/executor/aggfuncs/func_max_min_count.gopkg/executor/aggfuncs/func_max_min_count_test.gopkg/executor/aggfuncs/spill_deserialize_helper.gopkg/executor/aggfuncs/spill_helper_test.gopkg/executor/aggfuncs/spill_serialize_helper.gopkg/expression/aggregation/BUILD.bazelpkg/expression/aggregation/aggregation.gopkg/expression/aggregation/aggregation_test.gopkg/expression/aggregation/base_func.gopkg/expression/aggregation/base_func_test.gopkg/expression/aggregation/descriptor.gopkg/expression/aggregation/max_min_count.gopkg/expression/aggregation/window_func.gopkg/parser/ast/functions.gopkg/parser/misc.gopkg/parser/parser.gopkg/parser/parser.ypkg/parser/parser_test.gopkg/planner/core/cost/factors_thresholds.gopkg/planner/core/operator/physicalop/base_physical_agg.gopkg/planner/core/plan_test.gopkg/util/serialization/deserialization_util.gopkg/util/serialization/serialization_util.go
windtalker
force-pushed
the
min_count_max_count
branch
from
898557d to
2839561
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/kv/checker.go (1)
53-57: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider documenting the TiKV restriction for MAX_COUNT/MIN_COUNT.
This list marks
MaxCount/MinCountas generically "supported" expr types, but actual TiKV push-down is disallowed elsewhere (perCheckAggPushDown), same asGroupConcat. The existing comment only calls outGroupConcat's TiFlash-only restriction; a similar note forMaxCount/MinCountwould help future readers avoid assuming TiKV push-down works here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/kv/checker.go` around lines 53 - 57, Update the aggregate-function comment in checker.go so it also documents that tipb.ExprType_MaxCount and tipb.ExprType_MinCount are not valid for TiKV push-down, similar to the existing TiFlash-only note for tipb.ExprType_GroupConcat. Keep the explanation near the switch case in checker.CheckExpr, and make it clear that CheckAggPushDown enforces this TiKV restriction even though these expr types appear in the supported list.pkg/expression/aggregation/aggregation.go (1)
90-105: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting shared MaxCount/MinCount construction logic.
The
MaxCountandMinCountbranches are identical except forast.AggFuncMaxCount/ast.AggFuncMinCountandisMax: true/false. A small helper (e.g.newMaxMinCountFunc(args, mode, isMax bool, ctx) ...) would remove the duplication and reduce the chance the two branches drift apart ifcmpArgIdxlogic is tweaked later.♻️ Proposed refactor
- case tipb.ExprType_MaxCount: - aggF := newAggFunc(ast.AggFuncMaxCount, args, false) - aggF.Mode = AggFunctionMode(*expr.AggFuncMode) - cmpArgIdx := 0 - if (aggF.Mode == FinalMode || aggF.Mode == Partial2Mode) && len(args) > 1 { - cmpArgIdx = 1 - } - return &maxMinCountFunction{aggFunction: aggF, isMax: true, ctor: collate.GetCollator(args[cmpArgIdx].GetType(ctx.GetEvalCtx()).GetCollate())}, aggF.AggFuncDesc, nil - case tipb.ExprType_MinCount: - aggF := newAggFunc(ast.AggFuncMinCount, args, false) - aggF.Mode = AggFunctionMode(*expr.AggFuncMode) - cmpArgIdx := 0 - if (aggF.Mode == FinalMode || aggF.Mode == Partial2Mode) && len(args) > 1 { - cmpArgIdx = 1 - } - return &maxMinCountFunction{aggFunction: aggF, isMax: false, ctor: collate.GetCollator(args[cmpArgIdx].GetType(ctx.GetEvalCtx()).GetCollate())}, aggF.AggFuncDesc, nil + case tipb.ExprType_MaxCount, tipb.ExprType_MinCount: + isMax := expr.Tp == tipb.ExprType_MaxCount + name := ast.AggFuncMinCount + if isMax { + name = ast.AggFuncMaxCount + } + aggF := newAggFunc(name, args, false) + aggF.Mode = AggFunctionMode(*expr.AggFuncMode) + cmpArgIdx := 0 + if (aggF.Mode == FinalMode || aggF.Mode == Partial2Mode) && len(args) > 1 { + cmpArgIdx = 1 + } + return &maxMinCountFunction{aggFunction: aggF, isMax: isMax, ctor: collate.GetCollator(args[cmpArgIdx].GetType(ctx.GetEvalCtx()).GetCollate())}, aggF.AggFuncDesc, nil🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/expression/aggregation/aggregation.go` around lines 90 - 105, The MaxCount and MinCount cases in aggregation.go duplicate the same construction and cmpArgIdx selection logic, so refactor them into a shared helper to keep the behavior in one place. Add a helper around the newAggFunc, AggFunctionMode assignment, cmpArgIdx choice, collate.GetCollator lookup, and maxMinCountFunction creation, with only ast.AggFuncMaxCount vs ast.AggFuncMinCount and isMax differing. Update the tipb.ExprType_MaxCount and tipb.ExprType_MinCount branches to call that helper so future changes to the comparison-argument logic stay consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/expression/aggregation/aggregation.go`:
- Around line 90-105: The MaxCount and MinCount cases in aggregation.go
duplicate the same construction and cmpArgIdx selection logic, so refactor them
into a shared helper to keep the behavior in one place. Add a helper around the
newAggFunc, AggFunctionMode assignment, cmpArgIdx choice, collate.GetCollator
lookup, and maxMinCountFunction creation, with only ast.AggFuncMaxCount vs
ast.AggFuncMinCount and isMax differing. Update the tipb.ExprType_MaxCount and
tipb.ExprType_MinCount branches to call that helper so future changes to the
comparison-argument logic stay consistent.
In `@pkg/kv/checker.go`:
- Around line 53-57: Update the aggregate-function comment in checker.go so it
also documents that tipb.ExprType_MaxCount and tipb.ExprType_MinCount are not
valid for TiKV push-down, similar to the existing TiFlash-only note for
tipb.ExprType_GroupConcat. Keep the explanation near the switch case in
checker.CheckExpr, and make it clear that CheckAggPushDown enforces this TiKV
restriction even though these expr types appear in the supported list.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3a032b35-8cd3-4718-8f06-6817eeeda42c
📒 Files selected for processing (35)
pkg/executor/aggfuncs/BUILD.bazelpkg/executor/aggfuncs/aggfuncs.gopkg/executor/aggfuncs/builder.gopkg/executor/aggfuncs/func_max_min.gopkg/executor/aggfuncs/func_max_min_count.gopkg/executor/aggfuncs/func_max_min_count_test.gopkg/executor/aggfuncs/spill_deserialize_helper.gopkg/executor/aggfuncs/spill_helper_test.gopkg/executor/aggfuncs/spill_serialize_helper.gopkg/expression/aggregation/BUILD.bazelpkg/expression/aggregation/agg_to_pb.gopkg/expression/aggregation/agg_to_pb_test.gopkg/expression/aggregation/aggregation.gopkg/expression/aggregation/aggregation_test.gopkg/expression/aggregation/base_func.gopkg/expression/aggregation/base_func_test.gopkg/expression/aggregation/descriptor.gopkg/expression/aggregation/max_min_count.gopkg/expression/aggregation/window_func.gopkg/kv/checker.gopkg/kv/checker_test.gopkg/parser/ast/functions.gopkg/parser/misc.gopkg/parser/parser.gopkg/parser/parser.ypkg/parser/parser_test.gopkg/planner/core/cost/factors_thresholds.gopkg/planner/core/operator/physicalop/BUILD.bazelpkg/planner/core/operator/physicalop/base_physical_agg.gopkg/planner/core/operator/physicalop/physical_hash_agg.gopkg/planner/core/operator/physicalop/physical_utils_test.gopkg/planner/core/plan_test.gopkg/planner/core/task.gopkg/util/serialization/deserialization_util.gopkg/util/serialization/serialization_util.go
✅ Files skipped from review due to trivial changes (1)
- pkg/parser/ast/functions.go
🚧 Files skipped from review as they are similar to previous changes (21)
- pkg/executor/aggfuncs/BUILD.bazel
- pkg/util/serialization/deserialization_util.go
- pkg/planner/core/cost/factors_thresholds.go
- pkg/parser/misc.go
- pkg/expression/aggregation/BUILD.bazel
- pkg/planner/core/plan_test.go
- pkg/expression/aggregation/descriptor.go
- pkg/executor/aggfuncs/spill_deserialize_helper.go
- pkg/executor/aggfuncs/aggfuncs.go
- pkg/parser/parser_test.go
- pkg/expression/aggregation/base_func_test.go
- pkg/util/serialization/serialization_util.go
- pkg/parser/parser.y
- pkg/executor/aggfuncs/builder.go
- pkg/expression/aggregation/max_min_count.go
- pkg/executor/aggfuncs/spill_helper_test.go
- pkg/executor/aggfuncs/spill_serialize_helper.go
- pkg/executor/aggfuncs/func_max_min.go
- pkg/expression/aggregation/base_func.go
- pkg/executor/aggfuncs/func_max_min_count_test.go
- pkg/executor/aggfuncs/func_max_min_count.go
windtalker
force-pushed
the
min_count_max_count
branch
from
2839561 to
75c73ae
Compare
windtalker
force-pushed
the
min_count_max_count
branch
from
75c73ae to
cd59864
Compare
|
/test check-dev2 |
|
But in default:
args := make([]expression.Expression, 0, 1) // ← only one
...
args = append(args, &expression.Column{
Index: ordinal[0],
RetType: argRetTp,
})
finalAggDesc.Args = argsThis means if However, I searched the entire
The actual two-phase split in production goes through Suggestion: I would still lean toward adding a dedicated branch here (like the existing |
|
/test check-dev2 |
AilinKid
left a comment
AilinKid
left a comment
There was a problem hiding this comment.
/hold pls feel free to unhold when the comment above is scanned
ti-chi-bot
Bot
added
do-not-merge/hold
Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>
windtalker
force-pushed
the
min_count_max_count
branch
from
cd59864 to
27f1dd6
Compare
Good catch. Semantically, the row-based two-phase shape of One clarification: For the real row-based final/partial2 shape, I agree the current code is easy to misread, so I added a comment in |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/executor/aggfuncs/func_max_min_count_test.go (1)
336-359: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSliding-window coverage is int-only.
TestMaxMinCountSlidingWindowonly exercisesmysql.TypeLonglong. Given the string sliding variant uses a distinctcollatefield not exercised here (see companion comment infunc_max_min_count.go), consider adding a string/decimal/time case to this test to catch collation or comparator wiring issues in the sliding path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/executor/aggfuncs/func_max_min_count_test.go` around lines 336 - 359, Extend TestMaxMinCountSlidingWindow beyond integer inputs by adding sliding-window cases for string, decimal, or time values, especially one that exercises collation-sensitive comparison through the max_count/min_count paths. Keep the existing integer assertions and verify expected results for each added type so comparator and collate wiring are covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/executor/aggfuncs/func_max_min_count.go`:
- Around line 26-29: Remove the misleading DefPartialResult4MaxMinCountSize
constant and its compatibility comment from the constant block, after confirming
no external references exist; retain DefPartialResult4MaxMinCountIntSize as the
sole size constant.
- Around line 92-176: The sliding deque’s dynamic backing storage is not
accounted for after allocation. Update allocPartialResult4MaxMinCountSliding and
the deque mutation paths used by UpdatePartialResult and Slide to charge memory
for initial capacity and any growth of d.items (including reallocations), and
return the corresponding delta instead of always returning zero; apply the same
accounting fix to the analogous implementation in func_max_min.go.
---
Nitpick comments:
In `@pkg/executor/aggfuncs/func_max_min_count_test.go`:
- Around line 336-359: Extend TestMaxMinCountSlidingWindow beyond integer inputs
by adding sliding-window cases for string, decimal, or time values, especially
one that exercises collation-sensitive comparison through the
max_count/min_count paths. Keep the existing integer assertions and verify
expected results for each added type so comparator and collate wiring are
covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: be928e81-8ddd-4d43-9f7d-ba9c2724886f
📒 Files selected for processing (35)
pkg/executor/aggfuncs/BUILD.bazelpkg/executor/aggfuncs/aggfuncs.gopkg/executor/aggfuncs/builder.gopkg/executor/aggfuncs/func_max_min.gopkg/executor/aggfuncs/func_max_min_count.gopkg/executor/aggfuncs/func_max_min_count_test.gopkg/executor/aggfuncs/spill_deserialize_helper.gopkg/executor/aggfuncs/spill_helper_test.gopkg/executor/aggfuncs/spill_serialize_helper.gopkg/expression/aggregation/BUILD.bazelpkg/expression/aggregation/agg_to_pb.gopkg/expression/aggregation/agg_to_pb_test.gopkg/expression/aggregation/aggregation.gopkg/expression/aggregation/aggregation_test.gopkg/expression/aggregation/base_func.gopkg/expression/aggregation/base_func_test.gopkg/expression/aggregation/descriptor.gopkg/expression/aggregation/max_min_count.gopkg/expression/aggregation/window_func.gopkg/kv/checker.gopkg/kv/checker_test.gopkg/parser/ast/functions.gopkg/parser/misc.gopkg/parser/parser.gopkg/parser/parser.ypkg/parser/parser_test.gopkg/planner/core/cost/factors_thresholds.gopkg/planner/core/operator/physicalop/BUILD.bazelpkg/planner/core/operator/physicalop/base_physical_agg.gopkg/planner/core/operator/physicalop/physical_hash_agg.gopkg/planner/core/operator/physicalop/physical_utils_test.gopkg/planner/core/plan_test.gopkg/planner/core/task.gopkg/util/serialization/deserialization_util.gopkg/util/serialization/serialization_util.go
🚧 Files skipped from review as they are similar to previous changes (25)
- pkg/expression/aggregation/BUILD.bazel
- pkg/util/serialization/deserialization_util.go
- pkg/planner/core/task.go
- pkg/util/serialization/serialization_util.go
- pkg/executor/aggfuncs/BUILD.bazel
- pkg/executor/aggfuncs/spill_helper_test.go
- pkg/planner/core/cost/factors_thresholds.go
- pkg/expression/aggregation/base_func_test.go
- pkg/kv/checker_test.go
- pkg/expression/aggregation/window_func.go
- pkg/planner/core/operator/physicalop/physical_utils_test.go
- pkg/executor/aggfuncs/func_max_min.go
- pkg/expression/aggregation/agg_to_pb.go
- pkg/parser/ast/functions.go
- pkg/parser/parser.y
- pkg/planner/core/operator/physicalop/base_physical_agg.go
- pkg/kv/checker.go
- pkg/parser/parser_test.go
- pkg/expression/aggregation/aggregation.go
- pkg/expression/aggregation/descriptor.go
- pkg/planner/core/plan_test.go
- pkg/planner/core/operator/physicalop/physical_hash_agg.go
- pkg/expression/aggregation/base_func.go
- pkg/expression/aggregation/aggregation_test.go
- pkg/executor/aggfuncs/spill_deserialize_helper.go
| const ( | ||
| // DefPartialResult4MaxMinCountSize is kept for compatibility. | ||
| DefPartialResult4MaxMinCountSize = int64(unsafe.Sizeof(partialResult4MaxMinCountInt{})) | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Misleading "compatibility" constant in a brand-new file.
DefPartialResult4MaxMinCountSize duplicates DefPartialResult4MaxMinCountIntSize and the comment claims it's "kept for compatibility," but this is a new file with no prior version to be compatible with. This will confuse future readers into thinking there's a legacy contract to preserve.
🧹 Proposed fix
const (
- // DefPartialResult4MaxMinCountSize is kept for compatibility.
- DefPartialResult4MaxMinCountSize = int64(unsafe.Sizeof(partialResult4MaxMinCountInt{}))
-
// DefPartialResult4MaxMinCountIntSize is the size of partialResult4MaxMinCountInt.
DefPartialResult4MaxMinCountIntSize = int64(unsafe.Sizeof(partialResult4MaxMinCountInt{}))Confirm nothing outside this file references the unsuffixed name before removing:
#!/bin/bash
rg -n '\bDefPartialResult4MaxMinCountSize\b'Based on coding guidelines: "Comments SHOULD explain non-obvious intent... SHOULD NOT restate what the code already makes clear" — this comment does worse than restate, it actively misleads.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/executor/aggfuncs/func_max_min_count.go` around lines 26 - 29, Remove the
misleading DefPartialResult4MaxMinCountSize constant and its compatibility
comment from the constant block, after confirming no external references exist;
retain DefPartialResult4MaxMinCountIntSize as the sole size constant.
Source: Coding guidelines
| type maxMinCountDequeItem struct { | ||
| item any | ||
| idxs []uint64 | ||
| } | ||
|
|
||
| type minMaxCountDeque struct { | ||
| items []maxMinCountDequeItem | ||
| isMax bool | ||
| cmpFunc func(i, j any) int | ||
| } | ||
|
|
||
| func newMinMaxCountDeque(isMax bool, cmpFunc func(i, j any) int) *minMaxCountDeque { | ||
| return &minMaxCountDeque{ | ||
| items: make([]maxMinCountDequeItem, 0, 64), | ||
| isMax: isMax, | ||
| cmpFunc: cmpFunc, | ||
| } | ||
| } | ||
|
|
||
| func (d *minMaxCountDeque) reset() { | ||
| d.items = d.items[:0] | ||
| } | ||
|
|
||
| func (d *minMaxCountDeque) isEmpty() bool { | ||
| return len(d.items) == 0 | ||
| } | ||
|
|
||
| func (d *minMaxCountDeque) enqueue(idx uint64, item any) { | ||
| for !d.isEmpty() { | ||
| backIdx := len(d.items) - 1 | ||
| cmp := d.cmpFunc(item, d.items[backIdx].item) | ||
| if (d.isMax && cmp > 0) || (!d.isMax && cmp < 0) { | ||
| d.items = d.items[:backIdx] | ||
| continue | ||
| } | ||
| if cmp == 0 { | ||
| d.items[backIdx].idxs = append(d.items[backIdx].idxs, idx) | ||
| return | ||
| } | ||
| break | ||
| } | ||
| d.items = append(d.items, maxMinCountDequeItem{ | ||
| item: item, | ||
| idxs: []uint64{idx}, | ||
| }) | ||
| } | ||
|
|
||
| func (d *minMaxCountDeque) dequeue(boundary uint64) { | ||
| for !d.isEmpty() { | ||
| front := &d.items[0] | ||
| firstAlive := 0 | ||
| for firstAlive < len(front.idxs) && front.idxs[firstAlive] <= boundary { | ||
| firstAlive++ | ||
| } | ||
| if firstAlive == 0 { | ||
| return | ||
| } | ||
| front.idxs = front.idxs[firstAlive:] | ||
| if len(front.idxs) > 0 { | ||
| return | ||
| } | ||
| d.items = d.items[1:] | ||
| } | ||
| } | ||
|
|
||
| func (d *minMaxCountDeque) frontCount() int { | ||
| if d.isEmpty() { | ||
| return 0 | ||
| } | ||
| return len(d.items[0].idxs) | ||
| } | ||
|
|
||
| type partialResult4MaxMinCountSliding struct { | ||
| deque *minMaxCountDeque | ||
| count int64 | ||
| isNull bool | ||
| } | ||
|
|
||
| func allocPartialResult4MaxMinCountSliding(isMax bool, cmp func(i, j any) int) (PartialResult, int64) { | ||
| p := &partialResult4MaxMinCountSliding{ | ||
| deque: newMinMaxCountDeque(isMax, cmp), | ||
| isNull: true, | ||
| } | ||
| return PartialResult(p), DefPartialResult4MaxMinCountSlidingSize + DefMaxMinCountDequeSize | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== File list ==\n'
git ls-files 'pkg/executor/aggfuncs/*max*min*count*' 'pkg/executor/aggfuncs/*max*min*' | sed -n '1,120p'
printf '\n== Outline: func_max_min_count.go ==\n'
ast-grep outline pkg/executor/aggfuncs/func_max_min_count.go --view expanded || true
printf '\n== Outline: func_max_min.go ==\n'
ast-grep outline pkg/executor/aggfuncs/func_max_min.go --view expanded || true
printf '\n== Search for deque/memory accounting references ==\n'
rg -n "DefMaxMinCountDequeSize|DefPartialResult4MaxMinCountSlidingSize|memDelta|UpdatePartialResult|Slide|newMinMaxCountDeque|make\\(\\[\\]maxMinCountDequeItem|maxMinCountDeque" pkg/executor/aggfuncs/func_max_min_count.go pkg/executor/aggfuncs/func_max_min.go
printf '\n== Relevant slices: func_max_min_count.go ==\n'
sed -n '1,260p' pkg/executor/aggfuncs/func_max_min_count.go
printf '\n== Relevant slices: func_max_min.go ==\n'
sed -n '1,320p' pkg/executor/aggfuncs/func_max_min.goRepository: pingcap/tidb
Length of output: 50368
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== func_max_min_count.go: deque + sliding methods ==\n'
sed -n '87,260p' pkg/executor/aggfuncs/func_max_min_count.go
printf '\n== func_max_min.go: deque + sliding methods for int/uint/float32/float64/decimal/string ==\n'
sed -n '166,380p' pkg/executor/aggfuncs/func_max_min.go
sed -n '490,540p' pkg/executor/aggfuncs/func_max_min.go
sed -n '650,700p' pkg/executor/aggfuncs/func_max_min.go
sed -n '808,860p' pkg/executor/aggfuncs/func_max_min.go
sed -n '992,1045p' pkg/executor/aggfuncs/func_max_min.go
sed -n '1158,1210p' pkg/executor/aggfuncs/func_max_min.go
printf '\n== Search for any memory accounting helpers mentioning deque growth or sampling ==\n'
rg -n "sampling|Sample|track.*mem|memory.*track|MemDelta|MemTracker|unsafe.Sizeof\\(Pair|unsafe.Sizeof\\(maxMinCountDequeItem|DefMaxMinCountDequeSize|DefMaxMinDequeSize" pkg/executor/aggfuncs pkg/util pkg/executor | sed -n '1,200p'Repository: pingcap/tidb
Length of output: 38892
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== func_max_min.go: constructor + deque internals ==\n'
sed -n '1,140p' pkg/executor/aggfuncs/func_max_min.go
printf '\n== func_max_min_count.go: constructor + any constants around allocation ==\n'
sed -n '1,120p' pkg/executor/aggfuncs/func_max_min_count.goRepository: pingcap/tidb
Length of output: 8808
Sliding deque memory growth is still untracked. allocPartialResult4MaxMinCountSliding only charges the struct plus DefMaxMinCountDequeSize, but newMinMaxCountDeque preallocates 64 entries and enqueue can keep growing d.items as the frame expands. UpdatePartialResult and Slide both return 0 after mutating the deque, so large partitions can bypass the memory tracker; func_max_min.go has the same gap.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/executor/aggfuncs/func_max_min_count.go` around lines 92 - 176, The
sliding deque’s dynamic backing storage is not accounted for after allocation.
Update allocPartialResult4MaxMinCountSliding and the deque mutation paths used
by UpdatePartialResult and Slide to charge memory for initial capacity and any
growth of d.items (including reallocations), and return the corresponding delta
instead of always returning zero; apply the same accounting fix to the analogous
implementation in func_max_min.go.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: AilinKid, gengliqi 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 |
3 participants
What problem does this PR solve?
Issue Number: close #66124
Problem Summary:
Materialized view incremental refresh needs helper aggregate functions that can count how many rows match the current minimum or maximum value in a group.
What changed and how does it work?
This PR adds two aggregate functions:
max_count(expr)returns the number of non-NULL rows whose value equals the maximum non-NULL value ofexpr.min_count(expr)returns the number of non-NULL rows whose value equals the minimum non-NULL value ofexpr.The implementation includes:
MAX_COUNT(expr),MAX_COUNT(ALL expr),MIN_COUNT(expr), andMIN_COUNT(ALL expr).DISTINCTis intentionally not supported formax_countormin_count; the parser rejectsmax_count(distinct ...)andmin_count(distinct ...).Check List
Tests
Unit tests:
go test ./pkg/executor/aggfuncs -run 'Test(MaxMinCount|MergePartialResult4MaxMinCount|MemMaxMinCount)' -tags=intest,deadlock -count=1./tools/check/failpoint-go-test.sh pkg/expression/aggregation -run 'Test(MaxMinCount|BaseFunc_InferMaxMinCountRetType)' -count=1./tools/check/failpoint-go-test.sh pkg/planner/core -run 'TestBuildFinalModeAggregation(MaxMinCountSchema)?$' -count=1go test . -run '^TestBuiltin$' -tags=intest,deadlock -count=1inpkg/parsermake bazel_preparemake bazel_lint_changedSide effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
MAX_COUNTandMIN_COUNTaggregates end-to-end, including SQL parsing/validation, partial/final execution, collation-aware string handling, and sliding-window support across many data types (including vector float32).max_count/min_countrow-based final aggregation is explicitly unsupported.