github-actions[bot] commented on code in PR #69195:
URL: https://github.com/apache/doris/pull/69195#discussion_r4293328395
##########
be/src/storage/schema.cpp:
##########
@@ -181,6 +182,20 @@ void
ReadSchema::append_dropped_columns(std::vector<TabletColumn> columns) {
}
}
+void ReadSchema::append_rowid_column() {
+ DORIS_CHECK_EQ(_num_block_columns, _read_columns.size());
+ DORIS_CHECK_EQ(_rowid_ordinal, -1);
+ auto column = std::make_shared<TabletColumn>(
+ FieldAggregationMethod::OLAP_FIELD_AGGREGATION_REPLACE,
+ FieldType::OLAP_FIELD_TYPE_STRING, false,
std::numeric_limits<int32_t>::max(),
+ sizeof(RowLocation));
+ column->set_name(BeConsts::ROWID_COL);
Review Comment:
[P1] Declare the new compaction row-id column identifier
`BeConsts::ROWID_COL` has no declaration in this checkout: `common/consts.h`
defines `GLOBAL_ROWID_COL` and the other column names, and the only exact-word
`ROWID_COL` occurrences are the new uses here, in
`SegmentIterator::_init_column_iterators()`, and in the new schema test. Both
production files therefore fail C++ name lookup. Add the compaction identifier
and wire its schema ordinal recognition consistently with the segment dispatch.
##########
be/src/storage/rowset/segcompaction.cpp:
##########
@@ -88,10 +88,11 @@ Status SegcompactionWorker::_get_segcompaction_reader(
bool is_key, std::vector<uint32_t>& key_group_cluster_key_idxes,
std::unique_ptr<VerticalBlockReader>* reader) {
const auto& ctx = _writer->_context;
- bool record_rowids = need_convert_delete_bitmap() && is_key;
StorageReadOptions read_options(stat);
read_options.use_page_cache = false;
- read_options.record_rowids = record_rowids;
+ read_options.tablet_schema = ctx.tablet_schema;
Review Comment:
[P1] Use fields supported by StorageReadOptions
`StorageReadOptions` in `storage/iterators.h` has no `tablet_schema` member,
so this new assignment makes `segcompaction.cpp` fail to compile. The schema
already reaches `new_iterator()` through `read_schema` and the segment; remove
this stale assignment or pass the specific supported schema facts the reader
needs.
##########
be/src/storage/merger.cpp:
##########
@@ -96,6 +99,29 @@ Status set_key_range(TabletReader::ReaderParams*
reader_params, const TabletSche
return Status::OK();
}
+Block prepare_compaction_output_block(const Block& block, int32_t
rowid_ordinal,
+ std::vector<RowLocation>* row_locations)
{
+ DORIS_CHECK_GE(rowid_ordinal, 0);
+ auto ordinal = static_cast<size_t>(rowid_ordinal);
+ DORIS_CHECK_LT(ordinal, block.columns());
+ const auto& rowid_column =
+ assert_cast<const
ColumnString&>(*block.get_by_position(ordinal).column);
+ DORIS_CHECK_EQ(rowid_column.size(), block.rows());
+ row_locations->clear();
+ row_locations->reserve(rowid_column.size());
+ for (size_t i = 0; i < rowid_column.size(); ++i) {
+ const auto rowid = rowid_column.get_data_at(i);
+ DCHECK_EQ(rowid.size, sizeof(RowLocation));
+ RowLocation location;
+ std::memcpy(&location, rowid.data, sizeof(RowLocation));
+ row_locations->emplace_back(location);
+ }
+
+ Block output_block = block;
+ output_block.erase(ordinal);
Review Comment:
[P2] Release the output view before clearing the input block
The returned block shares every data column with `block`, and all three
conversion-enabled merge loops retain it in `output_block` across
`block.clear_column_data()` and the next read. `Block::clear_column_data()`
uses `clone_empty()` for shared columns, so the heap/unique merge paths now
discard their reusable column buffers every batch and allocate them again while
the previous batch is still retained. This adds allocation churn and an extra
live batch to normal MoW/index compaction. Scope or clear the output view after
writing/conversion and before clearing the input block.
##########
be/src/storage/iterators.h:
##########
@@ -130,7 +130,6 @@ class StorageReadOptions {
// The compaction output schema's variant path layout; null outside a
variant compaction.
VariantCompactionPathsSPtr variant_compaction_paths;
bool enable_unique_key_merge_on_write = false;
- bool record_rowids = false;
Review Comment:
[P1] Migrate the remaining users before removing record_rowids
`StorageReadOptions::record_rowids` is removed here, but
`SegmentIterator::_count_on_index_fastpath_safe()` and
`_should_engage_count_emit_shortcut()` still read `_opts.record_rowids`
(segment_iterator.cpp:1554 and :1614), and
`segment_iterator_count_emit_shortcut_test.cpp:367` still assigns it. The
production translation unit and that test therefore no longer compile. Migrate
these fast-path guards and their test to the synthetic-column contract as part
of this removal.
--
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]
