github-actions[bot] commented on code in PR #68734:
URL: https://github.com/apache/doris/pull/68734#discussion_r4237502646
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/NgramSearch.java:
##########
@@ -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.evaluate(gramNum, null));
+ }
+
+ @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) {
Review Comment:
[P2] Reject grams outside the INT signature range before coercion.
`ngram_search('abc', 'abc', 2147483648)` now passes this positive
`getLongValue()` check, but the function expects INT. In default non-strict
cast mode, the inserted BIGINT-to-INT cast overflows to NULL, and nullable
folding can return NULL before the later legality check or BE guard runs.
Validate representability as INT as well as positivity, and cover this boundary
value.
##########
be/src/exprs/function/function_string_misc.cpp:
##########
@@ -826,6 +826,13 @@ class FunctionNgramSearch : public IFunction {
}
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);
+ // 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) {
Review Comment:
[P2] Validate BE-only gram expressions before NULL propagation. For
`ngram_search(NULL, 'abc', crc32('abc') % 3 - 3)`, FE cannot determine the
negative gram during the early check, then nullable folding can replace the
whole call with NULL. If it reaches BE, the default null wrapper likewise
returns NULL before this guard. This contradicts the early-invalid-gram
behavior exercised for FE-foldable grams with NULL text; add this combination
to the regression and validate the gram independently.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/NgramSearch.java:
##########
@@ -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.evaluate(gramNum, null));
Review Comment:
[P2] Avoid folding context-dependent gram expressions with a null context.
`CAST(KEY gram_key AS INT)` is a constant integral argument, but
`FoldConstantRuleOnFE.evaluate` descends into `EncryptKeyRef`, whose visitor
reads `context.cascadesContext`; this raises `NullPointerException` during
analysis. Resolve the value with the live rewrite context or defer this check
until a context-aware fold, and cover a numeric encryption key in a regression.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
