github-actions[bot] commented on code in PR #63491:
URL: https://github.com/apache/doris/pull/63491#discussion_r3300945533
##########
be/src/core/column/column_nullable.cpp:
##########
@@ -53,55 +57,65 @@ void check_nullable_sizes(const IColumn& nested_column,
const IColumn& null_map)
} // namespace
+namespace {
+ColumnUInt8::MutablePtr assert_mutable_null_map(const MutableColumnPtr&
null_map) {
+ if (is_column_const(*null_map)) [[unlikely]] {
+ throw doris::Exception(ErrorCode::INTERNAL_ERROR,
+ "ColumnNullable cannot have constant null map");
+ }
+ return ColumnUInt8::cast_to_column_mutptr(
+ assert_cast<ColumnUInt8*,
TypeCheckOnRelease::DISABLE>(null_map.get()));
+}
+} // namespace
+
ColumnNullable::ColumnNullable(MutableColumnPtr&& nested_column_,
MutableColumnPtr&& null_map_)
+ : ColumnNullable(std::move(nested_column_),
assert_mutable_null_map(null_map_)) {}
+
+ColumnNullable::ColumnNullable(MutableColumnPtr&& nested_column_,
+ ColumnUInt8::MutablePtr&& null_map_)
: _nested_column(std::move(nested_column_)),
_null_map(std::move(null_map_)) {
check_const_only_in_top_level();
// after convert const column to full column, it may be a nullable column
if (_nested_column->is_nullable()) {
- assert_cast<ColumnNullable&>(*_nested_column)
- .apply_null_map(static_cast<const
ColumnUInt8&>(get_null_map_column()));
- _null_map =
assert_cast<ColumnNullable&>(*_nested_column).get_null_map_column_ptr();
- _nested_column =
assert_cast<ColumnNullable&>(*_nested_column).get_nested_column_ptr();
- }
-
- if (is_column_const(get_null_map_column())) [[unlikely]] {
- throw doris::Exception(ErrorCode::INTERNAL_ERROR,
- "ColumnNullable cannot have constant null map");
- __builtin_unreachable();
+ auto& nested_nullable = assert_cast<ColumnNullable&>(*_nested_column);
+ nested_nullable.apply_null_map(get_null_map_column());
+ _null_map = nested_nullable._null_map;
+ _nested_column = nested_nullable.get_nested_column_ptr();
}
Review Comment:
This delegating constructor no longer consumes the rvalue null-map pointer.
`assert_mutable_null_map(null_map_)` creates a `ColumnUInt8::MutablePtr` from
the raw pointer while the original `MutableColumnPtr` argument still owns the
same object. When the nested column is already nullable, the delegated
constructor immediately calls `get_null_map_column()` in the flattening path,
and the non-const `ColumnUInt8::WrappedPtr` access asserts exclusive ownership;
with the original argument plus `_null_map`, `use_count()` is greater than 1
and construction can throw `COW::assert_mutable`. Even when that path is not
taken, callers passing `std::move(null_map)` are left with a still-owning
pointer, which is a behavioral change from the old `std::move(null_map_)`
member initialization. Please make the helper consume/reset the source
`MutableColumnPtr` after type-checking (or otherwise move through a typed
owner) so the constructed `ColumnNullable` is the sole owner before any
non-const `_nul
l_map` access.
--
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]
