Revert "[feat](storage) add read_null_map support for COUNT_NULL push down aggregate (#60996)#61439
Open
HappenLee wants to merge 1 commit intoapache:masterfrom
Open
Revert "[feat](storage) add read_null_map support for COUNT_NULL push down aggregate (#60996)#61439HappenLee wants to merge 1 commit intoapache:masterfrom
HappenLee wants to merge 1 commit intoapache:masterfrom
Conversation
… down aggregate (apache#60996)" This reverts commit 03a9b93.
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
There was a problem hiding this comment.
Pull request overview
Reverts the previously added COUNT_NULL aggregate pushdown path (which required reading null-map pages from segments) due to performance concerns, restoring the prior pushdown behavior and aligning regression expectations.
Changes:
- Removes
COUNT_NULLfrom FE/BE pushdown aggregate enums and translator mappings. - Deletes BE
read_null_mapplumbing and theCOUNT_NULL-specific statistics-iterator logic. - Updates/removes regression tests that asserted
COUNT_NULLpushdown behavior (including explain expectations).
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| regression-test/suites/query_p0/aggregate/count_null_push_down.groovy | Removes regression suite that validated COUNT_NULL pushdown results. |
| regression-test/suites/nereids_p0/explain/test_pushdown_explain.groovy | Updates explain assertion to expect no pushdown (pushAggOp=NONE) for count(nullable_col) cases. |
| regression-test/data/query_p0/aggregate/count_null_push_down.out | Removes expected output corresponding to the deleted regression suite. |
| gensrc/thrift/PlanNodes.thrift | Removes COUNT_NULL from TPushAggOp. |
| fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/physical/PhysicalStorageLayerAggregate.java | Drops COUNT_NULL from FE physical pushdown op enum. |
| fe/fe-core/src/main/java/org/apache/doris/nereids/rules/implementation/AggregateStrategies.java | Removes FE rule logic that selected COUNT_NULL; blocks pushdown for count(nullable_col) instead. |
| fe/fe-core/src/main/java/org/apache/doris/nereids/glue/translator/PhysicalPlanTranslator.java | Removes translation from FE COUNT_NULL to Thrift TPushAggOp. |
| be/src/storage/segment/variant/variant_column_reader.h | Removes forwarding read_null_map implementation from variant iterator. |
| be/src/storage/segment/column_reader.h | Removes read_null_map API from ColumnIterator and related iterator overrides. |
| be/src/storage/segment/column_reader.cpp | Deletes implementations of read_null_map for column iterators. |
| be/src/storage/iterator/vgeneric_iterators.h | Removes unused _tablet_schema member tied to COUNT_NULL handling. |
| be/src/storage/iterator/vgeneric_iterators.cpp | Removes COUNT_NULL initialization and null-map reading logic in VStatisticsIterator. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| MIX = 3, | ||
| COUNT_ON_INDEX = 4, | ||
| COUNT_NULL = 5 | ||
| COUNT_ON_INDEX = 4 |
Comment on lines
72
to
76
| size_t size = _push_down_agg_type_opt == TPushAggOp::MINMAX | ||
| ? 2 | ||
| : std::min(_target_rows - _output_rows, MAX_ROW_SIZE_IN_COUNT); | ||
| if (_push_down_agg_type_opt == TPushAggOp::COUNT) { | ||
| for (auto& column : columns) { |
Contributor
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE UT Coverage ReportIncrement line coverage |
TPC-H: Total hot run time: 26789 ms |
TPC-DS: Total hot run time: 168433 ms |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
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
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.
This reverts commit 03a9b93. will do a new way to fix the performance problem
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)