Re: [PR] [fix](function) Reject invalid regex pattern read from a column instead of returning NULL [doris]

["HappenLee (via GitHub)" <gi...@apache.org>]

HappenLee commented on code in PR #68359:
URL: https://github.com/apache/doris/pull/68359#discussion_r4209900906


##########
be/src/exprs/function/function_regexp.cpp:
##########
@@ -288,17 +355,24 @@ class FunctionRegexpCount : public IFunction {
 
     Status execute_impl(FunctionContext* context, Block& block, const ColumnNumbers& arguments,
                         uint32_t result, size_t input_rows_count) const override {
-        auto result_data_column = ColumnInt32::create(input_rows_count);
-        auto& result_data = result_data_column->get_data();
-
-        ColumnPtr argument_columns[2];
-
-        argument_columns[0] = block.get_by_position(arguments[0]).column;
-        argument_columns[1] = block.get_by_position(arguments[1]).column;
-        RegexpCountImpl::execute_impl(context, argument_columns, input_rows_count, result_data);
-
-        block.get_by_position(result).column = std::move(result_data_column);
-        return Status::OK();
+        const bool result_nullable = block.get_by_position(result).type->is_nullable();
+        return execute_regexp_with_nulls(

Review Comment:
   Please preserve the non-nullable fast path of `regexp_count` before merging.
   
   When both input columns and the result are non-nullable, this call still enters `execute_regexp_with_nulls()`, which unconditionally allocates and zero-fills `ColumnUInt8::create(input_rows_count, 0)`. `RegexpCountImpl::execute_impl()` then reads/checks that all-zero map for every row, and the `!result_nullable` branch ultimately discards it. Previously, the framework skipped nullable handling for non-nullable inputs and the count loop did not perform these checks.
   
   This introduces an unnecessary per-block allocation/initialization (approximately one byte per row, excluding overhead) and per-row null-map checks on a path that cannot contain SQL NULLs. I have not benchmarked the impact, so this is a request to remove the avoidable work rather than a claim of a measured slowdown.
   
   Suggested minimal change:
   
   - Select the execution path once per block using the existing `have_null_column(block, arguments)` helper.
   - For non-nullable inputs/results, pass the original argument columns directly to the count implementation and return the plain integer result, without constructing the temporary nullable block or NULL map.
   - Reuse the count loop with a compile-time flag, e.g. `execute_impl<CheckNull>(..., const NullMap* null_map)`, and put the null check behind `if constexpr (CheckNull)`. The non-nullable specialization can receive `nullptr`, so it contains no per-row null-map check.
   - Keep the current helper and NULL-row skipping for the nullable path. Preserve the declared result type if a caller requests a nullable output, and keep the existing constant-pattern cache and invalid-pattern error handling.
   
   Please cover the non-nullable count path alongside the existing mixed-NULL/invalid-hidden-payload cases. This can stay local to `regexp_count`; there is no need to redesign the general function framework.



-- 
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: commits-unsubscribe@doris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org