[fix](statistics) Treat invalid column statistics as UNKNOWN instead of aborting analyze job - #66756
[fix](statistics) Treat invalid column statistics as UNKNOWN instead of aborting analyze job#66756codeDing18 wants to merge 2 commits into
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
github-actions
Bot
added
the
approved
|
PR approved by at least one committer and no changes requested. |
|
PR approved by anyone and no changes requested. |
|
建议改一下标题 |
FE Regression Coverage ReportIncrement line coverage |
OK |
…of aborting analyze job
9b55e11 to
fc5d3a1
Compare
codeDing18
changed the title
|
run buildall |
github-actions
Bot
removed
the
approved
TPC-H: Total hot run time: 17700 ms |
TPC-DS: Total hot run time: 86534 ms |
ClickBench: Total hot run time: 14.75 s |
FE Regression Coverage ReportIncrement line coverage |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes for four issues: one production cache/persistence ordering defect and three regression-test contract gaps.
Critical checkpoint conclusions
- Goal and proof: Persisting this extreme sampled row and consuming it as UNKNOWN matches the agreed issue contract, and both conversion entry points were updated. The cache ordering defect means the old value can still be observed, and the changed collection path is not proved deterministically.
- Scope and focus: The production change is small and focused. The new regression is disproportionately large for the behavior it proves and contains the three test findings below.
- Concurrency: Analysis-task threads, asynchronous Caffeine loaders, concurrent planners, and follower cache handlers share the statistics lifecycle. No common lock closes the invalidate-before-flush window; this is the blocking production finding. No new lock-order or deadlock issue was found.
- Lifecycle: I traced sync and registered jobs from collection through cache publication,
AnalysisJobbuffering/flush, task completion, metadata freshness, reload/preheat, and restart/failover. Aside from the cache publication order, the intended UNKNOWN lifecycle is consistent; no static-initialization or ownership issue applies. - Configuration: No product configuration was added. The new test mutates the existing cluster-global
enable_auto_analyzevalue without non-concurrent isolation or restoration. - Compatibility: No function symbol, storage format, or FE-BE protocol field changed. Current direct, cache-load, preheat, SHOW, leader, follower, legacy, and Nereids consumers pass through one of the new guards; no supported rolling-upgrade defect was established.
- Parallel paths: OLAP full/sample analysis, external and plugin-driven callers, direct SET STATS, connector fallback, and the separate partition-statistics representation were checked. No distinct missed production path survived review.
- Conditional checks: All three
ColStatsData.isValidpredicates and their boundaries were checked. Returning UNKNOWN at both construction paths is consistent for current consumers; no additional conditional bug was found. - Test coverage: The changed unit tests validate conversion only. Random tablet selection plus permissive assertions let the regression pass without exercising the new
runQueryno-throw/persist behavior, and there is no latch-controlled test for the cache race. - Test results: The changed SHOW, cached SHOW, memo-plan, and
invalid_stats.outexpectations are consistent with UNKNOWN statistics. Current FE UT, P0 regression, non-concurrent regression, CheckStyle, compile, and other required PR checks are green. - Observability: The existing invalid-stat metric and warning identify the accepted UNKNOWN path; no separate logging or metrics defect was found.
- Persistence: The statistics row and job freshness metadata are persisted through established paths, but cache invalidation happens before the buffered row is durable and is not repeated after flush.
- Data writes and failure: A successful buffered insert is not atomic with leader/follower cache publication, producing the stale-cache interleaving called out inline. Flush failures otherwise remain failures; no additional transaction or leak issue was found.
- FE-BE variables: No new variable or execution option crosses the FE-BE boundary.
- Performance: No distinct production hot-path regression was found. The two-million-row regression and long polling window buy no deterministic producer-path coverage, which is folded into the test-coverage finding.
- Other issues and user focus: The trailing database drop violates the required drop-before-use lifecycle, and the global-variable mutation leaks across suites. No additional user focus was supplied.
No local builds or tests were run, as required by the review environment; the conclusions above use code-path inspection and the live checks on the reviewed head.
| String message = String.format("ColStatsData is invalid, skip analyzing. %s", colStatsData.toSQL(true)); | ||
| LOG.warn(message); | ||
| throw new RuntimeException(message); | ||
| // Don't throw: keep writing the row into the statistics table so that the |
There was a problem hiding this comment.
[P1] Close the cache-reload window before persisting the invalid row — This branch now reaches syncColStats, which invalidates the cache before job.appendBuf has committed the replacement row. A concurrent planner can start the async loader in that gap, read the previous valid row, and install it after the eviction; flushBuffer never invalidates again, so that stale value can survive until the 48-hour refresh. Please publish a blocking UNKNOWN value or move/repeat leader and follower cache publication after the buffered insert succeeds, and cover this interleaving with a latch-controlled test.
| sql """drop database if exists regression_test_analyze_sample_almost_all_null""" | ||
| sql """create database regression_test_analyze_sample_almost_all_null""" | ||
| sql """use regression_test_analyze_sample_almost_all_null""" | ||
| sql """set global enable_auto_analyze=false""" |
There was a problem hiding this comment.
[P2] Isolate and restore the global auto-analyze setting — This ordinary suite sets a cluster-wide variable to false for a long insert/delete/poll run and never restores it. Normal suites run in parallel, so this can disable auto analysis for unrelated tests and leaves the setting changed after completion or failure. Please run this global-state test in the nonConcurrent group and use setGlobalVarTemporary([enable_auto_analyze: false]) { ... } (or otherwise restore the prior value in finally).
|
|
||
| def result = sql """show column stats tbl_del_big""" | ||
|
|
||
| // k (NOT NULL) always produces valid sampled stats. Whether v also survives |
There was a problem hiding this comment.
[P2] Exercise the changed collection path deterministically — Tablet selection uses SecureRandom here, and these assertions deliberately accept either a valid or invalid result for v. If the runQuery throw is reintroduced, this suite can still pass whenever the one x row is sampled; the later ALTER ... SET STATS only exercises the separate read/conversion path. Please retain a mocked runQuery test or force a deterministic sample that proves the invalid row does not throw, is flushed, and is then read as UNKNOWN.
| contains("v#1 -> unknown(") | ||
| } | ||
|
|
||
| sql """drop database if exists regression_test_analyze_sample_almost_all_null""" |
There was a problem hiding this comment.
[P3] Preserve the regression state after the run — The Doris test rule requires cleanup before setup, not after completion, so failures remain debuggable. This suite already drops the database at line 76; please remove this trailing drop so the MoW data and persisted statistics row survive for inspection.
|
/review |
There was a problem hiding this comment.
Requesting changes for three new, non-duplicate issues: two production correctness/compatibility defects and one FE test-build failure. The four existing live inline threads are treated as duplicate fences and are not repeated here.
Critical checkpoint conclusions
- Goal and proof: The intended contract is to let analysis finish, persist the anomalous row, and expose it as UNKNOWN. The two new reader guards implement that contract for fully upgraded FEs, but the follower replay race and old-reader behavior keep the end-to-end goal incomplete. The new deterministic unit proof cannot compile in
fe-core. - Scope and focus: The production edit is small and focused. The unit test introduces an undeclared mocking framework. No additional user-provided focus was supplied.
- Concurrency: Analysis tasks, Caffeine loaders, follower RPC handlers, and the independent journal replayer participate in this lifecycle. The blocking put addresses the already-reported invalid-row loader race, but the RPC has no replay ordering and turns a transient valid-payload conversion miss into a long-lived UNKNOWN (
MAIN-002). No new lock-order or deadlock issue was found. - Lifecycle: I traced collection, leader publication, follower publication, buffered persistence, load/refresh, preheat, SHOW, planning, replay, and failover. The affected negative entry lasts until the 48-hour refresh/eviction path; no static-initialization, ownership, or resource-release issue applies.
- Configuration: No product configuration or dynamic setting was added. The current regression revision isolates the global auto-analyze setting, so the older live thread is not duplicated.
- Compatibility: The persisted/RPC representation now relies on reader guards absent from the immediately previous FE. During a supported rolling upgrade, failure of the old master can promote an upgraded follower while an old follower remains, exposing numeric invalid statistics through both RPC and shared-table reload (
MAIN-003). There is no protocol or metadata-version fence. - Parallel paths: Current-version cache load, preheat, direct/cached SHOW, Nereids, OLAP full/sample collection, manual statistics, and external fallback were checked. They consistently suppress intrinsically invalid rows; no separate missed consumer survived review.
- Conditional checks: The three
isValid()predicates match the intended current-reader safety contract. The follower path incorrectly treats intrinsic invalidity and temporary local metadata unavailability as the same UNKNOWN state;data.isValid()provides the needed distinction. - Test coverage: Regression expectations cover UNKNOWN consumption, while the producer-path determinism concern remains in an existing live thread. The replacement deterministic unit test imports JMockit without a
fe-coredependency or javaagent, so normal FE build/test paths fail before the test can prove the behavior (MAIN-004). - Test results: The changed expected results are consistent with UNKNOWN semantics. On the current head, CheckStyle and repository-hygiene checks pass, but no FE compile, FE unit-test, or regression-test result is present. No local build or test was run, as required by this review environment.
- Observability: The existing invalid-stat metric and warning are adequate for the accepted UNKNOWN path; no additional logging or metric issue was found.
- Persistence and data writes: The row uses the existing statistics-table buffer/insert path. The remaining defects are the unordered follower cache publication and mixed-version interpretation after persistence/failover; no additional transaction, crash-leak, or lock issue was established.
- FE-BE propagation: No new FE-BE variable, Thrift option, or execution flag was added.
- Performance: No material CPU, memory, or algorithmic regression was found in the added validation. A wrongly retained UNKNOWN can still deprive the follower optimizer of valid statistics for the cache refresh interval, as described inline.
- Other issues and completion: All ten changed files and relevant adjacent paths were reviewed. After three rounds, every final-round reviewer returned
NO_NEW_VALUABLE_FINDINGS; the review is converged, with no unresolved candidate beyond these three comments and the existing live threads.
| // Publish a blocking UNKNOWN instead of invalidating, mirroring the leader-side | ||
| // syncColStats: invalidation would let a concurrent async loader re-install the | ||
| // previous stale row before the leader's buffered insert becomes visible here. | ||
| Env.getCurrentEnv().getStatisticsCache().updateColStatsCache( |
There was a problem hiding this comment.
[P2] Keep transient follower conversion misses retryable — toColumnStatistic() returns the same UNKNOWN both for the invalid-stat shape and when this follower has not replayed the referenced column/index yet. The stats RPC has no journal barrier, so a valid update can arrive first; this unconditional put then leaves UNKNOWN cached after metadata catches up, with no replay invalidation until the 48-hour refresh. This is distinct from the existing invalid-row race. Please publish a blocking UNKNOWN only when !data.isValid(); keep other conversion failures invalidated/retryable, and test RPC-before-schema-replay recovery.
| String message = String.format("ColStatsData is invalid, skip analyzing. %s", colStatsData.toSQL(true)); | ||
| LOG.warn(message); | ||
| throw new RuntimeException(message); | ||
| // Don't throw: keep writing the row into the statistics table so that the |
There was a problem hiding this comment.
[P1] Fence persisted invalid rows from old FE readers — The new contract depends on both new isValid() guards, but this branch now sends and persists the contradictory row while a base-version FE still converts it into ordinary statistics. During rolling FE upgrade, an upgraded follower can become master after failover while an old follower remains; that old FE then caches the RPC payload and later reloads the same bogus row from the shared table. This is distinct from the existing pre-flush cache-race thread on this line. Please gate persistence until all FEs support the reader contract or use a backward-compatible representation/version fence, and cover the immediately previous reader behavior.
| import org.apache.doris.thrift.TUniqueId; | ||
|
|
||
| import com.google.common.collect.Lists; | ||
| import mockit.Expectations; |
There was a problem hiding this comment.
[P1] Use fe-core's declared mocking framework — These are the only real mockit.* imports in fe-core, but this module declares only mockito-core/mockito-inline; neither its POM nor the FE parent provides org.jmockit:jmockit, and Surefire has no JMockit javaagent. The new deterministic test therefore cannot compile/run in the FE test target. This is distinct from the existing request for deterministic coverage: the supplied replacement itself is unbuildable. Please express it with the already-declared Mockito stack (the removed test already used MockedConstruction), or explicitly add and configure JMockit for this module.
040fbb8 to
bb7b2a4
Compare
|
run buildall |
FE UT Coverage ReportIncrement line coverage |
4 participants
What problem does this PR solve?
Issue Number: close #64122
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)