github-actions[bot] commented on code in PR #68962:
URL: https://github.com/apache/doris/pull/68962#discussion_r4246945018
##########
be/src/format_v2/table/iceberg_reader.cpp:
##########
@@ -199,6 +199,50 @@ Status IcebergTableReader::validate_variant_file_mappings(
file_format_name(format));
}
+Status IcebergTableReader::annotate_file_schema(
+ std::vector<format::ColumnDefinition>* file_schema) {
+ if (file_schema == nullptr || file_schema->empty()) {
+ return Status::OK();
+ }
+ // Iceberg's positional fallback is a Parquet-reader-only compatibility
path. Its ORC/Avro
+ // readers instead derive a name mapping from the expected schema for
id-less files, which is
+ // exactly the current-name resurrection that v2 scan semantics
deliberately rejects (#65784),
+ // so ORC keeps its existing behavior and stays out of scope here.
+ if (_format != FileFormat::PARQUET) {
+ return Status::OK();
+ }
+ // Iceberg's `hasIds` contract is existential: a single real field id
keeps the file on the id
+ // path and, exactly as in Iceberg, skips both the name mapping and the
positional fallback.
+ if (schema_has_any_field_id(*file_schema)) {
+ return Status::OK();
+ }
+ // An old-FE plan keeps the legacy name fallback (rolling upgrade), and an
explicit table-level
+ // name mapping stays authoritative even when it does not cover this file.
+ if (!supports_iceberg_scan_semantics_v2(_scan_params) ||
+ _scan_has_any_authoritative_name_mapping()) {
+ return Status::OK();
+ }
+ // ParquetSchemaUtil.addFallbackIds: ordinals start at 1, are counted over
the physical
+ // top-level fields in file order, and are not propagated to nested
fields. Reader-synthesized
+ // carriers (row number / global row id) are not file fields and must not
consume an ordinal.
+ int32_t ordinal = 0;
+ for (auto& field : *file_schema) {
+ if (field.column_type != format::ColumnType::DATA_COLUMN) {
+ continue;
+ }
+ ++ordinal;
+ // ID-less Parquet fields carry their physical name as a STRING
identifier
+ // (ParquetReader::_fill_column_definition), so only a real integer
field id may skip
+ // stamping; overwriting the string identifier is what lets
FieldIdMatcher resolve the file.
+ if (field.has_identifier_field_id()) {
+ continue;
+ }
+ field.identifier = Field::create_field<TYPE_INT>(ordinal);
Review Comment:
[P2] Keep missing struct children out of the physical projection
For a v2 ID-less file whose first field is s STRUCT<legacy_time
TIME_MILLIS>, with no name mapping and table s (ID 1) containing an optional
child new_value (ID 2), this stamps the root but leaves every child unmatched.
The mapper retains a full physical subtree for this empty child projection
(column_mapper.cpp:1840-1861; schema_projection.cpp:116), so
ParquetReader::open validates the discarded legacy_time leaf and returns
NotSupported rather than producing the expected NULL child. Supported wide
STRING children are likewise read and allocated only to be discarded. Carry a
shape-only dependency for all-missing STRUCT children and consume a retained
leaf's levels without requiring logical-type support or materializing its
payload; add a fallback test with an unsupported or large discarded child.
##########
be/src/format_v2/table/iceberg_reader.cpp:
##########
@@ -199,6 +199,50 @@ Status IcebergTableReader::validate_variant_file_mappings(
file_format_name(format));
}
+Status IcebergTableReader::annotate_file_schema(
+ std::vector<format::ColumnDefinition>* file_schema) {
+ if (file_schema == nullptr || file_schema->empty()) {
+ return Status::OK();
+ }
+ // Iceberg's positional fallback is a Parquet-reader-only compatibility
path. Its ORC/Avro
+ // readers instead derive a name mapping from the expected schema for
id-less files, which is
+ // exactly the current-name resurrection that v2 scan semantics
deliberately rejects (#65784),
+ // so ORC keeps its existing behavior and stays out of scope here.
+ if (_format != FileFormat::PARQUET) {
+ return Status::OK();
+ }
+ // Iceberg's `hasIds` contract is existential: a single real field id
keeps the file on the id
+ // path and, exactly as in Iceberg, skips both the name mapping and the
positional fallback.
+ if (schema_has_any_field_id(*file_schema)) {
+ return Status::OK();
+ }
+ // An old-FE plan keeps the legacy name fallback (rolling upgrade), and an
explicit table-level
+ // name mapping stays authoritative even when it does not cover this file.
+ if (!supports_iceberg_scan_semantics_v2(_scan_params) ||
+ _scan_has_any_authoritative_name_mapping()) {
+ return Status::OK();
+ }
+ // ParquetSchemaUtil.addFallbackIds: ordinals start at 1, are counted over
the physical
+ // top-level fields in file order, and are not propagated to nested
fields. Reader-synthesized
+ // carriers (row number / global row id) are not file fields and must not
consume an ordinal.
+ int32_t ordinal = 0;
+ for (auto& field : *file_schema) {
+ if (field.column_type != format::ColumnType::DATA_COLUMN) {
+ continue;
+ }
+ ++ordinal;
+ // ID-less Parquet fields carry their physical name as a STRING
identifier
+ // (ParquetReader::_fill_column_definition), so only a real integer
field id may skip
+ // stamping; overwriting the string identifier is what lets
FieldIdMatcher resolve the file.
+ if (field.has_identifier_field_id()) {
+ continue;
+ }
+ field.identifier = Field::create_field<TYPE_INT>(ordinal);
Review Comment:
[P1] Preserve logical struct values when localizing whole-root predicates
For an ID-less Parquet file with an OPTIONAL s STRUCT<inner OPTIONAL INT>
containing inner=7, let the v2 table have s (ID 1) and optional inner (ID 2),
matching names/types and no name mapping. This stamps s but leaves inner
unmatched, so final materialization returns {"inner":null}. However, the
whole-root rewrite in column_mapper.cpp:1291-1293 returns the raw file slot
when nullable DataTypes are equal, without checking missing child mappings.
WHERE CAST(s AS STRING) LIKE '%null%' therefore runs against {"inner":7} in
Parquet and drops a row that should survive; re-evaluating the original
predicate later cannot restore it. Keep generic complex-root predicates above
materialization whenever their child mappings require reconstruction, or
localize an equivalent mapped expression. Add a nullable ID-less STRUCT test
comparing output with whole-root filtering.
--
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]
