[BugFix] fix invalid expression usage #56064

xhumanoid · 2025-02-19T15:10:39Z

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:

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

Bugfix cherry-pick branch check:

The feature introduced only from 3.4 version and unavailable in previous releases

CLAassistant · 2025-02-19T15:10:47Z

All committers have signed the CLA.

xhumanoid · 2025-02-19T15:12:28Z

@ZiheLiu could you please review
you introduced initial feature

regression introduced in StarRocks#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 Signed-off-by: Aliaksei Dziomin <[email protected]>

ZiheLiu · 2025-02-26T02:37:44Z

be/src/exec/olap_scan_prepare.cpp

@@ -424,7 +424,7 @@ requires(!lt_is_date<SlotType>) Status ChunkPredicateBuilder<E, Type>::normalize
            ValueType value;
            ASSIGN_OR_RETURN(auto* expr_context, _exprs[i].expr_context(_opts.obj_pool, _opts.runtime_state));
            bool ok =
-                    get_predicate_value<Negative>(_opts.obj_pool, slot, root_expr, expr_context, &value, &op, &status);
+                    get_predicate_value<Negative>(_opts.obj_pool, slot, expr_context->root(), expr_context, &value, &op, &status);


Great! Thanks a lot for your contribution.

Should use get_root_expr(expr_context->root()) instead of expr_context->root().

Maybe it's better to modify BoxedExpr::root() as follows and call _exprs[i].root() instead of expr_context->root().

Expr* BoxedExpr::root() const { if (new_expr_ctx == nullptr) { return get_root_expr(root_expr); } else { return get_root_expr(new_expr_ctx); } }

Should use get_root_expr(expr_context->root()) instead of expr_context->root().

ok, will update

Maybe it's better to modify BoxedExpr::root() as follows and call _exprs[i].root() instead of expr_context->root().

i think it will be more error prone, because need to keep in mind about side effects of expr_context
and root() will return different results before and after expr_context()

i think it will be more error prone, because need to keep in mind about side effects of expr_context
and root() will return different results before and after expr_context()

Make sense. How about making expr_context() return std::pair<ExprContext*, Expr*> [ctx, root] instead of ExprContext*? Then we can use this new root and ctx, which gives a better way to avoid using the new ctx with the old root.

i see at least one place where we need just context, without second part
so as fix i think it have to be enough, but for future refactoring can discuss

Signed-off-by: Aliaksei Dziomin <[email protected]>

github-actions · 2025-02-28T04:48:44Z

[Java-Extensions Incremental Coverage Report]

✅ pass : 0 / 0 (0%)

github-actions · 2025-02-28T04:48:48Z

[FE Incremental Coverage Report]

✅ pass : 0 / 0 (0%)

github-actions · 2025-02-28T04:51:53Z

[BE Incremental Coverage Report]

❌ fail : 3 / 4 (75.00%)

file detail

	path	covered_line	new_line	coverage	not_covered_line_detail
🔵	be/src/exec/olap_scan_prepare.cpp	3	4	75.00%	[539]

github-actions · 2025-02-28T05:48:29Z

@Mergifyio backport branch-3.4

mergify · 2025-02-28T05:48:50Z

backport branch-3.4

✅ Backports have been created

#56405 [BugFix] fix invalid expression usage (backport #56064) has been created for branch branch-3.4

Signed-off-by: Aliaksei Dziomin <[email protected]> (cherry picked from commit 0fc6efc)

xhumanoid requested a review from a team as a code owner February 19, 2025 15:10

github-actions bot added the 3.4 label Feb 19, 2025

mergify bot assigned xhumanoid Feb 19, 2025

xhumanoid force-pushed the fix-incorrect-expression branch from 9852011 to 0b14ed1 Compare February 19, 2025 15:30

ZiheLiu reviewed Feb 26, 2025

View reviewed changes

github-actions bot added 3.3 and removed 3.3 labels Feb 26, 2025

fix comments

5795a73

Signed-off-by: Aliaksei Dziomin <[email protected]>

xhumanoid force-pushed the fix-incorrect-expression branch from a678ab6 to 5795a73 Compare February 27, 2025 13:23

satanson approved these changes Feb 28, 2025

View reviewed changes

kangkaisen approved these changes Feb 28, 2025

View reviewed changes

kangkaisen merged commit 0fc6efc into StarRocks:main Feb 28, 2025
48 of 49 checks passed

github-actions bot removed the 3.4 label Feb 28, 2025

mergify bot pushed a commit that referenced this pull request Feb 28, 2025

[BugFix] fix invalid expression usage (#56064)

d0fd6ff

Signed-off-by: Aliaksei Dziomin <[email protected]> (cherry picked from commit 0fc6efc)

mergify bot mentioned this pull request Feb 28, 2025

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

Open

18 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BugFix] fix invalid expression usage #56064

[BugFix] fix invalid expression usage #56064

xhumanoid commented Feb 19, 2025 •

edited by ZiheLiu

Loading

CLAassistant commented Feb 19, 2025 •

edited

Loading

xhumanoid commented Feb 19, 2025

ZiheLiu Feb 26, 2025 •

edited

Loading

xhumanoid Feb 26, 2025

ZiheLiu Feb 27, 2025 •

edited

Loading

xhumanoid Feb 27, 2025

github-actions bot commented Feb 28, 2025

github-actions bot commented Feb 28, 2025

github-actions bot commented Feb 28, 2025

github-actions bot commented Feb 28, 2025

mergify bot commented Feb 28, 2025 •

edited

Loading

[BugFix] fix invalid expression usage #56064

[BugFix] fix invalid expression usage #56064

Conversation

xhumanoid commented Feb 19, 2025 • edited by ZiheLiu Loading

Why I'm doing:

What I'm doing:

What type of PR is this:

Checklist:

Bugfix cherry-pick branch check:

CLAassistant commented Feb 19, 2025 • edited Loading

xhumanoid commented Feb 19, 2025

ZiheLiu Feb 26, 2025 • edited Loading

Choose a reason for hiding this comment

xhumanoid Feb 26, 2025

Choose a reason for hiding this comment

ZiheLiu Feb 27, 2025 • edited Loading

Choose a reason for hiding this comment

xhumanoid Feb 27, 2025

Choose a reason for hiding this comment

github-actions bot commented Feb 28, 2025

[Java-Extensions Incremental Coverage Report]

github-actions bot commented Feb 28, 2025

[FE Incremental Coverage Report]

github-actions bot commented Feb 28, 2025

[BE Incremental Coverage Report]

file detail

github-actions bot commented Feb 28, 2025

mergify bot commented Feb 28, 2025 • edited Loading

✅ Backports have been created

xhumanoid commented Feb 19, 2025 •

edited by ZiheLiu

Loading

CLAassistant commented Feb 19, 2025 •

edited

Loading

ZiheLiu Feb 26, 2025 •

edited

Loading

ZiheLiu Feb 27, 2025 •

edited

Loading

mergify bot commented Feb 28, 2025 •

edited

Loading