mrhhsg commented on code in PR #68125:
URL: https://github.com/apache/doris/pull/68125#discussion_r4226954349
##########
be/src/storage/segment/segment.cpp:
##########
@@ -149,11 +160,13 @@ Status segment_zone_maps_can_answer_agg(Segment* segment,
const ReadSchema& sche
const StorageReadOptions&
read_options, bool* usable) {
*usable = true;
for (size_t ordinal = 0; ordinal < schema.num_block_columns(); ++ordinal) {
- // The commit-tso column is only served correctly once its reader is
created with the
- // rowset's commit_tso as a const value. Creating it here without one
would cache a reader
- // that hands every later read the on-disk placeholder instead.
- if (static_cast<int32_t>(ordinal) == schema.commit_tso_ordinal()) {
- continue;
+ // VStatisticsIterator reads physical segment ZoneMaps. Read-time
hidden columns contain
+ // placeholders there, so let SegmentIterator materialize their
logical values instead.
+ if
(segment->get_read_time_constant_value(static_cast<int32_t>(ordinal), schema,
+ read_options)
Review Comment:
Addressed in 360150637fe. `segment_zone_maps_can_answer_agg` only sends a
hidden column to `SegmentIterator` when `get_read_time_constant_reader_value()`
cannot synthesize it: an assigned singleton COMMIT_TSO keeps
`VStatisticsIterator` with its `ConstantColumnReader` (metadata-only, two
logical rows), while VERSION / BINLOG_TSO, which only `SegmentIterator`
materializes, fall back.
`CommitTsoMinMaxUsesStatisticsIterator` asserts the iterator type and the
MIN/MAX rows; `VersionMinMaxFallsBackFromStatisticsIterator` /
`BinlogTsoMinMaxFallsBackFromStatisticsIterator` cover the fallback, including
forced pushdown.
##########
be/src/service/point_query_executor.cpp:
##########
@@ -633,6 +663,9 @@ Status PointQueryExecutor::_lookup_row_data() {
read_column, slot, row_ids, column,
storage_read_options, iter));
}
}
+
replace_point_query_read_time_hidden_columns(_reusable->read_time_hidden_columns(),
Review Comment:
Addressed in ac4694b3fe8 / c2ba3bafaf9: VERSION / COMMIT_TSO are left out of
the JSONB decode in both `PointQueryExecutor` and `RowIdStorageReader` and
resolved from the rowset (singleton) or the materialized column storage
(multi-version), so the stale zeros a load serialized into the row store are
never returned after compaction. The compacted-rowset cases of
`test_point_query_read_time_hidden_columns` assert exact per-row versions
through point queries (with and without column-store access) and lazy TopN
fetches after compacting five singleton rowsets.
##########
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:
Not changed in this PR. Evidence that the ordered link path is neither
introduced nor widened here, and is not reachable for COMMIT_TSO:
- **COMMIT_TSO**: `__DORIS_COMMIT_TSO_COL__` exists only on row-format
binlog tables (FE `CreateTableInfo.addRowBinlogHiddenColumns` is its only
producer). For those tablets `handle_ordered_data_compaction` never takes the
tidy-rowset link path; it links only in the LMax quick merge, which requires
every input at `compaction_level == kBinlogCompactionMaxLevel - 1` (= 2) with
the first input being the base rowset at version 0
(`BinlogCumulativeCompactionPolicy::pick_input_rowsets` /
`is_compaction_enough`). Load rowsets are level 0
(`RowsetWriterContext::compaction_level` default) and L0 -> L1 -> L2 are
physical rewrites ("L0/L1: only physical rewrite") whose reader passes each
input's own version and `commit_tso` (`BetaRowsetReader::_init_read_options`),
so every segment that reaches the link path already holds materialized VERSION
/ COMMIT_TSO. `COMMIT_TSO < targetTso` on a row-format DUP table never sees a
linked placeholder.
- **VERSION**: `__DORIS_VERSION_COL__` exists only on UNIQUE tables. MOW is
excluded from the tidy link path (`keys_type == UNIQUE_KEYS &&
enable_unique_key_merge_on_write && !is_binlog_compaction` returns false), so
the only tables where tidy singleton loads get hard-linked into a multi-version
rowset are merge-on-read UNIQUE tables. There the linked VERSION column reads 0
on master today as well: master's
`SegmentIterator::_replace_version_col_if_needed` has the same `version.first
!= version.second` gate, and this PR touches neither that function nor the
compaction code. Nothing in the BE consumes the MOR VERSION value (the only
users of `version_col_idx()` / `version_ordinal()` are that replacement and a
`partial_update_info.cpp` DCHECK); it is visible only through
`show_hidden_columns`.
Fixing the MOR case needs a compaction-side change (per-segment source
version metadata, or refusing the link path when the schema has a hidden
VERSION column and an input is a singleton), which changes compaction behaviour
and cost for every MOR UNIQUE table. The PR description already lists it as a
follow-up; keeping this PR to the read-side contract rather than folding a
compaction policy change into it.
##########
be/src/storage/segment/segment_iterator.cpp:
##########
@@ -1802,6 +1804,12 @@ Status SegmentIterator::_init_index_iterators() {
// Inverted index iterators
Review Comment:
Addressed in 553bf1491ea. Both online build paths
(`IndexBuilder::handle_single_rowset` and `_build_snii_indexes_for_segment`)
now read through `set_read_time_hidden_column_options(*output_rowset_meta,
read_options)`, i.e. with the owning rowset's version and commit TSO, so a
multi-version rowset is indexed from its materialized column and a singleton
from its logical value.
`test_build_index_read_time_hidden_column` builds the index on a compacted
`[2-6]` rowset and, as of 464ccfb9ad0, requires the index job to finish and the
query profile's `RowsInvertedIndexFiltered` to equal 4 / 3 for `= 3` / `in (2,
6)`; an index of zeros filters all five rows instead.
Indexes that an earlier build already wrote with zeros are not repaired at
read time: the index files carry no provenance that tells them apart from a
correct index, so a read-time guard would have to distrust every VERSION index.
The remedy is to rebuild the index after upgrading, which the PR description
states.
--
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]
