github-actions[bot] commented on code in PR #65684:
URL: https://github.com/apache/doris/pull/65684#discussion_r4061765393
##########
be/src/exec/rowid_fetcher.cpp:
##########
@@ -1032,13 +1034,18 @@ Status RowIdStorageReader::read_doris_format_row(
.rowset_id = rowset_id,
.segment_id = segment_id,
.slot_id = slots[x].id()};
- IteratorItem& iterator_item = iterator_map[iterator_key];
+ IteratorItem& iterator_item =
+ iterator_map.try_emplace(iterator_key,
stats).first->second;
if (iterator_item.segment == nullptr) {
- iterator_map[iterator_key].segment = segment;
- iterator_item.storage_read_options.stats = &stats;
+ iterator_item.segment = segment;
iterator_item.storage_read_options.io_ctx.reader_type =
ReaderType::READER_QUERY;
iterator_item.storage_read_options.io_ctx.file_cache_miss_policy =
file_cache_miss_policy;
+ // A rowid fetch bypasses TabletReader, so copy the rowset
context it would have
Review Comment:
[P1] Synthesize hidden columns in row-store lazy fetch
The rowset context only reaches the column-store branch. When
`fetch_row_store=true`, the branch above decodes full-row JSONB directly, even
though that JSONB persists hidden VERSION/COMMIT_TSO placeholders.
`RowStoreFetchChecker` does not exclude those hidden slots, so TopN lazy
materialization on a `store_row_column=true` table can still return 0. Please
either keep runtime hidden columns off row-store lazy fetch or overwrite them
from this rowset context, and cover that full-row-store case in the TopN
regression.
##########
be/src/storage/segment/variant/nested_group_streaming_write_plan.cpp:
##########
@@ -149,20 +150,18 @@ Status append_plan_from_rowset_reader(const
RowsetReaderSharedPtr& input_rs_read
std::static_pointer_cast<BetaRowset>(rowset), &segment_cache));
for (const auto& segment : segment_cache.get_segments()) {
- std::shared_ptr<ColumnReader> column_reader;
+ std::shared_ptr<VariantColumnReader> variant_reader;
OlapReaderStatistics stats;
- Status st = segment->get_column_reader(variant_uid, &column_reader,
&stats);
+ StorageReadOptions read_options(stats);
+ const auto& variant_column =
rowset->tablet_schema()->column_by_uid(variant_uid);
Review Comment:
[P1] Keep current VARIANT identity in compaction
This lookup happens in the historical rowset schema, so a rowset that
predates `ADD VARIANT` (or predates a same-name DROP/ADD with a new UID) has no
`variant_uid`; `column_by_uid()` uses `.at()` and throws before the NOT_FOUND
branch can run. The caller already has the current `variant_column`; please
pass that descriptor through and let `get_variant_root_reader()` decide whether
each segment has physical metadata, then cover both schema-evolution cases in
compaction.
##########
be/src/exprs/function/variant_inverted_index_search.cpp:
##########
@@ -669,14 +670,19 @@ Status VariantNestedSearchEvaluator::evaluate(
}
const ColumnId column_id = static_cast<ColumnId>(ordinal);
- std::shared_ptr<segment_v2::ColumnReader> column_reader;
-
RETURN_IF_ERROR(segment->get_column_reader(segment->tablet_schema()->column(column_id),
- &column_reader,
-
index_exec_ctx->column_iter_opts().stats));
- auto* variant_reader =
dynamic_cast<segment_v2::VariantColumnReader*>(column_reader.get());
- if (variant_reader == nullptr) {
- return Status::InvalidArgument("Column '{}' is not VARIANT for nested
query", root_field);
+ std::shared_ptr<segment_v2::VariantColumnReader> variant_reader;
+ DORIS_CHECK(index_exec_ctx->column_iter_opts().stats != nullptr);
+ StorageReadOptions read_options(*index_exec_ctx->column_iter_opts().stats);
+ read_options.io_ctx = index_exec_ctx->column_iter_opts().io_ctx;
+ Status st =
segment->get_variant_root_reader(segment->tablet_schema()->column(column_id),
Review Comment:
[P1] Resolve NESTED roots from the current schema
The descriptor passed here was resolved by name from the segment's
historical schema. For a segment predating `ADD VARIANT`, the earlier
`field_index` returns -1 and errors instead of reaching this NOT_FOUND
handling; after same-name DROP/ADD, it can bind the old UID and search dropped
data. Please resolve the current logical root/UID from the scan schema (the
current field map is presently ignored) and pass that descriptor here, with ADD
and DROP/ADD NESTED SEARCH coverage.
##########
be/src/service/point_query_executor.cpp:
##########
@@ -101,7 +101,12 @@ static void get_missing_and_include_cids(const
TabletSchema& schema,
}
const TabletColumn& target_rs_column =
schema.column_by_uid(target_rs_column_id);
DCHECK(target_rs_column.is_row_store_column());
- // The full column group is considered a full match, thus no missing cids
+ // An empty row_columns_uids() means the row-store column contains the
full row. Keep
+ // missing_cids empty so full-row point queries can be served entirely
from the row store,
+ // including row-cache hits. Read-time-synthesized hidden columns are
intentionally not
+ // supported on this fast path: for example, JSONB stores 0 for
__DORIS_VERSION_COL__, while
Review Comment:
[P1] Do not return hidden placeholders from point queries
This knowingly turns a valid hidden-column projection into the persisted
placeholder: full-row short-circuit eligibility does not reject
`__DORIS_VERSION_COL__`/`__DORIS_COMMIT_TSO_COL__`, and clearing their UIDs
makes both live JSONB decoding and row-cache hits skip the rowset-aware branch
below. A prepared point query can therefore return 0 instead of the visible
rowset value. Please preserve these runtime columns as missing (and bypass the
row cache for them), or disqualify such projections from the short-circuit
path, with a regression covering both hidden fields.
##########
be/src/storage/segment/segment.cpp:
##########
@@ -1056,26 +1013,176 @@ Status Segment::traverse_column_meta_pbs(const
std::function<void(const ColumnMe
return _column_meta_accessor->traverse_metas(*footer_pb_shared, visitor,
&dummy_stats);
}
-Status Segment::get_column_reader(const TabletColumn& col,
- std::shared_ptr<ColumnReader>* column_reader,
- OlapReaderStatistics* stats, const
io::IOContext* source_io_ctx,
- std::optional<Field> const_value) {
- RETURN_IF_ERROR(_create_column_meta_once(stats, source_io_ctx));
- SCOPED_RAW_TIMER(&stats->segment_create_column_readers_timer_ns);
- int col_uid = col.unique_id() >= 0 ? col.unique_id() :
col.parent_unique_id();
- // The column is not in this segment, return nullptr
- if (!_tablet_schema->has_column_unique_id(col_uid)) {
- *column_reader = nullptr;
- return Status::Error<ErrorCode::NOT_FOUND, false>("column not found in
segment, col_uid={}",
- col_uid);
- }
- if (col.has_path_info()) {
- PathInData relative_path = col.path_info_ptr()->copy_pop_front();
- return _column_reader_cache->get_path_column_reader(col_uid,
relative_path, column_reader,
- stats, nullptr,
source_io_ctx);
- }
- return _column_reader_cache->get_column_reader(col_uid, column_reader,
stats, source_io_ctx,
- std::move(const_value));
+Status Segment::_get_column_reader_for_read(const TabletColumn& col,
+ const StorageReadOptions&
read_options,
+ std::shared_ptr<ColumnReader>*
column_reader) {
+ DORIS_CHECK(read_options.stats != nullptr);
+ const int32_t col_uid = col.unique_id() >= 0 ? col.unique_id() :
col.parent_unique_id();
+ DORIS_CHECK_GE(col_uid, 0) << "column does not have a resolvable uid: " <<
col.debug_string();
+ RETURN_IF_ERROR(_create_column_meta_once(read_options.stats,
&read_options.io_ctx));
+
SCOPED_RAW_TIMER(&read_options.stats->segment_create_column_readers_timer_ns);
+
+ if (col.name() == VERSION_COL) {
+ // A singleton rowset exposes its rowset version for every row.
Segment writers store a
+ // placeholder because the publish version is not known when the
segment is written.
+ if (read_options.version.first == read_options.version.second) {
+ *column_reader = std::make_shared<ConstantColumnReader>(
+
Field::create_field<TYPE_BIGINT>(read_options.version.second), col.type());
+ return Status::OK();
+ }
+ if (!_column_meta_accessor->has_column_uid(col_uid)) {
+ return Status::InternalError("could not find version column read
version is {}-{}",
+ read_options.version.first,
read_options.version.second);
+ }
+ return _column_reader_cache->get_column_reader(col_uid, column_reader,
read_options.stats,
+ &read_options.io_ctx);
+ }
+
+ // Only row-binlog reads reinterpret the NULL/0 placeholder as commit_tso.
For example, an
+ // incremental binlog read of rowset [9-9] should expose its commit_tso,
while a checksum or
+ // schema-change read that happens to include the hidden column must keep
the physical value.
+ // TODO yiguolei: 需要问问boyang 这块read_row_binlog的逻辑
+ if (read_options.read_row_binlog && col.name() == BINLOG_TSO_COL) {
+ const int64_t commit_tso = read_options.commit_tso.end_tso();
+ if (read_options.version.first == read_options.version.second) {
+ DCHECK_EQ(read_options.commit_tso.start_tso(), commit_tso);
+ // TODO yiguolei: zhge -1 也得看看
+ *column_reader = std::make_shared<ConstantColumnReader>(
+ Field::create_field<TYPE_BIGINT>(commit_tso == -1 ? 0 :
commit_tso),
+ col.type());
+ return Status::OK();
+ }
+ if (!_column_meta_accessor->has_column_uid(col_uid)) {
+ return Status::InternalError("could not find binlog tso column");
+ }
+ return _column_reader_cache->get_column_reader(col_uid, column_reader,
read_options.stats,
+ &read_options.io_ctx);
+ }
+
+ if (col.name() == COMMIT_TSO_COL) {
+ const int64_t commit_tso = read_options.commit_tso.end_tso();
+ // For example, a published rowset [12-12] with commit_tso=100
replaces the on-disk 0 with
+ // 100. Before publish, commit_tso=-1 keeps the physical placeholder
reader.
+ if (read_options.version.first == read_options.version.second &&
commit_tso != -1) {
+ *column_reader = std::make_shared<ConstantColumnReader>(
+ Field::create_field<TYPE_BIGINT>(commit_tso), col.type());
+ return Status::OK();
+ }
+ if (!_column_meta_accessor->has_column_uid(col_uid)) {
+ return Status::InternalError("could not find commit tso column");
+ }
+ return _column_reader_cache->get_column_reader(col_uid, column_reader,
read_options.stats,
+ &read_options.io_ctx);
+ }
+
+ // A column added after this segment was written uses its schema default.
For example, after
+ // `ADD COLUMN city STRING DEFAULT 'Paris'`, old segments produce "Paris"
without a data page.
+ // For a BE-generated path such as `v.city` whose uid is -1, col_uid is
the VARIANT parent uid,
+ // so sparse/doc data in the existing root is considered before falling
back to the path
+ // default.
+ if (!_column_meta_accessor->has_column_uid(col_uid)) {
+ Field field;
+ RETURN_IF_ERROR(get_default_value_field(col, &field));
+ *column_reader =
std::make_shared<ConstantColumnReader>(std::move(field), col.type());
+ return Status::OK();
+ }
+
+ // Preserve the established value-read contract: a VARIANT root or a
BE-generated path
+ // (uid=-1, resolved to its parent) returns the root reader, and
new_column_iterator() delegates
+ // typed/sparse/default selection to VariantColumnReader. A schema path
with its own uid keeps
+ // using that physical reader, exactly like an ordinary persisted column.
+ return _column_reader_cache->get_column_reader(col_uid, column_reader,
read_options.stats,
+ &read_options.io_ctx);
+}
+
+Status Segment::get_variant_root_reader(const TabletColumn& col,
+ const StorageReadOptions& read_options,
+ std::shared_ptr<VariantColumnReader>*
column_reader) {
+ DORIS_CHECK(read_options.stats != nullptr);
+
+ // A root VARIANT has parent_uid=-1 and therefore uses its own uid. An
extracted path may have
+ // its own schema uid, but its physical path metadata is owned by the root
identified by
+ // parent_uid. For example, root `v` uses uid=7 and `v.user.id` also
resolves to root uid=7.
+ const int32_t root_uid = col.parent_unique_id() >= 0 ?
col.parent_unique_id() : col.unique_id();
+ DORIS_CHECK_GE(root_uid, 0) << "VARIANT column does not have a resolvable
root uid: "
+ << col.debug_string();
+
+ // Validate the root rather than an extracted path's leaf type. For
example, `v.user.id` may be
+ // BIGINT while its root uid=7 still identifies a physical VARIANT reader.
+ const TabletColumn* root_column = &col;
Review Comment:
[P1] Handle a historically absent VARIANT root before leaf validation
On a production pre-ADD rowset, `Segment` is opened with the historical
rowset schema, so it does not contain the current `root_uid`. For a generated
child such as `v.a`, `root_column` then remains this scalar leaf and the
VARIANT check below returns `InvalidArgument`; `get_data_type_of()` never
receives the NOT_FOUND it expects for its declared-type fallback, and ordinary
`SegmentIterator` initialization fails. The new unit test masks this by opening
old physical data with the current full schema. Please preserve/resolve the
current root descriptor (or detect physical absence before leaf validation) and
test with an actually historical segment schema, including same-name DROP/ADD.
##########
regression-test/suites/query_p0/topn_lazy/test_constant_column_topn_lazy_rowid.groovy:
##########
@@ -0,0 +1,121 @@
+// 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.
+
+suite("test_constant_column_topn_lazy_rowid") {
+ sql "DROP TABLE IF EXISTS test_constant_column_topn_lazy_rowid"
+ sql """
+ CREATE TABLE test_constant_column_topn_lazy_rowid (
+ id INT NOT NULL,
+ score INT NOT NULL
+ )
+ UNIQUE KEY(id)
+ DISTRIBUTED BY HASH(id) BUCKETS 1
+ PROPERTIES (
+ "replication_num" = "1",
+ "enable_unique_key_merge_on_write" = "true",
+ "light_schema_change" = "true",
+ "disable_auto_compaction" = "true"
+ )
+ """
+
+ // Case 1: prepare a TopN result containing old constant-backed rows and a
new physical row.
+ sql """
+ INSERT INTO test_constant_column_topn_lazy_rowid VALUES
+ (1, 100),
+ (2, 80)
+ """
+
+ sql """
+ ALTER TABLE test_constant_column_topn_lazy_rowid
+ ADD COLUMN payload VARCHAR(32) NOT NULL DEFAULT 'old-default'
+ """
+ waitForSchemaChangeDone {
+ sql """
+ SHOW ALTER TABLE COLUMN
+ WHERE TableName = 'test_constant_column_topn_lazy_rowid'
+ ORDER BY CreateTime DESC LIMIT 1
+ """
+ time 600
+ }
+
+ sql """
+ INSERT INTO test_constant_column_topn_lazy_rowid VALUES
+ (3, 90, 'new-physical'),
+ (4, 70, 'not-in-topn')
+ """
+
+ // Case 2: lazy rowid fetch must produce the same added-column and hidden
VERSION values as a
+ // normal scan when constant-backed and physical rows are mixed.
+ sql "SET show_hidden_columns = true"
+ sql "SET topn_lazy_materialization_threshold = -1"
+ def normalRead = sql """
+ SELECT id, payload, __DORIS_VERSION_COL__
+ FROM test_constant_column_topn_lazy_rowid
+ ORDER BY score DESC
+ LIMIT 3
+ """
+ def normalHiddenOnlyRead = sql """
+ SELECT __DORIS_VERSION_COL__
+ FROM test_constant_column_topn_lazy_rowid
+ ORDER BY score DESC
+ LIMIT 3
+ """
+
+ sql "SET topn_lazy_materialization_threshold = 1024"
+ explain {
+ sql """
+ SHAPE PLAN
+ SELECT __DORIS_VERSION_COL__
+ FROM test_constant_column_topn_lazy_rowid
+ ORDER BY score DESC
+ LIMIT 3
+ """
+ contains "PhysicalLazyMaterialize"
+ }
+
+ def lazyRead = sql """
+ SELECT id, payload, __DORIS_VERSION_COL__
+ FROM test_constant_column_topn_lazy_rowid
+ ORDER BY score DESC
+ LIMIT 3
+ """
+ assertEquals(normalRead, lazyRead)
+ assertTrue(lazyRead.every { row -> (row[2] as Long) > 0 },
+ "TopN rowid fetch returned a hidden version placeholder:
${lazyRead}")
+
+ // Case 3: VERSION is the only projected value in this query. Seeing
PhysicalLazyMaterialize above
+ // therefore proves that the hidden column itself reaches the rowid-fetch
phase.
+ def lazyHiddenOnlyRead = sql """
+ SELECT __DORIS_VERSION_COL__
+ FROM test_constant_column_topn_lazy_rowid
+ ORDER BY score DESC
+ LIMIT 3
+ """
+ assertEquals(normalHiddenOnlyRead, lazyHiddenOnlyRead)
+ assertTrue(lazyHiddenOnlyRead.every { row -> (row[0] as Long) > 0 },
+ "TopN rowid fetch returned an invalid hidden version:
${lazyHiddenOnlyRead}")
+
+ order_qt_topn_mixes_constant_and_physical_rows """
Review Comment:
[P1] Commit the TopN expected output
This `order_qt` requires a checked-in expected-output file, but
`regression-test/data/query_p0/topn_lazy/test_constant_column_topn_lazy_rowid.out`
is absent. Normal regression mode throws `Missing outputFile` rather than
generating it, so this new suite cannot pass as committed. Please generate and
commit the `.out` with the regression runner.
##########
regression-test/suites/time_travel_p0/test_constant_hidden_column_statistics.groovy:
##########
@@ -0,0 +1,204 @@
+// 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.
+
+suite("test_constant_hidden_column_statistics", "nonConcurrent") {
+ sql "DROP TABLE IF EXISTS test_constant_hidden_column_statistics FORCE"
+ sql """
+ CREATE TABLE test_constant_hidden_column_statistics (
+ `k` INT NOT NULL,
+ `v` INT NOT NULL
+ )
+ UNIQUE KEY(`k`)
+ DISTRIBUTED BY HASH(`k`) BUCKETS 1
+ PROPERTIES (
+ "replication_num" = "1",
+ "enable_unique_key_merge_on_write" = "true",
+ "light_schema_change" = "true",
+ "disable_auto_compaction" = "true",
+ "binlog.enable" = "true",
+ "binlog.format" = "ROW",
+ "binlog.need_historical_value" = "true"
+ )
+ """
+
+ sql "SET show_hidden_columns = true"
+
+ def readHiddenState = {
+ def rows = sql """
+ SELECT k, __DORIS_VERSION_COL__
+ FROM test_constant_hidden_column_statistics
+ ORDER BY k
+ """
+ return rows.collectEntries { row ->
+ [(Integer.parseInt(row[0].toString())): [
+ version: Long.parseLong(row[1].toString())
+ ]]
+ }
+ }
+
+ def readHiddenMinMax = {
+ def row = sql """
+ SELECT MIN(__DORIS_VERSION_COL__), MAX(__DORIS_VERSION_COL__)
+ FROM test_constant_hidden_column_statistics
+ """
+ return row[0].collect { value -> Long.parseLong(value.toString()) }
+ }
+
+ // Case 1: the first rowset predates c_default. Its hidden version is
captured before ALTER so the
+ // test can verify that schema evolution does not replace it with a
persisted placeholder.
+ sql """
+ INSERT INTO test_constant_hidden_column_statistics VALUES
+ (1, 10),
+ (2, 20)
+ """
+ sql "SYNC"
+
+ def beforeAlter = readHiddenState()
+ assertEquals(2, beforeAlter.size())
+ assertEquals(beforeAlter[1].version, beforeAlter[2].version)
+ assertTrue(beforeAlter[1].version > 0)
+ def oldVersion = beforeAlter[1].version
+
+ sql """
+ ALTER TABLE test_constant_hidden_column_statistics
+ ADD COLUMN `c_default` INT NOT NULL DEFAULT "10"
+ """
+ waitForSchemaChangeDone({
+ sql """
+ SHOW ALTER TABLE COLUMN
+ WHERE IndexName = 'test_constant_hidden_column_statistics'
+ ORDER BY CreateTime DESC LIMIT 1
+ """
+ time 600
+ })
+
+ order_qt_hidden_after_alter_add_default """
+ SELECT k, v, c_default
+ FROM test_constant_hidden_column_statistics
+ ORDER BY k
+ """
+
+ def afterAlter = readHiddenState()
+ assertEquals(beforeAlter, afterAlter)
+
+ // Case 2: with only the pre-ALTER rowset, the hidden version is a
rowset-scoped constant. SQL
+ // MIN/MAX must observe that runtime value rather than the persisted zero
placeholder. FE does
+ // not push this aggregate through a MOW delete bitmap; the direct
statistics-iterator contract
+ // is covered by
SegmentIteratorExprZonemapTest.HiddenConstantsFeedStatisticsIterator.
+ def oldMinMax = readHiddenMinMax()
+ assertEquals([oldVersion, oldVersion], oldMinMax)
+
+ // Case 3: these predicates are evaluated against the constant zonemaps of
the historical rowset.
+ // c_default in the same predicates is also a constant because it was
added after the write.
+ order_qt_hidden_initial_version_equal """
Review Comment:
[P1] Generate every expected time-travel block
The committed `.out` contains only `hidden_after_alter_add_default`; this
label and the other nine labels that follow have no output blocks.
`Suite.quickRunTest` throws `Missing output block for tag` in normal mode, so
the suite stops here. Please regenerate and commit all expected blocks using
the regression runner rather than hand-writing them.
--
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]
