-
Notifications
You must be signed in to change notification settings - Fork 4k
[fix](fe) Accept constant expressions as the gram_num of 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.
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
1
commit into
apache:master
Choose a base branch
from
mrhhsg:fix/ngram-search-foldable-gram
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
148 changes: 148 additions & 0 deletions
148
...est/java/org/apache/doris/nereids/trees/expressions/functions/scalar/NgramSearchTest.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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()); | ||
| } | ||
| } |
73 changes: 73 additions & 0 deletions
73
regression-test/data/query_p0/sql_functions/string_functions/test_ngram_search_gram_num.out
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 | ||
|
|
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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_implis not guaranteed to run.crc32('abc') % 0remains 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.There was a problem hiding this comment.
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, andLIMIT 0, in both fold modes. This also covers the case where NULL text previously removed the entire function on FE, which a BE-onlyopen()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.