github-actions[bot] commented on code in PR #65784:
URL: https://github.com/apache/doris/pull/65784#discussion_r3609650017
##########
be/src/exec/scan/access_path_parser.cpp:
##########
@@ -87,6 +87,9 @@ void inherit_schema_metadata(format::ColumnDefinition* column,
return;
}
column->name_mapping = schema_column->name_mapping;
+ // The presence bit is part of the mapping contract: an explicit empty
mapping must remain
+ // authoritative after access-path pruning instead of enabling
current-name fallback.
+ column->has_name_mapping = schema_column->has_name_mapping;
Review Comment:
[P1] Preserve nested Iceberg binary initial defaults
`getBase64EncodedInitialDefaults()` indexes nested UUID/BINARY/FIXED field
IDs, `ExternalUtil` recursively writes the value and Base64 marker, and schema
reconstruction restores them. This pruning copy then carries only aliases and
authority. Once an explicit empty nested mapping correctly rejects a physical
child, the V2 child mapping has no default expression and inserts the generic
type default; V1's missing-child node does the same. An older file missing such
an `s.b` therefore returns NULL/empty data instead of its decoded initial
default. Please carry and materialize recursive initial defaults for
struct/array/map children in both scanners, with binary/Base64 result coverage.
##########
be/src/format/table/table_schema_change_helper.cpp:
##########
@@ -556,25 +562,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;
Review Comment:
[P1] Match ID-less wrapper projection to each file format
This computes the any-ID decision independently for each V1 struct, while V2
switches globally but matches only the current root ID. For a physical ID-less
wrapper `s` containing `legacy_a` with ID 1, and table `s` ID 10 containing `a`
ID 1, V1 name-matches the wrapper and returns `a`, whereas V2 cannot match root
ID 10 and materializes the struct as missing/default. Iceberg Parquet retains
the selected physical child and its nonempty wrapper; Iceberg ORC instead
synthesizes the absent parent ID rather than reusing the physical wrapper, so
V1's fallback is inconsistent there. Please make both scanner paths honor the
format-specific projection contract, with Parquet/ORC whole and pruned nested
result tests.
##########
be/src/format_v2/table/iceberg_reader.h:
##########
@@ -77,24 +77,21 @@ class IcebergTableReader : public format::TableReader {
private:
struct EqualityDeleteFilter;
- bool _has_field_id(const std::vector<format::ColumnDefinition>& schema)
const {
+ bool _has_any_field_id(const std::vector<format::ColumnDefinition>&
schema) const {
for (const auto& field : schema) {
- // TopN lazy materialization asks the file reader to synthesize
GLOBAL_ROWID in the
- // first-phase scan. That virtual column is not an Iceberg data
field and therefore has
- // no Iceberg field id. Do not let it downgrade schema-evolution
reads to BY_NAME,
- // otherwise old data files whose physical names predate a rename
(for example,
- // table column `new_new_id` stored as file column `id`) are
materialized as defaults.
+ // Iceberg's hasIds contract is existential. Ignore synthesized
columns and keep ID
+ // projection as soon as any real field (including a nested field)
carries an ID.
if (field.column_type != format::ColumnType::DATA_COLUMN) {
continue;
}
- if (!field.has_identifier_field_id()) {
- return false;
+ if (field.has_identifier_field_id()) {
Review Comment:
[P1] Reconcile the unchanged stale-ID equality-delete test
With this new existential switch,
`IcebergEqualityDeleteByNameIgnoresStaleFileFieldId` can no longer produce its
asserted `[1, 3]`. Its file has ID-less `legacy_id=[1,2,3]` plus
`stale_key=[100,200,300]` carrying ID 0; table `current_id` also has ID 0. This
helper now selects `BY_FIELD_ID`, so both projection and delete-key lookup bind
to `stale_key`, value 2 deletes nothing, and the result is `[100,200,300]`.
Please explicitly resolve this compatibility contract and update the
fixture/expectation (or implement the supported exception); the current full V2
reader suite is deterministically inconsistent with the patch.
--
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]
