github-actions[bot] commented on code in PR #68125: URL: https://github.com/apache/doris/pull/68125#discussion_r4071127625
########## be/src/storage/read_time_hidden_column.cpp: ########## @@ -0,0 +1,91 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +#include "storage/read_time_hidden_column.h" + +#include "common/logging.h" +#include "core/column/column.h" +#include "storage/tablet/tablet_schema.h" +#include "storage/utils.h" + +namespace doris { + +ReadTimeHiddenColumnType get_read_time_hidden_column_type(const TabletColumn& column) { + const auto& column_name = column.name(); + if (column_name == VERSION_COL) { + return ReadTimeHiddenColumnType::VERSION; + } + if (column_name == COMMIT_TSO_COL) { + return ReadTimeHiddenColumnType::COMMIT_TSO; + } + if (column_name == BINLOG_TSO_COL) { + return ReadTimeHiddenColumnType::BINLOG_TSO; + } + return ReadTimeHiddenColumnType::NONE; +} + +ReadTimeHiddenColumnType get_read_time_hidden_column_type(const TabletSchema& schema, + int32_t column_unique_id) { + const int32_t column_idx = schema.field_index(column_unique_id); + if (column_idx < 0) { + return ReadTimeHiddenColumnType::NONE; + } + return get_read_time_hidden_column_type(schema.column(column_idx)); +} + +std::optional<Field> get_read_time_hidden_column_value(ReadTimeHiddenColumnType column_type, + const Version& version, + const TsoRange& commit_tso, + bool read_row_binlog) { + if (version.first != version.second) { Review Comment: [P1] Preserve hidden values across ordered link compaction. Ordered-data compaction can hard-link eligible singleton segment files and publish them under a multi-version output without running SegmentIterator, so their VERSION/COMMIT_TSO columns still contain the load-time zero placeholders. This branch then returns `nullopt` and normal scans, direct reads, predicates, and indexes trust those zeros; on a row-format DUP table, `COMMIT_TSO < targetTso` can expose rows committed after the requested snapshot. This differs from the existing rewriting-compaction/JSONB thread because no column is materialized here. Please disable link compaction for these schemas, materialize the hidden values, or retain each linked segment's source version/TSO metadata, and cover an eligible ordered-compaction table with exact-value assertions. ########## be/src/storage/segment/segment_iterator.cpp: ########## @@ -1802,6 +1804,12 @@ Status SegmentIterator::_init_index_iterators() { // Inverted index iterators Review Comment: [P1] Do not trust online-built VERSION indexes for multi-version rowsets. Both online index build paths create bare `StorageReadOptions`, whose version defaults to `(0,0)`; their SegmentIterator therefore replaces every VERSION value with zero before feeding the index writer, even when the cloned rowset preserves a range such as `[2,3]`. A later query over that range gets no read-time constant here and applies the zero-only index, so `VERSION = 2` can be filtered out before row-level evaluation. The existing singleton thread is guarded by this branch, but this multi-version precondition is not. Please pass the owning rowset metadata into both index-builder reads, protect already-built indexes, and test online construction over a rowset containing multiple VERSION values. ########## be/src/storage/read_time_hidden_column.cpp: ########## @@ -0,0 +1,91 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +#include "storage/read_time_hidden_column.h" + +#include "common/logging.h" +#include "core/column/column.h" +#include "storage/tablet/tablet_schema.h" +#include "storage/utils.h" + +namespace doris { + +ReadTimeHiddenColumnType get_read_time_hidden_column_type(const TabletColumn& column) { + const auto& column_name = column.name(); + if (column_name == VERSION_COL) { + return ReadTimeHiddenColumnType::VERSION; + } + if (column_name == COMMIT_TSO_COL) { + return ReadTimeHiddenColumnType::COMMIT_TSO; + } + if (column_name == BINLOG_TSO_COL) { + return ReadTimeHiddenColumnType::BINLOG_TSO; + } + return ReadTimeHiddenColumnType::NONE; +} + +ReadTimeHiddenColumnType get_read_time_hidden_column_type(const TabletSchema& schema, + int32_t column_unique_id) { + const int32_t column_idx = schema.field_index(column_unique_id); + if (column_idx < 0) { + return ReadTimeHiddenColumnType::NONE; + } + return get_read_time_hidden_column_type(schema.column(column_idx)); +} + +std::optional<Field> get_read_time_hidden_column_value(ReadTimeHiddenColumnType column_type, + const Version& version, + const TsoRange& commit_tso, + bool read_row_binlog) { + if (version.first != version.second) { + return std::nullopt; + } + switch (column_type) { + case ReadTimeHiddenColumnType::VERSION: + return Field::create_field<TYPE_BIGINT>(version.second); + case ReadTimeHiddenColumnType::COMMIT_TSO: + if (commit_tso.end_tso() != -1) { Review Comment: [P1] Preserve COMMIT_TSO through zero-copy rowset replacement. Missing metadata here does not mean the physical column is valid: IndexBuilder hard-links a published singleton's segment into a same-version replacement, but its fresh spec `RowsetMeta` never copies `input_rowset_meta->commit_tso()`, and `manual_build()` only preserves that field when the spec contains it. The replacement therefore has `commit_tso=(-1,-1)` while the linked COMMIT_TSO column is still the load-time 0; this branch returns `nullopt`, so scans, direct reads, and `COMMIT_TSO < targetTso` can expose future rows after adding or dropping even an unrelated index. Snapshot rowset-ID conversion and restore use the same metadata-copy shape. Please preserve the field across every linked replacement, audit the other publish-time metadata, and add exact-value/time-travel coverage around index add/drop and clone or migration. -- 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]
