github-actions[bot] commented on code in PR #65784:
URL: https://github.com/apache/doris/pull/65784#discussion_r3611861386
##########
be/src/format/table/table_schema_change_helper.cpp:
##########
@@ -604,6 +726,13 @@ Status
TableSchemaChangeHelper::BuildTableInfoUtil::by_parquet_field_id_with_nam
Status
TableSchemaChangeHelper::BuildTableInfoUtil::by_parquet_field_id_with_name_mapping(
const schema::external::TField& table_schema, const FieldSchema&
parquet_field,
std::shared_ptr<TableSchemaChangeHelper::Node>& node) {
+ return by_parquet_field_id_with_name_mapping(table_schema, parquet_field,
node,
Review Comment:
[P1] Base V1 position-delete mapping on the full file
This overload derives `use_field_id` only from the supplied field, but the
still-dispatched V1 `IcebergPositionDeleteSysTableReader` first finds physical
`row` by current name and calls the Parquet/ORC helpers with only that row
subtree. For `file_path#..., pos#..., row(no id)<a(no id)>`, the complete
delete file has IDs, so Iceberg ID projection treats table `row#2147483544` as
absent; V1 sees no ID locally and returns the physical row by name. Resolve
`row` under the complete delete-file schema and carry that file-wide mode
through its children for both formats. The new mixed-ID tests instantiate only
V2 and keep `row` ID-bearing, so they do not cover this parallel path.
##########
be/src/format/table/table_schema_change_helper.cpp:
##########
@@ -825,27 +958,27 @@ Status
TableSchemaChangeHelper::BuildTableInfoUtil::by_orc_field_id_with_name_ma
auto struct_node = std::make_shared<TableSchemaChangeHelper::StructNode>();
std::map<int32_t, size_t> file_column_id_idx_map;
- bool all_have_field_id = true;
+ // Iceberg ORC builds projection by ID for the entire subtree. An ID-less
wrapper containing
+ // an ID-bearing child is therefore absent rather than rebound by its
physical name.
+ bool has_field_id = orc_subtree_has_field_id(orc_root,
field_id_attribute_key);
Review Comment:
[P1] Keep ORC ID mode through container recursion
This existential decision is recomputed whenever recursion reaches another
struct. After matching an ID-bearing ARRAY/MAP parent, the `TField` overload
descends positionally and calls this overload on only the element/value struct.
For table `items#10: ARRAY<STRUCT<a#12>>` and ORC `items#10: ARRAY<STRUCT<a(no
id)>>`, the complete file has IDs, but the nested recomputation is false and
current-name binds physical `a`; Iceberg's file-wide ID projection treats
`a#12` as missing/defaulted. Thread one `use_field_id` value through ORC
STRUCT/MAP/ARRAY recursion, as the Parquet path now does, and add array/map
reader-result coverage.
##########
be/src/format_v2/table/iceberg_position_delete_sys_table_reader.cpp:
##########
@@ -132,24 +133,14 @@ void set_iceberg_delete_field_id(ColumnDefinition*
column) {
}
}
-bool has_field_id(const std::vector<ColumnDefinition>& schema) {
- for (const auto& field : schema) {
- if (!field.has_identifier_field_id()) {
- return false;
- }
- if (!has_field_id(field.children)) {
- return false;
- }
- }
- return true;
-}
-
class PositionDeleteFileTableReader final : public format::TableReader {
protected:
format::TableColumnMappingMode mapping_mode() const override {
- return !_data_reader.file_schema.empty() &&
has_field_id(_data_reader.file_schema)
- ? format::TableColumnMappingMode::BY_FIELD_ID
- : format::TableColumnMappingMode::BY_NAME;
+ if (!_data_reader.file_schema.empty() &&
+ schema_has_any_field_id(_data_reader.file_schema)) {
Review Comment:
[P1] Enable Parquet wrapper projection in this reader
The new any-ID rule puts this child reader in `BY_FIELD_ID`, but unlike
ordinary `IcebergTableReader` it never enables
`allow_idless_complex_wrapper_projection`. For Parquet `row#2147483544<s(no
id)<legacy_a#1>>` projected as `row#2147483544<s#10<a#1>>`, `row` matches, the
nested `s` ID misses, and the gated descendant-ID fallback never runs, so `s`
is synthesized NULL/default instead of reading `a`. Add the same Parquet-only
`configure_mapper_options()` override and a nested-wrapper result test. This is
distinct from r3610668442's direct ID-bearing child case.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/source/IcebergScanNode.java:
##########
@@ -549,12 +524,24 @@ Map<Integer, String>
getBase64EncodedInitialDefaultsForScan() throws UserExcepti
// schema that produced source.getTargetTable().getColumns() to
keep defaults aligned.
return
IcebergUtils.getBase64EncodedInitialDefaults(icebergTable.schema());
}
+ IcebergTableQueryInfo selectedSnapshot = getSpecifiedSnapshot();
+ if (selectedSnapshot == null) {
+ Optional<MvccSnapshot> mvccSnapshot =
MvccUtil.getSnapshotFromContext(source.getTargetTable());
+ Schema scanSchema = icebergTable.schema();
+ if (mvccSnapshot.isPresent() && mvccSnapshot.get() instanceof
IcebergMvccSnapshot) {
+ long schemaId = ((IcebergMvccSnapshot) mvccSnapshot.get())
+ .getSnapshotCacheValue().getSnapshot().getSchemaId();
+ scanSchema =
icebergTable.schemas().get(Math.toIntExact(schemaId));
+ }
+ // The statement snapshot produced the target columns; cache
invalidation must not
+ // let initial-default metadata advance to a different schema
during the same scan.
+ return IcebergUtils.getBase64EncodedInitialDefaults(
+ Preconditions.checkNotNull(scanSchema, "Schema for Iceberg
scan is null"));
+ }
TableScan tableScan = createTableScan();
Snapshot snapshot = tableScan.snapshot();
- // TableScan.schema() starts from the table's current schema even for
useSnapshot/useRef.
- // Resolve the selected snapshot's schema id explicitly so this
metadata describes the same
- // snapshot as source.getTargetTable().getColumns(). Otherwise a later
type change can make
- // BE decode a historical non-binary default as Base64, or fail to
decode a binary default.
+ // Explicit time travel and ref scans expose the selected snapshot
schema rather than the
Review Comment:
[P1] Use the statement-pinned branch schema for default markers
Branch binding deliberately pairs the ref's data snapshot with
`SnapshotUtil.schemaFor(table, ref)` (the existing test expects branch snapshot
1 with schema 2), so a schema-only add can expose a new UUID/BINARY/FIXED
default on an old branch snapshot. This path switches back to
`snapshot.schemaId()`, omitting that field from the Base64-marker map even
though the target `Column` still carries its encoded default; with the
STRING/CHAR compatibility mapping, both BEs then materialize the carrier text
instead of the bytes. Resolve markers from the statement-pinned
`IcebergMvccSnapshot` schema ID that produced
`source.getTargetTable().getColumns()` rather than recomputing a branch schema
that can also advance after refresh, and add an old-branch/schema-only
binary-default test. This is distinct from the ordinary-scan schema and
cache-refresh threads because only branches intentionally use a schema newer
than their data 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]
