LuciferYang commented on code in PR #65684: URL: https://github.com/apache/doris/pull/65684#discussion_r4120227936
########## 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: [HIGH] 两个新加的回归测试套件在 normal 模式跑不起来：`topn_lazy/test_constant_column_topn_lazy_rowid` 的 `.out` 整个没提交，跑到 `order_qt_topn_mixes_constant_and_physical_rows` 直接报 `Missing outputFile`；`time_travel` 的 `test_constant_hidden_column_statistics.out` 只有 `hidden_after_alter_add_default` 一个 block，groovy 有 11 个 tag，跑到第 2 个 `order_qt_hidden_initial_version_equal` 就报 `Missing output block for tag`。 这两条套子恰是这次 const 列重构在 TopN 回捞 / 时间旅行统计上的回归测试保护（走的是已修好的列存 / segment 路径），跑不起来就等于这块没了回归测试兜底。 用 regression runner 重新生成并提交完整 `.out`（`-forceGenOut`），别手写补 block。 ########## 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 + // its published value must be obtained from the rowset. Resolving it would require bypassing + // the row cache and reading the column store with rowset context, defeating this optimization. if (schema.row_columns_uids().empty()) { Review Comment: [MEDIUM] 整行行存表（`store_row_column=true`）上，短路点查和 TopN lazy 回捞直接解 JSONB，`__DORIS_VERSION_COL__`/`__DORIS_COMMIT_TSO_COL__` 读回的是盘上占位符 0 而不是 rowset 真实值。rowset context（version/commit_tso）只传到列存分支，这两条快路径没接上。 这处（`point_query_executor.cpp`）你已用注释标成主动接受的限制，但 row cache 没绕过、命中也返回 0；`be/src/exec/rowid_fetcher.cpp:1023` 那处没任何说明，一声不响返回错值。 要么把带 hidden 投影的查询从这两条快路径踢出去、回退到正确的 segment 读，要么从 rowset context 覆盖这两列。两处一起改，至少给 rowid_fetcher 补上说明或报错。 注：这条定 MEDIUM——是内部 hidden 列、要 `show_hidden_columns` 才投影得到，且本 PR 没有 `store_row_column=true` 的用例覆盖这两条路径。 -- 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]
