github-actions[bot] commented on code in PR #67749:
URL: https://github.com/apache/doris/pull/67749#discussion_r4002247084
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/NgramSearch.java:
##########
@@ -47,19 +48,28 @@ public class NgramSearch extends ScalarFunction
*/
public NgramSearch(Expression arg0, Expression arg1, Expression arg2) {
super("ngram_search", arg0, arg1, arg2);
- if (!(arg1.isConstant())) {
+ }
+
+ /** constructor for withChildren and reuse signature */
+ private NgramSearch(ScalarFunctionParams functionParams) {
+ super(functionParams);
+ }
+
+ @Override
+ public void checkLegalityBeforeTypeCoercion() {
+ if (!child(1).isConstant()) {
throw new AnalysisException(
"ngram_search(text,pattern,gram_num): pattern support
const value only.");
}
- if (!(arg2.isConstant())) {
+ Expression gramNum = child(2);
+ if (!(gramNum instanceof IntegerLikeLiteral)) {
Review Comment:
[P1] Defer `gram_num` value validation past binding/folding. This
pre-coercion shape check rejects previously valid constants such as `CAST(3 AS
INT)` before they fold, rejects the temporary `StringLiteral` used for an
unbound `?` during COM_STMT_PREPARE before a positive value can be supplied,
and rejects `NULL` despite the documented NULL-propagation contract. The prior
thread suggested a literal check but did not cover these resulting
compatibility failures. Please preserve constant/prepared-parameter handling,
let NULL propagate, validate the resolved non-NULL value at the appropriate
later stage, and add regressions for these paths.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/NgramSearch.java:
##########
@@ -47,19 +48,28 @@ public class NgramSearch extends ScalarFunction
*/
public NgramSearch(Expression arg0, Expression arg1, Expression arg2) {
super("ngram_search", arg0, arg1, arg2);
- if (!(arg1.isConstant())) {
+ }
+
+ /** constructor for withChildren and reuse signature */
+ private NgramSearch(ScalarFunctionParams functionParams) {
+ super(functionParams);
+ }
+
+ @Override
+ public void checkLegalityBeforeTypeCoercion() {
+ if (!child(1).isConstant()) {
throw new AnalysisException(
"ngram_search(text,pattern,gram_num): pattern support
const value only.");
}
- if (!(arg2.isConstant())) {
+ Expression gramNum = child(2);
+ if (!(gramNum instanceof IntegerLikeLiteral)) {
throw new AnalysisException(
"ngram_search(text,pattern,gram_num): gram_num support
const value only.");
}
- }
-
- /** constructor for withChildren and reuse signature */
- private NgramSearch(ScalarFunctionParams functionParams) {
- super(functionParams);
+ if (((IntegerLikeLiteral) gramNum).getIntValue() <= 0) {
Review Comment:
[P2] Check the sign before narrowing the literal. For `ngram_search('abc',
'abc', -2147483649)`, the parser builds a `BigIntLiteral`, but `getIntValue()`
turns it into `2147483647`, so this guard passes. The later BIGINT-to-INT cast
becomes NULL in default non-strict mode (or raises a cast error in strict
mode), meaning a negative gram value still bypasses the FE validation this PR
adds. Please compare the full-precision literal value with zero and add an
out-of-INT-range negative 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]
