mrhhsg commented on code in PR #68579:
URL: https://github.com/apache/doris/pull/68579#discussion_r4241361449
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/Sha2.java:
##########
@@ -63,15 +66,35 @@ 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());
+ }
+ // the type is checked before the type coercion casts the argument to
the INT signature, so a decimal
+ // constant is rejected like a decimal literal
+ if (!digestLength.getDataType().isIntegralType()) {
+ throw new AnalysisException("the second parameter of sha2 must be
an integer but got: "
+ + digestLength.toSql());
+ }
+ // the value of a constant FE cannot fold is validated by BE when it
is evaluated
+ if (!(digestLength instanceof Literal)) {
+ return;
+ }
+ if (digestLength instanceof NullLiteral) {
+ throw new AnalysisException("sha2 functions only support digest
length of " + validDigest.toString());
}
- final int constParam = ((IntegerLikeLiteral) child(1)).getIntValue();
+ final int constParam = ((IntegerLikeLiteral)
digestLength).getIntValue();
Review Comment:
Fixed in 93453c07c66. `checkDigestLength` now compares the exact
`getBigDecimalValue()` of the folded literal against the supported lengths
instead of narrowing it with `getIntValue()`, so `sha2('abc', cast(4294967552
as bigint) + cast(0 as bigint))` is rejected with the digest-length error
instead of wrapping into 256. FE UT and the regression suite cover the wide
BIGINT expression.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/SplitByRegexp.java:
##########
@@ -79,9 +81,15 @@ 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; the type is checked before the type coercion
casts the argument to the INT
+ // signature, and the value of a constant FE cannot fold is
validated by BE when it is evaluated.
+ // A typed NULL, e.g. cast(null as int), is an integral constant
but not a positive one.
+ Expression thirdArgument =
ExpressionUtils.foldConstantArgument(getArgument(2));
+ if (!thirdArgument.isConstant() ||
!thirdArgument.getDataType().isIntegralType()
+ || thirdArgument instanceof NullLiteral
+ || (thirdArgument instanceof IntegerLikeLiteral
+ && ((IntegerLikeLiteral) thirdArgument).getIntValue() <
0)) {
Review Comment:
Fixed in 93453c07c66. The limit check now reads the sign of the exact
`getBigDecimalValue()` instead of `getIntValue()`, so `split_by_regexp('a,b',
',', cast(-4294967296 as bigint) + cast(0 as bigint))` is rejected as a
non-positive constant instead of wrapping into 0. FE UT and the regression
suite cover the wide negative expression.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/UtcTime.java:
##########
@@ -94,6 +96,17 @@ public void checkLegalityAfterRewrite() {
}
}
+ @Override
+ public Expression prepareBeforeTypeCoercion() {
+ // computeSignature derives the return type from the value of the
scale, but only from an INT literal,
+ // and a wider literal would keep a narrowing cast to INT, so fold
only a scale whose type is at most INT
+ return withChildren(scale -> {
+ DataType type = scale.getDataType();
+ return type.isTinyIntType() || type.isSmallIntType() ||
type.isIntegerType()
Review Comment:
Addressed in 93453c07c66. `computeSignature` now derives the scale from any
`IntegerLikeLiteral` and range-checks it as a `long`, like
`DateTimeWithPrecision`, so a folded TINYINT / SMALLINT scale is read directly.
For the record, the mismatch did not surface end to end before this change:
`implicitCastInputTypes` turns the folded `SmallIntLiteral(3)` into an INT
literal and rebuilds the function, and the analyzer recomputes the signature on
the rebuilt function, so `utc_time(cast(3 as smallint))` already analyzed to
TIMEV2(3) and `cast(7 as smallint)` already hit the 0..6 error (the new FE UT
assertions also pass on the previous code). The change makes `computeSignature`
self-contained instead of relying on that recomputation. FE UT covers `cast(3
as smallint)` / `cast(5 as tinyint)` / `cast(7 as smallint)`; the regression
suite adds `utc_time_narrow_scale` and the error case.
--
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]
