-
Notifications
You must be signed in to change notification settings - Fork 3.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[opt](assert_cast) Make assert cast do type check in release build by default #39030
Conversation
Thank you for your contribution to Apache Doris. Since 2024-03-18, the Document has been moved to doris-website. |
1ea4f6a
to
b05c6dc
Compare
clang-tidy review says "All clean, LGTM! 👍" |
2 similar comments
clang-tidy review says "All clean, LGTM! 👍" |
clang-tidy review says "All clean, LGTM! 👍" |
a39ee67
to
59433b2
Compare
run buildall |
clang-tidy review says "All clean, LGTM! 👍" |
4 similar comments
clang-tidy review says "All clean, LGTM! 👍" |
clang-tidy review says "All clean, LGTM! 👍" |
clang-tidy review says "All clean, LGTM! 👍" |
clang-tidy review says "All clean, LGTM! 👍" |
be/src/olap/schema_change.cpp
Outdated
@@ -360,7 +360,8 @@ Status BlockChanger::change_block(vectorized::Block* ref_block, | |||
// not nullable to nullable | |||
if (new_col_nullable) { | |||
auto* new_nullable_col = | |||
assert_cast<vectorized::ColumnNullable*>(new_col->assume_mutable().get()); | |||
assert_cast<vectorized::ColumnNullable*, TypeCheckOnRelease::DISABLE>( | |||
new_col->assume_mutable().get()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed disable
be/src/olap/bloom_filter_predicate.h
Outdated
@@ -69,13 +70,15 @@ class BloomFilterColumnPredicate : public ColumnPredicate { | |||
|
|||
uint16_t new_size = 0; | |||
if (column.is_column_dictionary()) { | |||
const auto* dict_col = assert_cast<const vectorized::ColumnDictI32*>(&column); | |||
const auto* dict_col = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed disable
/** Perform static_cast in release build. | ||
* Checks type by comparing typeid and throw an exception in debug build. | ||
* The exact match of the type is checked. That is, cast to the ancestor will be unsuccessful. | ||
*/ | ||
template <typename To, typename From> | ||
template <typename To, TypeCheckOnRelease check = TypeCheckOnRelease::ENABLE, typename From> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add comment to explain why need this parameter
@@ -408,7 +408,10 @@ struct MethodKeysFixed : public MethodBase<TData> { | |||
CHECK_EQ(sizeof(Fixed), key_sizes[j]); | |||
if (!nullmap_columns.empty() && nullmap_columns[j]) { | |||
const auto& nullmap = | |||
assert_cast<const ColumnUInt8&>(*nullmap_columns[j]).get_data().data(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not need disable
be/src/vec/core/block.cpp
Outdated
@@ -518,7 +518,8 @@ std::string Block::dump_data(size_t begin, size_t row_limit, bool allow_null_mis | |||
if (data[i].column) { | |||
if (data[i].type->is_nullable() && !data[i].column->is_nullable()) { | |||
assert(allow_null_mismatch); | |||
s = assert_cast<const DataTypeNullable*>(data[i].type.get()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not need disable,this is a debug function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -239,7 +239,8 @@ DataTypes make_nullable(const DataTypes& types) { | |||
|
|||
DataTypePtr remove_nullable(const DataTypePtr& type) { | |||
if (type->is_nullable()) { | |||
return assert_cast<const DataTypeNullable*>(type.get())->get_nested_type(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not need
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -155,12 +155,15 @@ Status TransactionalHiveReader::init_row_filters(const TFileRangeDesc& range) { | |||
static int ORIGINAL_TRANSACTION_INDEX = 0; | |||
static int BUCKET_ID_INDEX = 1; | |||
static int ROW_ID_INDEX = 2; | |||
const ColumnInt64& original_transaction_column = assert_cast<const ColumnInt64&>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not need
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -382,8 +382,10 @@ Status VOrcTransformer::_resize_row_batch(const DataTypePtr& type, const IColumn | |||
for (auto* child : struct_batch->fields) { | |||
const IColumn& child_column = struct_col.get_column(idx); | |||
child->resize(child_column.size()); | |||
auto child_type = assert_cast<const vectorized::DataTypeStruct*>(real_type.get()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe not need
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -576,8 +577,8 @@ struct Trim2Impl { | |||
const auto& rcol = | |||
assert_cast<const ColumnConst*>(block.get_by_position(arguments[1]).column.get()) | |||
->get_data_column_ptr(); | |||
if (const auto* col = assert_cast<const ColumnString*>(column.get())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not need
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -429,7 +429,8 @@ ColumnPtr convert_to_ipv6(const StringColumnType& string_column, | |||
(*vec_null_map_to)[i] = true; | |||
} | |||
if constexpr (std::is_same_v<ToColumn, ColumnString>) { | |||
auto* column_string = assert_cast<ColumnString*>(col_res.get()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not need
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -55,7 +55,7 @@ template <typename Type> | |||
const ColumnConst* check_and_get_column_const(const IColumn* column) { | |||
if (!column || !is_column_const(*column)) return {}; | |||
|
|||
const ColumnConst* res = assert_cast<const ColumnConst*>(column); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe this not need
because it already has may if logic
@@ -148,7 +148,7 @@ void validate_argument_type(const IFunction& func, const DataTypes& arguments, | |||
const ColumnConst* check_and_get_column_const_string_or_fixedstring(const IColumn* column) { | |||
if (!is_column_const(*column)) return {}; | |||
|
|||
const ColumnConst* res = assert_cast<const ColumnConst*>(column); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe not need
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Const property is guaranteed by is_column_const
@@ -327,14 +328,15 @@ class FunctionArrayElement : public IFunction { | |||
const UInt8* src_null_map, UInt8* dst_null_map) const { | |||
// check array nested column type and get data | |||
auto left_column = arguments[0].column->convert_to_full_column_if_const(); | |||
const auto& array_column = reinterpret_cast<const ColumnArray&>(*left_column); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe not need
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
TPC-H: Total hot run time: 39587 ms
|
TPC-DS: Total hot run time: 202453 ms
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
PR approved by at least one committer and no changes requested. |
assert_cast<const Derived*>(this)->deserialize_and_merge(place, deserialized_place, | ||
buffer_reader, arena); | ||
(assert_cast<const ColumnString&, TypeCheckOnRelease::DISABLE>(column)) | ||
.get_data_at(i)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this one should enable check.
Also an optimization can be done: perform assert cast outside of the loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make sense
clang-tidy review says "All clean, LGTM! 👍" |
run buildall |
run buildall |
run buildall |
clang-tidy review says "All clean, LGTM! 👍" |
1 similar comment
clang-tidy review says "All clean, LGTM! 👍" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
PR approved by at least one committer and no changes requested. |
TPC-H: Total hot run time: 39516 ms
|
TPC-DS: Total hot run time: 199574 ms
|
ClickBench: Total hot run time: 30.66 s
|
… default (apache#39030) * Problem to solve We encountered many issues that ultimately proved to be caused by memory insecurity. These issues are hard to solve, and the final crash log maybe not related to the root problem. * Fix We make `assert_cast` do type check in release build by default. And we can use a template arg `TypeCheckOnRelease::DISABLE` to disable type check in release build. `TypeCheckOnRelease::DISABLE` should be used when user agrees that this function will be called many many times (eg. add method of AggregatedData, which will be called by rows) or you think type safe has already been guaranteed (eg. `assert_cast<const Derived*, TypeCheckOnRelease::DISABLE>(this)`.
… default (#39030) * Problem to solve We encountered many issues that ultimately proved to be caused by memory insecurity. These issues are hard to solve, and the final crash log maybe not related to the root problem. * Fix We make `assert_cast` do type check in release build by default. And we can use a template arg `TypeCheckOnRelease::DISABLE` to disable type check in release build. `TypeCheckOnRelease::DISABLE` should be used when user agrees that this function will be called many many times (eg. add method of AggregatedData, which will be called by rows) or you think type safe has already been guaranteed (eg. `assert_cast<const Derived*, TypeCheckOnRelease::DISABLE>(this)`.
We encountered many issues that ultimately proved to be caused by memory insecurity. These issues are hard to solve, and the final crash log maybe not related to the root problem.
We make
assert_cast
do type check in release build by default. And we can use a template argTypeCheckOnRelease::DISABLE
to disable type check in release build.TypeCheckOnRelease::DISABLE
should be used when user agrees that this function will be called many many times (eg. add method of AggregatedData, which will be called by rows) or you think type safe has already been guaranteed (eg.assert_cast<const Derived*, TypeCheckOnRelease::DISABLE>(this)
.