[feat](aggregate) support FIRST aggregate type#59017
Closed
AntiTopQuark wants to merge 1 commit intoapache:masterfrom
Closed
[feat](aggregate) support FIRST aggregate type#59017AntiTopQuark wants to merge 1 commit intoapache:masterfrom
AntiTopQuark wants to merge 1 commit intoapache:masterfrom
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
AntiTopQuark
force-pushed
the
support_first
branch
from
5fa4308 to
f27976f
Compare
Author
|
run buildall |
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE UT Coverage ReportIncrement line coverage |
TPC-H: Total hot run time: 35104 ms |
TPC-DS: Total hot run time: 179139 ms |
ClickBench: Total hot run time: 28.45 s |
Signed-off-by: AntiTopQuark <AntiTopQuark1350@outlook.com>
AntiTopQuark
force-pushed
the
support_first
branch
from
f27976f to
57a351f
Compare
Author
|
run buildall |
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
TPC-H: Total hot run time: 35773 ms |
TPC-DS: Total hot run time: 178241 ms |
ClickBench: Total hot run time: 28.31 s |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE Regression Coverage ReportIncrement line coverage |
There was a problem hiding this comment.
Pull request overview
This PR adds support for the FIRST aggregate type to Apache Doris. The FIRST aggregate function keeps the first value inserted for a column, complementing the existing REPLACE aggregate type which keeps the last value. This is implemented as part of the "replace family" of aggregation types.
Key Changes:
- Added FIRST as a new aggregate type across all system layers (Thrift definitions, Java enums, C++ enums)
- Implemented FIRST aggregate function with separate reader and load phases following existing patterns
- Added FIRST to the "replace family" alongside REPLACE and REPLACE_IF_NOT_NULL for consistent handling in schema operations
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| gensrc/thrift/Types.thrift | Added FIRST = 9 to TAggregationType enum |
| fe/fe-core/src/main/java/org/apache/doris/catalog/AggregateType.java | Added FIRST enum value, registered in aggTypeMap, added to isReplaceFamily(), compatibility map, and toThrift() conversion |
| fe/fe-core/src/main/antlr4/org/apache/doris/nereids/DorisParser.g4 | Added FIRST to aggTypeDef grammar rule |
| fe/fe-core/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java | Added FIRST checks to prevent dropping key columns when FIRST value columns exist |
| be/src/olap/olap_common.h | Added OLAP_FIELD_AGGREGATION_FIRST = 11 to FieldAggregationMethod enum |
| be/src/olap/tablet_schema.cpp | Added string conversion support for FIRST aggregation type |
| be/src/olap/field.h | Added FIRST case to field factory aggregation method handling |
| be/src/olap/memtable.cpp | Added logic to select first_load function for FIRST aggregation in memtable |
| be/src/vec/aggregate_functions/aggregate_function_reader.h | Added function declaration for register_aggregate_function_first_reader_load |
| be/src/vec/aggregate_functions/aggregate_function_reader.cpp | Implemented FIRST aggregate function registration for both reader and load phases |
| be/src/vec/aggregate_functions/aggregate_function_simple_factory.cpp | Registered FIRST reader/load functions in factory |
| regression-test/suites/query_p0/aggregate/aggregate.groovy | Added end-to-end test validating FIRST keeps first value while REPLACE keeps last value |
| fe/fe-core/src/test/java/org/apache/doris/catalog/AggregateTypeTest.java | Added unit tests for FIRST type conversions and replace family membership |
| fe/fe-core/src/test/java/org/apache/doris/nereids/trees/plans/CreateTableCommandTest.java | Added test for creating aggregate table with FIRST column |
| fe/fe-core/src/test/java/org/apache/doris/alter/SchemaChangeHandlerTest.java | Added test verifying key column drop is forbidden when FIRST columns exist |
| be/test/vec/aggregate_functions/aggregate_function_first_test.cpp | Added unit tests for FIRST aggregate function factory and load semantics |
| be/test/olap/tablet_schema_test.cpp | Added tests for FIRST aggregation type string conversions and thrift serialization |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
349
to
+351
| } else if (AggregateType.REPLACE == column.getAggregationType() | ||
| || AggregateType.REPLACE_IF_NOT_NULL == column.getAggregationType()) { | ||
| || AggregateType.REPLACE_IF_NOT_NULL == column.getAggregationType() | ||
| || AggregateType.FIRST == column.getAggregationType()) { |
There was a problem hiding this comment.
Consider using the isReplaceFamily() method instead of explicitly checking each aggregate type. This would make the code more maintainable and consistent with how the replace family is identified elsewhere. Replace the condition with: column.getAggregationType().isReplaceFamily()
Comment on lines
369
to
+371
| } else if (AggregateType.REPLACE == column.getAggregationType() | ||
| || AggregateType.REPLACE_IF_NOT_NULL == column.getAggregationType()) { | ||
| || AggregateType.REPLACE_IF_NOT_NULL == column.getAggregationType() | ||
| || AggregateType.FIRST == column.getAggregationType()) { |
There was a problem hiding this comment.
Consider using the isReplaceFamily() method instead of explicitly checking each aggregate type. This would make the code more maintainable and consistent with how the replace family is identified elsewhere. Replace the condition with: column.getAggregationType().isReplaceFamily()
zclllyybb
reviewed
Dec 15, 2025
Contributor
There was a problem hiding this comment.
for adding a new aggregate type, we need comprehensive test coverage. like schema change, insert, streamload and so on. maybe you need to reference other testcases about aggregate table. then make sure FIRST is coveraged well
Author
|
Hi, @zclllyybb Naming the syntax directly as FIRST conflicts with the existing MySQL-style syntax Would it be possible to implement the keyword as |
Contributor
It looks fine for me~ |
3 tasks
Contributor
|
by https://doris.apache.org/zh-CN/docs/dev/table-design/data-model/aggregate#agg_state we can implement this~ |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: close #58204
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)