github-actions[bot] commented on code in PR #68962:
URL: https://github.com/apache/doris/pull/68962#discussion_r4246537986
##########
be/src/format_v2/table/iceberg_reader.cpp:
##########
@@ -199,6 +199,47 @@ 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;
+ if (field.has_identifier()) {
Review Comment:
[P1] Replace name identifiers when assigning positional field IDs
ID-less Parquet columns already have identifiers:
`ParquetReader::_fill_column_definition()` sets `identifier` to
`TYPE_STRING(column_schema.name)` when `parquet_field_id` is absent, and
`has_identifier()` returns true for that string. Consequently every physical
field takes this `continue` and no ordinal ID is assigned. With v2 semantics
and no name mapping, `BY_FIELD_ID` still resolves none of the columns, so the
migrated-file query remains all NULL and `WHERE id = 1` drops its real row.
Remove this guard or use `has_identifier_field_id()` so string identifiers can
be replaced with ordinal IDs.
##########
be/src/format_v2/table/iceberg_reader.h:
##########
@@ -205,6 +216,10 @@ class IcebergTableReader : public format::TableReader {
size_t _row_position_block_position = 0;
std::optional<TIcebergFileDesc> _iceberg_params;
bool _delete_predicates_initialized = false;
+ // Set when this split's Parquet file carried no field ids and the table
exposed no usable name
+ // mapping, so the reader stamped Iceberg's positional fallback ids on the
file's top-level
+ // fields. Observability only; the decision itself lives in
annotate_file_schema().
+ bool _positional_id_fallback = false;
Review Comment:
[P3] Reset the fallback diagnostic when preparing a new split
This member belongs to the reused `IcebergTableReader`, and
`prepare_split()` never clears it. Once an ID-less Parquet file sets it true, a
subsequent ID-bearing Parquet or ORC file returns early from
`annotate_file_schema()` and continues to report `positional_id_fallback=1` in
`debug_string()`/VLOG, although no fallback was used for that file. Reset the
flag at the start of `prepare_split()` so the per-file diagnostic also
describes pruned and metadata-only splits correctly.
##########
be/test/format_v2/table/iceberg_reader_test.cpp:
##########
@@ -2971,6 +3033,285 @@ TEST(IcebergV2ReaderTest,
IcebergV2IdlessFileRequiresAuthoritativeNameMapping) {
TableColumnMappingMode::BY_NAME);
}
+// DORIS-29685: an add_files split (no field ids) cherry-picked back into an
evolved table read as a
+// whole all-NULL row because the table no longer carried a usable name
mapping. Iceberg's Parquet
+// reader binds such files by position (`ParquetSchemaUtil.addFallbackIds` +
`pruneColumnsFallback`,
+// `iceberg.netflix.unsafe-parquet-id-fallback.enabled` defaults to true), so
Doris must do the same
+// instead of silently materializing NULL. The physical names deliberately
differ from the table
+// names: the fallback is "file ordinal k == Iceberg field id k", never name
based.
+TEST(IcebergV2ReaderTest,
IcebergIdlessParquetWithoutNameMappingUsesPositionalIds) {
+ const auto test_dir =
+ std::filesystem::temp_directory_path() /
"doris_iceberg_positional_id_fallback_test";
+ std::filesystem::remove_all(test_dir);
+ std::filesystem::create_directories(test_dir);
+ const auto file_path = (test_dir / "split.parquet").string();
+ write_idless_int_columns_parquet_file(file_path, {"legacy_id",
"legacy_value"},
+ {{1, 2, 3}, {10, 20, 30}});
+
+ std::vector<ColumnDefinition> projected_columns;
+ projected_columns.push_back(make_table_column(1, "id",
std::make_shared<DataTypeInt32>()));
+ projected_columns.push_back(make_table_column(2, "value",
std::make_shared<DataTypeInt32>()));
+
+ RuntimeProfile profile("test_profile");
+ RuntimeState state {TQueryOptions(), TQueryGlobals()};
+ auto scan_params = make_local_parquet_scan_params();
+
scan_params.__set_iceberg_scan_semantics_version(ICEBERG_SCAN_SEMANTICS_VERSION_2);
+ io::FileReaderStats file_reader_stats;
+ io::FileCacheStatistics file_cache_stats;
+ auto io_ctx = make_io_context(&file_reader_stats, &file_cache_stats);
+ ShardedKVCache cache(1);
+ doris::format::iceberg::IcebergTableReader reader;
+ init_iceberg_reader(&reader, projected_columns, &scan_params, io_ctx,
&state, &profile);
+
+ auto split_options = build_split_options(file_path);
+ split_options.cache = &cache;
+ split_options.current_range.__set_table_format_params(
+ make_iceberg_table_format_desc(file_path, {}));
+ ASSERT_TRUE(reader.prepare_split(split_options).ok());
+ EXPECT_NE(reader.debug_string().find("positional_id_fallback=1"),
std::string::npos);
Review Comment:
[P2] Check the fallback flag after the lazy reader opens
`prepare_split()` records the ScanTask and delete metadata; it does not open
the data file. `annotate_file_schema()`, the only place that sets
`_positional_id_fallback=true`, first runs through `get_block() ->
create_next_reader() -> open_reader()`. A fresh reader therefore still reports
`positional_id_fallback=0` at this `EXPECT_NE`, so this test and
`IcebergIdlessParquetStructChildStaysNull` fail even after the
identifier-stamping bug is fixed. Move the flag checks after the first
successful `get_block()`; the `=0` checks should also happen after opening so
they exercise the intended exclusion paths.
--
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]
