Skip to content

[refine](function) unify use_default_implementation_for_constants interface#62711

Open
Mryange wants to merge 1 commit intoapache:masterfrom
Mryange:fix-array_match-const-process
Open

[refine](function) unify use_default_implementation_for_constants interface#62711
Mryange wants to merge 1 commit intoapache:masterfrom
Mryange:fix-array_match-const-process

Conversation

@Mryange
Copy link
Copy Markdown
Contributor

@Mryange Mryange commented Apr 22, 2026

Problem

IFunctionBase declared a pure virtual method is_use_default_implementation_for_constants(),
while PreparedFunctionImpl declared a separate method use_default_implementation_for_constants()
with the same semantics but different name.

IFunction bridged them with a forwarding override, but this caused confusion:

  • Subclasses that directly overrode is_use_default_implementation_for_constants() on IFunction
    bypassed the PreparedFunctionImpl method, causing the two to return inconsistent values.
  • For example, ArrayMatchFunction overrode is_use_default_implementation_for_constants() to
    return false, but use_default_implementation_for_constants() still returned true (default),
    so the execution framework still unpacked const columns, while execute_impl also called
    unpack_if_const redundantly.

Solution

  • Rename IFunctionBase::is_use_default_implementation_for_constants() to
    use_default_implementation_for_constants() to unify the two interfaces into one name.
  • Add an explicit override in IFunction to resolve the multiple-inheritance ambiguity between
    IFunctionBase and PreparedFunctionImpl.
  • Remove the redundant is_use_default_implementation_for_constants() bridge in IFunction.
  • Fix ArrayMatchFunction: remove the incorrect is_use_default_implementation_for_constants()
    override and the unused unpack_if_const call, letting the framework handle const columns via
    the default use_default_implementation_for_constants() = true path.
  • Remove the duplicate override in FunctionSearch.

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

@Mryange Mryange requested a review from airborne12 as a code owner April 22, 2026 09:20
@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 Apr 22, 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.

No blocking findings.

Critical Checkpoints

  • Goal and correctness: The PR goal is to unify the constant-handling interface so the function property checked by VectorizedFnCall and the property used by execution share the same override path. The rename and the explicit IFunction override accomplish that, and the array_match_* cleanup is consistent with the framework constant path.
  • Scope: The change is small and focused to the constant-handling interface, its call sites, and the two affected functions.
  • Concurrency: No new concurrency, lock, or thread-safety behavior is introduced in the reviewed paths.
  • Lifecycle and static initialization: No special lifecycle or static initialization changes are involved.
  • Configuration: No config changes.
  • Compatibility: This is an internal BE interface rename only. I did not find FE/BE protocol, storage format, or rolling-upgrade compatibility impact.
  • Parallel code paths: I checked both IFunction and direct IFunctionBase implementations touched here (cast, Java UDF, Python UDF, RPC, search). The rename is applied consistently across the relevant dispatch paths.
  • Special conditions: ArrayMatchFunction no longer manually unpacks const columns and now relies on the default constant implementation, which matches PreparedFunctionImpl behavior.
  • Tests: Existing regression coverage exercises array_match_all/any, and the obsolete FunctionSearch property assertion was updated. There is still no new targeted test for the unified constant-dispatch path itself, so that remains a small residual gap but not a blocker from my review.
  • Test result files: None changed.
  • Observability: No new observability requirement for this refactor.
  • Transaction, persistence, and data-write safety: Not applicable.
  • Performance: The change slightly simplifies the array_match_* path by removing redundant const unpacking.
  • Other issues: None found in the reviewed diff.

Overall, this PR looks good to merge from the reviewed code paths.

@Mryange
Copy link
Copy Markdown
Contributor Author

Mryange commented Apr 22, 2026

run buildall

@hello-stephen
Copy link
Copy Markdown
Contributor

BE Regression && UT Coverage Report

Increment line coverage 70.00% (7/10) 🎉

Increment coverage report
Complete coverage report

Category Coverage
Function Coverage 73.55% (27520/37417)
Line Coverage 57.36% (297828/519243)
Region Coverage 54.65% (248241/454263)
Branch Coverage 56.14% (107303/191129)

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.

2 participants