mrhhsg commented on code in PR #68579:
URL: https://github.com/apache/doris/pull/68579#discussion_r4131580578
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/DateTrunc.java:
##########
@@ -62,13 +64,41 @@ private DateTrunc(ScalarFunctionParams functionParams) {
super(functionParams);
}
+ @Override
+ public boolean needFoldToLiteral(int index) {
+ // a string argument may be the time unit unless the other argument is
already a literal time unit
+ return getArgument(index).getDataType().isStringLikeType() &&
!isTimeUnit(getArgument(1 - index));
+ }
+
+ @Override
+ public boolean acceptFoldedLiteral(int index, Literal folded) {
+ // Only the time unit is replaced by its folded literal. A string date
value is kept unfolded, because
+ // folding it would change the derived return type. When the other
argument is a date, the folded
+ // literal is the time unit and an illegal value is reported by
checkLegalityBeforeTypeCoercion.
+ return isTimeUnit(folded) || getArgument(1 -
index).getDataType().isDateLikeType();
+ }
+
+ private static boolean isTimeUnit(Expression expression) {
+ return expression instanceof StringLikeLiteral
+ && LEGAL_TIME_UNIT.contains(((StringLikeLiteral)
expression).getStringValue().toLowerCase());
+ }
+
+ private static boolean isConstantString(Expression expression) {
+ return expression.isConstant() &&
expression.getDataType().isStringLikeType();
+ }
+
@Override
public void checkLegalityBeforeTypeCoercion() {
boolean firstArgIsStringLiteral =
getArgument(0).isConstant() && getArgument(0) instanceof
StringLikeLiteral;
boolean secondArgIsStringLiteral =
getArgument(1).isConstant() && getArgument(1) instanceof
StringLikeLiteral;
if (!firstArgIsStringLiteral && !secondArgIsStringLiteral) {
+ // a time unit FE cannot fold is accepted beside a date argument,
and BE validates it
+ if ((getArgument(0).getDataType().isDateLikeType() &&
isConstantString(getArgument(1)))
Review Comment:
Fixed in b4c6f6d. `DateTrunc::create_state` now compares the lower-cased
unit with `==` instead of a prefix `strncmp`, so `date_trunc(DATE '2024-03-15',
concat('month', crc32('')))` (unit `month0`) fails with "Illegal second
argument ..." for both argument orders. Every FE producer of the unit (the
literal check, auto partition, MTMV) already sends an exact lower-case unit, so
literal plans are unchanged. Covered by BE UT
`FunctionConstantArgumentTest.date_trunc_unit` (`x`, `month0`, `days`, both
orders) and two regression `test{}` cases in `fold_literal_arguments`.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/TopN.java:
##########
@@ -104,7 +110,9 @@ public void checkLegalityAfterRewrite() {
if (topNCount.isNullLiteral()) {
return;
}
- if (!(topNCount instanceof Literal) || ((Literal)
topNCount).getDouble() <= 0) {
+ // a constant FE cannot fold is evaluated by BE
+ if (!topNCount.isConstant()
Review Comment:
Fixed in b4c6f6d. `AggregateFunctionTopNData::set_paramenters`, which the
`add()` of topn, topn_array and topn_weighted calls for every row, now throws
INVALID_ARGUMENT for a count <= 0 before computing the capacity, so `topn(s,
crc32('') - 1)` and the same shape for topn_array / topn_weighted fail instead
of serializing every candidate. Covered by BE UT
`AggregateFunctionTopNTest.NonPositiveCountIsRejected` and regression `test{}`
cases for all three functions (the old `topn_zero_be` block that recorded `{}`
was removed).
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/SequenceFunction.java:
##########
@@ -20,31 +20,54 @@
import org.apache.doris.analysis.FunctionCallExpr;
import org.apache.doris.nereids.exceptions.AnalysisException;
import org.apache.doris.nereids.trees.expressions.Expression;
+import
org.apache.doris.nereids.trees.expressions.functions.FoldLiteralArguments;
import org.apache.doris.nereids.trees.expressions.functions.FunctionTrait;
+import org.apache.doris.nereids.trees.expressions.literal.Literal;
import org.apache.doris.nereids.trees.expressions.literal.StringLikeLiteral;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
/** SequenceFunction */
-public interface SequenceFunction extends FunctionTrait {
+public interface SequenceFunction extends FunctionTrait, FoldLiteralArguments {
Pattern EVENT_PATTERN = Pattern.compile("\\(\\?(\\d+)\\)");
+ @Override
+ default boolean needFoldToLiteral(int index) {
+ // the pattern
+ return index == 0;
+ }
+
@Override
default void checkLegalityBeforeTypeCoercion() {
String functionName = getName();
Expression firstArg = getArgument(0);
- if (!(firstArg instanceof StringLikeLiteral)) {
+ if (!firstArg.isConstant() || (firstArg instanceof Literal &&
!(firstArg instanceof StringLikeLiteral))) {
throw new AnalysisException("The pattern param `" +
firstArg.toSql() + "` of " + functionName
- + " function must be string literal, but it is "
+ + " function must be string constant, but it is "
+ firstArg.getClass().getSimpleName());
}
if (!getArgumentType(1).isDateLikeType()) {
throw new AnalysisException("The timestamp params of " +
functionName
+ " function must be DATE, DATETIME, TIMESTAMP_NS or
TIMESTAMPTZ, but it is "
+ getArgumentType(1));
}
- String pattern = ((StringLikeLiteral) firstArg).getStringValue();
+ // a pattern FE cannot fold is parsed by BE when it is evaluated
+ if (firstArg instanceof StringLikeLiteral) {
Review Comment:
Fixed in b4c6f6d. `parse_pattern` now throws INVALID_ARGUMENT (with the
position and the pattern) instead of logging and clearing the state, so
`sequence_match` / `sequence_count` with `lpad('(?9)', 4, '(')` return "Event
number 9 is out of range ...". The exception reaches the query through the task
scheduler's catch like the existing merge error, and the state stays
destroyable. This also turns FE-accepted patterns that BE cannot parse (two
consecutive time conditions) into an error, which is declared in the commit
message and PR description. The BE UTs that expected "invalid pattern returns
0" now expect the exception, and regression `test{}` cases replace the old
`sequence_invalid_pattern_be` block.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/Tokenize.java:
##########
@@ -58,13 +60,25 @@ private Tokenize(ScalarFunctionParams functionParams) {
super(functionParams);
}
+ @Override
+ public boolean needFoldToLiteral(int index) {
+ return index == 1;
+ }
+
@Override
public void checkLegalityBeforeTypeCoercion() {
Expression rightChild = getArgument(1);
// tokenize(k7, null) could return NULL
if (rightChild instanceof NullLiteral) {
return;
}
+ if (!rightChild.isConstant()) {
+ throw new AnalysisException("tokenize second argument must be a
string constant");
+ }
+ // a constant FE cannot fold is parsed by BE when it is evaluated
+ if (!(rightChild instanceof Literal)) {
Review Comment:
Fixed in b4c6f6d. The BE tokenize parser now requires the whole string to be
comma separated key-value pairs (`match_continuous`, only whitespace and a
comma between pairs, no trailing comma, a bare key cannot contain `=`), trims
keys and rejects a duplicate key like FE's property parser, and checks the char
filter properties like `InvertedIndexUtil.checkCharFilterProperties`.
`tokenize('hello world', lpad('x', 1, 'x'))` now fails with "tokenize second
argument must be properties format". Every property string used by the existing
tokenize UTs and regression suites parses to the same map as before. Covered by
BE UT `FunctionConstantArgumentTest.tokenize_properties` and regression
`test{}` cases.
--
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]
