github-actions[bot] commented on code in PR #68579:
URL: https://github.com/apache/doris/pull/68579#discussion_r4220686217
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/SplitByRegexp.java:
##########
@@ -79,9 +80,13 @@ 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
+ Expression thirdArgument =
ExpressionUtils.foldConstantArgument(getArgument(2));
+ if (!thirdArgument.isConstant() ||
!thirdArgument.getDataType().isIntegralType()
+ || (thirdArgument instanceof IntegerLikeLiteral
Review Comment:
[P2] Reject a typed NULL split limit before treating it as a valid constant.
`split_by_regexp('a,b', ',', CAST(NULL AS INT))` folds to
`NullLiteral(IntegerType)`, so it passes the new constant and integral checks
but skips the `IntegerLikeLiteral` range check. `PropagateNullable` then
returns NULL, whereas this call was rejected before this PR and a bare NULL
limit is still rejected. Handle `NullLiteral` explicitly here, as the SHA2
length check does, and cover the typed-NULL case.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/DateTrunc.java:
##########
@@ -62,13 +63,57 @@ private DateTrunc(ScalarFunctionParams functionParams) {
super(functionParams);
}
+ @Override
+ public Expression prepareBeforeTypeCoercion() {
+ // When an argument is a date, the other one is the time unit, and the
signature does not need its value.
+ if (getArgument(0).getDataType().isDateLikeType() ||
getArgument(1).getDataType().isDateLikeType()) {
+ return this;
+ }
+ // Otherwise customSignature tells the time unit from the date value
by the literal time unit, so fold a
+ // constant string that evaluates to a time unit, unless the other
argument already is one. A string date
+ // value is kept unfolded, because folding it would change the derived
return type.
+ return withChildren((argument, index) -> {
+ if (!argument.getDataType().isStringLikeType() ||
isTimeUnit(getArgument(1 - index))) {
+ return argument;
+ }
+ Expression folded = ExpressionUtils.foldConstantArgument(argument);
+ return isTimeUnit(folded) ? folded : argument;
+ });
+ }
+
+ 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) {
+ for (int i = 0; i < 2; i++) {
+ // The other argument is the time unit when this one is a
date-typed value, or when this one
+ // is simply nonconstant (e.g. a VARCHAR date column) and the
other side can only be the unit.
+ boolean thisArgIsDateRole =
getArgument(i).getDataType().isDateLikeType()
+ || !getArgument(i).isConstant();
Review Comment:
[P2] Accept a BE-only unit beside a constant string date.
`date_trunc('2024-03-15', lpad('nth', 5, 'mo'))` has an evaluated unit of
`month`, but the sole FE literal is the date string. This role inference covers
nonconstant string dates only, so the later branch treats the date as the unit
and rejects it; the literal `'month'` succeeds. An FE-foldable date expression
such as `concat('2024-03-15', '')` also fails because preparation keeps it
nonliteral and the new loop sees two constants. Handle this date role in both
argument orders and in `customSignature`, preserving timezone type selection.
##########
be/src/exprs/function/function_regexp.cpp:
##########
@@ -404,69 +398,108 @@ class FunctionRegexpReplace : public IFunction {
for (int i = 0; i < 3; ++i) {
col_const[i] =
is_column_const(*block.get_by_position(arguments[i]).column);
}
- argument_columns[0] = col_const[0] ? static_cast<const ColumnConst&>(
-
*block.get_by_position(arguments[0]).column)
- .convert_to_full_column()
- :
block.get_by_position(arguments[0]).column;
+ const auto& [source_column, source_const] =
+ unpack_if_const(block.get_by_position(arguments[0]).column);
+ argument_columns[0] = source_column;
default_preprocess_parameter_columns(argument_columns, col_const, {1,
2}, block, arguments);
+ if constexpr (std::is_same_v<FourParamTypes, ParamTypes>) {
+ // The regex of a constant pattern was not compiled in open
because the options were
+ // not evaluated there. Compile it once with the options of the
first row.
+ // Gate this on the query-level constant-ness of the pattern (as
open() does), not on
+ // col_const[1]: a lazy join can broadcast a non-constant probe
pattern as a physical
+ // ColumnConst for one block, which would otherwise cache that
block's pattern and
+ // wrongly reuse it for later blocks with a different pattern
value.
+ if (context->is_col_constant(1) && !context->is_col_constant(3) &&
+ input_rows_count > 0 &&
+ context->get_function_state(FunctionContext::THREAD_LOCAL) ==
nullptr) {
+ RETURN_IF_ERROR(compile_constant_pattern(
+ context, argument_columns[1]->get_data_at(0),
+
block.get_by_position(arguments[3]).column->get_data_at(0)));
+ }
+ }
+
StringRef options_value;
if (col_const[1] && col_const[2]) {
- Impl::execute_impl_const_args(context, argument_columns,
options_value,
+ Impl::execute_impl_const_args(context, argument_columns,
source_const, options_value,
Review Comment:
[P1] Pass the evaluated fourth argument to the physical-constant pattern
path. For `regexp_replace('a', '', '\\x', if(uniform(1, 2, crc32('x')) > 0,
'ignore_invalid_escape', ''))` over a multirow `numbers` source, `uniform`
produces a full options column and bypasses the all-constant wrapper. The empty
pattern leaves no cached regex state, and this branch passes empty
`options_value` to per-row compilation, ignoring the requested invalid-escape
behavior. The equivalent literal option takes the all-constant wrapper and
reads its option. Both replacement variants share this path. Read options
before branching and cover this mixed-column 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]
