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" + } + } +}