Repository navigation
Conversation
### What problem does this PR solve? Issue Number: N/A Related PR: apache#69616 Problem Summary: Outer ASOF joins can declare a NOT NULL input expression as nullable while BE evaluates the match key before null padding. Root cause: the final `AdjustNullable` pass uses join output nullability for the match condition. Preserve child input nullability for ASOF match conditions, retaining existing behavior for ordinary joins. ### Release note Fix incorrect nullability in ASOF JOIN match conditions. ### Check List (For Author) - Test: Added FE unit coverage for all four ASOF join types and a regular LEFT OUTER JOIN control; tests not run locally. Targeted Checkstyle passed with zero violations. - Manual diagnosis: The full CTAS reproducer fails on master with nullable output normalization removed (`declared=Nullable(DateTimeV2(0)), actual=DATETIMEV2`). No post-fix build or runtime validation performed. - Behavior changed: Yes. ASOF match conditions retain child input nullability. - Does this need documentation: No
Mryange
requested review from
924060929,
englefly,
morrySnow and
starocean999
as code owners
October 26, 2026 10:02
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
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 both changed files at head e4fe1ff13272ce0ac7a0021ccfacae4b31d1f1c1. No verified new defects were found, and there are no existing inline findings to carry forward.
The change aligns ASOF match expressions with their immediate child inputs: FE translates the match condition before outer output null extension, and BE prepares/evaluates the two match keys against the corresponding child schemas. The helper still registers nullable outer outputs for parent operators. Nullable children, nested outer joins, aliases/casts, ordinary joins, and hash/mark predicates were traced without finding a new mismatch.
Critical checkpoint conclusions:
- Goal and evidence: child nullability is retained for ASOF matching. The added outer ASOF cases distinguish the previous ordering by inspection; the inner cases and ordinary LEFT OUTER case provide controls.
- Scope and reuse: the change is focused in the existing shared helper, with a small unit-test addition.
- Concurrency and locks: no new shared state, thread, lock, I/O, or locking-order change; the map remains local to a sequential rewrite traversal.
- Lifecycle and static initialization: no new ownership, resource lifetime, or production static initialization behavior.
- Configuration: none added or modified.
- Compatibility: no API signature, storage format, or protocol schema change; existing match-condition metadata is corrected.
- Thrift/protobuf IDs: no definition changes; cross-branch ID comparison is not applicable.
- Parallel paths: analyzed-phase and ordinary-join ordering is retained; all four ASOF enums select the child-input branch. Current SQL/BE execution uses left ASOF variants; right enum cases validate the helper contract. Hash/mark ordering and outer output null extension remain intact.
- Conditional logic: the new boolean exactly separates final-phase ordinary joins from ASOF joins, and the comment explains evaluation before padding.
- Test coverage: JUnit 5 tests cover all four ASOF enums and a regular LEFT OUTER control. They exercise the helper directly; populated maps, nullable/nested inputs, and parent outputs were checked by static tracing rather than additional executed tests.
- Expected results: no regression result files changed; the new nullability expectations agree with the input/output distinction by inspection.
- Observability: no new execution path requiring metrics or logging was introduced; existing assertions and error propagation are preserved.
- Transactions and persistence: no transaction, EditLog, replay, or failover behavior changed.
- Writes and atomicity: no data-write or crash-recovery path changed.
- FE-to-BE variables: no new variable or sending point; existing ASOF match serialization carries the corrected metadata.
- Performance: the same expression replacement and output-map traversals remain, with one constant-time join-type check and no additional asymptotic work.
- Other invariants: no new slot/ExprId, expression-type, error-handling, or query-correctness defect was found. Version visibility, delete bitmaps, and BE allocation ownership are not modified by this patch.
User focus: no additional focus was supplied. Both initial risk items were dismissed with concrete evidence; all assigned reviewers returned NO_NEW_VALUABLE_FINDINGS, with no unresolved candidate or uncovered changed-file scope.
Validation limits: this was a static-only review. No builds, style checks, unit tests, regression tests, or runtime reproducer were run. The new tests do not provide full planner/BE reproduction coverage, and this review does not claim runtime correctness or passing CI.
Review converged after 1 round(s).
Contributor
Author
|
run buildall |
HappenLee
approved these changes
Oct 26, 2026
HappenLee
left a comment
HappenLee
left a comment
Contributor
There was a problem hiding this comment.
Reviewed e4fe1ff. No blocking correctness, performance, or reuse issues found. ASOF match conditions now retain immediate child nullability, while null-extended join outputs remain available to parent operators. The change is focused in the existing shared helper.
Non-blocking follow-up: add end-to-end coverage with NOT NULL DATETIMEV2 computed match keys and matched/unmatched rows, plus nullable nested-child and parent-output propagation coverage.
Validation: static review of both changed files and the FE-to-BE call chain; git diff --check passed. No local build, unit test, or SQL regression was run.
Contributor
TPC-H: Total hot run time: 28912 ms |
Contributor
TPC-DS: Total hot run time: 150555 ms |
Contributor
ClickBench: Total hot run time: 24.29 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
Author
|
run external |
Contributor
FE Regression 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
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: #69616
Problem Summary: Outer ASOF joins can declare a NOT NULL input expression as nullable while BE evaluates the match key before null padding. Root cause: the final
AdjustNullablepass uses join output nullability for the match condition. Preserve child input nullability for ASOF match conditions, retaining existing behavior for ordinary joins.Release note
Fix incorrect nullability in ASOF JOIN match conditions.
Check List (For Author)
declared=Nullable(DateTimeV2(0)), actual=DATETIMEV2). No post-fix build or runtime validation performed.