github-actions[bot] commented on code in PR #66031:
URL: https://github.com/apache/doris/pull/66031#discussion_r3666026344
##########
be/src/exprs/function/cast/function_cast.cpp:
##########
@@ -192,24 +193,34 @@ WrapperType prepare_remove_nullable(FunctionContext*
context, const DataTypePtr&
bool replace_null_data_to_default =
need_replace_null_data_to_default(
context, from_type_not_nullable, to_type_not_nullable);
+ std::optional<NullableColumnInfo> source_info;
+ if (block.get_by_position(arguments[0]).type->is_nullable()) {
+ source_info =
block.get_by_position(arguments[0]).get_nullable_column_info();
+ }
Review Comment:
[P1] Preserve empty result slots for nullable casts
`VCastExpr::execute_column_impl` (and the TRY_CAST/direct-test paths)
inserts `{nullptr, nullable_type, ...}` for the result before invoking this
wrapper. This call therefore reaches `get_nullable_column_info()`, whose
`unpack_if_const(column)` immediately dereferences that null pointer, before
the inner conversion has a chance to populate the slot. The previous
`unnest_nullable()` explicitly allowed a null result column. Please construct a
nested null result placeholder without requiring metadata (or otherwise retain
that null-column contract), then wrap the column after `prepare_impl` produces
it.
##########
be/src/exprs/function/function.cpp:
##########
@@ -213,7 +228,8 @@ Status
PreparedFunctionImpl::default_implementation_for_nulls(
Review Comment:
[P1] Keep non-nullable arguments out of the metadata dereference
`build_nullable_column_infos()` intentionally leaves `std::nullopt` for
every non-nullable argument, but once any argument is nullable this loop
dereferences every entry. A normal mixed call such as `add(nullable_col,
non_nullable_col)` therefore evaluates `std::optional::operator*` without a
value before `unnest_nullable()` can take its non-nullable early return, which
is undefined behavior (and can abort with checked standard-library builds).
Please branch on the optional and insert the original argument when no nullable
metadata exists, as the CAST source path already does.
##########
be/src/core/block/column_with_type_and_name.cpp:
##########
@@ -105,41 +105,47 @@ void
ColumnWithTypeAndName::to_pb_column_meta(PColumnMeta* col_meta) const {
type->to_pb_column_meta(col_meta);
}
-ColumnWithTypeAndName ColumnWithTypeAndName::unnest_nullable(
- bool replace_null_data_to_default) const {
- if (type->is_nullable()) {
- auto nested_type =
- assert_cast<const DataTypeNullable*,
TypeCheckOnRelease::DISABLE>(type.get())
- ->get_nested_type();
- ColumnPtr nested_column = column;
- if (column) {
- // A column_ptr is needed here to ensure that the column in
convert_to_full_column_if_const is not released.
- auto [column_ptr, is_const] = unpack_if_const(column);
- const auto* source_column =
- assert_cast<const ColumnNullable*,
TypeCheckOnRelease::DISABLE>(
- column_ptr.get());
- if (is_const) {
- nested_column =
-
ColumnConst::create(source_column->get_nested_column_ptr(), column->size());
- } else {
- nested_column = source_column->get_nested_column_ptr();
- }
-
- if (replace_null_data_to_default) {
- const auto& null_map = source_column->get_null_map_data();
- // only need to mutate nested column, avoid to copy nullmap
- auto mutable_nested_col = (*std::move(nested_column)).mutate();
- if (simd::contain_one(null_map.data(), null_map.size())) {
-
mutable_nested_col->replace_column_null_data(null_map.data());
- }
-
- return {std::move(mutable_nested_col), nested_type, ""};
- }
- }
- return {nested_column, nested_type, ""};
+NullableColumnInfo ColumnWithTypeAndName::get_nullable_column_info() const {
+ DCHECK(type->is_nullable());
+ DCHECK(column);
+
+ // A column_ptr is needed here to ensure that the column in
convert_to_full_column_if_const is
+ // not released.
+ auto [column_ptr, is_const] = unpack_if_const(column);
+ const auto* source_column =
+ assert_cast<const ColumnNullable*,
TypeCheckOnRelease::DISABLE>(column_ptr.get());
+ const auto& null_map = source_column->get_null_map_data();
+ const size_t non_null_count =
Review Comment:
[P2] Preserve early exit for mixed null maps
`count_zero_num()` always scans and popcounts the entire null map, whereas
the old `only_null()`/`has_null()` checks use `contain_zero()`/`contain_one()`
and return as soon as the relevant byte appears. For a large mixed map with
both a null and a valid row in the first SIMD chunk, a cheap default-null
function previously examined only that chunk but now walks the whole map for
every nullable argument. Since the exact count is only used to derive these two
booleans, please cache those predicates with a scan that can stop once both
states have been seen (or otherwise preserve the short-circuit behavior).
--
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]
