[test](nereids) Replace flaky null stats case with unit test - #65445
Merged
Conversation
### What problem does this PR solve? Issue Number: N/A Related PR: apache#62265 Problem Summary: The test_scale_num_nulls regression case checks selected-partition null-count scaling immediately after insert and analyze. In Cloud P0, the physical partition row count is reported asynchronously and can still be zero, while waiting for it can take up to two minutes. Extract the existing scaling formula into a package-private helper, cover the exact 12/4/3 to 1 calculation with a deterministic FE unit test, and remove the timing-dependent Groovy case. Planner behavior is unchanged. ### Release note None ### Check List (For Author) - Test: Unit Test - `StatsCalculatorTest#testScaleNumNullsForSelectedPartitions` passed three consecutive runs - Full `StatsCalculatorTest` passed 17 tests - Behavior changed: No - Does this need documentation: No
shuke987
requested review from
924060929,
englefly,
morrySnow and
starocean999
as code owners
July 10, 2026 04:12
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
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.
Reviewed PR 65445. I did not find a substantiated issue that needs an inline review comment.
Critical checkpoints:
- Goal and test: the PR replaces a timing-dependent regression case for partition-pruned null-count scaling with deterministic FE unit coverage. The production formula is unchanged, and the new test verifies table count 12, selected count 4, and numNulls 3 scaling to 1.
- Scope: the change is small and focused: helper extraction, one unit test, and removal of the flaky Groovy case.
- Concurrency/lifecycle/config/persistence/data writes: not applicable; this PR does not add concurrent state, lifecycle ownership, config items, persisted formats, transaction paths, or write paths.
- Compatibility and FE/BE protocol: not applicable; no serialized format, thrift field, or FE/BE interface changes.
- Parallel paths: the helper remains called from the same partition-pruned OlapScan stats path as the previous inline code. Non-pruned table stats and MV fallback paths are not changed.
- Conditional checks: the
tableRowCount == 0 ? 1 : ...branch is preserved from the original code; no new special condition was introduced. - Tests and expected results: the deleted regression case had no
.outfile and depended on asynchronous physical row-count reporting; the replacement unit test directly exercises the extracted scaling logic. I did not see an expected-output issue. - Observability/performance: no new logging or metrics are needed for this test-only/helper extraction; the production hot path keeps the same arithmetic and allocation behavior.
- Other issues: no additional correctness, style, or duplicate-review issue found.
User focus: no additional user-provided focus points were supplied.
Subagent conclusions:
optimizer-rewritefound no valuable optimizer/rewrite candidate and did not append findings.tests-session-configfound no valuable test/session/config candidate and did not append findings.- Final convergence round 1 ended with both live subagents replying
NO_NEW_VALUABLE_FINDINGSfor the same ledger and proposed final comment set with no inline comments.
Validation performed:
- Read the required review prompt, repo code-review skill, required AGENTS files, existing review context, changed-file list, focus file, and shared ledger before diff review.
- Used GitHub PR metadata/patch as authoritative because the local checkout lacks the base commit.
- Reconfirmed current head
509ca74e437846fa0341a7e1d8be7c85b33bcac3, file list, and absence of existing inline comments before submission. - Verified the GitHub patch reverse-applies to the local PR head.
- Ran
git diff --check HEAD --on the touched files successfully.
I did not run FE tests/build in this runner because .worktree_initialized, thirdparty/installed, and thirdparty/installed/bin/protoc are missing; per fe/AGENTS.md, FE build/test should stop until thirdparty dependencies are present.
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
Contributor
There was a problem hiding this comment.
I found one coverage regression in the replacement test. The new unit test is useful and deterministic, but it no longer proves the deleted SQL regression's full optimizer path from a partition predicate to pruned scan statistics.
Critical checkpoints:
- Goal and test: the PR aims to replace a flaky regression case for selected-partition null-count scaling. The current test proves the already-pruned
computeOlapScanbranch scales 12/4/3 to 1, but it does not prove thatwhere d = '2017-01-01'still reaches that branch with a prunedLogicalOlapScan. - Scope: the code change is small and focused, but deleting the regression removes planner-level coverage that should be replaced in a non-flaky way.
- Concurrency/lifecycle/config/persistence/data writes: not applicable; no concurrent state, lifecycle ownership, config, persistence, transaction, or write path is changed.
- Compatibility and FE/BE protocol: not applicable; no serialized format, thrift field, or FE/BE interface is changed.
- Parallel paths: the production stats formula is unchanged. The issue is that the replacement test covers only the direct stats path, not the parallel planner/pruning path formerly covered by the regression.
- Conditional checks: no new special condition is introduced; the preserved
tableRowCount == 0 ? 1 : ...branch was not a concern. - Tests and expected results: the deleted Groovy case had no
.outfile and was timing-sensitive, but it did exercise predicate planning, partition pruning, and memo stats together. The replacement should keep deterministic unit coverage and add a non-flaky planner-level check for that integration. - Observability/performance: no observability or performance issue found.
- Other issues: targeted checkstyle for
StatsCalculatorTest.javapassed; no duplicate inline comments existed.
User focus: no additional user-provided focus points were supplied.
Subagent conclusions:
optimizer-rewriteproposed OR-1; after main-agent verification it became the inline comment M-1.tests-session-configreported no TS candidates; its no-finding note included a successful targeted Maven checkstyle pass.- Final convergence round 1 ended with both live subagents replying
NO_NEW_VALUABLE_FINDINGSfor the same ledger and one-comment proposed final set after M-1 was accepted.
Validation performed:
- Read the required review prompt, repo code-review skill, required AGENTS files, existing review context, changed-file list, focus file, and shared ledger before diff review.
- Used GitHub PR metadata/patch as authoritative because the local checkout lacks the base commit.
- Reconfirmed current head
74b125ed09ed2018c699737c00b580dce619d72e, file list, and absence of existing inline comments before submission. - Ran targeted Maven checkstyle for
src/test/java/org/apache/doris/nereids/stats/StatsCalculatorTest.javasuccessfully. - Ran
git diff --check HEAD --on the touched files successfully.
I did not run FE unit tests/build in this runner because .worktree_initialized, thirdparty/installed, and thirdparty/installed/bin/protoc are missing; per fe/AGENTS.md, FE build/test should stop until thirdparty dependencies are present.
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
Contributor
There was a problem hiding this comment.
Review completed for PR 65445 at head ea04ab6.
I did not find any new non-duplicative inline issue to submit. The only meaningful concern I found is the planner-level coverage gap already raised in discussion_r3556961223: the deleted Groovy regression exercised SQL predicate planning, partition pruning, analyze-backed stats, and memo stats together, while the new unit test pre-stubs selected partitions and directly calls computeOlapScan. I am not duplicating that existing inline thread.
Checkpoint conclusions:
- Goal and test proof: the new JUnit test does prove the direct computeOlapScan selected-partition null-scaling arithmetic: table row count 12, selected partition row count 4, table-level numNulls 3, resulting row count/count 4 and numNulls 1.
- Scope: the PR is focused and test-only, adding one FE unit test and deleting one regression suite.
- Concurrency, lifecycle, persistence, compatibility, configuration, and observability: no production code or runtime behavior is changed. The test scopes Env static mocking and restores/removes the ConnectContext thread-local in finally.
- Parallel paths and coverage: the only coverage loss is the already-open planner-level regression coverage thread, so no new duplicate comment is submitted.
- User focus: no additional user-provided focus was present.
Validation: static review only. I did not run builds or tests because the review prompt explicitly says this environment is only for review operations and not to attempt builds or code modifications.
Contributor
FE UT Coverage ReportIncrement line coverage `` 🎉 |
Contributor
TPC-H: Total hot run time: 29751 ms |
Contributor
TPC-DS: Total hot run time: 180151 ms |
Contributor
ClickBench: Total hot run time: 24.9 s |
morrySnow
approved these changes
Jul 13, 2026
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
Contributor
|
PR approved by anyone and no changes requested. |
This was referenced Jul 15, 2026
yiguolei
pushed a commit
that referenced
this pull request
Jul 16, 2026
…est (#65656) ### What problem does this PR solve? Related PR: #65445 Problem Summary: Backport #65445 to branch-4.1. The regression case `test_scale_num_nulls` depends on asynchronously visible physical row counts. When the physical row count is still 0, Nereids scales the selected-partition column statistics to 0 even though the injected partition/table statistics are correct, making the case flaky. Replace the environment-dependent Groovy regression case with a deterministic FE unit test that directly constructs the partition/table statistics and verifies selected-partition scaling, including row count, NDV, min/max, count, and numNulls. ### Coverage rationale - This is a branch backport of merged PR #65445. The same planner-level coverage concern was raised in [the upstream review thread](#65445 (comment)), then explicitly resolved by a maintainer before the PR was approved and merged. - The independent behavior introduced with #62265 is selected-partition statistics scaling. The replacement test invokes the real `StatsCalculator.computeOlapScan()` path and verifies row count, NDV, min/max, count, and `numNulls`. SQL predicate-to-partition selection remains covered by `PruneOlapScanPartitionTest` on this release branch. - The removed cluster case sampled the strict physical partition row count immediately after insert/analyze. In Cloud P0 that value can remain 0 for up to two minutes even when table-level column statistics are correct. Retaining the case or waiting for that report would preserve the flake or add up to two minutes to the suite. - `SHOW COLUMN STATS` was only failure diagnostics, and memo text formatting was not an independent assertion target of this case. ### Release note None ### Check List (For Author) - Test - [ ] Regression test - [x] Unit Test - [ ] Manual test (add detailed scripts or steps below) - [ ] No need to test or manual test. Explain why: - [ ] This is a refactor/code format and no logic has been changed. - [ ] Previous test can cover this change. - [ ] No code files have been changed. - [ ] Other reason Local validation: - `StatsCalculatorTest#testComputeOlapScanScalesNumNullsForSelectedPartitions`: passed (1 test) - `StatsCalculatorTest`: passed (17 tests) - `mvn -pl fe-core -am checkstyle:check`: passed (0 violations) - Behavior changed: - [x] No. - [ ] Yes. - Does this need documentation? - [x] No. - [ ] Yes. ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label
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: N/A
Related PR: #62265
Problem Summary:
test_scale_num_nullschecks that table-levelnumNulls=3is scaled to1when partition pruning selects 4 of 12 rows. The regression case executesEXPLAIN MEMO PLANimmediately after insert and analyze, but Cloud P0 reports the physical partition row count asynchronously. The planner can therefore observe a transient selected-partition row count of zero, while waiting for the real row count can take up to two minutes.This change:
StatsCalculator.computeOlapScan()with a three-partition OLAP scan;ndv=1,min=max=2, andnumNulls=3;ndv=1,min=max=2,count=4, and scalednumNulls=1;The unit test exercises the production partition-pruning statistics path using fixed in-memory inputs. It has no cluster, physical row-count reporting, polling, sleep, network, or storage dependency. Production behavior is unchanged.
Release note
None
Check List (For Author)
Test
StatsCalculatorTest#testComputeOlapScanScalesNumNullsForSelectedPartitions: passed 3 consecutive runsStatsCalculatorTest: 17 tests passedBehavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)