github-actions[bot] commented on code in PR #66321:
URL: https://github.com/apache/doris/pull/66321#discussion_r3689368895
##########
be/test/core/data_type_serde/data_type_variant_v2_serde_output_test.cpp:
##########
@@ -379,4 +425,30 @@ TEST(DataTypeVariantV2SerdeOutputTest,
ConstNullableAndOuterMasksPreserveBoundar
EXPECT_TRUE(invalid_dates->is_typed());
}
+TEST(DataTypeVariantV2SerdeOutputTest,
BinaryStructPreservesEncodedAndTypedBytesAndOuterNulls) {
+ DataTypeVariantV2SerDe serde;
+ auto documents = encoded_json({R"({"a":[1,null,"x"]})",
R"({"hidden":true})", "null"});
+ NullMap mask {0, 1, 0};
+ expect_binary_variant_bytes(serde, *documents, *documents, &mask);
+
+ auto typed = typed_strings(
+ {std::string_view("plain"), std::nullopt,
std::string_view(R"({"text":"value"})")});
+ ColumnPtr encoded = encoded_copy(*typed);
+ expect_binary_variant_bytes(serde, *typed, assert_cast<const
ColumnVariantV2&>(*encoded));
+ EXPECT_TRUE(typed->is_typed());
+}
+
+TEST(DataTypeVariantV2SerdeOutputTest,
BinaryStructLeavesPrimitiveCompatibilityToConsumer) {
+ DataTypeVariantV2SerDe serde;
+ auto time = ColumnTimeV2::create();
+ time->insert_value(1500000.0);
+ const std::array<uint8_t, 1> not_null {0};
+ auto typed_time = ColumnVariantV2::create_typed(nullable(std::move(time),
not_null),
Review Comment:
[P1] Avoid an unsupported typed TIMEV2 test input
`ColumnVariantV2::create_typed` immediately checks
`is_supported_variant_typed_identity`, whose whitelist excludes TYPE_TIMEV2;
`ColumnVariantV2Test` already `EXPECT_DEATH`s this exact construction. This
ordinary test therefore aborts here before reaching `encoded_copy` or Arrow
serialization. Construct an encoded TIME_NTZ_MICROS value with
`VariantBatchBuilder::Row::add_time_ntz_micros` (or use a supported typed
identity) so the test actually exercises the intended transport boundary.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/Array.java:
##########
@@ -69,9 +70,11 @@ public void checkLegalityBeforeTypeCoercion() {
if (children.isEmpty()) {
return;
}
- DataType firstChildType = getArgument(0).getDataType();
- if (firstChildType.isJsonType() || firstChildType.isVariantType()) {
- throw new AnalysisException("array does not support jsonb/variant
type");
+ for (Expression child : children) {
Review Comment:
[P2] Use the legality-check argument accessors
The required expressions `AGENTS.md` says every
`checkLegalityBeforeTypeCoercion` override must read arguments through
`getArguments()` or `getArgument()`, which unwrap `Variable` nodes. This loop
still reads `children` directly, and the same violation remains in `CreateMap`,
`CreateNamedStruct`, and `CreateStruct`; please update all four touched checks
consistently.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonVariantWriteAnalyzer.java:
##########
@@ -0,0 +1,109 @@
+// 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.datasource.paimon;
+
+import org.apache.doris.catalog.Column;
+import org.apache.doris.catalog.Type;
+import org.apache.doris.nereids.exceptions.AnalysisException;
+import org.apache.doris.nereids.trees.expressions.NamedExpression;
+import org.apache.doris.nereids.types.ArrayType;
+import org.apache.doris.nereids.types.DataType;
+import org.apache.doris.nereids.types.MapType;
+import org.apache.doris.nereids.types.StructField;
+import org.apache.doris.nereids.types.StructType;
+import org.apache.doris.nereids.types.VariantType;
+
+import java.util.List;
+import java.util.Map;
+
+/** Analysis checks for the V2-only Paimon Variant write protocol. */
+public final class PaimonVariantWriteAnalyzer {
+ private PaimonVariantWriteAnalyzer() {
+ }
+
+ /**
+ * Rejects a disabled V2 protocol and legacy Variant inputs before sink
coercion.
+ */
+ public static void validate(
+ PaimonWriteTarget writeTarget,
+ List<Column> writeColumns,
+ Map<String, NamedExpression> columnToOutput,
+ boolean enableVariantV2) throws AnalysisException {
+ boolean targetContainsVariant =
writeTarget.getColumnTypes().values().stream()
+ .map(DataType::fromCatalogType)
+ .anyMatch(VariantType::containsVariant);
+ if (!targetContainsVariant) {
+ return;
+ }
+ if (!enableVariantV2) {
+ throw new AnalysisException(
+ "Paimon VARIANT write only supports Variant V2; "
+ + "set enable_variant_v2=true");
+ }
+
+ for (Column column : writeColumns) {
+ NamedExpression output = columnToOutput.get(column.getName());
+ Type targetCatalogType =
writeTarget.getColumnTypes().get(column.getName());
+ if (output == null || targetCatalogType == null) {
+ continue;
+ }
+ rejectLegacyVariant(
+ output.getDataType(),
+ DataType.fromCatalogType(targetCatalogType),
+ column.getName());
+ }
+ }
+
+ private static void rejectLegacyVariant(
+ DataType sourceType, DataType targetType, String path) throws
AnalysisException {
+ if (targetType instanceof VariantType) {
Review Comment:
[P1] Validate every source before Variant V2 sink coercion
This branch returns for every non-Variant source, but FE admits more casts
than the V2 kernel can execute. `ARRAY<VARIANT V1>` fails on its legacy leaf;
MAP, STRUCT, TIMEV2, and DECIMAL256 also pass `CheckCast`, yet
`execute_to_variant` has no supported path for them, so these writes fail only
in the BE. Please recursively reject legacy leaves across shape changes and
either implement or analysis-reject every source shape and type FE admits, with
regressions.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/insert/InsertUtils.java:
##########
@@ -482,6 +483,17 @@ private static Plan normalizePlanWithoutLock(LogicalPlan
plan, TableIf table,
return plan.withChildren(new
LogicalInlineTable(optimizedRowConstructors.build()));
}
+ private static DataType targetTypeForInlineValue(Column column) {
+ DataType targetType = DataType.fromCatalogType(column.getType());
+ if (VariantType.containsVariant(targetType)) {
Review Comment:
[P1] Preserve per-row types before Variant sink coercion
Returning null here defers the target cast past
InlineTableToUnionOrOneRowRelation. For a Variant column, `VALUES (1), ('x')`
is first unified as STRING, so BindSink later converts both rows to Variant
strings and silently changes `1` from an integer Variant to `"1"`. Please keep
per-row Variant target steering (including nested Variant targets) before the
inline-table common-type pass, or make that pass use the sink target, and add a
heterogeneous multi-row 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]
