github-actions[bot] commented on code in PR #66614:
URL: https://github.com/apache/doris/pull/66614#discussion_r3749733499
##########
be/src/core/column/column.cpp:
##########
@@ -28,6 +28,16 @@
namespace doris {
+ColumnPtrWrapper::ColumnPtrWrapper(ColumnPtr column) {
+ DORIS_CHECK(column.get() != nullptr);
+ if (const auto* const_column =
check_and_get_column<ColumnConst>(column.get())) {
+ _column_ptr = const_column->get_data_column_ptr();
+ } else {
+ _column_ptr = std::move(column);
+ }
+ DORIS_CHECK_EQ(_column_ptr->size(), 1);
Review Comment:
[P1] Update the existing two-row mock-constant tests
This invariant is reached immediately by the existing non-disabled
`ScanNormalizePredicate.test_eval_const_conjuncts3` and
`test_eval_const_conjuncts4`: lines 130 and 154 pass two-row non-const columns
through `MockFnCall::set_const_expr_col()`, which constructs this wrapper
outside any death/exception assertion. The normal BE-UT CMake glob includes
`exec/*.cpp`, so those tests now abort or throw before reaching their remaining
code. (The green macOS check only builds BE with `MAKE_TEST=OFF`.) Please
update/remove those malformed fixtures, or assert the new failure at
construction, together with this invariant.
##########
be/test/core/column/column_const_test.cpp:
##########
@@ -42,6 +42,26 @@ TEST(ColumnConstTest, TestCreate) {
EXPECT_TRUE(!is_column_const(column_const2->get_data_column()));
}
+TEST(ColumnPtrWrapperTest, StoresSingleRowDataColumn) {
+ auto data_column = ColumnHelper::create_column<DataTypeInt64>({7});
+
+ ColumnPtrWrapper data_wrapper(data_column);
+ EXPECT_EQ(data_wrapper.column_ptr().get(), data_column.get());
+ EXPECT_EQ(data_wrapper.column().get_int(0), 7);
+ EXPECT_FALSE(is_column_const(data_wrapper.column()));
+
+ auto const_column = ColumnConst::create(data_column, 3);
+ ColumnPtrWrapper const_wrapper(std::move(const_column));
+ EXPECT_EQ(const_wrapper.column_ptr().get(), data_column.get());
+ EXPECT_EQ(const_wrapper.column().get_int(0), 7);
+ EXPECT_FALSE(is_column_const(const_wrapper.column()));
+}
+
+TEST(ColumnPtrWrapperTest, RejectsNonConstMultiRowColumn) {
+ auto column = ColumnHelper::create_column<DataTypeInt64>({7, 8});
+ EXPECT_DEATH(static_cast<void>(ColumnPtrWrapper(column)), "");
Review Comment:
[P1] Assert the release-mode exception path here
This only behaves as a death assertion while DCHECKs are enabled. With
`NDEBUG`, `DORIS_CHECK_EQ` uses the release comparison macro and
`doris_check_fail()` throws `doris::Exception`; GoogleTest does not count
leaving a death-test statement by exception as death. The existing
`be/test/common/check_test.cpp` handles this contract by capturing the
exception under `NDEBUG` and using `EXPECT_DEATH` only under `DCHECK_IS_ON()`.
Please mirror that split (or use a build-mode-neutral helper), otherwise
release BE-UT fails this new test.
--
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]
