[fix](asof_join)PhysicalHashJoin's computeUniform method should process asof join properly#62730
Draft
starocean999 wants to merge 3 commits into
Draft
[fix](asof_join)PhysicalHashJoin's computeUniform method should process asof join properly#62730starocean999 wants to merge 3 commits into
starocean999 wants to merge 3 commits into
Conversation
Contributor
Author
|
/review |
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Found 1 blocking issue.
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/physical/PhysicalHashJoin.java: this switch update is logically consistent with the existingLogicalJoinhandling, but there is still no test that covers the exact runtime path this PR fixes. Current tests cover logical trait derivation and shuffle/output-property derivation; the regression here is specifically physical logical-property recomputation after implementation, whenRecomputeLogicalPropertiesProcessorrebuilds traits fromPhysicalHashJoin.computeUniform(). Please add a unit test that drives an ASOF hash join through that physical/post-process path and asserts the expected uniform trait.
Critical checkpoint conclusions:
- Goal and correctness: The goal is to propagate uniform traits for ASOF hash joins during physical logical-property recomputation. The code change matches the existing logical ASOF handling and looks correct, but there is no test proving this physical path.
- Scope: The modification is small, clear, and focused.
- Concurrency: Not involved.
- Lifecycle: The relevant lifecycle is
LogicalJoin -> PhysicalHashJoin -> RecomputeLogicalPropertiesProcessor; this PR addresses a real recomputation path. - Configuration / compatibility / persistence / FE-BE protocol: Not applicable.
- Parallel code paths: I checked the nearby logical join and distribution-property derivation code. Those ASOF paths are already covered; this PR fills the physical
computeUniformgap. - Special conditions: The added switch cases are straightforward and consistent with existing join-type handling.
- Test coverage: Incomplete for the bug being fixed.
- Observability: Not applicable.
- User focus: No additional focus points were provided.
I did not find another distinct correctness issue in the code change itself.
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
I found one blocking issue in the added regression coverage. The production change in PhysicalHashJoin.computeUniform() looks aligned with the existing LogicalJoin handling for ASOF joins, but the new tests do not actually provide a valid physical-plan recompute path.
Critical checkpoints:
- Goal and correctness: The implementation change appears to accomplish the ASOF uniform-propagation fix, but the PR still lacks a valid passing regression test for the recomputed physical-plan path because the new tests are built on an invalid plan shape.
- Scope/minimality: The production diff is small and focused. The test addition is larger than necessary, and its current setup is incorrect.
- Concurrency: Not involved.
- Lifecycle/static initialization: Not involved.
- Configuration changes: None.
- Compatibility / FE-BE protocol / persistence / data writes: Not involved.
- Parallel code paths: I checked the other relevant physical join path;
PhysicalNestedLoopJoinblocks FD propagation, soPhysicalHashJoinis the right implementation site. - Special conditional checks: No additional concern beyond the ASOF join-type cases added here.
- Test coverage: Blocking gap. The existing concern about physical-plan coverage is not fully resolved because the newly added tests fail before they can validate the recompute path.
- Test results: I could not run
ChildOutputPropertyDeriverTestend-to-end in this runner becausefe-coredepends onorg.apache.doris:fe-foundation:1.2-SNAPSHOT, which is unavailable here. Independent code-path inspection shows the new tests will throwClassCastExceptionwhen the processor visitsGroupPlan. - Observability / performance: No concerns from the production change.
- Other issues: None beyond the invalid tests.
User focus: no additional review focus was provided.
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
No additional findings on the current head.
Critical checkpoints:
- Goal / correctness: The patch aligns
PhysicalHashJoin.computeUniform()with the existing ASOF handling inLogicalJoin, so recomputed physical traits now preserve uniform slots for ASOF inner and outer joins. The added unit tests cover all four ASOF variants through the sameresetLogicalProperties()+copyStatsAndGroupIdFrom()flow used byRecomputeLogicalPropertiesProcessor. - Scope / focus: The change is small and focused: one missing physical-join switch update plus targeted unit coverage.
- Concurrency: No concurrency-sensitive code paths are introduced or modified.
- Lifecycle / static init: No special lifecycle or static-initialization concerns are involved.
- Config / compatibility / persistence: No new config, protocol, persistence, or rolling-upgrade compatibility concerns.
- Parallel code paths:
LogicalJoinandChildOutputPropertyDeriveralready handled ASOF variants; this change correctly bringsPhysicalHashJoinback into alignment with those paths. - Tests: Unit coverage is present for
ASOF_LEFT_INNER_JOIN,ASOF_RIGHT_INNER_JOIN,ASOF_LEFT_OUTER_JOIN, andASOF_RIGHT_OUTER_JOINafter logical-property recomputation. I did not run FE tests locally because this runner is missingthirdparty/installed/bin/protoc. - Observability / performance: No additional observability is needed, and the runtime impact is negligible.
- Focus points: No additional user-provided focus; nothing extra was outstanding there.
I treated the existing inline review threads as known context and did not find any new blocking issues beyond that context in this review pass.
Contributor
Author
|
run buildall |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
Author
|
run buildall |
…ss asof join properly
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29414 ms |
Contributor
TPC-DS: Total hot run time: 170883 ms |
Contributor
FE UT Coverage ReportIncrement line coverage |
1 similar comment
Contributor
FE UT Coverage ReportIncrement line coverage |
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
3 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?
Extend PhysicalHashJoin to correctly handle ASOF join variants so trait propagation, equal-set extraction, and functional dependency (FD) calculations work for ASOF join
Issue Number: close #xxx
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)