github-actions[bot] commented on code in PR #65784:
URL: https://github.com/apache/doris/pull/65784#discussion_r3610384732
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/source/IcebergScanNode.java:
##########
@@ -549,12 +552,16 @@ 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) {
+ // A schema-only update does not create a data snapshot. Ordinary
scans expose current
+ // table columns, so their default metadata must come from that
same current schema.
+ return
IcebergUtils.getBase64EncodedInitialDefaults(icebergTable.schema());
Review Comment:
[P1] Read binary-default markers from the statement's pinned schema
The `Column`s serialized above are MVCC-pinned: relation binding captures an
`IcebergSnapshotCacheValue`, and `getColumns()` continues to resolve that
captured schema ID. This ordinary branch instead reads the `Table` object
cached later by `IcebergScanNode`. If a schema-only DROP plus cache
invalidation occurs between those points, the columns remain S1 while
`icebergTable` is S2. For a UUID/BINARY/FIXED field mapped to STRING/CHAR,
added with an initial default in S1, absent from an older file, and then
dropped in S2, S2 omits its Base64 marker; both BEs materialize the carrier
text rather than the bytes. This is distinct from r3610150298, which fixed
current-vs-data-snapshot alignment for one stable table generation. Resolve the
marker map by the same MVCC schema ID that produced `getColumns()`, and add a
cache-invalidation-between-binding-and-scan test.
##########
be/src/format/table/table_schema_change_helper.cpp:
##########
@@ -556,25 +587,26 @@ Status
TableSchemaChangeHelper::BuildTableInfoUtil::by_parquet_field_id_with_nam
const auto& parquet_fields_schema = parquet_field_desc.get_fields_schema();
std::map<int32_t, size_t> file_column_id_idx_map;
- bool all_have_field_id = true;
+ // Iceberg considers the schema ID-bearing when any field has an ID;
requiring all IDs would
+ // discard authoritative matches merely because an unrelated sibling is
ID-less.
+ bool has_field_id = false;
for (size_t idx = 0; idx < parquet_fields_schema.size(); idx++) {
- if (parquet_fields_schema[idx].field_id == -1) {
- all_have_field_id = false;
- break;
+ if (parquet_fields_schema[idx].field_id != -1) {
+ has_field_id = true;
+
file_column_id_idx_map.emplace(parquet_fields_schema[idx].field_id, idx);
}
- file_column_id_idx_map.emplace(parquet_fields_schema[idx].field_id,
idx);
}
std::map<std::string, size_t> file_column_name_idx_map;
- if (!all_have_field_id) {
+ if (!has_field_id) {
file_column_name_idx_map =
build_lowercase_field_name_idx_map(parquet_fields_schema);
}
for (const auto& table_field : table_schema.fields) {
const auto& table_column_name = table_field.field_ptr->name;
size_t file_column_idx = 0;
bool matched = false;
- if (all_have_field_id) {
+ if (has_field_id) {
Review Comment:
[P1] Retain ID-less wrappers when another root has an ID
For Parquet roots `id#1, s(no id)<a#2>` and table `id#1, s#10<a#2>`, `id#1`
makes `has_field_id` true, but this branch can only match direct root IDs. It
marks `s` missing without looking at `a#2`, so V1 returns NULL/the initial
default instead of the physical value. V2 intentionally handles this shape via
the descendant-ID wrapper fallback. This is distinct from r3609650016: its file
had no unrelated direct ID, so V1 stayed in name mode. Apply the same
Parquet-only wrapper rule in V1 at every struct recursion and add a V1 result
test with the unrelated ID-bearing sibling.
##########
be/src/format_v2/column_mapper.cpp:
##########
@@ -2084,7 +2161,19 @@ const ColumnDefinition*
TableColumnMapper::_find_file_field(
});
return field_it == file_schema.end() ? nullptr : &*field_it;
}
- return matcher_for_mode(_options.mode).find(table_column, file_schema);
+ const auto* matched = matcher_for_mode(_options.mode).find(table_column,
file_schema);
+ if (matched != nullptr || _options.mode !=
TableColumnMappingMode::BY_FIELD_ID ||
+ !_options.allow_idless_complex_wrapper_projection ||
table_column.children.empty()) {
+ return matched;
+ }
+ const auto* wrapper = find_column_by_name(table_column, file_schema);
Review Comment:
[P1] Let descendant IDs retain wrappers despite an empty name mapping
With a valid authoritative `[]` mapping, FE sends `has_name_mapping=true`
with zero aliases for every field. For table `s#10<a#2>` and Parquet `s(no
id)<a#2>`, recursive Iceberg ID detection selects `BY_FIELD_ID`, and Parquet
retains `s` because `a#2` is selected. Here the root ID miss calls
`find_column_by_name`, but the authoritative-empty aliases reject the current
name before `has_shared_descendant_field_id` can run, so V2 fills `s` as
missing. V1 also fails: it sees no direct root ID and its strict name lookup
rejects `s`. Existing wrapper tests omit the mapping bit, while empty-mapping
tests are flat. Discover the Parquet wrapper from a unique shared descendant ID
independently of alias matching in both scanners, with
present-`[]`/partial-mapping result tests.
--
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]
