Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BugFix] fix invalid expression usage (backport #56064) #56405

Merged
merged 1 commit into from
Feb 28, 2025

Conversation

mergify[bot]
Copy link
Contributor

@mergify mergify bot commented Feb 28, 2025

regression introduced in
#44588

expr_context(...)

        // Copy expr to prevent two ExprContexts from owning the same Expr, which will cause the same Expr to be
        // closed twice.
        // - The ExprContext in the `original _opts.conjunct_ctxs_ptr` will own an Expr and all its children.
        // - The newly created ExprContext here will also own this Expr.
        auto* new_expr = Expr::copy(obj_pool, root_expr);
        new_expr_ctx = obj_pool->add(new ExprContext(new_expr));
        RETURN_IF_ERROR(new_expr_ctx->prepare(state));
        RETURN_IF_ERROR(new_expr_ctx->open(state));

create copy of root_expr as new_expr and new ExprContext pupulate ExprContext._fn_contexts and update VectorizedFunctionCallExpr._fn_context_index inside new_expr

after return from function we do call of get_predicate_value with new_expr_ctx + root_expr inside root_expr we can have incorrect VectorizedFunctionCallExpr._fn_context_index, because indexes apply for another context

as result later we have errors

    @         0x16c18359 google::LogMessageFatal::~LogMessageFatal()
    @         0x104f7c96 starrocks::ExprContext::fn_context(int)
    @         0x105b4c11 starrocks::VectorizedFunctionCallExpr::evaluate_checked(starrocks::ExprContext*, starrocks::Chunk*)
    @         0x103a737a starrocks::VectorizedCastToStringExpr<(starrocks::LogicalType)51, false>::evaluate_checked(starrocks::ExprContext*, starrocks::Chunk*)
    FunctionContext* fn_context(int i) {
        DCHECK_GE(i, 0);
        DCHECK_LT(i, _fn_contexts.size());
        return _fn_contexts[i];
    }

for our case _fn_context_index = 3 and _fn_contexts.size() = 2

after fix we will use proper updated expressions with correct expression context

Why I'm doing:

What I'm doing:

Fixes #issue

What type of PR is this:

  • BugFix
  • Feature
  • Enhancement
  • Refactor
  • UT
  • Doc
  • Tool

Does this PR entail a change in behavior?

  • Yes, this PR will result in a change in behavior.
  • No, this PR will not result in a change in behavior.

If yes, please specify the type of change:

  • Interface/UI changes: syntax, type conversion, expression evaluation, display information
  • Parameter changes: default values, similar parameters but with different default values
  • Policy changes: use new policy to replace old one, functionality automatically enabled
  • Feature removed
  • Miscellaneous: upgrade & downgrade compatibility, etc.

Checklist:

  • I have added test cases for my bug fix or my new feature
  • This pr needs user documentation (for new or modified features or behaviors)
    • I have added documentation for my new feature or new function
  • This is a backport pr

Signed-off-by: Aliaksei Dziomin <[email protected]>
(cherry picked from commit 0fc6efc)
@mergify mergify bot mentioned this pull request Feb 28, 2025
24 tasks
@wanpengfei-git wanpengfei-git enabled auto-merge (squash) February 28, 2025 05:50
@wanpengfei-git wanpengfei-git merged commit 89fd419 into branch-3.4 Feb 28, 2025
32 of 33 checks passed
@wanpengfei-git wanpengfei-git deleted the mergify/bp/branch-3.4/pr-56064 branch February 28, 2025 06:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants