github-actions[bot] commented on code in PR #63508: URL: https://github.com/apache/doris/pull/63508#discussion_r3338313008
########## be/test/storage/tablet/tablet_schema_cache_test.cpp: ########## @@ -0,0 +1,115 @@ +// 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/tablet/tablet_schema_cache.h" + +#include <gtest/gtest.h> + +#include <memory> +#include <string> + +#include "storage/tablet/tablet_schema.h" +#include "storage/tablet_info.h" + +namespace doris { + +static TabletColumn create_test_column(int32_t unique_id, const std::string& name, + bool is_nullable = false) { + TabletColumn column; + column.set_unique_id(unique_id); + column.set_name(name); + column.set_type(FieldType::OLAP_FIELD_TYPE_INT); + column.set_is_key(true); + column.set_is_nullable(is_nullable); + column.set_length(4); + column.set_index_length(4); + return column; +} + +static void set_schema_param(OlapTableSchemaParam* param, int64_t table_id = 20, int64_t db_id = 10, + int64_t version = 3) { Review Comment: This helper writes `param->_db_id`, `_table_id`, and `_version`, but those members are private in `OlapTableSchemaParam`. The new unit test will not compile. Please build the parameter through `POlapTableSchemaParam`/`TOlapTableSchemaParam` and `OlapTableSchemaParam::init(...)`, or add an appropriate test-only builder that does not access private fields directly. ########## be/src/load/delta_writer/delta_writer_v2.cpp: ########## @@ -214,28 +215,39 @@ Status DeltaWriterV2::cancel_with_status(const Status& st) { Status DeltaWriterV2::_build_current_tablet_schema(int64_t index_id, const OlapTableSchemaParam* table_schema_param, const TabletSchema& ori_tablet_schema) { - _tablet_schema->copy_from(ori_tablet_schema); // find the right index id - int i = 0; - auto indexes = table_schema_param->indexes(); - for (; i < indexes.size(); i++) { - if (indexes[i]->index_id == index_id) { + const OlapTableIndexSchema* index_schema = nullptr; + for (const auto* schema : table_schema_param->indexes()) { + if (schema->index_id == index_id) { + index_schema = schema; break; } } + DORIS_CHECK(index_schema != nullptr); Review Comment: This turns a missing `index_schema` into a process-level check failure. The adjacent `BaseRowsetBuilder::_build_current_tablet_schema()` path still treats `index_schema == nullptr` as valid and falls back to `copy_from(ori_tablet_schema)`, and the old DeltaWriterV2 code also allowed an empty `indexes()` list by just using the original tablet schema. If a load request reaches this path without a matching index schema, BE now crashes instead of building the rowset with the original schema. Please mirror the rowset-builder fallback and include `nullptr` in the cache key rather than asserting here. -- 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]
