[refine](exec) reject redundant same-type assert_cast#63059
[refine](exec) reject redundant same-type assert_cast#63059HappenLee merged 2 commits intoapache:masterfrom
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
There was a problem hiding this comment.
Automated review completed for PR 63059.
Findings: no blocking correctness issues found in the reviewed diff.
Critical checkpoint conclusions:
- Goal and tests: The PR aims to reject redundant same-type
assert_castusages and updates existing BE call sites accordingly. The changed implementation and representative call paths are consistent with that goal. No new dedicated test was added, but this is primarily a compile-time enforcement/refactor and existing BE compilation should exercise the updated call sites. - Scope and clarity: The changes are broad but mechanically focused on removing redundant casts or replacing them with direct typed access/static casts where the local construction makes the type invariant explicit.
- Concurrency: No new shared state, locks, threads, bthreads, dependencies, or atomics were introduced.
- Lifecycle and static initialization: No new static/global object lifecycle concern was introduced.
- Configuration: No configuration item was added or changed.
- Compatibility: No storage format, wire protocol, function symbol, or serialized metadata compatibility change was found.
- Parallel paths: The PR touches the directly affected BE call sites across core, exec, exprs, format, and storage paths. I did not find an obvious missed functionally parallel path that would break the new
assert_castrule. - Conditional checks: The new
static_assertis simple and compile-time only; no speculative runtime defensive branch was added. - Test coverage: No new tests are included, which is acceptable for a compile-time refactor if CI builds the affected BE code. Runtime behavior should remain unchanged.
- Test results: No generated test result files were modified.
- Observability: No new runtime path requiring additional logs or metrics was introduced.
- Transaction/persistence/data writes: No transaction, persistence, delete bitmap, or data write semantics were changed.
- FE/BE variable passing: No FE/BE transmitted variable or thrift path was changed.
- Performance: The change removes redundant dynamic/type checks in places where static type is already known, with no apparent new hot-path cost.
- User focus: No additional user-provided review focus was present.
Residual risk: Because this PR is intentionally compile-time enforcement across many C++ templates, the main validation should be a BE build/style check to catch any remaining redundant assert_cast instantiations that are only compiled under specific template paths.
|
run buildall |
|
run buildall |
|
run cloud_p0 |
|
/review |
There was a problem hiding this comment.
Reviewed the PR with the Doris code-review checklist.
Critical checkpoint conclusions:
- Goal and proof: the change rejects redundant same-type
assert_castat compile time and updates the affected BE call sites/tests; this matches the stated refactor goal. Existing modified BE unit tests cover representative touched code paths, though I did not run the test suite in this review runner. - Scope/focus: the actual GitHub PR diff is focused on
assert_castnormalization and mechanical call-site cleanup in BE code. - Concurrency/lifecycle/config/compatibility: no new concurrency, lifecycle ownership, config, persistence, storage-format, FE/BE protocol, or rolling-upgrade compatibility concerns were introduced by the reviewed changes.
- Parallel paths: reviewed the converted column/function/storage/test call-site patterns for equivalent static types and obvious remaining redundant cast risks; no blocking issue found.
- Error handling/data correctness: no ignored
Status, silent failure path, data visibility, delete-bitmap, or transaction correctness issue found in the actual PR diff. - Memory/performance: no new allocations or memory-tracking issues found; most changes remove redundant casts and preserve existing behavior.
- Tests: modified tests are consistent with the mechanical refactor. I did not independently execute BE unit tests, so residual risk is compile/test coverage only.
- User focus: no additional user-provided review focus was present.
No blocking findings from the reviewed diff.
What problem does this PR solve?
Issue Number: N/A
Problem Summary:
BE code currently allows assert_cast usages where From and To are effectively the same type after removing cv/ref qualifiers, so a large number of no-op casts have accumulated across column, expression, storage, and format code. This weakens the signal of assert_cast, obscures the real static type of nullable maps and result columns, and leaves obviously redundant casts unnoticed during development.
Root cause: assert_cast only checked the requested cast form and did not reject semantically identical types such as cv/ref-qualified aliases or pointer variants with cv-qualified pointees. This change normalizes assert_cast template arguments, adds a compile-time static_assert to ban same-type casts, and updates the existing C++ call sites to use direct access or explicit static_cast only where the holder is still base-typed.
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)