From bb73b32f983e8a2ac13ebb860c06b60c908c7aff Mon Sep 17 00:00:00 2001 From: Hu Shenggang Date: Tue, 22 Sep 2026 10:45:39 +0800 Subject: [PATCH] [fix](fe) Accept constant expressions as the gram_num of ngram_search ### What problem does this PR solve? Issue Number: None Problem Summary: `ngram_search('abc', 'abc', 3)` works, but the semantically equivalent `ngram_search('abc', 'abc', 1 + 2)` fails with `gram_num support const value only`. `NgramSearch.checkLegalityBeforeTypeCoercion()` runs before constant folding and only accepted an `IntegerLikeLiteral`, so any constant expression (`1 + 2`, `CAST('3' AS INT)`, `ABS(-3)`, `crc32('abc') % 3 + 1`) was rejected as nonconstant. Fix: - FE checks that `gram_num` is a constant integral expression instead of a literal. Values FE can already evaluate at analysis time (`1 - 1`, `CAST('0' AS INT)`, NULL) are rejected right there, before NULL propagation or plan pruning could drop the call. After constant folding, `checkLegalityAfterRewrite()` validates the folded literal the same way as LAG/LEAD do for their offset. - A constant that FE cannot fold (e.g. `crc32('abc') % 3 + 1`) is left to the normal execution path; no planning-time RPC is added. BE validates the evaluated `gram_num` once per batch at the start of `execute_impl` and returns `InvalidArgument` for a nonpositive value instead of using it as a substring length. This path is also reached when the whole constant call is folded on BE. Column references, nondeterministic expressions, noninteger literals, NULL and nonpositive literals are still rejected with the existing messages. The function signature, protocol and scoring algorithm are unchanged. ### Release note `ngram_search` now accepts any constant integer expression as `gram_num`, not only an integer literal. ### Check List (For Author) - Test: - Unit Test: `./run-fe-ut.sh --run org.apache.doris.nereids.trees.expressions.functions.scalar.NgramSearchTest` (10 passed); `./run-be-ut.sh --run --filter='function_string_test.ngram_search*'` (2 passed, ASAN) - Regression test: new `test_ngram_search_gram_num` and existing `test_string_function` passed on a local ASAN cluster; `.out` generated by the runner - Build/style: `./build.sh --be --fe` (ASAN) passed, FE Checkstyle 0 violations, clang-format v16 and `check-build-hygiene.sh` passed - Behavior changed: Yes. Constant integer expressions are accepted as `gram_num`; a nonpositive value that only BE can evaluate is rejected at execution time. - Does this need documentation: No Claude-Session: https://claude.ai/code/session_01Xi4iWPAWJZ1bVphpLpKZ2j --- .../exprs/function/function_string_misc.cpp | 7 + .../exprs/function/function_string_test.cpp | 28 ++++ .../functions/scalar/NgramSearch.java | 28 +++- .../functions/scalar/NgramSearchTest.java | 148 ++++++++++++++++++ .../test_ngram_search_gram_num.out | 73 +++++++++ .../test_ngram_search_gram_num.groovy | 87 ++++++++++ 6 files changed, 367 insertions(+), 4 deletions(-) create mode 100644 fe/fe-core/src/test/java/org/apache/doris/nereids/trees/expressions/functions/scalar/NgramSearchTest.java create mode 100644 regression-test/data/query_p0/sql_functions/string_functions/test_ngram_search_gram_num.out create mode 100644 regression-test/suites/query_p0/sql_functions/string_functions/test_ngram_search_gram_num.groovy diff --git a/be/src/exprs/function/function_string_misc.cpp b/be/src/exprs/function/function_string_misc.cpp index 86918be9556f91..fdbedae326c4c5 100644 --- a/be/src/exprs/function/function_string_misc.cpp +++ b/be/src/exprs/function/function_string_misc.cpp @@ -826,6 +826,13 @@ class FunctionNgramSearch : public IFunction { } auto pattern = assert_cast(argument_columns[1].get())->get_data_at(0); auto gram_num = assert_cast(argument_columns[2].get())->get_element(0); + // FE only rejects a nonpositive gram_num once it is a literal. A constant expression that + // FE cannot evaluate (e.g. `crc32('abc') % 3 - 3`) reaches BE unchecked when the whole + // call is folded on BE, so validate here before it is used as a substring length. + if (gram_num <= 0) { + return Status::InvalidArgument( + "ngram_search(text,pattern,gram_num): gram_num must be a positive constant."); + } const auto* text_col = assert_cast(argument_columns[0].get()); if (col_const[0]) { diff --git a/be/test/exprs/function/function_string_test.cpp b/be/test/exprs/function/function_string_test.cpp index 3a8debab7ad0c4..ca1838fc955129 100644 --- a/be/test/exprs/function/function_string_test.cpp +++ b/be/test/exprs/function/function_string_test.cpp @@ -81,6 +81,34 @@ DataSet make_md5_varbinary_dataset(const std::vector& inputs) { } // namespace +TEST(function_string_test, ngram_search_gram_num) { + const InputTypeSet input_types = {Notnull {TYPE_STRING}, ConstedNotnull {TYPE_STRING}, + ConstedNotnull {TYPE_INT}}; + // 2 * |intersection| / (|text grams| + |pattern grams|); one row per data set because the + // const arguments are built from a single value. + const DataSet data_set = {{{std::string("abc"), std::string("abc"), int32_t(3)}, 1.0}, + {{std::string("ab"), std::string("abc"), int32_t(3)}, 0.0}, + {{std::string("ab"), std::string("abc"), int32_t(1)}, 0.8}}; + for (const auto& row : data_set) { + ASSERT_TRUE(check_function("ngram_search", input_types, {row}).ok()); + } +} + +TEST(function_string_test, ngram_search_nonpositive_gram_num) { + // FE folds a constant expression like `crc32('abc') % 3 - 3` on BE before it can check the + // value, so BE must reject a nonpositive gram_num instead of using it as a substring length. + const InputTypeSet input_types = {Notnull {TYPE_STRING}, ConstedNotnull {TYPE_STRING}, + ConstedNotnull {TYPE_INT}}; + for (int32_t gram_num : {0, -1}) { + const DataSet data_set = {{{std::string("abc"), std::string("abc"), gram_num}, 0.0}}; + auto st = check_function("ngram_search", input_types, data_set, -1, -1, + true); + EXPECT_TRUE(st.is()) << st; + EXPECT_NE(st.to_string().find("gram_num must be a positive constant"), std::string::npos) + << st; + } +} + TEST(function_string_test, parse_data_size_nullable) { const InputTypeSet input_types = {PrimitiveType::TYPE_STRING}; const DataSet data_set = {{{Null()}, Null()}, diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/NgramSearch.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/NgramSearch.java index 396244657bc6e1..9f2616dbd2dc69 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/NgramSearch.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/NgramSearch.java @@ -19,10 +19,12 @@ import org.apache.doris.catalog.FunctionSignature; import org.apache.doris.nereids.exceptions.AnalysisException; +import org.apache.doris.nereids.rules.expression.rules.FoldConstantRuleOnFE; 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.NullLiteral; import org.apache.doris.nereids.trees.expressions.visitor.ExpressionVisitor; import org.apache.doris.nereids.types.DoubleType; import org.apache.doris.nereids.types.IntegerType; @@ -57,16 +59,34 @@ 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) { + // Constant folding has not run yet, so a constant expression such as `1 + 2` is not a + // literal here. Reject the values FE can already determine now, before NULL propagation or + // plan pruning can drop the whole call and skip checkLegalityAfterRewrite. + checkGramNumValue(FoldConstantRuleOnFE.evaluateWithoutContext(gramNum)); + } + + @Override + public void checkLegalityAfterRewrite() { + // Constant folding (FE or BE) may have produced the literal by now. A constant that is + // still not a literal is evaluated by BE, which rejects a nonpositive gram_num itself. + checkGramNumValue(getArgument(2)); + } + + private static void checkGramNumValue(Expression 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).getLongValue() <= 0) { throw new AnalysisException( "ngram_search(text,pattern,gram_num): gram_num must be a positive constant."); } diff --git a/fe/fe-core/src/test/java/org/apache/doris/nereids/trees/expressions/functions/scalar/NgramSearchTest.java b/fe/fe-core/src/test/java/org/apache/doris/nereids/trees/expressions/functions/scalar/NgramSearchTest.java new file mode 100644 index 00000000000000..d085a0f508f4bf --- /dev/null +++ b/fe/fe-core/src/test/java/org/apache/doris/nereids/trees/expressions/functions/scalar/NgramSearchTest.java @@ -0,0 +1,148 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.apache.doris.nereids.trees.expressions.functions.scalar; + +import org.apache.doris.nereids.exceptions.AnalysisException; +import org.apache.doris.nereids.rules.analysis.ExpressionAnalyzer; +import org.apache.doris.nereids.rules.expression.rules.FoldConstantRuleOnFE; +import org.apache.doris.nereids.trees.expressions.Add; +import org.apache.doris.nereids.trees.expressions.Cast; +import org.apache.doris.nereids.trees.expressions.Expression; +import org.apache.doris.nereids.trees.expressions.Mod; +import org.apache.doris.nereids.trees.expressions.SlotReference; +import org.apache.doris.nereids.trees.expressions.Subtract; +import org.apache.doris.nereids.trees.expressions.literal.DoubleLiteral; +import org.apache.doris.nereids.trees.expressions.literal.IntegerLiteral; +import org.apache.doris.nereids.trees.expressions.literal.NullLiteral; +import org.apache.doris.nereids.trees.expressions.literal.StringLiteral; +import org.apache.doris.nereids.types.IntegerType; +import org.apache.doris.nereids.types.StringType; + +import com.google.common.collect.ImmutableList; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +class NgramSearchTest { + + @Test + void testLiteralGramNumber() { + assertFoldableGramNumber(new IntegerLiteral(3)); + } + + @Test + void testFoldableArithmeticGramNumber() { + assertFoldableGramNumber(new Add(new IntegerLiteral(1), new IntegerLiteral(2))); + } + + @Test + void testFoldableCastGramNumber() { + assertFoldableGramNumber(new Cast(new StringLiteral("3"), IntegerType.INSTANCE)); + } + + @Test + void testFoldableFunctionGramNumber() { + assertFoldableGramNumber(new Abs(new IntegerLiteral(-3))); + } + + @Test + void testConstantGramNumberBeyondFeEvaluator() { + // FE cannot evaluate crc32, so the constant stays unfolded and is left for BE to + // evaluate and validate; FE must not reject it as nonconstant at either stage. + Expression gram = new Add(new Mod(new Crc32(new StringLiteral("abc")), new IntegerLiteral(3)), + new IntegerLiteral(1)); + NgramSearch analyzed = analyze(gram); + Assertions.assertFalse(analyzed.child(2).isLiteral()); + Assertions.assertDoesNotThrow(analyzed::checkLegalityAfterRewrite); + } + + @Test + void testAfterRewriteRejectsInvalidLiteral() { + NgramSearch analyzed = analyze(new IntegerLiteral(3)); + assertAfterRewriteFails(withGramNumber(analyzed, new IntegerLiteral(0)), + "gram_num must be a positive constant"); + assertAfterRewriteFails(withGramNumber(analyzed, new IntegerLiteral(-1)), + "gram_num must be a positive constant"); + assertAfterRewriteFails(withGramNumber(analyzed, new NullLiteral(IntegerType.INSTANCE)), + "gram_num support const value only"); + } + + @Test + void testNonPositiveGramNumber() { + assertAnalyzeFails(new IntegerLiteral(0), "gram_num must be a positive constant"); + assertAnalyzeFails(new IntegerLiteral(-1), "gram_num must be a positive constant"); + assertAnalyzeFails(new Subtract(new IntegerLiteral(1), new IntegerLiteral(1)), + "gram_num must be a positive constant"); + assertAnalyzeFails(new Subtract(new IntegerLiteral(1), new IntegerLiteral(2)), + "gram_num must be a positive constant"); + assertAnalyzeFails(new Cast(new StringLiteral("0"), IntegerType.INSTANCE), + "gram_num must be a positive constant"); + } + + @Test + void testNonConstantGramNumber() { + SlotReference gram = SlotReference.of("gram", IntegerType.INSTANCE); + assertAnalyzeFails(gram, "gram_num support const value only"); + assertAnalyzeFails(new Add(gram, new IntegerLiteral(1)), "gram_num support const value only"); + assertAnalyzeFails(new Cast(new Random(), IntegerType.INSTANCE), "gram_num support const value only"); + } + + @Test + void testNonIntegerGramNumber() { + assertAnalyzeFails(new StringLiteral("3"), "gram_num support const value only"); + assertAnalyzeFails(new DoubleLiteral(3.0), "gram_num support const value only"); + assertAnalyzeFails(new NullLiteral(), "gram_num support const value only"); + assertAnalyzeFails(new Cast(new NullLiteral(), IntegerType.INSTANCE), "gram_num support const value only"); + } + + @Test + void testNonConstantPattern() { + NgramSearch function = new NgramSearch(new StringLiteral("abc"), + SlotReference.of("pattern", StringType.INSTANCE), new IntegerLiteral(3)); + AnalysisException exception = Assertions.assertThrows(AnalysisException.class, + () -> ExpressionAnalyzer.analyzeFunction(null, null, function)); + Assertions.assertTrue(exception.getMessage().contains("pattern support const value only")); + } + + private NgramSearch analyze(Expression gram) { + Expression analyzed = ExpressionAnalyzer.analyzeFunction(null, null, + new NgramSearch(new StringLiteral("abc"), new StringLiteral("abc"), gram)); + return (NgramSearch) analyzed; + } + + private NgramSearch withGramNumber(NgramSearch function, Expression gram) { + return function.withChildren(ImmutableList.of(function.child(0), function.child(1), gram)); + } + + private void assertFoldableGramNumber(Expression gram) { + NgramSearch analyzed = analyze(gram); + Expression folded = FoldConstantRuleOnFE.evaluateWithoutContext(analyzed); + Assertions.assertEquals(new IntegerLiteral(3), folded.child(2)); + Assertions.assertDoesNotThrow(folded::checkLegalityAfterRewrite); + } + + private void assertAnalyzeFails(Expression gram, String message) { + AnalysisException exception = Assertions.assertThrows(AnalysisException.class, () -> analyze(gram)); + Assertions.assertTrue(exception.getMessage().contains(message), exception.getMessage()); + } + + private void assertAfterRewriteFails(NgramSearch function, String message) { + AnalysisException exception = Assertions.assertThrows(AnalysisException.class, + function::checkLegalityAfterRewrite); + Assertions.assertTrue(exception.getMessage().contains(message), exception.getMessage()); + } +} diff --git a/regression-test/data/query_p0/sql_functions/string_functions/test_ngram_search_gram_num.out b/regression-test/data/query_p0/sql_functions/string_functions/test_ngram_search_gram_num.out new file mode 100644 index 00000000000000..29dd047597c3e5 --- /dev/null +++ b/regression-test/data/query_p0/sql_functions/string_functions/test_ngram_search_gram_num.out @@ -0,0 +1,73 @@ +-- This file is automatically generated. You should know what you did if you want to edit this +-- !literal -- +1 + +-- !arithmetic -- +1 + +-- !cast -- +1 + +-- !function -- +1 + +-- !nested -- +1 + +-- !null_text -- +\N + +-- !rows -- +1 1 1 +2 0 0.8 +3 0.3333333333333333 0.75 +4 \N \N + +-- !be_function -- +1 + +-- !be_null_gram -- +\N + +-- !be_rows -- +1 1 \N +2 0.8 \N +3 0.75 \N +4 \N \N + +-- !literal -- +1 + +-- !arithmetic -- +1 + +-- !cast -- +1 + +-- !function -- +1 + +-- !nested -- +1 + +-- !null_text -- +\N + +-- !rows -- +1 1 1 +2 0 0.8 +3 0.3333333333333333 0.75 +4 \N \N + +-- !be_function -- +1 + +-- !be_null_gram -- +\N + +-- !be_rows -- +1 1 \N +2 0.8 \N +3 0.75 \N +4 \N \N + diff --git a/regression-test/suites/query_p0/sql_functions/string_functions/test_ngram_search_gram_num.groovy b/regression-test/suites/query_p0/sql_functions/string_functions/test_ngram_search_gram_num.groovy new file mode 100644 index 00000000000000..11b7ceed46441f --- /dev/null +++ b/regression-test/suites/query_p0/sql_functions/string_functions/test_ngram_search_gram_num.groovy @@ -0,0 +1,87 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +suite("test_ngram_search_gram_num") { + sql "drop table if exists test_ngram_search_gram_num" + sql """ + create table test_ngram_search_gram_num ( + k1 int not null, + s string null + ) distributed by hash (k1) buckets 1 + properties ("replication_num" = "1") + """ + sql """insert into test_ngram_search_gram_num values (1, 'abc'), (2, 'ab'), (3, 'abc1313131'), (4, null)""" + + for (def foldOnBe in [false, true]) { + sql "set enable_fold_constant_by_be = ${foldOnBe}" + + // constant expressions folded on FE are equivalent to the literal + qt_literal "select ngram_search('abc', 'abc', 3)" + qt_arithmetic "select ngram_search('abc', 'abc', 1 + 2)" + qt_cast "select ngram_search('abc', 'abc', cast('3' as int))" + qt_function "select ngram_search('abc', 'abc', abs(-3))" + qt_nested "select ngram_search('abc', 'abc', cast(abs(-2) + 1 as int))" + qt_null_text "select ngram_search(cast(null as string), 'abc', 1 + 2)" + order_qt_rows """ + select k1, ngram_search(s, 'abc', 1 + 2), ngram_search(s, 'abc', cast('1' as int)) + from test_ngram_search_gram_num + """ + + // constant expressions that only BE can evaluate are executed and validated by BE + qt_be_function "select ngram_search('abc', 'abc', crc32('abc') % 3 + 1)" + qt_be_null_gram "select ngram_search('abc', 'abc', crc32('abc') % 0)" + order_qt_be_rows """ + select k1, ngram_search(s, 'abc', crc32('abc') % 3 + 1), ngram_search(s, 'abc', crc32('abc') % 0) + from test_ngram_search_gram_num + """ + for (def gram in ["crc32('abc') % 3 - 3", "crc32('abc') % 3 - 4"]) { + test { + sql "select ngram_search('abc', 'abc', ${gram})" + exception "gram_num must be a positive constant" + } + test { + sql "select k1, ngram_search(s, 'abc', ${gram}) from test_ngram_search_gram_num" + exception "gram_num must be a positive constant" + } + } + + for (def gram in ["0", "-1", "1 - 1", "1 - 2", "cast('0' as int)"]) { + test { + sql "select ngram_search('abc', 'abc', ${gram})" + exception "gram_num must be a positive constant" + } + test { + sql "select ngram_search(cast(null as string), 'abc', ${gram})" + exception "gram_num must be a positive constant" + } + test { + sql "select k1, ngram_search(s, 'abc', ${gram}) from test_ngram_search_gram_num" + exception "gram_num must be a positive constant" + } + } + for (def gram in ["'3'", "3.5", "null", "cast(null as int)", "cast(rand() as int)", "k1"]) { + test { + sql "select k1, ngram_search(s, 'abc', ${gram}) from test_ngram_search_gram_num" + exception "gram_num support const value only" + } + } + test { + sql "select k1, ngram_search('abc', s, 3) from test_ngram_search_gram_num" + exception "pattern support const value only" + } + } +}