[test](fe) Remove flaky DistributeHintTest.testLeading#65802
Draft
shuke987 wants to merge 5 commits into
Draft
Conversation
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.
Request changes: this patch narrowly removes an expensive, unseeded, muted randomized test, but it also removes the remaining executable signal for a known unresolved leading-hint semi/anti correctness failure without adding a deterministic replacement. See the inline finding for the concrete invalid plan and a small replacement-test direction.
Critical checkpoint conclusions:
- Goal and proof: the deletion achieves the stated stability/runtime goal, but the two reported targeted FE-UT rounds now execute only
testHintJoinwithwithLeading=false; they do not prove the failing leading path is covered. - Scope and clarity: the source change is minimal and focused. The dormant leading helper branch is not a separate defect and can be reused for the focused replacement.
- Correctness and parallel paths: current
LeadingHint.getJoinConstraintcan still match a semi constraint before the retained side exists, build a right-semi child that drops a later-referenced slot, and produce an invalid join tree. The broad regression suites do not directly assert this constraint-level rejection; the exact focused tests added in #65205 were reverted with #65546. - Concurrency, lifecycle/static initialization, configuration/dynamic behavior, compatibility/rolling upgrade, persistence/transactions/data writes, FE-BE variable passing, and observability: not involved in this FE test-only deletion.
- Test coverage/results: existing Groovy suites and generated
.outbaselines were inspected; they cover many leading shapes/results but not the reverted direct invariant. Per the review-run contract, no build or test was run locally.git diff --checkis clean; the author reports two successful targeted FE-UT runs. - Performance: removing roughly 51,000 unseeded optimization attempts is worthwhile, and a small direct deterministic unit test preserves the important signal without restoring that cost.
- User focus: no additional user-provided focus was supplied.
Review completion: the full changed-file sweep, main risk scan, complete-review and risk-focused subagent passes, candidate deduplication, and unresolved-point audit are complete. One blocking inline issue remains.
| for (int e = t - 1; e <= (t * (t - 1)) / 2; e++) { | ||
| for (int i = 0; i < 10; i++) { | ||
| System.out.println("TableNumber: " + String.valueOf(t) + " EdgeNumber: " + e + " Iteration: " + i); | ||
| randomTest(t, e, false, true); |
Contributor
There was a problem hiding this comment.
[P1] Preserve a deterministic guard for the known semi-join failure
Please replace this randomized entry with a small deterministic guard before deleting it. The reported missing map entry is a concrete production-invalid rewrite, not only evaluator noise. For an original A LEFT SEMI JOIN B ON A.k = B.k, an extra C, and leading(B C A), current LeadingHint.getJoinConstraint can produce:
Join(A.k = B.k) // B.k is absent from both children
RightSemiJoin
Scan(B)
Scan(C) // right-semi output is C only
Scan(A)
The guard at LeadingHint.java:420-422 tests isSubset(...) && !isSubset(...) on the same arguments, so it never rejects getJoinConstraint(B|C, B, C); the child keeps bitmap B|C after dropping B, and the saved predicate is attached above it. #65205 added the exact withoutRetainedSide assertion after this muted test exposed the issue, and #65546 reverted that fix plus its focused tests while a correct fix remained outstanding. The cited FE-UT validation now only runs withLeading=false, while the broad result suites do not directly assert this constraint rejection. A tiny direct LeadingHintTest covering early left/right semi/anti matches would preserve the important signal without the ~51,000 unseeded attempts.
Contributor
TPC-H: Total hot run time: 29657 ms |
Contributor
TPC-DS: Total hot run time: 178715 ms |
Contributor
ClickBench: Total hot run time: 25.4 s |
shuke987
changed the title
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
Contributor
There was a problem hiding this comment.
Request changes: the deterministic replacement test closes the existing singleton-side review thread, and removing the non-asserting randomized loop is reasonable. However, the new legality guard still accepts a composite constrained side split across both children; see the inline P1.
Critical checkpoint conclusions:
- Goal and proof: the singleton-plus-extra case is fixed and directly tested for left/right semi/anti joins, but nested composite/multiple constraints can still produce wrong rows, so the correctness goal is incomplete.
- Scope, clarity, and parallel paths: the change is focused and the bitmap work is constant-time, but the exact-side gate is incomplete. Nested semi/anti, reversed children, source right variants after normalization, and null-aware anti handling were checked; no separate issue was found.
- Tests and results: the new JUnit test is a useful deterministic replacement for the existing singleton concern, while the removed random test recorded mismatches without asserting them. A nested/composite end-to-end guard is still missing. Per the review contract, no local build was run. The author reports the targeted FE unit tests pass; the live FE UT check did not reach fe-core because Maven received a truncated
paimon-common:1.3.1artifact, while Checkstyle is green. - Errors and observability: caught invalid orders mark the hint
UNUSED; the composite miss instead reachesSUCCESSwith wrong cardinality, so existing status reporting does not expose it. - Concurrency, lifecycle, configuration, compatibility, persistence, transactions, writes, and FE/BE transport: not involved in this per-query planner change.
- User-requested focus: none was specified.
- Review completion: the full authoritative patch and all changed files were reviewed, existing comments were deduplicated, and all suspicious points resolve to the one blocking inline finding.
| continue; | ||
| Long constrainedSide = joinConstraint.getJoinType().isRightSemiOrAntiJoin() | ||
| ? joinConstraint.getLeftHand() : joinConstraint.getRightHand(); | ||
| if ((LongBitmap.isSubset(constrainedSide, leftTableBitmap) |
Contributor
There was a problem hiding this comment.
[P1] Reject composite constrained sides split across both children
These isSubset checks only catch a constrained side wholly contained in one oversized child. They miss a composite side split between both children. For example, start from:
LeftSemi[Pab]
A
LeftSemi[Pbc]
B
C
Collection records inner constraint JC1=(B,C) and outer JC2=(A,B|C). With leading(B A C), the final children are B|A and C: JC1 matches, while JC2's full constrained side B|C is a subset of neither child, so this guard misses it and the later overlap branch leaves JC1 selected. Both predicates are semi-compatible, producing LeftSemi[Pab,Pbc](Cross(B,A),C). That retains one A row per matching B, whereas the original outer semi retains each A once (two matching B rows already make the results differ). Nested anti joins fail analogously.
Once both minimum hands are present, please reject any constrained-side overlap unless that child equals the full side (for example, isOverlap(...) && !equals(...)), and add a nested/composite constraint test. This is distinct from the earlier singleton-side-plus-extra-table thread.
Contributor
There was a problem hiding this comment.
Automated review result: no new inline comments were submitted because the one blocking planner-correctness gap identified in this run is now covered by the live P1 at #65802 (comment). This is not an approval; that composite constrained-side defect remains unresolved on the current head.
Critical checkpoint conclusions:
- Goal and proof: the patch addresses the previously reported atomic extra-table and missing-retained-side cases and gives deterministic unit coverage for all four atomic directions, but it does not preserve the full constrained-side invariant for composite and nested paths and has no analyzer-level regression for them.
- Scope and clarity: the code change is small and focused, but the subset predicate is too narrow for the full/minimal-hand model used by
CollectJoinConstraint. - Parallel paths and special conditions: left/right semi and anti, reversal, analyzer stage order, nested/composite hands, and LEADING fallback were checked. The
SemiJoinCommute.hasDistributeHint()exception and nested same-family constraint processing are the material missed paths. - Concurrency, lifecycle/static initialization, configuration dynamics, persistence/transactions/data writes, FE-BE protocol, compatibility, and observability: not involved in this local FE analysis rewrite.
- Performance: the change does not materially alter complexity; no separate performance issue was found.
- Tests and validation: the new JUnit style and atomic assertions are appropriate, and
git diff --checkpassed. Per the review-runner contract I did not run builds or FE tests; the PR description reports the two targeted FE UTs passing. No generated result files are changed. - User focus: no additional user-provided focus was supplied.
- Review completion: every candidate was independently verified, deduplicated, merged, or dismissed with code evidence. Three review rounds converged with no additional valuable findings. The live composite-side P1 remains for the author; this review adds no duplicate inline comment.
Independent full-review and risk-focused passes corroborated the landed issue. A direct LEFT-composite early match and the risk that the new immediate failure rejects a valid hint were dismissed after tracing bottom-up constraint construction and fallback behavior; the separate nested same-family path resolves to the same full-side overlap defect already reported live.
Contributor
|
Codex automated review failed and did not complete. Error: Codex completed, but no new pull request review was submitted for the current head SHA. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
shuke987
changed the title
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
Contributor
There was a problem hiding this comment.
Automated review follow-up: no new inline comments are being submitted because the current one-file deletion is already covered by the live deterministic-guard P1 at #65802 (comment), while the related nested/composite product path is covered by #65802 (comment). This is not an approval: current head f9b67e8c3c32e348771ef1df9dc88f319f2b3dea reverts the attempted planner fixes, so both concerns remain unresolved.
Critical checkpoint conclusions:
- Goal and proof: the patch does narrowly remove the flaky unseeded JUnit entry and its 51,030 randomized leading optimizations, but the reported targeted FE-UT runs now execute only the remaining
testHintJoinpath withwithLeading=false; they do not prove the exposed leading path is covered. - Scope and clarity: the source diff is a small, focused test-only deletion. The now-unreachable private leading helper branch is minor cleanup, not a separate correctness defect.
- Correctness and parallel paths:
testLeadingwas the onlywithLeading=truecaller. The broad leading regression suites exercise many result shapes, but they do not directly assert rejection of the exact early singleton constrained-side order across left/right semi/anti, and the nested/composite family is likewise still unresolved on this head. - Tests and results: no direct deterministic replacement is present and no generated result file changes. Per the review-runner contract, no build or test was run locally; the author reports two successful runs of the remaining FE test.
- Performance: removing the 51,030 unseeded optimization attempts is a worthwhile CI improvement; a small direct legality test can preserve the important signal without restoring that cost. No production performance change is introduced.
- Concurrency, lifecycle/static initialization, configuration/dynamic behavior, compatibility/rolling upgrade, persistence/transactions/data writes, FE-BE variable passing, observability, and security boundaries are not involved in this FE test-only deletion.
- User focus: no additional user-provided focus was supplied.
- Review completion: the full authoritative diff and changed-file list were swept, the main risk was traced through
LeadingHintandHyperGraphBuilderOld, and all round-one candidates were independently verified and deduplicated. No suspicious point remains without a concrete conclusion.
Contributor
FE UT Coverage ReportIncrement line coverage `` 🎉 |
Contributor
TPC-H: Total hot run time: 29521 ms |
Contributor
TPC-DS: Total hot run time: 176715 ms |
Contributor
ClickBench: Total hot run time: 24.89 s |
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
2 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: #65205, #65546
Problem Summary:
org.apache.doris.nereids.jobs.joinorder.joinhint.DistributeHintTest.testLeadingis a muted, unseeded randomized FE UT. TeamCity build 1000223 recorded the test asFAILUREwhile the parent build remainedSUCCESS; the random optimized plan reached the test-sideHyperGraphBuilderOldevaluator with a missing map entry and threw aNullPointerExceptionbefore the equality oracle could complete.The triggering PR #65784 only changes Iceberg, Parquet, table-reader, and external-catalog paths. A bounded history snapshot also found muted failures across independent master and release-branch PR builds.
This PR removes only the
testLeadingJUnit entry. It leavestestHintJoin, shared helper code, and all product logic unchanged. The known leading-hint correctness issue exposed by this randomized test is intentionally not fixed here because that product path will be redesigned separately.Validation:
Both rounds completed with
BUILD SUCCESS, including FE checkstyle and compilation.git diff --checkalso passes.Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)