Skip to content

[refine](exec) reject redundant same-type assert_cast#63059

Merged
HappenLee merged 2 commits intoapache:masterfrom
Mryange:reject-redundant-assert-cast
May 8, 2026
Merged

[refine](exec) reject redundant same-type assert_cast#63059
HappenLee merged 2 commits intoapache:masterfrom
Mryange:reject-redundant-assert-cast

Conversation

@Mryange
Copy link
Copy Markdown
Contributor

@Mryange Mryange commented May 7, 2026

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

    • Regression test
    • Unit Test
    • Manual test (add detailed scripts or steps below)
    • No need to test or manual test. Explain why:
      • This is a refactor/code format and no logic has been changed.
      • Previous test can cover this change.
      • No code files have been changed.
      • Other reason
  • Behavior changed:

    • No.
    • Yes.
  • Does this need documentation?

    • No.
    • Yes.

Check List (For Reviewer who merge this PR)

  • Confirm the release note
  • Confirm test cases
  • Confirm document
  • Add branch pick label

@hello-stephen
Copy link
Copy Markdown
Contributor

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

Please clearly describe your PR:

  1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
  2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
  3. What features were added. Why was this function added?
  4. Which code was refactored and why was this part of the code refactored?
  5. Which functions were optimized and what is the difference before and after the optimization?

@Mryange
Copy link
Copy Markdown
Contributor Author

Mryange commented May 7, 2026

/review

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_cast usages 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_cast rule.
  • Conditional checks: The new static_assert is 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.

@Mryange
Copy link
Copy Markdown
Contributor Author

Mryange commented May 7, 2026

run buildall

@Mryange
Copy link
Copy Markdown
Contributor Author

Mryange commented May 8, 2026

run buildall

@Mryange
Copy link
Copy Markdown
Contributor Author

Mryange commented May 8, 2026

run cloud_p0

@HappenLee
Copy link
Copy Markdown
Contributor

/review

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the PR with the Doris code-review checklist.

Critical checkpoint conclusions:

  • Goal and proof: the change rejects redundant same-type assert_cast at 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_cast normalization 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.

Copy link
Copy Markdown
Contributor

@HappenLee HappenLee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@HappenLee HappenLee merged commit 7ca6ffc into apache:master May 8, 2026
30 of 31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants