github-actions[bot] commented on code in PR #66007:
URL: https://github.com/apache/doris/pull/66007#discussion_r3664954661
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/logical/LogicalFileScan.java:
##########
@@ -113,14 +156,41 @@ protected LogicalFileScan(RelationId id, ExternalTable
table, List<String> quali
}
private static SelectedPartitions initialSelectedPartitions(
- ExternalTable table, Optional<TableScanParams> scanParams) {
+ ExternalTable table, Optional<TableScanParams> scanParams,
+ Optional<MvccSnapshot> relationSnapshot) {
if ((table instanceof PaimonExternalTable || table instanceof
PaimonSysExternalTable)
&& scanParams.isPresent() && scanParams.get().isOptions()) {
// A relation-scoped historical snapshot cannot reuse partitions
cached for the
// statement-level latest snapshot; Paimon will prune its selected
snapshot instead.
return SelectedPartitions.NOT_PRUNED;
}
- return
table.initSelectedPartitions(MvccUtil.getSnapshotFromContext(table));
+ return table.initSelectedPartitions(relationSnapshot);
+ }
+
+ private static Optional<List<Column>> captureRelationSchema(
+ ExternalTable table, Optional<TableScanParams> scanParams,
+ Optional<MvccSnapshot> relationSnapshot) {
+ if (scanParams.isPresent() && scanParams.get().isOptions()) {
+ if (table instanceof PaimonExternalTable) {
+ return Optional.of(ImmutableList.copyOf(
+ ((PaimonExternalTable)
table).getFullSchema(scanParams.get())));
+ }
+ if (table instanceof PaimonSysExternalTable) {
+ return Optional.of(ImmutableList.copyOf(
+ ((PaimonSysExternalTable)
table).getFullSchema(scanParams.get())));
+ }
+ }
+ return captureRelationSchema(table, relationSnapshot);
+ }
+
Review Comment:
[P1] Do not pin ordinary Hive slots behind an empty snapshot
This captures a relation schema for every `MvccTable`, but a
non-Hudi/non-Iceberg `HMSExternalTable` returns only `EmptyMvccSnapshot`. After
a metastore event or `REFRESH TABLE` between binding and physical
initialization, logical slots remain on S0 while
`HMSExternalTable.getFullSchema(Optional)` falls through to `ExternalTable`,
ignores that placeholder, and reloads S1 for `FileQueryScanNode` column
positions, defaults, and types. A rename then fails with `Column old_name not
found`; type/default changes can mix generations. This is distinct from the
live Iceberg empty-state comment because ordinary Hive has no executable
snapshot at all. Please either give Hive a snapshot that reproduces its schema
or exclude empty placeholders from this fence, with a binding-to-init
refresh-barrier test.
##########
be/src/core/data_type_serde/data_type_array_serde.cpp:
##########
@@ -366,13 +366,14 @@ Status DataTypeArraySerDe::write_column_to_orc(const
std::string& timezone, cons
const IColumn& nested_column = array_col.get_data();
const auto& offsets = array_col.get_offsets();
for (size_t row_id = start; row_id < end; row_id++) {
- size_t offset = offsets[row_id - 1];
size_t next_offset = offsets[row_id];
- RETURN_IF_ERROR(nested_serde->write_column_to_orc(timezone,
nested_column, nullptr,
-
cur_batch->elements.get(), offset,
- next_offset, arena,
options));
cur_batch->offsets[row_id + 1] = next_offset;
}
+ const size_t nested_start = start == 0 ? 0 : offsets[start - 1];
+ const size_t nested_end = end == 0 ? nested_start : offsets[end - 1];
+ RETURN_IF_ERROR(nested_serde->write_column_to_orc(timezone, nested_column,
nullptr,
+
cur_batch->elements.get(), nested_start,
+ nested_end, arena,
options));
Review Comment:
[P1] Compact collection payload hidden by the parent mask
The struct writer now marks a child collection null from its parent mask,
but this batched Array path ignores that mask and forwards the full
offset-derived element span; Map does the same for keys and values. ORC
suppresses the null row's collection length and then consumes the supplied
child span as packed values, so when a null struct row retains nonempty
physical collection payload, the following visible row reads that hidden
payload and later elements shift. Nullable nested payload is not required to be
defaulted. This is a write-side packing defect, distinct from the live
TableReader parent-null issue. Please remove parent-masked entries or normalize
offsets/payload equivalently, and round-trip a null parent with nonempty
Array/Map payload followed by a present row.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/expression/check/CheckCast.java:
##########
@@ -414,7 +414,9 @@ && check(((MapType) originalType).getValueType(),
((MapType) targetType).getValu
return false;
}
for (int i = 0; i < targetFields.size(); i++) {
- if (originalFields.get(i).isNullable() !=
targetFields.get(i).isNullable()) {
+ // A nullable target can safely accept a required source, but
the inverse would
+ // allow a possible NULL into a required nested field.
+ if (originalFields.get(i).isNullable() &&
!targetFields.get(i).isNullable()) {
return false;
}
Review Comment:
[P1] Preserve value nullability for `named_struct` too
Separately from the live fallible-cast issue on this line, this check also
rejects safe values because the parallel constructors disagree. `struct(1)` now
reports a required child through `StructLiteral.computeDataType()`, but
`CreateNamedStruct.customSignature()` still hard-codes every field nullable.
Thus `CAST(named_struct('a', 1) AS STRUCT<a:BIGINT NOT NULL>)` and the
equivalent INSERT are rejected even though the value is provably non-null.
Please derive each named field's nullability from its value child as well, with
cast/INSERT tests for both non-null and nullable values.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/PruneFileScanPartition.java:
##########
@@ -116,4 +116,10 @@ private SelectedPartitions
pruneExternalPartitions(ExternalTable externalTable,
return new SelectedPartitions(nameToPartitionItem.size(),
selectedPartitionItems, true,
result.hasPartitionPredicate);
}
+
+ static List<Column> getPartitionColumnsForScan(ExternalTable
externalTable, LogicalFileScan scan) {
+ // The partition map is frozen on the logical relation, so its column
list must come from
+ // the same snapshot rather than the mutable table-scoped
compatibility entry.
Review Comment:
[P1] Derive partition columns from the frozen generation
Although this applies the existing thread's prescribed `relationSnapshot`
argument, the Iceberg helper does not honor that generation. Its frozen
snapshot value supplies the T0 partition map, but
`getIcebergPartitionColumns()` enters a schema-ID-only cache whose miss path
reloads the current table and derives columns from `table.spec()`. A
metadata-only spec change from identity `p` to `q` keeps snapshot/schema IDs
unchanged, so this rule can evaluate `q` against T0's encoded `p` ranges and
prune the only matching partition. Please freeze partition columns with the
partition map or derive them from the retained table, and add a spec-refresh
barrier 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());
rootFragment.setSink(sink);
sink.setOutputExprs(outputExprs);
Review Comment:
[P1] Carry this pinned table into the Iceberg transaction
The existing writer-generation thread stops at writer construction; this
fixes that reload but leaves a distinct later one.
`IcebergInsertExecutor.beforeExec()` calls `IcebergTransaction.beginInsert()`,
which independently reloads the current `Table`. If a metadata-only spec change
replaces identity partition `p` with `q` after planning, BE writes T0 files
while static-overwrite commit logic uses T1; `buildPartitionFilter()` finds no
T1 field for key `p`, returns `alwaysTrue`, and `overwriteByRowFilter` deletes
the whole table. Please create or validate the transaction against this same
target generation and make unmatched static keys fail, with a refresh-barrier
static-overwrite test.
--
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]
