github-actions[bot] commented on code in PR #63491:
URL: https://github.com/apache/doris/pull/63491#discussion_r3300964056
##########
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);
Review Comment:
This can throw when the mutable constructor is used to flatten
`Nullable(Nullable(T))`. The delegating constructor above calls
`assert_mutable_null_map(null_map_)` while `null_map_` still owns the same
`ColumnUInt8`, so `_null_map` has an extra owner until the delegating
constructor parameter is destroyed. If `_nested_column` is nullable, this
non-const `get_null_map_column()` goes through
`ColumnUInt8::WrappedPtr::operator*()` and `assert_mutable_ref()`, sees
`use_count() > 1`, and fails before the null maps can be merged. A valid path
is `ColumnNullable::create(std::move(nullable_nested),
std::move(null_map_as_mutable_column_ptr))`. Please consume/move the original
`MutableColumnPtr` before creating the typed owner, or pass the null map to
`apply_null_map` through a const path that does not require exclusive ownership.
--
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]
