mrhhsg commented on code in PR #68579:
URL: https://github.com/apache/doris/pull/68579#discussion_r4148029908
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/Sha2.java:
##########
@@ -63,15 +65,30 @@ private Sha2(ScalarFunctionParams functionParams) {
@Override
public void checkLegalityBeforeTypeCoercion() {
- checkLegalityAfterRewrite();
+ // validate the value FE can evaluate here, because constant folding,
e.g. of sha2(null, 1 + 2), may remove
+ // this function before checkLegalityAfterRewrite
+
checkDigestLength(ExpressionUtils.foldConstantArgument(getArgument(1)));
}
@Override
public void checkLegalityAfterRewrite() {
- if (!(child(1) instanceof IntegerLikeLiteral)) {
- throw new AnalysisException("the second parameter of sha2 must be
a literal but got: " + child(1).toSql());
+ checkDigestLength(getArgument(1));
+ }
+
+ private void checkDigestLength(Expression digestLength) {
+ if (!digestLength.isConstant()) {
+ throw new AnalysisException("the second parameter of sha2 must be
a constant but got: "
+ + digestLength.toSql());
+ }
+ // a constant FE cannot fold is validated by BE when it is evaluated
+ if (!(digestLength instanceof Literal)) {
Review Comment:
Fixed in a66e13c5e12. `checkDigestLength` now checks the type of every
constant argument, not only of a literal, before the type coercion casts it to
the INT signature (`getDataType().isIntegralType()`, which is what the
`IntegerLikeLiteral` check amounted to for a literal). `sha2('abc', 256.0 +
crc32(''))` is rejected with "the second parameter of sha2 must be an integer",
like `sha2('abc', 255.5 + 0.5)` and the decimal literal. Covered in
`ConstantFunctionArgumentTest.testNonConstantArgumentIsStillRejected` and in
`fold_literal_arguments`.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/SplitByRegexp.java:
##########
@@ -79,9 +81,12 @@ public SplitByRegexp withChildren(List<Expression> children)
{
public void checkLegalityBeforeTypeCoercion() {
List<Expression> arguments = getArguments();
if (arguments.size() == 3) {
- Expression thirdArgument = getArgument(2);
- if (!thirdArgument.isConstant() || !(thirdArgument instanceof
IntegerLikeLiteral)
- || (((IntegerLikeLiteral) thirdArgument).getIntValue() <
0)) {
+ // validate the value FE can evaluate here, because constant
folding may remove this function before
+ // any later check; a constant FE cannot fold is validated by BE
when it is evaluated
+ Expression thirdArgument =
ExpressionUtils.foldConstantArgument(getArgument(2));
+ if (!thirdArgument.isConstant() || (thirdArgument instanceof
Literal
Review Comment:
Fixed in a66e13c5e12. The check before type coercion now requires the limit
to have an integral type for every constant (`getDataType().isIntegralType()`),
so `split_by_regexp('a,b,c', ',', 2.0 + crc32(''))` is rejected with "must be a
positive constant" like the decimal literal, instead of being cast to INT and
left to BE. Covered in
`ConstantFunctionArgumentTest.testNonConstantArgumentIsStillRejected` and in
`fold_literal_arguments`.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/Sha2.java:
##########
@@ -63,15 +65,30 @@ private Sha2(ScalarFunctionParams functionParams) {
@Override
public void checkLegalityBeforeTypeCoercion() {
- checkLegalityAfterRewrite();
+ // validate the value FE can evaluate here, because constant folding,
e.g. of sha2(null, 1 + 2), may remove
+ // this function before checkLegalityAfterRewrite
+
checkDigestLength(ExpressionUtils.foldConstantArgument(getArgument(1)));
}
@Override
public void checkLegalityAfterRewrite() {
- if (!(child(1) instanceof IntegerLikeLiteral)) {
- throw new AnalysisException("the second parameter of sha2 must be
a literal but got: " + child(1).toSql());
+ checkDigestLength(getArgument(1));
+ }
+
+ private void checkDigestLength(Expression digestLength) {
+ if (!digestLength.isConstant()) {
+ throw new AnalysisException("the second parameter of sha2 must be
a constant but got: "
+ + digestLength.toSql());
+ }
+ // a constant FE cannot fold is validated by BE when it is evaluated
+ if (!(digestLength instanceof Literal)) {
+ return;
Review Comment:
Not changed, documented instead. For `sha2(NULL, 300 + crc32(''))` the
length is a constant FE cannot evaluate, and the function is never evaluated
anywhere: `FoldConstantRuleOnFE.preProcess` folds the `PropagateNullable` call
to NULL on FE, and if the call reaches BE (INSERT ... VALUES,
`debug_skip_fold_constant`)
`PreparedFunctionImpl::default_implementation_for_nulls` returns a NULL column
for the only-null argument without calling `execute`. No value is evaluated, so
there is nothing to validate; the result is NULL, as it is with a valid length,
so no wrong result is produced (a literal 300 is still rejected on FE, as
before). Reporting the error here would require disabling FE null propagation
and BE's only-null shortcut for these functions, a cost on every query for a
diagnostic that does not change the result. The same holds for the tokenize
properties and the split limit. The PR description now lists this under "Not
covered here".
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/AIAgg.java:
##########
@@ -83,24 +83,40 @@ public void checkLegalityAfterRewrite() {
if (!child(0).isLiteral() || !child(2).isLiteral()) {
throw new AnalysisException("AI_AGG must accept literal for
the resource name.");
}
+ checkResource();
+ }
+ }
- //Check if the resource is valid
- String resourceName =
getArgument(0).toString().replaceAll("^['\"]|['\"]$", "");
- Resource resource =
Env.getCurrentEnv().getResourceMgr().getResource(resourceName);
- if (!(resource instanceof AIResource)) {
- throw new AnalysisException("AI resource '" + resourceName +
"' does not exist");
+ @Override
+ public void checkLegalityBeforeTypeCoercion() {
+ // An aggregate function is not removed by constant folding, so a
constant task or resource name only has
+ // to be a literal in checkLegalityAfterRewrite, once the rewrite has
folded it. A literal resource name
+ // is already validated here.
+ if (!child(arity() - 1).isConstant()) {
Review Comment:
The DISTINCT form cannot reach this gate: `AIAgg` has only the `(arg0,
arg1)` and `(arg0, arg1, arg2)` constructors, no constructor with a distinct
flag, so `ai_agg(DISTINCT 'resource', s, 'task')` fails function binding with
"Can not found function 'ai_agg' which has 4 arity", on master as well. A
scalar AI function nested in a DISTINCT aggregate is not affected either:
`NormalizeAggregate` pushes it into the project below the aggregate, where the
rewrite folds it (checked with `count(distinct ai_summarize(concat(...), s))`,
whose resource name is a literal after the rewrite). What does leave the
constant unfolded is `debug_skip_fold_constant`, which the description listed
as not covered. In a66e13c5e12 the checks after the rewrite of `AIAgg`,
`AIFunction` and `Embed` evaluate the argument through `foldConstantArgument`,
like the checks before type coercion, so a resource name or task FE can
evaluate passes there too, and BE reads the value it evaluates from the first
row (`get_d
ata_at(0)`), as it already did. A constant FE cannot fold is still rejected,
because FE resolves the resource. Covered in
`ConstantFunctionArgumentTest.testAiFunctionResolvesTheResourceNameItEvaluates`.
--
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]
