Copilot commented on code in PR #61383:
URL: https://github.com/apache/doris/pull/61383#discussion_r2939083072
##########
be/src/storage/segment/variant/variant_column_reader.h:
##########
@@ -452,10 +452,6 @@ class VariantRootColumnIterator : public ColumnIterator {
ordinal_t get_current_ordinal() const override { return
_inner_iter->get_current_ordinal(); }
- Status read_null_map(size_t* n, NullMap& null_map) override {
- return _inner_iter->read_null_map(n, null_map);
- }
-
Status init_prefetcher(const SegmentPrefetchParams& params) override;
void collect_prefetchers(
std::map<PrefetcherInitMethod, std::vector<SegmentPrefetcher*>>&
prefetchers,
Review Comment:
VariantRootColumnIterator no longer overrides read_null_map(). The base
ColumnIterator implementation returns NotSupported, so callers that rely on
reading the null map (e.g. COUNT_NULL paths that call
ColumnIterator::read_null_map) will fail for VARIANT root columns. Consider
re-adding the override to delegate to _inner_iter->read_null_map().
##########
be/src/exec/common/variant_util.cpp:
##########
@@ -969,6 +983,16 @@ Status VariantCompactionUtil::check_path_stats(const
std::vector<RowsetSharedPtr
return Status::OK();
}
}
+ for (const auto& column : output->tablet_schema()->columns()) {
+ if (column->is_variant_type() &&
!should_check_variant_path_stats(*column)) {
+ return Status::OK();
+ }
+ }
+ for (const auto& column : output->tablet_schema()->columns()) {
+ if (!column->is_variant_type()) {
+ continue;
+ }
+ }
Review Comment:
This loop has an empty body and no side effects, so it can be removed. As-is
it looks like leftover scaffolding and makes the control flow harder to follow
in check_path_stats().
##########
be/src/storage/segment/column_writer.h:
##########
@@ -679,6 +679,8 @@ class VariantColumnWriter : public ColumnWriter {
return Status::NotSupported("variant writer has no data, can not
finish_current_page");
}
+ VariantColumnWriterImpl* impl_for_test() const { return _impl.get(); }
Review Comment:
impl_for_test() is a test-only escape hatch but it is exposed
unconditionally in a production header. Other segment headers gate test-only
helpers behind #ifdef BE_TEST (e.g.
ColumnReader::check_data_by_zone_map_for_test). Consider guarding this method
similarly (or using a friend test) to avoid expanding the public API surface
and dependencies in non-test builds.
--
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]
