github-actions[bot] commented on code in PR #65784:
URL: https://github.com/apache/doris/pull/65784#discussion_r3609339720
##########
be/src/format/table/table_schema_change_helper.cpp:
##########
@@ -73,6 +73,9 @@ bool find_file_field_idx_by_name_mapping(
return true;
}
}
+ // An explicit empty or unmatched Iceberg mapping means the legacy
field is absent;
+ // falling back to its current name would bind an unrelated physical
column.
+ return false;
Review Comment:
[P1] Preserve ID matches when only part of a file schema has IDs
This strict return is used for every field once `all_have_field_id` sees any
ID-less sibling (and V2 similarly downgrades the whole file through
`IcebergTableReader::_has_field_id`). For a partial mapping with `a` (ID 1,
`names=[]`) and a file containing `a` (ID 1) plus `b` (no ID), both scanners
now enter name mode and materialize `a` as missing/default even though its
physical ID matches. Iceberg 1.10.1's `ParquetSchemaUtil.hasIds` and
`ORCSchemaUtil.hasIds` select ID projection when any field has an ID, so that
ID must remain authoritative. Please preserve ID-first matching before applying
strict empty/unmatched mappings in both V1 and V2 (including nested paths), and
add mixed-ID Parquet/ORC coverage.
##########
be/src/format_v2/column_mapper.cpp:
##########
@@ -82,12 +82,16 @@ bool column_has_name(const ColumnDefinition& column, const
std::string& name) {
}
bool column_names_match(const ColumnDefinition& lhs, const ColumnDefinition&
rhs) {
- if (column_has_name(rhs, lhs.name)) {
- return true;
- }
- if (lhs.has_identifier_name() && column_has_name(rhs,
lhs.get_identifier_name())) {
- return true;
+ if (!lhs.has_name_mapping) {
Review Comment:
[P1] Keep mapping semantics consistent through rolling upgrades
This matcher (and the parallel V1 early return) changes the meaning of the
existing optional `name_mapping` field without a version signal. During Doris's
documented BE-first rollout, the old FE already sends present-empty or
unmatched mappings for explicit mapped-field entries; old BEs fall back to a
matching current-name column while new BEs materialize missing/default, so one
scan can combine unrelated values and NULL/defaults. After all BEs are new, the
FE rollout still mixes old FEs that omit unmapped entries from partial/`[]`
mappings with new FEs that send empty lists, making identical queries depend on
the FE endpoint. Please version/gate this contract or enforce homogeneous
compatible execution in both phases, and add old-FE/mixed-BE plus mixed-FE
coverage.
--
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]
