github-actions[bot] commented on code in PR #66007:
URL: https://github.com/apache/doris/pull/66007#discussion_r3656315810
##########
be/src/format_v2/table_reader.h:
##########
@@ -1262,7 +1262,12 @@ class TableReader {
Block cast_block;
cast_block.insert({*column, input_type, column_name});
auto slot_ref = VSlotRef::create_shared(0, 0, -1, input_type,
column_name);
- auto cast_expr = Cast::create_shared(table_type);
+ // Preserve the source null map through conversion; the caller
validates and unwraps it
+ // against a required table field after the value conversion finishes.
+ const auto cast_target_type = input_type->is_nullable() &&
!table_type->is_nullable()
Review Comment:
[P1] Keep parent-null placeholders out of required-child validation
For an Iceberg `optional struct s { required a }`, the native Parquet path
represents `a` as nullable and emits a null child placeholder when `s` itself
is null. This nullable cast target preserves that placeholder, but
`_materialize_present_child_mapping_column` immediately calls
`_align_column_nullability` for required `a`, so `[NULL, {a: 1}]` fails before
`_materialize_struct_mapping_column` reapplies `parent_null_map`. The added
test uses an all-zero child null map and misses this valid shape. Please
validate child nulls with the enclosing struct map in scope—allow only rows
masked by a null parent, still reject null children under non-null parents—and
add both cases.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/insert/InsertUtils.java:
##########
@@ -373,6 +373,8 @@ private static Plan normalizePlanWithoutLock(LogicalPlan
plan, TableIf table,
UnboundInlineTable unboundInlineTable = (UnboundInlineTable) query;
ImmutableList.Builder<List<NamedExpression>> optimizedRowConstructors
=
ImmutableList.builderWithExpectedSize(unboundInlineTable.getConstantExprsList().size());
+ // Iceberg branch writes follow the shared table schema; historical
schemas only apply
Review Comment:
[P1] Fence one target metadata generation across the whole write plan
This still calls no-argument `getBaseSchema` before `BindSink` creates the
statement's latest target snapshot. External-table `readLock()` is a no-op, so
a cache invalidation can split the phases: normalization expands `DEFAULT` from
S0, then BindSink pins S1 and maps that value into S1. With drop/re-add of the
same name and a changed default, this silently writes S0's default into the new
field; the later schema check reads statement-pinned S1 and misses the
transition. Final `IcebergTableSink.bindDataSink()` also reloads live
schema/spec/sort order, so pinning only these lookups can still pair S1
projections with S2 writer metadata. Please fence one target metadata
generation before normalization and use it through target coercion and writer
construction, with a refresh-barrier DEFAULT test.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergSysExternalTable.java:
##########
@@ -84,6 +84,16 @@ public boolean isPositionDeletesTable() {
return
MetadataTableType.POSITION_DELETES.name().equalsIgnoreCase(sysTableType);
}
+ public boolean supportsSnapshotSelection() {
+ MetadataTableType tableType = MetadataTableType.from(sysTableType);
+ // Iceberg's ALL_* tables intentionally scan every reachable snapshot
and reject useSnapshot.
+ return tableType != MetadataTableType.ALL_DATA_FILES
Review Comment:
[P1] Do not select snapshots on Iceberg's static metadata tables
`HISTORY`, `SNAPSHOTS`, `REFS`, and `METADATA_LOG_ENTRIES` need the same
`false` result here. In Iceberg 1.10.1 these are `StaticTableScan`s: inherited
`useSnapshot(S0)` first rejects the scan if S0 has expired, but their
overridden `planFiles()` tasks ignore S0 and enumerate the table's current/all
metadata. Thus a cache refresh after binding can make a valid `t$history` query
fail with `Cannot find snapshot with ID S0`; while S0 exists, the call still
does not fence returned rows. Please exclude these four types, document their
intentionally unfenced semantics, and cover an expired bound base snapshot.
--
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]
