[fix](fd) Guard uniform aggregate inference by participation - #67881
Merged
morrySnow merged 1 commit intoSep 14, 2026
Merged
Conversation
morrySnow
requested review from
924060929,
englefly and
starocean999
as code owners
September 11, 2026 16:58
Contributor
Author
|
run buildall |
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: COUNT and NDV outputs were inferred as uniform for single-row groups solely from aggregate shape. When an argument is nullable, row participation can differ by group, so downstream GROUP BY elimination can merge distinct results and change query output. Share a conservative participation proof between logical and physical aggregates: COUNT(*) is safe, while argument-based COUNT and NDV require every complete argument expression to be definitely non-null.
### Release note
Prevent unsafe GROUP BY elimination for aggregates whose row participation can vary.
### Check List (For Author)
- Test: Unit tests, regression test, and FE build
- `UniformTest` (12 tests passed)
- `eliminate_group_by_key_by_uniform` regression suite
- `DISABLE_BUILD_UI=ON ./build.sh --fe`
- Behavior changed: Yes. Uniform traits are no longer inferred when nullable aggregate arguments can change participation.
- Does this need documentation: No
morrySnow
force-pushed
the
fix/uniform-aggregate-participation
branch
from
September 11, 2026 19:21
65b831b to
8b38b29
Compare
Contributor
Author
|
run buildall |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 16710 ms |
Contributor
TPC-DS: Total hot run time: 81336 ms |
Contributor
ClickBench: Total hot run time: 14.66 s |
8 tasks
morrySnow
changed the title
hello-stephen
pushed a commit
that referenced
this pull request
Sep 12, 2026
…67897) Since today every `Doris_DorisCloudRegression_VaultP0` run dies in the `run` step before executing a single test, e.g. #67883 (TeamCity build 39010) and #67881 / #67882 / #67885 / #67886 / #67892 / #67893: ``` doris-external--minio Pulling doris-external--minio Error Error response from daemon: pull access denied for minio/minio, repository does not exist or may require 'docker login': denied: requested access to the resource is denied ERROR: start minio docker twice failed ``` MinIO stopped publishing container images in October 2025 (the project is a source-only distribution now, see minio/minio#21647) and the `minio/minio` and `minio/mc` repositories have since been removed from Docker Hub altogether (`https://hub.docker.com/v2/repositories/minio/minio/` answers 404, same for `minio/mc`). The few VaultP0 runs that still pass do so only on agents that have the image cached locally (their logs have no `Pulling` line). The iceberg, hudi and polaris third-party fixtures, `test_file_cache_warmup_read_metrics_docker` (which runs a `docker run minio/minio` itself), the all-in-one `cloud.yml` and the datalake samples reference the same images and are one cache eviction away from the same failure. `quay.io/minio/minio` and `quay.io/minio/mc` still serve every tag we use -- `RELEASE.2024-11-07T00-52-20Z`, `RELEASE.2025-01-20T14-49-07Z`, mc `RELEASE.2025-01-17T23-25-50Z`, the two 2022 tags of the samples and `latest` -- and MinIO keeps pushing hotfix tags there (latest one dated 2026-04). `docker manifest inspect` resolves all of them (amd64 / arm64 / ppc64le). So every reference gets the `quay.io/` prefix and the tags stay exactly as they were: same builds, different registry. The CI agents already pull from quay.io for the OceanBase fixture. A longer-term option is to mirror these three tags into the project's own `doristhirdpartydocker` namespace, which already hosts hive / zookeeper / kafka / trinodb; that needs someone with push access to that Docker Hub organization and can follow separately.
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review summary
Requesting changes for one physical-path issue. The logical fix is sound: the new participation check correctly keeps nullable COUNT/NDV outputs from being treated as uniform on singleton groups, while preserving the safe star and all-non-null cases. The new regression reaches the outer group-key-elimination consumer and its ordered results are pre-fix-sensitive.
The corresponding physical change is ineffective in normal implemented plans because aggregate outputs are wrapped in AggregateExpression; the helper only accepts direct Count/Ndv. Physical logical properties are reset and recomputed after implementation, so this is a reachable metadata path, although I found no demonstrated wrong result or current plan-optimization impact from the conservative false result. Any fix must be phase-aware: only an individual INPUT_TO_RESULT expression generically has the original singleton-row guarantee; buffer-producing and buffer-consuming modes do not.
Scope and checkpoints: all six changed files and the logical-to-physical lifecycle were reviewed, including aggregate normalization, nullability through casts/conditionals/outer joins, multi-argument distinct forms, one- through four-phase splits, trait reset/consumers, and test oracles. The patch introduces no applicable concurrency, resource-lifecycle, persistence, configuration/protocol compatibility, security, FE/BE boundary, or observability concern. The helper adds only a bounded scan over aggregate arguments, with no material performance cost. No additional user focus was provided.
This was a static-only review under the supplied contract; I did not run builds or tests. At submission time, compile, CheckStyle, FE UT, P0 regression, cloud_p0, and performance checks pass. FE incremental coverage fails at 85.71% (6/7), with this physical line as the sole uncovered changed FE line. vault_p0 also currently fails, but its logs were unavailable, so I am not attributing that failure to this patch.
Review completion: two bounded rounds completed. The second-round full and risk-focused reviewers both returned NO_NEW_VALUABLE_FINDINGS; every candidate was adjudicated and duplicate-fenced, leaving only the inline issue below.
github-actions Bot
pushed a commit
that referenced
this pull request
Sep 18, 2026
## Problem
When an aggregate groups by a unique, non-null key, each group contains
one row. The planner used that fact to mark every `COUNT` and `NDV`
output as uniform. That is not true for nullable arguments: `COUNT(v)`
and `NDV(v)` return `0` for a null value and `1` for a non-null value.
An outer aggregation can consequently remove such an output from its
group keys and merge rows that must remain separate.
## Root cause
The logical and physical aggregate trait derivations classified an
output as uniform solely from the aggregate function class. They did not
distinguish `COUNT(*)` from argument-based aggregates or check whether
the complete argument expressions always participate in the aggregate.
## Reproduction
Create a unique-key table containing two rows whose nullable value
differs:
```sql
create table uniform_agg_witness (
pk int not null,
b int not null,
v int null
) unique key(pk)
distributed by hash(pk) buckets 1
properties("replication_num"="1");
insert into uniform_agg_witness values (1, 7, null), (2, 7, 9);
select b, c, count(*) as n, sum(h) as sh
from (
select pk, b, count(v) as c, ndv(v) as h
from uniform_agg_witness
group by pk, b
) s
group by b, c
order by b, c;
```
The invalid uniform trait removed `c` from the outer group keys and
produced one merged row. The correct result has separate `(7, 0)` and
`(7, 1)` groups.
## Fix
- Share one uniform-aggregate proof between logical and physical
aggregate plans.
- Keep `COUNT(*)` uniform for a single-row group.
- Treat argument-based `COUNT` and `NDV` as uniform only when every
complete argument expression is definitely non-null.
- Default all other cases to non-uniform. This conservatively rejects
nullable arguments, nullable conditional expressions, narrowing and try
casts whose result may be null, multi-argument counts with any nullable
argument, and null-extended outer-join outputs.
- Preserve the safe optimization for non-null arguments.
## Tests
- `./run-fe-ut.sh --run org.apache.doris.nereids.properties.UniformTest`
(12 tests passed)
- `./build.sh --fe`
- `./run-regression-test.sh --run -f
regression-test/suites/nereids_rules_p0/eliminate_gby_key/eliminate_group_by_key_by_uniform.groovy
... -forceGenOut`
- Re-ran the same regression suite normally against the generated
expected output.
The regression asserts both results and plan group keys: nullable
`COUNT`/`NDV` remain in the outer grouping, while non-null `COUNT` still
permits safe group-key elimination.
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.
Problem
When an aggregate groups by a unique, non-null key, each group contains one row. The planner used that fact to mark every
COUNTandNDVoutput as uniform. That is not true for nullable arguments:COUNT(v)andNDV(v)return0for a null value and1for a non-null value. An outer aggregation can consequently remove such an output from its group keys and merge rows that must remain separate.Root cause
The logical and physical aggregate trait derivations classified an output as uniform solely from the aggregate function class. They did not distinguish
COUNT(*)from argument-based aggregates or check whether the complete argument expressions always participate in the aggregate.Reproduction
Create a unique-key table containing two rows whose nullable value differs:
The invalid uniform trait removed
cfrom the outer group keys and produced one merged row. The correct result has separate(7, 0)and(7, 1)groups.Fix
COUNT(*)uniform for a single-row group.COUNTandNDVas uniform only when every complete argument expression is definitely non-null.Tests
./run-fe-ut.sh --run org.apache.doris.nereids.properties.UniformTest(12 tests passed)./build.sh --fe./run-regression-test.sh --run -f regression-test/suites/nereids_rules_p0/eliminate_gby_key/eliminate_group_by_key_by_uniform.groovy ... -forceGenOutThe regression asserts both results and plan group keys: nullable
COUNT/NDVremain in the outer grouping, while non-nullCOUNTstill permits safe group-key elimination.