github-actions[bot] commented on code in PR #66620:
URL: https://github.com/apache/doris/pull/66620#discussion_r3763576072
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergMetadataOps.java:
##########
@@ -935,8 +965,14 @@ private void modifyTopLevelColumn(ExternalTable
dorisTable, ColumnPath columnPat
currentCol.type(), currentCol);
validateModifyColumnMetadata(column, resolvedPath.getFullPath(), true);
+ boolean variantModify = validateVariantTypeChange(
+ currentCol.type(), column.getType(),
resolvedPath.getFullPath());
+ validateVariantSchema(icebergTable, column.getType(),
columnPath.getFullPath(), false);
Review Comment:
[P2] Allow metadata-only Variant MODIFY on existing ORC tables
`variantModify` identifies a same-type change to an existing Iceberg
Variant, but this unconditional write-capability check still rejects the
operation whenever the table's current default is ORC. That state is legal and
already exercised by this PR's `variant_orc` fixture: Spark adds a Variant
column to a v3 ORC table, and Doris can read it. `ALTER TABLE variant_orc
MODIFY COLUMN v VARIANT COMMENT 'payload'` now fails here even though the
dedicated branch below only updates the doc/nullability/position through
`UpdateSchema` and never writes a data file. Please apply the Parquet gate only
when introducing/converting to Variant, and cover metadata-only MODIFY on the
existing ORC fixture.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/VariantWritePlanValidator.java:
##########
@@ -0,0 +1,273 @@
+// 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;
+
+import org.apache.doris.catalog.Column;
+import org.apache.doris.nereids.exceptions.AnalysisException;
+import org.apache.doris.nereids.trees.expressions.CTEId;
+import org.apache.doris.nereids.trees.expressions.Cast;
+import org.apache.doris.nereids.trees.expressions.ExprId;
+import org.apache.doris.nereids.trees.expressions.Expression;
+import org.apache.doris.nereids.trees.expressions.NamedExpression;
+import org.apache.doris.nereids.trees.expressions.Slot;
+import org.apache.doris.nereids.trees.expressions.SlotReference;
+import org.apache.doris.nereids.trees.plans.Plan;
+import org.apache.doris.nereids.trees.plans.algebra.SetOperation;
+import org.apache.doris.nereids.trees.plans.logical.LogicalCTEConsumer;
+import org.apache.doris.nereids.trees.plans.logical.LogicalCTEProducer;
+import org.apache.doris.nereids.trees.plans.logical.LogicalUnion;
+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.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.Set;
+
+/** Shared source-plan validation for external Variant sinks. */
+public final class VariantWritePlanValidator {
+ private VariantWritePlanValidator() {
+ }
+
+ /**
+ * Rejects an implicit Variant-to-non-Variant cast in the lineage of a
Variant target column.
+ *
+ * <p>Common-type analysis for UNION, IF and CASE runs before sink
binding. Without this
+ * check, an object or array Variant can become SQL NULL while being cast
to a scalar, and the
+ * sink will only see that scalar/NULL and encode it back as Variant.
Explicit casts remain an
+ * intentional user conversion and are not rejected.</p>
+ */
+ public static void validateNoLossyCoercion(
+ String sinkName, List<Column> targetColumns, Plan sourcePlan) {
+ if (targetColumns.size() != sourcePlan.getOutput().size()) {
+ throw new AnalysisException(
+ sinkName + " Variant write target and source columns are
not aligned");
+ }
+
+ List<Integer> variantOrdinals = new ArrayList<>();
+ for (int i = 0; i < targetColumns.size(); i++) {
+ DataType targetType =
DataType.fromCatalogType(targetColumns.get(i).getType());
+ if (VariantType.containsVariant(targetType)) {
+ variantOrdinals.add(i);
+ }
+ }
+ if (variantOrdinals.isEmpty()) {
+ return;
+ }
+
+ TraceContext traceContext = new TraceContext(sourcePlan);
+ for (int ordinal : variantOrdinals) {
+ Column targetColumn = targetColumns.get(ordinal);
+ traceOutputLineage(
+ sourcePlan,
+
Collections.singleton(sourcePlan.getOutput().get(ordinal).getExprId()),
+ sinkName,
+ targetColumn.getName(),
+ traceContext);
+ }
+ }
+
+ private static void traceOutputLineage(
+ Plan plan, Set<ExprId> requiredExprIds, String sinkName, String
targetColumn,
+ TraceContext context) {
+ if (requiredExprIds.isEmpty()) {
+ return;
+ }
+ if (plan instanceof SetOperation) {
Review Comment:
[P1] Trace recursive-union outputs before validating Variant writes
`LogicalRecursiveUnion` is not a `SetOperation`, so it falls through this
branch even though it also exposes fresh union output ExprIds and separate
child-output mappings. The generic walk cannot match the required fresh output
ID to either child's slots, and therefore never reaches a lossy cast in the
anchor. For example, a recursive CTE whose anchor returns `IF(TRUE,
parse_to_variant('{"kind":"recursive"}'), 1)` and whose recursive term simply
forwards that INT column passes analysis; inserting its output into an Iceberg
or Paimon Variant target then casts the already-lost scalar/NULL back to
Variant V2. Please trace `RecursiveCte` outputs by ordinal like regular set
operations and add this analyzed recursive-anchor case for both sinks.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/VariantWritePlanValidator.java:
##########
@@ -0,0 +1,273 @@
+// 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;
+
+import org.apache.doris.catalog.Column;
+import org.apache.doris.nereids.exceptions.AnalysisException;
+import org.apache.doris.nereids.trees.expressions.CTEId;
+import org.apache.doris.nereids.trees.expressions.Cast;
+import org.apache.doris.nereids.trees.expressions.ExprId;
+import org.apache.doris.nereids.trees.expressions.Expression;
+import org.apache.doris.nereids.trees.expressions.NamedExpression;
+import org.apache.doris.nereids.trees.expressions.Slot;
+import org.apache.doris.nereids.trees.expressions.SlotReference;
+import org.apache.doris.nereids.trees.plans.Plan;
+import org.apache.doris.nereids.trees.plans.algebra.SetOperation;
+import org.apache.doris.nereids.trees.plans.logical.LogicalCTEConsumer;
+import org.apache.doris.nereids.trees.plans.logical.LogicalCTEProducer;
+import org.apache.doris.nereids.trees.plans.logical.LogicalUnion;
+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.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.Set;
+
+/** Shared source-plan validation for external Variant sinks. */
+public final class VariantWritePlanValidator {
+ private VariantWritePlanValidator() {
+ }
+
+ /**
+ * Rejects an implicit Variant-to-non-Variant cast in the lineage of a
Variant target column.
+ *
+ * <p>Common-type analysis for UNION, IF and CASE runs before sink
binding. Without this
+ * check, an object or array Variant can become SQL NULL while being cast
to a scalar, and the
+ * sink will only see that scalar/NULL and encode it back as Variant.
Explicit casts remain an
+ * intentional user conversion and are not rejected.</p>
+ */
+ public static void validateNoLossyCoercion(
+ String sinkName, List<Column> targetColumns, Plan sourcePlan) {
+ if (targetColumns.size() != sourcePlan.getOutput().size()) {
+ throw new AnalysisException(
+ sinkName + " Variant write target and source columns are
not aligned");
+ }
+
+ List<Integer> variantOrdinals = new ArrayList<>();
+ for (int i = 0; i < targetColumns.size(); i++) {
+ DataType targetType =
DataType.fromCatalogType(targetColumns.get(i).getType());
+ if (VariantType.containsVariant(targetType)) {
+ variantOrdinals.add(i);
+ }
+ }
+ if (variantOrdinals.isEmpty()) {
+ return;
+ }
+
+ TraceContext traceContext = new TraceContext(sourcePlan);
+ for (int ordinal : variantOrdinals) {
+ Column targetColumn = targetColumns.get(ordinal);
+ traceOutputLineage(
+ sourcePlan,
+
Collections.singleton(sourcePlan.getOutput().get(ordinal).getExprId()),
+ sinkName,
+ targetColumn.getName(),
+ traceContext);
+ }
+ }
+
+ private static void traceOutputLineage(
+ Plan plan, Set<ExprId> requiredExprIds, String sinkName, String
targetColumn,
+ TraceContext context) {
+ if (requiredExprIds.isEmpty()) {
+ return;
+ }
+ if (plan instanceof SetOperation) {
+ traceSetOperation(
+ plan, (SetOperation) plan, requiredExprIds, sinkName,
targetColumn, context);
+ return;
+ }
+ if (plan instanceof LogicalCTEConsumer) {
+ traceCteConsumer(
+ (LogicalCTEConsumer) plan, requiredExprIds, sinkName,
targetColumn, context);
+ return;
+ }
+
+ Set<ExprId> unresolvedExprIds = new HashSet<>(requiredExprIds);
+ Set<ExprId> inputExprIds = new HashSet<>();
+ for (Expression expression : plan.getExpressions()) {
Review Comment:
[P1] Validate generator expressions behind fresh output slots
A bound `LogicalGenerate` appends fresh `generatorOutput` slots, but
`getExpressions()` returns the generator functions themselves. Because those
are not `NamedExpression`s, this loop skips them; the required generated-slot
ExprId then matches neither an expression nor a child output, so tracing stops.
For example, `LATERAL VIEW explode([IF(TRUE,
parse_to_variant('{"kind":"generate"}'), 1)]) ... AS e` hides the implicit
Variant-to-INT cast behind `e`; inserting `e` into an Iceberg or Paimon Variant
target passes this check and casts the already-lost scalar/NULL back to Variant
V2. Please map generator-output ordinals to their generator expressions,
validate them, and add the lateral-view case for both sinks.
--
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]
