github-actions[bot] commented on code in PR #66204: URL: https://github.com/apache/doris/pull/66204#discussion_r3746318327
########## be/src/storage/segment/variant/v2/variant_shredder.cpp: ########## @@ -0,0 +1,703 @@ +// 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/segment/variant/v2/variant_shredder.h" + +#include <algorithm> +#include <limits> +#include <numeric> +#include <optional> +#include <unordered_map> +#include <utility> + +#include "common/exception.h" +#include "core/assert_cast.h" +#include "core/column/column_map.h" +#include "core/column/column_string.h" +#include "core/column/column_variant.h" +#include "core/column/column_vector.h" +#include "core/data_type/data_type_factory.hpp" +#include "exec/common/hash_table/phmap_fwd_decl.h" +#include "exec/common/variant_util.h" +#include "exprs/function/parse/variant_jsonb_parse.h" +#include "storage/tablet/tablet_schema.h" +#include "util/jsonb_writer.h" + +namespace doris::segment_v2 { +namespace { + +PathInData normalize_doc_publication_path(const PathInData& path) { + if (path.empty()) { + return path; + } + for (const PathInData::Part& part : path.get_parts()) { + if (part.is_nested || part.anonymous_array_level != 0) { + return path; + } + } + return PathInData(path.get_path(), path.get_is_typed()); +} + +size_t path_allocated_bytes(const PathInData& path) { + return path.get_path().capacity() + path.get_parts().capacity() * sizeof(PathInData::Part); +} + +} // namespace + +struct VariantShredder::Impl { + enum class State : uint8_t { COLLECTING, FINISHED, FAILED }; + + using PathIndex = uint32_t; + using ParentFieldKey = uint64_t; + using ChildPathCache = doris::flat_hash_map<ParentFieldKey, PathIndex>; + + // Metadata bytes belong to the input ReadView. This cache never escapes one append call, so + // it can borrow the dictionary and retain only parent+field transitions observed in that + // batch. Canonical paths themselves remain owned by PathState across appends. + struct MetadataPathCache { + explicit MetadataPathCache(VariantMetadataRef metadata_) : metadata(metadata_) {} + + VariantMetadataRef metadata; + ChildPathCache child_paths; + }; + + // Keep all state for one canonical dotted path together. This replaces three parallel + // containers (path plan, builders, and last-row markers), so a path has one index and one + // lifetime throughout shredding. + struct PathState { + explicit PathState(const PathInData& path_) : path(path_) {} + + PathInData path; + std::optional<VariantPathBuilder> builder; + size_t last_row_marker = 0; + }; + + struct SparsePlan { + VariantPathBuilder* builder = nullptr; + uint32_t bucket = 0; + const std::string* path = nullptr; + bool track_statistics = false; + }; + + struct DocPlan { + VariantPathBuilder* builder = nullptr; + uint32_t bucket = 0; + const std::string* path = nullptr; + size_t candidate_index = 0; + }; + + explicit Impl(VariantShredderOptions options_) : options(std::move(options_)) { + paths.emplace_back(PathInData()); + if (options.physical_layout == VariantShredderPhysicalLayout::ORDINARY && + options.sparse_bucket_count == 0) { + failure = Status::InvalidArgument( + "Variant shredder sparse bucket count must be positive"); + state = State::FAILED; + } else if (options.physical_layout == VariantShredderPhysicalLayout::DOC && + options.doc_bucket_count == 0) { + failure = Status::InvalidArgument("Variant shredder doc bucket count must be positive"); + state = State::FAILED; + } else if (options.tablet_schema != nullptr && options.parent_column_unique_id < 0) { + failure = Status::InvalidArgument( + "Variant shredder tablet schema requires a parent column unique id"); + state = State::FAILED; + } + } + + Status require_collecting() const { + if (state == State::FAILED) { + return failure; + } + if (state == State::FINISHED) { + return Status::InvalidArgument("Variant shredder is already finished"); + } + return Status::OK(); + } + + Status fail(Status status) { + if (state != State::FAILED) { + failure = std::move(status); + state = State::FAILED; + } + return failure; + } + + VariantPathBuilder* get_or_create_builder(PathIndex path_index) { + PathState& path_state = paths[path_index]; + if (!path_state.builder.has_value()) { + path_state.builder.emplace(path_state.path, rows); + } + return &*path_state.builder; + } + + Status validate_doc_path(PathIndex path_index) const { + if (options.physical_layout != VariantShredderPhysicalLayout::DOC) { + return Status::OK(); + } + const auto& parts = paths[path_index].path.get_parts(); + if (parts.empty()) { + return Status::Corruption("Variant doc path must not be empty"); + } + return Status::OK(); + } + + Status append_leaf(VariantRef value, PathIndex path_index, size_t row) { + PathState& path_state = paths[path_index]; + const size_t row_marker = row + 1; + if (path_state.last_row_marker == row_marker) { + if (!options.check_duplicate_json_path) { + return Status::InvalidArgument("may contains duplicated entry : {}", + path_state.path.get_path()); + } + return Status::OK(); + } + path_state.last_row_marker = row_marker; + if (value.is_null()) { + return Status::OK(); + } + return get_or_create_builder(path_index)->append(value, row); + } + + // Pack two uint32 values into one key without allocating a pair object for every transition. + static ParentFieldKey parent_field_key(PathIndex parent, uint32_t field) { + return (static_cast<uint64_t>(parent) << 32) | field; + } + + PathIndex resolve_child_path(MetadataPathCache& metadata_cache, PathIndex parent, + uint32_t field) { + const ParentFieldKey cache_key = parent_field_key(parent, field); + if (const auto found = metadata_cache.child_paths.find(cache_key); + found != metadata_cache.child_paths.end()) { + return found->second; + } + + PathInDataBuilder builder; + builder.append(paths[parent].path.get_parts(), false) + .append(metadata_cache.metadata.key_at(field).to_string_view(), false); + PathInData child = builder.build(); + // V2 object traversal keeps arrays as leaves. Canonicalizing into the dotted on-disk + // namespace also makes {"a.b": 1} and {"a": {"b": 1}} share one path. + child = PathInData(child.get_path()); + + PathIndex child_index = 0; + if (const auto found = path_indices.find(child); found != path_indices.end()) { + child_index = found->second; + } else { + if (paths.size() > std::numeric_limits<PathIndex>::max()) { + throw Exception(ErrorCode::INVALID_ARGUMENT, + "Variant path count exceeds uint32 limit"); + } + child_index = static_cast<PathIndex>(paths.size()); + path_indices.emplace(child, child_index); + paths.emplace_back(child); + } + metadata_cache.child_paths.emplace(cache_key, child_index); + return child_index; + } + + Status visit(VariantRef value, MetadataPathCache& metadata_cache, PathIndex path_index, + size_t row) { + if (value.is_null()) { + return options.check_duplicate_json_path ? append_leaf(value, path_index, row) + : Status::OK(); + } + if (value.basic_type() != VariantBasicType::OBJECT) { + return append_leaf(value, path_index, row); + } + const uint32_t children = value.num_elements(); + for (uint32_t index = 0; index < children; ++index) { + uint32_t field = 0; + VariantRef child = value.object_value_at(index, &field); Review Comment: [P1] Preserve source order before collapsing dotted paths This loop consumes the encoded object in metadata field-id order, but `resolve_child_path()` later canonicalizes both literal `"a.b"` and nested `"a":{"b":...}` to `a.b`; with duplicate checking enabled, `append_leaf()` keeps whichever one this sorted traversal visits first. Thus `{"a.b":1,"a":{"b":2}}` persists `2` in V2 while V1's input-first rule persists `1`, and reversing key order still selects the nested form. The new writer test and `duplicate_json_path.groovy` explicitly encode different V1/V2 results even though the suite says first value wins. Please resolve these collisions while encounter order is available (or preserve that order through encoding), and assert the same result for both representations and both key orders. -- 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]
