Skip to content

[fix](fe) Accept foldable gram numbers in ngram_search #68311

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.

Sign up for GitHub

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

Draft
mrhhsg wants to merge 4 commits into
base: master
Choose a base branch
from
+723 −8
Draft

[fix](fe) Accept foldable gram numbers in ngram_search #68311

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c808a29
[fix](fe) Accept foldable gram numbers in ngram_search
mrhhsg Sep 21, 2026
ea279c4
[fix](be) Validate backend-evaluated ngram sizes
mrhhsg Sep 21, 2026
d487595
[fix](fe) Validate ngram constants before expression rewrites
mrhhsg Sep 21, 2026
38b7a46
[fix](fe) Preserve exclusions in required BE constant folding
mrhhsg Sep 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 40 additions & 4 deletions be/src/exprs/function/function_string_misc.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -809,8 +809,14 @@ class FunctionNgramSearch : public IFunction {
static FunctionPtr create() { return std::make_shared<FunctionNgramSearch>(); }
String get_name() const override { return name; }
size_t get_number_of_arguments() const override { return 3; }
bool use_default_implementation_for_nulls() const override { return false; }

DataTypePtr get_return_type_impl(const DataTypes& arguments) const override {
return std::make_shared<DataTypeFloat64>();
auto result = std::make_shared<DataTypeFloat64>();
if (std::ranges::any_of(arguments, [](const auto& type) { return type->is_nullable(); })) {
return make_nullable(result);
}
return result;
}

// ngram_search(text,pattern,gram_num)
Expand All @@ -820,12 +826,37 @@ class FunctionNgramSearch : public IFunction {
auto col_res = ColumnFloat64::create();
bool col_const[3];
ColumnPtr argument_columns[3];
NullableColumnInfos nullable_infos(block.columns());
bool only_null = false;
for (int i = 0; i < 3; ++i) {
const auto& argument = block.get_by_position(arguments[i]);
auto& info = nullable_infos[arguments[i]];
if (argument.type->is_nullable()) {
info = argument.get_nullable_column_info();
only_null |= info.only_null;
}
std::tie(argument_columns[i], col_const[i]) =
unpack_if_const(block.get_by_position(arguments[i]).column);
unpack_if_const(argument.unnest_nullable(info, false).column);
}
// A semantic constant can arrive as a materialized CSE slot. Validate its first value
// before propagating NULL from any argument, including text or pattern.
if (nullable_infos[arguments[2]].is_nullable &&
block.get_by_position(arguments[2]).get_nullable_null_map_column()->get_data()[0]) {
return Status::InvalidArgument(
"ngram_search(text,pattern,gram_num): gram_num support const value only.");
}
auto pattern = assert_cast<const ColumnString*>(argument_columns[1].get())->get_data_at(0);
auto gram_num = assert_cast<const ColumnInt32*>(argument_columns[2].get())->get_element(0);
if (gram_num <= 0) {

@github-actions github-actions Bot Sep 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[P2] Validate the gram even when execution is skipped

This is the only value check, but execute_impl is not guaranteed to run. crc32('abc') % 0 remains a constant integral tree in FE and evaluates to NULL on BE, where default NULL propagation returns before this line. Also, select ngram_search(cast(number as string), 'abc', crc32('abc') % 3) from numbers("number"="0") has a row-dependent root, so the empty projection skips the function and the known-zero gram is never rejected. Literal NULL/zero grams are rejected during analysis regardless of these shapes, so newly admitted BE-only constants change the contract. Please validate semantic constants on a path that survives CSE and zero-row execution, while retaining a pre-NULL batch check for materialized nonempty slots, and cover both cases in both fold modes.

@mrhhsg mrhhsg Sep 21, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed in d487595.

The gram is now resolved and validated during function binding, before NULL propagation, CSE, or empty-plan rewrites can discard the call. FE-evaluable constants stay local; other constants use the existing BE evaluator regardless of the optional BE-fold setting. The evaluated INT literal is retained in the plan, and failed evaluation is reported rather than silently deferring validation to execution.

BE also checks materialized grams before propagating NULL from text/pattern. Regression coverage includes crc32('abc') % 0, zero/negative grams with zero rows, CSE-shaped expressions, NULL text, WHERE false, and LIMIT 0, in both fold modes. This also covers the case where NULL text previously removed the entire function on FE, which a BE-only open() fix would miss.

Validation: FE UT 12/12, ASAN BE UT 7/7, ASAN FE/BE build, and the new/existing string regression suites 2/2 passed. The prior 26 SQL probes now produce the expected outcomes. clang-tidy remains blocked by the existing unmatched NOLINTEND in core/types.h, with no emitted changed-line diagnostic.

The PR description explicitly records the additional planning RPC for BE-only grams, its existing five-second timeout, and the planner-lock waiting tradeoff. This thread is left for re-review rather than manually resolved.

return Status::InvalidArgument(
"ngram_search(text,pattern,gram_num): gram_num must be a positive constant.");
}
if (only_null) {
block.replace_by_position(result,
block.get_by_position(result).type->create_column_const(
input_rows_count, Field()));
return Status::OK();
}
auto pattern = assert_cast<const ColumnString*>(argument_columns[1].get())->get_data_at(0);
const auto* text_col = assert_cast<const ColumnString*>(argument_columns[0].get());

if (col_const[0]) {
Expand All @@ -834,7 +865,12 @@ class FunctionNgramSearch : public IFunction {
_execute_impl<false>(text_col, pattern, gram_num, *col_res, input_rows_count);
}

block.replace_by_position(result, std::move(col_res));
if (block.get_by_position(result).type->is_nullable()) {
block.replace_by_position(result, wrap_in_nullable(std::move(col_res), block, arguments,
nullable_infos, input_rows_count));
} else {
block.replace_by_position(result, std::move(col_res));
}
return Status::OK();
}

Expand Down
91 changes: 91 additions & 0 deletions be/test/exprs/function/function_string_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,97 @@ DataSet make_md5_varbinary_dataset(const std::vector<std::string>& inputs) {

} // namespace

TEST(function_string_test, ngram_search_constant_gram) {
const InputTypeSet input_types = {ConstedNotnull {TYPE_STRING}, ConstedNotnull {TYPE_STRING},
ConstedNotnull {TYPE_INT}};
for (int32_t gram : {1, 3, 4}) {
const double expected = gram <= 3 ? 1.0 : 0.0;
const DataSet data_set = {{{std::string("abc"), std::string("abc"), gram}, expected}};
ASSERT_TRUE(check_function<DataTypeFloat64>("ngram_search", input_types, data_set).ok());
}
}

TEST(function_string_test, ngram_search_nullable_gram) {
const InputTypeSet input_types = {TYPE_STRING, Consted {TYPE_STRING}, Consted {TYPE_INT}};
const DataSet data_set = {{{std::string("abc"), std::string("abc"), int32_t(1)}, 1.0},
{{std::string("ab"), std::string("abc"), int32_t(1)}, 0.8},
{{Null(), std::string("abc"), int32_t(1)}, Null()}};
for (const auto& row : data_set) {
ASSERT_TRUE(
(check_function<DataTypeFloat64, true>("ngram_search", input_types, {row}).ok()));
}
}

TEST(function_string_test, ngram_search_nonpositive_gram) {
const InputTypeSet input_types = {Notnull {TYPE_STRING}, ConstedNotnull {TYPE_STRING},
ConstedNotnull {TYPE_INT}};
const InputTypeSet nullable_types = {TYPE_STRING, Consted {TYPE_STRING}, Consted {TYPE_INT}};
for (int32_t gram : {0, -1}) {
const DataSet data_set = {{{std::string("abc"), std::string("abc"), gram}, 0.0}};
auto st = check_function<DataTypeFloat64>("ngram_search", input_types, data_set, -1, -1,
true);
EXPECT_FALSE(st.ok());
EXPECT_NE(st.to_string().find("gram_num must be a positive constant"), std::string::npos);
st = check_function<DataTypeFloat64, true>("ngram_search", nullable_types, data_set, -1, -1,
true);
EXPECT_FALSE(st.ok());
EXPECT_NE(st.to_string().find("gram_num must be a positive constant"), std::string::npos);
}
}

TEST(function_string_test, ngram_search_materialized_gram) {
// CSE can materialize a constant gram expression into an intermediate slot.
const InputTypeSet input_types = {Notnull {TYPE_STRING}, ConstedNotnull {TYPE_STRING},
Notnull {TYPE_INT}};
ASSERT_TRUE(check_function<DataTypeFloat64>(
"ngram_search", input_types,
{{{std::string("ab"), std::string("abc"), int32_t(1)}, 0.8}})
.ok());
const auto st = check_function<DataTypeFloat64>(
"ngram_search", input_types,
{{{std::string("ab"), std::string("abc"), int32_t(0)}, 0.0}}, -1, -1, true);
EXPECT_FALSE(st.ok());
EXPECT_NE(st.to_string().find("gram_num must be a positive constant"), std::string::npos);
}

TEST(function_string_test, ngram_search_null_gram) {
for (const auto& input_types :
{InputTypeSet {TYPE_STRING, Consted {TYPE_STRING}, Consted {TYPE_INT}},
InputTypeSet {TYPE_STRING, Consted {TYPE_STRING}, TYPE_INT}}) {
for (const auto& text : {AnyType(std::string("abc")), AnyType(Null())}) {
const auto st = check_function<DataTypeFloat64, true>(
"ngram_search", input_types, {{{text, std::string("abc"), Null()}, Null()}}, -1,
-1, true);
EXPECT_FALSE(st.ok());
EXPECT_NE(st.to_string().find("gram_num support const value only"), std::string::npos);
}
}
}

TEST(function_string_test, ngram_search_invalid_gram_with_null_text) {
for (const auto& input_types :
{InputTypeSet {Consted {TYPE_STRING}, Consted {TYPE_STRING}, Consted {TYPE_INT}},
InputTypeSet {TYPE_STRING, Consted {TYPE_STRING}, TYPE_INT}}) {
for (int32_t gram : {0, -1}) {
const auto st = check_function<DataTypeFloat64, true>(
"ngram_search", input_types, {{{Null(), std::string("abc"), gram}, Null()}}, -1,
-1, true);
EXPECT_FALSE(st.ok());
EXPECT_NE(st.to_string().find("gram_num must be a positive constant"),
std::string::npos);
}
}
}

TEST(function_string_test, ngram_search_nullable_multirow) {
const InputTypeSet input_types = {TYPE_STRING, TYPE_STRING, TYPE_INT};
const DataSet data_set = {{{std::string("abc"), std::string("abc"), int32_t(1)}, 1.0},
{{Null(), std::string("abc"), int32_t(1)}, Null()},
{{std::string("ab"), std::string("abc"), int32_t(1)}, 0.8}};
ASSERT_TRUE(
(check_function<DataTypeFloat64, true>("ngram_search", input_types, data_set).ok()));
}

TEST(function_string_test, parse_data_size_nullable) {
const InputTypeSet input_types = {PrimitiveType::TYPE_STRING};
const DataSet data_set = {{{Null()}, Null()},
Expand Down
15 changes: 15 additions & 0 deletions ...e/src/main/java/org/apache/doris/nereids/rules/expression/rules/FoldConstantRuleOnBE.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,21 @@ public static Expression foldByBE(ExpressionMatchingContext<Expression> context)
return root;
}

/** Evaluate a semantic constant whose value is required for argument validation. */
public static Expression evaluateConstant(Expression expression, ConnectContext context) {

@github-actions github-actions Bot Sep 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[P1] Preserve the BE-fold safety exclusions here

This direct path skips the anyMatch(shouldSkipFold) gate used by collectConst. For example, 1 + cast(sleep(3600) as int) is a deterministic integral constant, so ngram_search('abc', 'abc', 1 + cast(sleep(3600) as int)) reaches this method during binding even under WHERE false. The BE fold RPC then runs FunctionSleep; FE times out after five seconds without cancelling the future, so the BE light-pool task keeps sleeping, while shorter sleeps execute during planning and are replaced by a literal. Sleep is explicitly excluded from ordinary BE folding for exactly this timeout reason, and the same gate also protects AI/search/context-bound expressions. Please preserve those exclusions for required evaluation (reject unsafe grams without dispatching them, while still allowing safe cases such as crc32) and add a no-RPC regression for a skipped expression.

@mrhhsg mrhhsg Sep 21, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

已在 38b7a46 修复。

evaluateConstant 现在在翻译表达式、发送 RPC 之前，复用普通折叠的递归 anyMatch(shouldSkipFold) 检查。被排除的表达式保持未求值，由 ngram 参数校验明确拒绝；没有另建排除表，也没有把校验退回 batch 执行阶段。安全的 crc32 常量仍可正常求值。

补充验证：

  • RPC 边界单测覆盖直接/嵌套 Sleep 和 NonNullable，断言零 RPC；同一可用 backend fixture 下，安全 crc32 对照确实发送 RPC 并得到 literal。两种 folding 设置均覆盖。这两个 no-RPC 用例在旧代码上失败，修复后通过。
  • SQL 回归增加 SELECT、EXPLAIN、WHERE false 和 NULL text 下的 sleep(0) 拒绝用例，两种 folding 设置均覆盖。
  • 17 个 FE 单测、2 套回归、ASAN FE/BE 构建、FE Checkstyle 和 diff 检查通过。
  • 禁用 SQL cache 的有限手工测试中，sleep(1)/sleep(8) gram 现在约 14–15 ms 即被拒绝，拒绝后未观察到睡眠中的 BE light-pool worker；安全 crc32 对照正常。

本次保留的是执行准入限制，并未将 FE 的五秒等待上限描述为 BE 取消保证。

// Required evaluation must also honor exclusions such as Sleep, which can outlive
// the RPC timeout. Leave excluded expressions unevaluated for the caller to reject.
if (expression.anyMatch(e -> shouldSkipFold((Expression) e))) {
return expression;
}
Expr legacyExpr = ExpressionTranslator.translate(expression, null);
Map<String, Expression> constants = Collections.singletonMap("0", expression);
Map<String, TExpr> thriftExpressions = Collections.singletonMap(
"0", ExprToThriftVisitor.treeToThrift(legacyExpr));
return evalOnBE(Collections.singletonMap("0", thriftExpressions), constants, context)

@github-actions github-actions Bot Sep 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[P2] Retry another healthy peer before rejecting the gram

This required path turns evalOnBE's best-effort miss into an analysis error, but evalOnBE shuffles the heartbeat-alive IDs and tries only backendIds.get(0). A selected BE whose BRPC endpoint is restarting, whose light pool rejects the request, or whose transport fails returns the original expression; withFoldedGramNumber then rejects a valid safe gram such as crc32('abc') % 3 + 1 even when other BEs can answer. Same-address channel retries do not provide peer fallback. Please retry remaining compatible peers for retryable endpoint/transport/overload failures within one shared overall deadline (not five seconds per peer, and not deterministic expression failures), and cover first-peer failure followed by second-peer success.

.getOrDefault("0", expression);
}

private static Expression replace(
Expression root, Map<String, Expression> constMap, Map<String, Expression> resultMap) {
for (Entry<String, Expression> entry : constMap.entrySet()) {
Expand Down
40 changes: 36 additions & 4 deletions ...rc/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/NgramSearch.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,20 @@

import org.apache.doris.catalog.FunctionSignature;
import org.apache.doris.nereids.exceptions.AnalysisException;
import org.apache.doris.nereids.rules.expression.rules.FoldConstantRuleOnBE;
import org.apache.doris.nereids.rules.expression.rules.FoldConstantRuleOnFE;
import org.apache.doris.nereids.trees.expressions.Cast;
import org.apache.doris.nereids.trees.expressions.Expression;
import org.apache.doris.nereids.trees.expressions.functions.ExplicitlyCastableSignature;
import org.apache.doris.nereids.trees.expressions.functions.PropagateNullable;
import org.apache.doris.nereids.trees.expressions.literal.IntegerLikeLiteral;
import org.apache.doris.nereids.trees.expressions.literal.Literal;
import org.apache.doris.nereids.trees.expressions.literal.NullLiteral;
import org.apache.doris.nereids.trees.expressions.visitor.ExpressionVisitor;
import org.apache.doris.nereids.types.DoubleType;
import org.apache.doris.nereids.types.IntegerType;
import org.apache.doris.nereids.types.StringType;
import org.apache.doris.qe.ConnectContext;

import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
Expand Down Expand Up @@ -57,21 +63,47 @@ private NgramSearch(ScalarFunctionParams functionParams) {

@Override
public void checkLegalityBeforeTypeCoercion() {
if (!child(1).isConstant()) {
if (!getArgument(1).isConstant()) {
throw new AnalysisException(
"ngram_search(text,pattern,gram_num): pattern support const value only.");
}
Expression gramNum = child(2);
if (!(gramNum instanceof IntegerLikeLiteral)) {
Expression gramNum = getArgument(2);
if (!gramNum.isConstant() || !gramNum.getDataType().isIntegralType()) {
throw new AnalysisException(
"ngram_search(text,pattern,gram_num): gram_num support const value only.");
}
if (((IntegerLikeLiteral) gramNum).getIntValue() <= 0) {
gramNum = FoldConstantRuleOnFE.evaluateWithoutContext(gramNum);
if (gramNum instanceof NullLiteral) {
throw new AnalysisException(
"ngram_search(text,pattern,gram_num): gram_num support const value only.");
}
if (gramNum instanceof IntegerLikeLiteral && ((IntegerLikeLiteral) gramNum).getIntValue() <= 0) {
throw new AnalysisException(
"ngram_search(text,pattern,gram_num): gram_num must be a positive constant.");
}
}

/** Resolve the required constant before rewrites can discard the function call. */
public NgramSearch withFoldedGramNumber() {
Expression gramNum = getArgument(2);
if (!gramNum.getDataType().equals(IntegerType.INSTANCE)) {
gramNum = new Cast(gramNum, IntegerType.INSTANCE);
}
gramNum = FoldConstantRuleOnFE.evaluateWithoutContext(gramNum);
// Argument validation is independent of the optional BE folding setting. Keep the
// evaluated value in the plan so CSE and execution use exactly the value we validate.
if (!(gramNum instanceof Literal)) {

@github-actions github-actions Bot Sep 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[P1] Batch required folds before binding under table locks

For a cold plan over an internal table, collectAndLockTable acquires read locks before analyze(), and those locks are released only after planning. Every BE-only gram reaching this line then performs its own synchronous singleton RPC. A single projection expression containing several safe ngram_search(..., crc32(...) % 3 + 1) calls therefore creates N serialized RPCs and N fresh BE fold executors/runtime states before CSE or ordinary batched folding can run. Successful-but-slow calls can multiply the same metadata-lock interval, blocking DDL; even healthy calls add repeated network/setup cost. This is distinct from the already disclosed single planning RPC. Please collect/batch these required constants outside table-lock ownership (or evaluate them locally) and add multi-expression/lock-ownership coverage.

gramNum = FoldConstantRuleOnBE.evaluateConstant(gramNum, ConnectContext.get());
if (!(gramNum instanceof Literal)) {
throw new AnalysisException(
"ngram_search(text,pattern,gram_num): failed to evaluate constant gram_num.");
}
}
NgramSearch folded = withChildren(ImmutableList.of(getArgument(0), getArgument(1), gramNum));
folded.checkLegalityBeforeTypeCoercion();
return folded;
}

/**
* withChildren.
*/
Expand Down
5 changes: 5 additions & 0 deletions fe/fe-core/src/main/java/org/apache/doris/nereids/util/TypeCoercionUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@
import org.apache.doris.nereids.trees.expressions.functions.scalar.CreateMap;
import org.apache.doris.nereids.trees.expressions.functions.scalar.CreateStruct;
import org.apache.doris.nereids.trees.expressions.functions.scalar.ElementAt;
import org.apache.doris.nereids.trees.expressions.functions.scalar.NgramSearch;
import org.apache.doris.nereids.trees.expressions.literal.ArrayLiteral;
import org.apache.doris.nereids.trees.expressions.literal.BigIntLiteral;
import org.apache.doris.nereids.trees.expressions.literal.BooleanLiteral;
Expand Down Expand Up @@ -847,6 +848,10 @@ public static Expression processBoundFunction(BoundFunction boundFunction) {
return new MapLiteral();
}

if (boundFunction instanceof NgramSearch) {
boundFunction = ((NgramSearch) boundFunction).withFoldedGramNumber();
}

// type coercion
return implicitCastInputTypes(boundFunction, boundFunction.expectedInputTypes());
}
Expand Down
Loading
Loading