github-actions[bot] commented on code in PR #66204:
URL: https://github.com/apache/doris/pull/66204#discussion_r3701079357
##########
fe/fe-type/src/main/java/org/apache/doris/catalog/VariantType.java:
##########
@@ -226,15 +205,11 @@ public ArrayList<VariantField> getPredefinedFields() {
@Override
public void toThrift(TTypeDesc container) {
super.toThrift(container);
+ TScalarType scalarType =
container.getTypes().get(container.getTypes().size() - 1).scalar_type;
// set the count
- container.getTypes().get(container.getTypes().size() - 1)
-
.scalar_type.setVariantMaxSubcolumnsCount(variantMaxSubcolumnsCount);
- container.getTypes().get(container.getTypes().size() - 1)
- .scalar_type.setVariantEnableDocMode(enableVariantDocMode);
- if (computeV2) {
- container.getTypes().get(container.getTypes().size() - 1)
- .scalar_type.setVariantIsV2(true);
- }
+ scalarType.setVariantMaxSubcolumnsCount(variantMaxSubcolumnsCount);
+ scalarType.setVariantEnableDocMode(enableVariantDocMode);
+ scalarType.setVariantIsV2(Config.enable_variant_v2);
Review Comment:
[P1] Freeze the V2 mode before plan serialization
This reads a runtime-mutable global each time a type is serialized, so one
load plan can contain both physical Variant ABIs.
`fileScanNode.finalizeForNereids()` serializes destination expressions into
`TFileScanRangeParams` through `ExprToThriftVisitor`, while
`NereidsStreamLoadPlanner.plan()` serializes the descriptor table only
afterward. If the flag flips between those calls, the injected
`TryParseToVariant` expression can return `ColumnVariantV2` while the
destination slot creates legacy `ColumnVariant` (or vice versa).
`FileScanner::_convert_to_output_block()` then inserts the expression result
into that destination, and both column implementations exact-cast the source
class, so the load fails. The same race can let `COUNT(DISTINCT variant)` pass
legality under V2 and later reach BE with a V1 argument, which
`aggregate_function_uniq` rejects. Please snapshot the mode once in the
statement/load plan and carry it through all function and descriptor types, or
make the flag non-
mutable until that fence exists; add a barrier test that flips it between
expression and descriptor serialization.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/util/TypeCoercionUtils.java:
##########
@@ -1195,10 +1195,7 @@ private static Optional<DataType>
findWiderComplexTypeForTwo(
}
private static Optional<DataType> findCommonVariantType(VariantType left,
VariantType right) {
- if (!left.isExecutionCompatibleWith(right)) {
- return Optional.empty();
- }
- return Optional.of(left.isComputeV2() ?
VariantType.COMPUTE_V2_INSTANCE : left);
+ return left.equals(right) ? Optional.of(left) : Optional.empty();
Review Comment:
[P1] Preserve the V2 common type across storage properties
When `enable_variant_v2=true`, Variant slots from both tables serialize to
the same `ColumnVariantV2` execution ABI, even if their table schemas use
different max-subcolumn, predefined-path, or doc properties. Requiring
`left.equals(right)` here nevertheless rejects `UNION ALL`, CASE/COALESCE, and
nested common-type derivation across those tables. The removed implementation
handled this by returning a neutral compute-V2 type for two V2 operands; the
new test only codifies rejection without enabling the global mode, and existing
V2 UNION tests use identical singleton types. Please derive this from the
statement-frozen execution mode: retain property equality for V1, but return an
appropriate neutral V2 common type for two V2 operands, with cross-schema UNION
and CASE coverage.
##########
fe/fe-common/src/main/java/org/apache/doris/common/Config.java:
##########
@@ -3663,6 +3663,10 @@ public static int metaServiceRpcRetryTimes() {
"Whether to allow the use of inverted index v1 for variant."})
public static boolean enable_inverted_index_v1_for_variant = false;
+ @ConfField(mutable = true, description = {
+ "Whether to enable ColumnVariantV2 for Variant execution and
storage."})
+ public static boolean enable_variant_v2 = false;
Review Comment:
[P1] Re-key or invalidate SQL cache on V2 mode changes
This replaces a session variable marked `affectQueryResultInPlan=true` with
a runtime-mutable global, but the SQL-cache key still hashes only those session
variables and the config callback does not invalidate the cache. For example,
with V2 enabled a cacheable `COUNT(DISTINCT variant_col)` can produce a result;
after `ADMIN SET ... enable_variant_v2=false`, the same SQL can hit
`LogicalSqlCache` before `Count.checkLegacyVariantArgument()` and return that
result even though a fresh plan must reject legacy Variant. `variant_type` also
has different V1/V2 results in this patch. Please include the snapshotted mode
in SQL-cache discrimination or invalidate affected caches whenever the global
changes, and add a toggle-between-identical-queries test.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/types/VariantType.java:
##########
@@ -139,13 +116,11 @@ public VariantType(List<VariantField> fields, int
variantMaxSubcolumnsCount,
this.variantDocMaterializationMinRows =
variantDocMaterializationMinRows;
this.variantDocShardCount = variantDocShardCount;
this.enableNestedGroup = enableNestedGroup;
- this.computeV2 = computeV2;
}
@Override
public boolean isInjectiveCastTo(DataType target) {
- return target instanceof VariantType
- && isExecutionCompatibleWith((VariantType) target);
+ return target instanceof VariantType;
Review Comment:
[P1] Keep property-changing casts illegal in V1 mode
With the global flag left at its default `false`, this now declares every
Variant-to-Variant cast injective, and the removed special case in `CheckCast`
lets the generic Variant branch accept it. A cast from
`VARIANT<...max_subcolumns=100>` to the same type with `200` therefore passes
FE, but both descriptors are legacy `DataTypeVariant`; because their properties
differ, BE does not use the identity wrapper and
`create_cast_to_variant_wrapper` explicitly rejects a Variant source. The
changed unit test currently codifies this FE-only acceptance. Please base
castability/injectivity on a statement-frozen mode: require equal properties
for V1, and only treat differing properties as compatible when both sides are
definitively V2. Add an end-to-end negative V1 case and positive V2 case.
##########
be/src/storage/segment/variant/variant_writer_helpers.cpp:
##########
@@ -0,0 +1,339 @@
+// 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/variant_writer_helpers.h"
+
+#include <algorithm>
+#include <memory>
+#include <span>
+#include <string>
+#include <string_view>
+#include <utility>
+
+#include "common/cast_set.h"
+#include "common/config.h"
+#include "common/exception.h"
+#include "core/column/column_variant.h"
+#include "core/column/variant_v2/column_variant_v2.h"
+#include "core/data_type/data_type_factory.hpp"
+#include "exec/common/variant_util.h"
+#include "storage/index/indexed_column_writer.h"
+#include "storage/iterator/olap_data_convertor.h"
+#include "storage/rowset/rowset_writer_context.h"
+#include "storage/segment/encoding_info.h"
+#include "storage/segment/variant/v2/variant_shredder.h"
+#include "storage/types.h"
+
+namespace doris::segment_v2::variant_writer_helpers {
+
+Status make_variant_shredder_options(const TabletSchema& tablet_schema,
+ const TabletColumn& parent_column,
+ VariantShredderPhysicalLayout
physical_layout,
+ PathInData logical_root_path,
+ VariantShredderOptions* options) {
+ DORIS_CHECK(options != nullptr);
+ if (parent_column.variant_enable_nested_group()) {
Review Comment:
[P1] Keep unsupported table layouts off the V2 writer
`enable_variant_v2` marks every Variant sink type as V2, but this writer
rejects nested-group tables (and the following branch rejects deprecated
flatten-nested tables). For an ordinary INSERT or load, the block therefore
arrives as `ColumnVariantV2`, `_ensure_writer()` selects
`VariantV2ColumnWriter`, and `init()` fails here; only nested-group compaction
is pre-forced to V1. The new unit test even codifies the deprecated-layout
rejection without providing any FE routing guard or fallback, so enabling this
global flag breaks writes to existing tables with these persistent layouts.
Please either select V1 per target table until V2 supports the layout,
implement the missing V2 path, or reject enabling the global mode before such
tables can receive writes; add behavior-level INSERT/load coverage for both
layouts.
--
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]
