From b8aad70d13074d413c5dcd3045c53c3875f95890 Mon Sep 17 00:00:00 2001 From: "lijinghan.1029" Date: Wed, 9 Sep 2026 18:19:30 +0800 Subject: [PATCH] [fix](function) Fix BE crash in trim when input column is ColumnConst Trim1Impl/Trim2Impl directly assert_cast on the input column. When the input reaching the implementation is a ColumnConst (which can happen on pushdown / materialization paths that invoke the function implementation directly, bypassing the const-unwrapping in default_implementation_for_constant_arguments), the assert_cast hits the wrong type and aborts the BE (undefined behavior in release builds). Unpack the const first via unpack_if_const, and use check_and_get_column, which returns nullptr on a type mismatch and falls through to the existing RuntimeError branch instead of crashing. This matches the defensive pattern used elsewhere in the codebase and does not change behavior on the normal column path. Add a regression test that builds a ColumnConst block and calls execute_impl directly, since the normal execute() entry unwraps all-constant arguments before reaching the implementation and cannot reproduce the crash. Signed-off-by: lijinghan.1029 Conflicts: be/test/exprs/function/function_string_test.cpp --- be/src/exprs/function/function_string.cpp | 14 ++-- .../exprs/function/function_string_test.cpp | 76 +++++++++++++++++++ 2 files changed, 85 insertions(+), 5 deletions(-) diff --git a/be/src/exprs/function/function_string.cpp b/be/src/exprs/function/function_string.cpp index 91e044343745c8..ae5efe6d40f7b9 100644 --- a/be/src/exprs/function/function_string.cpp +++ b/be/src/exprs/function/function_string.cpp @@ -37,11 +37,13 @@ #include "common/logging.h" #include "common/status.h" #include "core/column/column.h" +#include "core/column/column_const.h" #include "core/column/column_string.h" #include "core/data_type/data_type_nullable.h" #include "core/pod_array_fwd.h" #include "core/string_ref.h" #include "exprs/expr_zonemap_filter.h" +#include "exprs/function/function_helpers.h" #include "exprs/function/function_reverse.h" #include "exprs/function/function_string_concat.h" #include "exprs/function/function_string_format.h" @@ -886,8 +888,9 @@ struct Trim1Impl { static Status execute(FunctionContext* context, Block& block, const ColumnNumbers& arguments, uint32_t result, size_t input_rows_count) { - const ColumnPtr column = block.get_by_position(arguments[0]).column; - if (const auto* col = assert_cast(column.get())) { + const auto& [column, left_const] = + unpack_if_const(block.get_by_position(arguments[0]).column); + if (const auto* col = check_and_get_column(column.get())) { auto col_res = ColumnString::create(); char blank[] = " "; const StringRef remove_str(blank, 1); @@ -915,12 +918,13 @@ struct Trim2Impl { static Status execute(FunctionContext* context, Block& block, const ColumnNumbers& arguments, uint32_t result, size_t input_rows_count) { - const ColumnPtr column = block.get_by_position(arguments[0]).column; + const auto& [column, left_const] = + unpack_if_const(block.get_by_position(arguments[0]).column); const auto& rcol = assert_cast(block.get_by_position(arguments[1]).column.get()) ->get_data_column_ptr(); - if (const auto* col = assert_cast(column.get())) { - if (const auto* col_right = assert_cast(rcol.get())) { + if (const auto* col = check_and_get_column(column.get())) { + if (const auto* col_right = check_and_get_column(rcol.get())) { auto col_res = ColumnString::create(); const auto* remove_str_raw = col_right->get_chars().data(); const ColumnString::Offset remove_str_size = col_right->get_offsets()[0]; diff --git a/be/test/exprs/function/function_string_test.cpp b/be/test/exprs/function/function_string_test.cpp index 9b7e0d793abfea..94ae64e09c4246 100644 --- a/be/test/exprs/function/function_string_test.cpp +++ b/be/test/exprs/function/function_string_test.cpp @@ -21,11 +21,15 @@ #include #include +#include "core/block/block.h" +#include "core/column/column_const.h" +#include "core/column/column_string.h" #include "core/data_type/data_type_number.h" #include "core/data_type/data_type_string.h" #include "core/field.h" #include "core/types.h" #include "exprs/function/function_test_util.h" +#include "exprs/function/simple_function_factory.h" #include "util/encryption_util.h" #include "util/md5.h" @@ -697,6 +701,78 @@ TEST(function_string_test, function_string_rtrim_test) { check_function_all_arg_comb(func_name, input_types, data_set); } +// Regression for a BE crash: when the input column reaching Trim1Impl/Trim2Impl +// is a ColumnConst (as happens on some pushdown / materialization paths that call +// the function implementation directly instead of through the normal execute() +// entry), the implementation used to assert_cast the const-wrapped +// column and abort. The fix unpacks the const first via unpack_if_const. +// +// Note: the normal check_function path cannot reproduce this, because an +// all-constant call is unwrapped by default_implementation_for_constant_arguments +// before reaching the implementation, and a partially-constant call is impossible +// since trim forces its second argument to be constant. So we build the +// ColumnConst block ourselves and call execute_impl directly. +TEST(function_string_test, function_string_trim_const_input_regression) { + const size_t input_rows_count = 4; + + auto call_trim_impl_with_const = [&](const std::string& func_name, + const ColumnsWithTypeAndName& args, + const std::string& expected) { + auto return_type = std::make_shared(); + auto function = + SimpleFunctionFactory::instance().get_function(func_name, args, return_type); + ASSERT_TRUE(function != nullptr) << func_name; + + // prepare() on the DefaultFunction wrapper returns the underlying IFunction, + // whose execute_impl is public and skips the const-unwrapping layer. + Block dummy(args); + auto prepared = function->prepare(nullptr, dummy, {}, 0); + auto impl = std::dynamic_pointer_cast(prepared); + ASSERT_TRUE(impl != nullptr) << func_name; + + Block block(args); + ColumnNumbers arguments(args.size()); + for (size_t i = 0; i < args.size(); ++i) { + arguments[i] = static_cast(i); + } + uint32_t result_idx = static_cast(args.size()); + block.insert({nullptr, return_type, "result"}); + + Status st = impl->execute_impl(nullptr, block, arguments, result_idx, input_rows_count); + ASSERT_TRUE(st.ok()) << func_name << ": " << st.to_string(); + + auto res_col = block.get_by_position(result_idx).column; + ASSERT_TRUE(res_col.get() != nullptr) << func_name; + // The result may itself be wrapped in a ColumnConst; unwrap for comparison. + if (const auto* const_col = check_and_get_column(res_col.get())) { + res_col = const_col->get_data_column_ptr(); + } + const auto* str_col = assert_cast(res_col.get()); + auto ref = str_col->get_data_at(0); + EXPECT_EQ(std::string(ref.data, ref.size), expected) << func_name; + }; + + auto make_const_string = [&](const std::string& value) -> ColumnWithTypeAndName { + auto nested = ColumnString::create(); + nested->insert_data(value.data(), value.size()); + return {ColumnConst::create(std::move(nested), input_rows_count), + std::make_shared(), "arg"}; + }; + + // Single-argument trim family: the sole argument reaches Trim1Impl as a + // ColumnConst. This is the exact shape that previously crashed. + call_trim_impl_with_const("trim", {make_const_string(" spaced out string ")}, + "spaced out string"); + call_trim_impl_with_const("ltrim", {make_const_string(" leading ")}, "leading "); + call_trim_impl_with_const("rtrim", {make_const_string(" trailing ")}, " trailing"); + + // Two-argument trim: the first argument reaches Trim2Impl as a ColumnConst, + // exercising its unpack_if_const path. The second (remove-string) argument is + // always constant. + call_trim_impl_with_const("trim", {make_const_string("xxhelloxx"), make_const_string("x")}, + "hello"); +} + TEST(function_string_test, function_string_repeat_test) { std::string func_name = "repeat"; {