branch-4.1: [test](nereids) Replace flaky null stats case with unit test - #65656
Conversation
…65445) Related PR: apache#62265 Problem Summary: `test_scale_num_nulls` checks that table-level `numNulls=3` is scaled to `1` when partition pruning selects 4 of 12 rows. The regression case executes `EXPLAIN MEMO PLAN` immediately 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: - adds deterministic FE unit coverage that invokes `StatsCalculator.computeOlapScan()` with a three-partition OLAP scan; - fixes the table row count at 12, selected-partition row count at 4, and cached table-level column statistics at `ndv=1`, `min=max=2`, and `numNulls=3`; - verifies the final scan statistics have `ndv=1`, `min=max=2`, `count=4`, and scaled `numNulls=1`; - removes the timing-dependent Groovy regression case. 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.
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
I found one blocking review issue. The added unit test is useful and appears to exercise the local selected-partition null-scaling branch, but the PR also deletes the only SQL-level regression I could find for the original analyze/cache/pruning/memo-plan path.
Static validation only: per the review prompt I did not run builds or FE/regression tests. git diff --check passed for the PR range. No additional user focus was provided, and no existing inline review comments were present to deduplicate against.
| """ | ||
| def colStats = sql "show column stats ptable"; | ||
| def memo = sql "explain memo plan select * from ptable where d='2017-01-01'" | ||
| // check numNulls=1.0000 for column val, which is scaled from 3 to 1 according to the partition pruning result. |
There was a problem hiding this comment.
This removes the only SQL-level guard I could find for the original behavior. The replacement unit test stubs getSelectedPartitionIds(), table/partition row counts, and OlapTableStatistics.getColumnStatistics() before calling computeOlapScan() directly, so it proves the local numNulls scaling arithmetic but no longer exercises the path that can regress in production: analyze table ... with sync populating table stats, planner partition pruning selecting p201701, and memo-plan stats rendering count=4.0000, numNulls=1.0000. Please keep this regression, or replace it with a smaller SQL-level case that still drives analyze/cache loading, partition pruning, and memo output; the new unit test can remain as fast coverage.
FE UT Coverage ReportIncrement line coverage `` 🎉 |
|
/review |
There was a problem hiding this comment.
Completed the review for PR 65656 at head cf4803f26eb023380c5ed88b42d673f42c50807a.
Summary:
- I found no new distinct inline issue beyond the existing review thread on
regression-test/suites/statistics/test_scale_num_nulls.groovy, which already covers the actionable concern that deleting the SQL regression removes end-to-end coverage foranalyze table ... with sync, stats cache loading, partition pruning, and memo stats rendering. - The added
StatsCalculatorTest.testComputeOlapScanScalesNumNullsForSelectedPartitionsappears to exercise the intended base-index partition-prunedcomputeOlapScanbranch: selected/table row counts are 4/12, table-level cachednumNullsis 3, and the assertion expects scalednumNulls1. - I did not find a separate compile/runtime/isolation problem in the new unit test. Mockito inline/static mocking is available in the FE test dependencies, the
Envstatic mock is scoped, andConnectContextis restored infinally.
Review checkpoints:
- Existing inline comments were checked and duplicate findings were suppressed.
- The changed-file list and authoritative local diff were reviewed; no extra changed paths were found.
- Round 1 normal subagents and the risk-focused subagent all returned
NO_NEW_VALUABLE_FINDINGS; no unresolved candidate findings remain. - No additional user-provided review focus was present.
Validation limits:
- Static review only. I did not run builds or tests per the review prompt.
- No repo-local code-review
SKILL.mdpath was present in the confirmed file listing, so I followed the explicit review prompt and prepared review bundle.
|
skip buildall |
github-actions
Bot
added
the
approved
|
PR approved by at least one committer and no changes requested. |
3 participants
What problem does this PR solve?
Related PR: #65445
Problem Summary:
Backport #65445 to branch-4.1.
The regression case
test_scale_num_nullsdepends 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
StatsCalculator.computeOlapScan()path and verifies row count, NDV, min/max, count, andnumNulls. SQL predicate-to-partition selection remains covered byPruneOlapScanPartitionTeston this release branch.SHOW COLUMN STATSwas only failure diagnostics, and memo text formatting was not an independent assertion target of this case.Release note
None
Check List (For Author)
Test
Local validation:
StatsCalculatorTest#testComputeOlapScanScalesNumNullsForSelectedPartitions: passed (1 test)StatsCalculatorTest: passed (17 tests)mvn -pl fe-core -am checkstyle:check: passed (0 violations)Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)