github-actions[bot] commented on code in PR #66413: URL: https://github.com/apache/doris/pull/66413#discussion_r3762578502
########## fe/fe-core/src/main/java/org/apache/doris/nereids/types/ConnectorComputeVariantType.java: ########## @@ -0,0 +1,42 @@ +// 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. + +package org.apache.doris.nereids.types; + +import org.apache.doris.catalog.Type; + +/** Execution-only Variant marker retained while connector scan slots pass through Nereids. */ +public final class ConnectorComputeVariantType extends VariantType { Review Comment: [P1] Exempt this compute-only subtype from the legacy distinct guard This marker is always translated to a V2 catalog Variant, but inheriting `VariantType` also makes `Count.checkLegacyVariantArgument()` reject it whenever the default `Config.enable_variant_v2=false`; `MultiDistinctCount` repeats that check after rewriting `COUNT(DISTINCT)`. The external suites now intentionally run with that config off, yet the added Iceberg case expects `COUNT(DISTINCT v)` to succeed, so analysis fails before the V2 carrier or backend-version fence is reached. Please distinguish this execution-only subtype in the aggregate legality check while retaining the legacy guard for storage-configured Variant, and add a default-off FE analysis test. ########## fe/fe-connector/fe-connector-paimon/src/main/java/org/apache/doris/connector/paimon/PaimonScanPlanProvider.java: ########## @@ -1600,6 +1658,12 @@ private static boolean supportNativeReader(Optional<List<RawFile>> optRawFiles) return true; } + private static boolean supportNativeVariantReader(Optional<List<RawFile>> optRawFiles) { + return optRawFiles.isPresent() && !optRawFiles.get().isEmpty() Review Comment: [P1] Allow pre-Variant ORC files through schema evolution This all-Parquet gate is based on the current projected table type, so it also rejects an old ORC split that predates a newly added nullable Variant column. That file has no Variant carrier to decode: the native mapper can leave the field unmapped and synthesize `NULL`, while newer Parquet splits decode the actual Variant. Instead this returns false, the JNI arm cannot carry the current Variant schema, and planning throws, making a valid ORC-to-Parquet table unreadable after adding the column. Please make the rejection depend on the physical file mapping (allow ORC when the Variant field is absent, still reject it when present) and add that mixed-format evolution case. ########## be/src/format_v2/column_mapper.cpp: ########## @@ -2425,8 +2679,23 @@ Status TableColumnMapper::localize_filters(const std::vector<TableFilter>& table if (predicate_it == file_request->predicate_columns.end()) { continue; } - file_request->non_predicate_columns.push_back(std::move(*predicate_it)); + LocalColumnIndex demoted_projection = std::move(*predicate_it); file_request->predicate_columns.erase(predicate_it); + const auto output_it = std::ranges::find_if(file_request->non_predicate_columns, + [local_id](const LocalColumnIndex& projection) { + return projection.column_id() == local_id; + }); + if (output_it != file_request->non_predicate_columns.end()) { + // A rejected complex predicate still needs its filter-only subtree on Scanner's + // table-level path. Merge it with the deferred output before collapsing the two block + // positions, or branch-4.1 can silently drop the child used by the residual filter. + RETURN_IF_ERROR(merge_local_column_index(&demoted_projection, *output_it)); + file_request->non_predicate_columns.erase(output_it); + file_request->non_predicate_positions.erase(local_id); Review Comment: [P1] Keep the finalized file-block positions dense This demotion erases an already allocated deferred position without repacking later roots. If root A first gets predicate/output positions 0/1 and is demoted here, while an independent root B keeps 2/3, the finalized request retains positions 0, 2, and 3. `block_column_count()` still returns 4, so `TableReader::open_reader()` leaves layout entry 1 uninitialized and then dereferences its null `type` while constructing the file block. Please compact every finalized position map/projection after demotion (or otherwise preserve a populated dense slot) and add the combined demoted-root plus retained-independent-root open-reader regression. ########## be/src/core/column/variant_v2/column_variant_v2.cpp: ########## @@ -724,7 +1107,7 @@ void ColumnVariantV2::insert_many_defaults(size_t length) { return; } - if (_typed) { + if (_typed || _shredded) { Review Comment: [P1] Do not materialize projected Variants when appending defaults A leaf-only Parquet Variant projection is intentionally non-materializable, but this new `_shredded` arm sends it through `ensure_encoded()` before adding the default. Outer-join null extension can hit exactly that state: a matched row first inserts the direct scan's projected `v['n']` into an empty nullable output column (which adopts the shredded state), then a later unmatched row calls `ColumnNullable::insert_many_defaults()` and reaches this line, where `materialized_column()` throws. This is separate from exchange merging and is reachable with a direct Variant scan on the probe side of a RIGHT/FULL non-equi join. Please serialize/preserve the retained projection before appending the null payload instead of reconstructing omitted root fields, and add a matched-then-unmatched outer-join regression. -- 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]
