github-actions[bot] commented on code in PR #66007:
URL: https://github.com/apache/doris/pull/66007#discussion_r3666710923
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/util/TypeCoercionUtils.java:
##########
@@ -1162,7 +1166,11 @@ private static Optional<DataType>
findWiderComplexTypeForTwo(
leftFields.get(i).getDataType(),
rightFields.get(i).getDataType(),
overflowToDouble, stringIsHighPriority);
if (newDataType.isPresent()) {
-
newFields.add(leftFields.get(i).withDataType(newDataType.get()));
+ // The common layout must admit NULLs produced while
either child is cast to it.
Review Comment:
[P2] Apply the same nullability union to legacy CASE-WHEN
This correction is missing from both legacy reducers used when
`enable_new_type_coercion_behavior=false`. `findCommonComplexTypeForCaseWhen()`
keeps only the left field's nullability, so a required `struct(1)` arm followed
by a nullable struct arm is rejected while the reverse order succeeds.
`LogicalSetOperation.getAssignmentCompatibleTypeLegacy()` unions declared
nullability but still omits `Cast.castNullable`, so required
`DATETIMEV2(3)`/`DATETIMEV2(6)` struct children produce a required common child
and UNION/INTERSECT/EXCEPT insert a cast that the stricter `CheckCast` rejects.
Please mirror the full declared-plus-cast nullability calculation in both
legacy struct reducers and test them with the flag disabled.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/BindSink.java:
##########
@@ -730,6 +741,16 @@ private Plan
bindIcebergTableSink(MatchingContext<UnboundIcebergTableSink<Plan>>
IcebergExternalDatabase database = pair.first;
IcebergExternalTable table = pair.second;
LogicalPlan child = ((LogicalPlan) sink.child());
+ Optional<MvccSnapshot> targetSnapshot =
ctx.cascadesContext.getStatementContext()
Review Comment:
[P1] Fence rewrite tasks to the transaction's table generation
Every rewrite group gets a fresh `StatementContext` and independently
executes this latest-target lookup after `beginRewrite()` has already captured
the shared transaction table. A metadata-only spec/schema change between task
analyses can therefore make concurrent writers use different generations, while
`finishRewrite()` converts all commit data with only
`transaction.table().spec()/schema()/sortOrder()`. `validateFromSnapshot()`
does not catch metadata-only evolution that leaves the data snapshot unchanged,
and commit data carries no per-file spec identity to repair the mismatch.
Please pass one frozen table generation through transaction start, every
rewrite task/sink, and commit conversion, with a multi-group refresh-barrier
test.
##########
be/src/format_v2/table_reader.h:
##########
@@ -1290,7 +1308,8 @@ class TableReader {
mapping.file_column_name));
}
}
- RETURN_IF_ERROR(_align_column_nullability(column, mapping.table_type));
+ RETURN_IF_ERROR(
+ _align_column_nullability(column, mapping.table_type,
nullable_parent_null_map));
Review Comment:
[P1] Preserve complex-container null masks until validation
This validates only after complex rematerialization has consumed the masks
it needs. For `outer NULLABLE STRUCT<inner STRUCT<a INT NOT NULL> NOT NULL>`,
the required `inner` rematerializes without the ancestor mask and rejects valid
masked `a` placeholders; trivial struct recursion likewise does not forward the
mask. Conversely, ARRAY/MAP rematerializers unwrap their own nullable source
root and reattach it only for a nullable target, so an optional-file NULL
mapped to a required ARRAY/MAP becomes bare offsets/default payload instead of
an error (and the top-level complex path returns without another alignment).
Please carry both inherited and container-root masks through rematerialization,
validate before dropping them, and test masked required-STRUCT descendants plus
masked/unmasked required ARRAY/MAP roots.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/source/IcebergScanNode.java:
##########
@@ -697,6 +722,20 @@ public TableScan createTableScan() throws UserException {
return icebergTableScan;
}
+ private Table useFrozenEmptyTableGeneration(Table currentTable) {
+ Optional<MvccSnapshot> snapshot = getPinnedRelationSnapshot();
+ if (!isSystemTable &&
snapshot.filter(IcebergMvccSnapshot.class::isInstance).isPresent()) {
+ IcebergSnapshotCacheValue cacheValue =
+ ((IcebergMvccSnapshot)
snapshot.get()).getSnapshotCacheValue();
+ if (cacheValue.getSnapshot().getSnapshotId() < 0 &&
cacheValue.getIcebergTable().isPresent()) {
Review Comment:
[P1] Reuse the frozen table for non-empty snapshots too
The snapshot cache now retains the exact T0 `Table`, but this helper returns
it only for the `-1` empty sentinel. A normal relation bound to T0/S0 still
reloads T1 in `doInitialize()`, then runs `T1.newScan().useSnapshot(S0)` and
reads T1's current spec/format/properties. For example, removing the last
partition field in a metadata-only commit leaves S0 files on the old spec, but
T1 makes `isPartitionedTable` false, so split construction omits S0's spec
ID/partition JSON while the bound plan still treats that identity field as a
partition column. Expiration can also make T1 reject S0 outright. Please use
the retained generation for every snapshot-selectable normal scan and add a
non-empty T0-bind/T1-replacement test.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/glue/translator/PhysicalPlanTranslator.java:
##########
@@ -585,7 +585,9 @@ public PlanFragment
visitPhysicalIcebergTableSink(PhysicalIcebergTableSink<? ext
List<Expr> outputExprs = Lists.newArrayList();
icebergTableSink.getOutput().stream().map(Slot::getExprId)
.forEach(exprId ->
outputExprs.add(context.findSlotRef(exprId)));
- IcebergTableSink sink = new IcebergTableSink((IcebergExternalTable)
icebergTableSink.getTargetTable());
+ IcebergTableSink sink = new IcebergTableSink(
+ (IcebergExternalTable) icebergTableSink.getTargetTable(),
+ icebergTableSink.getTargetIcebergTable());
Review Comment:
[P1] Fence UPDATE/MERGE with the same target generation
This retained target is added only to the ordinary Iceberg table sink.
UPDATE/MERGE directly construct the parallel logical/physical merge sinks,
whose planner sink and `beginMerge()` still independently call
`getIcebergTable()`. If an ADD COLUMN refresh occurs after T0 outputs bind but
before sink finalization, BE receives T1 `schema_json` while the output
expressions still match T0, so the writer rejects the column count; spec-only
evolution can also split distribution, writer, and transaction generations.
Please capture one target before UPDATE/MERGE normalization and carry it
through the merge logical/physical sinks, distribution, `IcebergMergeSink`, and
`beginMerge()`, with a refresh-barrier test.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/source/PaimonScanNode.java:
##########
@@ -177,6 +180,11 @@ public PaimonScanNode(PlanNodeId id,
@Override
protected void doInitialize() throws UserException {
+ Optional<MvccSnapshot> relationSnapshot = getRelationSnapshot();
+ if (desc.getTable() instanceof PaimonExternalTable) {
+ // Rebuild the source before applying query options so both layers
use this relation's snapshot.
+ source = new PaimonSource(desc, relationSnapshot);
Review Comment:
[P1] Keep behavioral OPTIONS on this relation's source table
Rebuilding `source` here does not fence the `OPTIONS` path:
`getProcessedTable()` calls `source.getPaimonTable(scanParams)`, which bypasses
`originTable` and rebuilds `statementTable` from
`MvccUtil.getSnapshotFromContext(this)`. If a later relation of the same table
binds a historical snapshot, an earlier latest relation with a behavioral
option such as `scan.plan-sort-partition` applies that option to the historical
table and reads old data; reversing relation order changes the result. Please
resolve/apply behavioral options from this relation-local `originTable` and
cover both relation 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]
