mrhhsg commented on code in PR #68125: URL: https://github.com/apache/doris/pull/68125#discussion_r4226955451
########## 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: Addressed in 553bf1491ea for the replacement paths that hard-link a published rowset into fresh metadata: `IndexBuilder::update_inverted_index_info` copies `commit_tso`, the row-binlog mark and `compaction_level`; `SnapshotManager::_rename_rowset_id` and `CloudSnapshotMgr::_create_rowset_meta` copy `commit_tso` (clone, restore, storage migration). Covered by `IndexBuilderTest.ReplacementRowsetKeepsPublishTimeMetadata`, `SnapshotManagerTest.TestConvertRowsetIdsNormal` and `CloudSnapshotMgrTest.TestConvertRowsets`. Audit of the other metadata-copy shapes: - `LinkedSchemaChange` builds its output meta from a `RowsetWriterContext` without `commit_tso`, but it is unreachable for the only tables that carry `__DORIS_COMMIT_TSO_COL__`: the FE adds that column exclusively through `CreateTableInfo.addRowBinlogHiddenColumns` (row-format binlog tables), and `SchemaChangeHandler` rejects every non-light schema change on such tables ("table with binlog<Row> only support light schema change" for add / drop / modify column and the generic operator check), so no schema-change job ever converts their rowsets. - Ordered compaction's `do_compact_ordered_rowsets` already sets `commit_tso_range(_input_rowsets)` on the linked output. - The cloud snapshot path also does not copy `is_row_binlog`; that predates this PR and does not feed the hidden-column value (readers take `read_row_binlog` from the reader context, not from the rowset meta), so it is left out of this change. ########## regression-test/suites/inverted_index_p0/test_build_index_read_time_hidden_column.groovy: ########## @@ -0,0 +1,126 @@ +// 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_build_index_read_time_hidden_column", "nonConcurrent") { + // BUILD INDEX rebuilds the indexes of existing rowsets through IndexBuilder in local mode only. + if (isCloudMode()) { + return + } + + def timeout = 60000 + def delta_time = 1000 + def alter_res = "null" + def useTime = 0 + + def wait_for_latest_op_on_table_finish = { table_name, OpTimeout -> + for (int t = delta_time; t <= OpTimeout; t += delta_time) { + alter_res = sql """SHOW ALTER TABLE COLUMN WHERE TableName = "${table_name}" ORDER BY CreateTime DESC LIMIT 1;""" + alter_res = alter_res.toString() + if (alter_res.contains("FINISHED")) { + sleep(3000) // wait change table state to normal + logger.info(table_name + " latest alter job finished, detail: " + alter_res) + break + } + useTime = t + sleep(delta_time) + } + assertTrue(useTime <= OpTimeout, "wait_for_latest_op_on_table_finish timeout") + } + + def wait_for_build_index_on_partition_finish = { table_name, OpTimeout -> + for (int t = delta_time; t <= OpTimeout; t += delta_time) { + alter_res = sql """SHOW BUILD INDEX WHERE TableName = "${table_name}";""" + def expected_finished_num = alter_res.size() + def finished_num = 0 + for (int i = 0; i < expected_finished_num; i++) { + logger.info(table_name + " build index job state: " + alter_res[i][7] + i) + if (alter_res[i][7] == "FINISHED") { + ++finished_num + } + } + if (finished_num == expected_finished_num) { Review Comment: Addressed in 464ccfb9ad0. - Waits: the CREATE INDEX wait requires a job to exist and be `FINISHED` (`CANCELLED` and timeout fail). The BUILD INDEX wait is keyed to the job ids that `BUILD INDEX` adds and requires every new job to reach `FINISHED`; `SHOW BUILD INDEX WHERE TableName = ...` also lists the finished job of a dropped table with the same name, so a rerun could otherwise pass on the previous run's job. - Index use: the two selective queries read their query profile and require `RowsInvertedIndexFiltered` to be present and equal 4 for `= 3` and 3 for `in (2, 6)`; an index of placeholder zeros filters all five rows. This needed `inverted_index_skip_threshold = 0`: on a five-row segment the BKD hit estimate exceeds the default 50 % and the index was bypassed (`InvertedIndexDowngradeCount: 1`, `RowsInvertedIndexFiltered: 0`), which is exactly the case the old waits could not distinguish. `= 0` keeps a result-only check because the materialized zone map `[2, 6]` prunes the segment before the index is consulted (`RowsStatsFiltered: 5`). Ran the suite on a local cluster: it failed on the profile oracle until the threshold was pinned, then passed; the `.out` is unchanged. -- 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]
