HappenLee commented on code in PR #68125:
URL: https://github.com/apache/doris/pull/68125#discussion_r4310939379
##########
be/src/exec/rowid_fetcher.cpp:
##########
@@ -165,6 +166,45 @@ struct SegItem {
SegmentSharedPtr segment;
};
+// Appends the values of `read_column` at `row_ids` to `column`.
VERSION/COMMIT_TSO of a
+// single-version rowset are the rowset's own version/TSO; every other value
is read straight
+// from `segment`, whose column iterator and read options live in
`iterator_map` per slot.
+static Status read_slot_column_by_rowid(
+ const SegKey& seg_key, const BetaRowset& rowset, const
SegmentSharedPtr& segment,
+ SlotDescriptor& slot, const TabletColumn& read_column, const
std::vector<uint32_t>& row_ids,
+ OlapReaderStatistics& stats, io::FileCacheMissPolicy
file_cache_miss_policy,
+ std::unordered_map<IteratorKey, IteratorItem, HashOfIteratorKey>&
iterator_map,
+ MutableColumnPtr& column) {
+ if (auto value =
+
get_read_time_hidden_column_value(get_read_time_hidden_column_type(read_column),
+ rowset.version(),
rowset.commit_tso());
+ value.has_value()) {
+ for (size_t i = 0; i < row_ids.size(); ++i) {
+ column->insert(*value);
+ }
Review Comment:
Could we append this BIGINT constant with a batched fill instead of calling
`insert(Field)` once per row? `ColumnVector::insert_many_vals(value, count)`
performs one resize followed by `std::fill`. Simply replacing this loop with
`insert_duplicate_fields` would not provide that implementation for numeric
columns today: its inherited implementation still loops over `insert`.
One reusable option would be to override `insert_duplicate_fields` in
`ColumnVector` to delegate to `insert_many_vals(x.get<T>(), n)`, and use that
interface here. That also lets `ColumnNullable` retain its existing batched
null-map handling, avoiding an unconditional cast of the destination to a
non-nullable BIGINT column. The change should preserve zero-count and append
semantics.
The caller already constructs `Block(slots, row_ids.size())`, which reserves
the batch capacity, so the opportunity is reducing per-row dispatch, Field
extraction and push-back overhead, rather than avoiding repeated allocations.
##########
be/src/storage/read_time_hidden_column.cpp:
##########
@@ -0,0 +1,76 @@
+// 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 "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));
+}
+
+bool has_load_placeholder_hidden_column(const TabletSchema& schema) {
+ // by name, like the readers: TabletMeta does not fill version_col_idx for
FE-created tablets
+ return schema.field_index(VERSION_COL) != -1 ||
schema.field_index(COMMIT_TSO_COL) != -1;
+}
+
+std::optional<Field>
get_read_time_hidden_column_value(ReadTimeHiddenColumnType column_type,
+ const Version& version,
+ const TsoRange&
commit_tso) {
+ 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:
Could we share the VERSION/COMMIT_TSO resolution policy with
`Segment::_get_column_reader_for_read()`? Both paths decide whether to
synthesize a singleton value or read physical storage, but their COMMIT_TSO
checks differ: this helper only checks `end_tso != -1`, whereas Segment also
validates the TSO range and requires a singleton TSO for a singleton rowset.
For example, with version `[7,7]` and TSO `[100,200]`, this helper returns
`200` while Segment rejects the metadata. This illustrates a policy difference,
not a demonstrated valid-data failure.
Could a shared function return a constant, a physical-read decision, or an
error, with both callers using it? Please preserve the pre-publish
physical-read case and the separate BINLOG_TSO semantics. Direct reads should
still resolve valid constants without opening a segment. This would keep the
policy in one place without coupling the two read implementations.
--
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]
