github-actions[bot] commented on code in PR #66858: URL: https://github.com/apache/doris/pull/66858#discussion_r4002943567
########## be/src/core/column/column_variant.h: ########## @@ -1,688 +0,0 @@ -// 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. -// This file is copied from -// https://github.com/ClickHouse/ClickHouse/blob/master/src/Columns/ColumnVariant.h -// and modified by Doris - -#pragma once -#include <butil/compiler_specific.h> -#include <glog/logging.h> -#include <rapidjson/document.h> -#include <rapidjson/stringbuffer.h> -#include <sys/types.h> - -#include <algorithm> -#include <memory> -#include <ostream> -#include <string> -#include <string_view> -#include <unordered_set> -#include <utility> -#include <vector> - -#include "common/status.h" -#include "core/column/column.h" -#include "core/column/column_map.h" -#include "core/column/column_nullable.h" -#include "core/column/column_vector.h" -#include "core/column/subcolumn_tree.h" -#include "core/cow.h" -#include "core/data_type/data_type.h" -#include "core/data_type/data_type_array.h" -#include "core/data_type/data_type_jsonb.h" -#include "core/data_type/data_type_map.h" -#include "core/data_type/data_type_nullable.h" -#include "core/data_type/data_type_variant.h" -#include "core/data_type_serde/data_type_serde.h" -#include "core/field.h" -#include "core/string_ref.h" -#include "core/types.h" -#include "storage/tablet/tablet_schema.h" -#include "util/json/path_in_data.h" - -class SipHash; - -namespace doris { -class Arena; -} // namespace doris - -namespace doris { - -#ifdef NDEBUG -#define ENABLE_CHECK_CONSISTENCY (void)/* Nothing */ -#else -#define ENABLE_CHECK_CONSISTENCY(this) (this)->check_consistency() -#endif - -/** A column that represents object with dynamic set of subcolumns. - * Subcolumns are identified by paths in document and are stored in - * a trie-like structure. ColumnVariant is not suitable for writing into tables - * and it should be converted to Tuple with fixed set of subcolumns before that. - */ -class ColumnVariant final : public COWHelper<IColumn, ColumnVariant> { Review Comment: [P2] Update the Variant benchmark before deleting V1. The supported `./build.sh --benchmark` path still compiles `benchmark_main.cpp`, which includes `benchmark_variant_segment.hpp`; that header still includes this deleted file, constructs/checks `ColumnVariant`, and calls the removed `TabletColumn::{set_,}variant_is_v2` accessors. Consequently the benchmark target fails at preprocessing/compilation even though the default BE target no longer sees these references. Please migrate or remove the V1 benchmark cases in the same change. ########## fe/fe-core/src/main/java/org/apache/doris/nereids/util/TypeCoercionUtils.java: ########## @@ -1243,7 +1243,7 @@ public static Optional<DataType> findWiderTypeForTwo(DataType left, DataType rig } else if (right instanceof NullType) { return Optional.of(left); } else if (left instanceof VariantType && right instanceof VariantType) { - return findCommonVariantType((VariantType) left, (VariantType) right); + return findCommonVariantType(); Review Comment: [P2] Do not let incomplete Variant equality bypass the neutral type. This new branch is reached only after `left.equals(right)`, but Nereids `VariantType.equals` ignores the sparse-statistics limit, sparse shard count, doc shard count, and nested-group flag even though `conversion()` preserves them into catalog metadata. For example, UNIONing columns with sparse shard counts 1 and 2 returns the first arm's type; CTAS then persists that choice, and reversing the arms creates a different schema instead of using `VariantType.INSTANCE`. Please compare every persisted Variant property (and align `hashCode`) or otherwise ensure all genuinely different Variant types take this neutral path, with reversed-order CTAS coverage. ########## be/src/storage/segment/variant/variant_column_writer_impl.cpp: ########## @@ -1797,59 +431,29 @@ VariantColumnWriterImpl::~VariantColumnWriterImpl() = default; Status VariantColumnWriterImpl::init() { DORIS_CHECK(!_initialized); _initialized = true; - if (can_use_nested_group_streaming_compaction(_opts, *_tablet_column)) { - return _initialize_v1_writer(); - } - return Status::OK(); -} - -Status VariantColumnWriterImpl::_initialize_v1_writer() { - DORIS_CHECK(_initialized); - DORIS_CHECK(_input_format == VariantWriterInputFormat::UNSET); - auto writer = std::make_unique<VariantV1ColumnWriter>(_opts, _tablet_column); - RETURN_IF_ERROR(writer->init()); - _v1_writer = std::move(writer); - _input_format = VariantWriterInputFormat::V1; - return Status::OK(); + return _ensure_writer(); } -Status VariantColumnWriterImpl::_ensure_writer(const VariantColumnData& column) { +Status VariantColumnWriterImpl::_ensure_writer() { DORIS_CHECK(_initialized); - VariantWriterInputFormat input_format; - RETURN_IF_ERROR(variant_writer_helpers::classify_variant_writer_input( - column, _input_format, "Variant writer", &input_format)); - if (_input_format != VariantWriterInputFormat::UNSET) { + if (_v2_writer) { return Status::OK(); } - - if (input_format == VariantWriterInputFormat::V1) { - RETURN_IF_ERROR(_initialize_v1_writer()); - } else { - auto writer = std::make_unique<VariantV2ColumnWriter>(_opts, _tablet_column); - RETURN_IF_ERROR(writer->init()); - _v2_writer = std::move(writer); - } - _input_format = input_format; + auto writer = std::make_unique<VariantV2ColumnWriter>(_opts, _tablet_column); Review Comment: [P2] Preserve the still-supported flatten-nested layout before making V2 unconditional. FE still accepts and persists `deprecated_variant_enable_flatten_nested=true`, and active suites create, load, compact, and schema-change those tables. This new eager route always initializes `VariantV2ColumnWriter`, whose layout validator rejects that flag, so segment flush and every rewrite fail before append. Existing flattened segments are also stranded: the V1 nested-array reconstruction is deleted while the V2 assembler rejects their nested paths. Please retain compatible read/write handling or provide a metadata/data migration and reject the property at FE, with an immutable flattened-V1 query plus rewrite test. -- 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]
