github-actions[bot] commented on code in PR #67574:
URL: https://github.com/apache/doris/pull/67574#discussion_r3949099302
##########
regression-test/suites/external_table_p0/iceberg/test_gen_iceberg_by_api.groovy:
##########
@@ -48,7 +48,9 @@ suite("test_gen_iceberg_by_api", "p0,external") {
try {
qt_q02 """ select count(*) from table_with_append_file where
MAN_ID is not null """
} catch (Exception e) {
- assertTrue(e.getMessage().contains("name_mapping must be set when
read missing field id data file."), e.getMessage());
+ // This fixture has no field IDs or authoritative name mapping, so
its required
+ // columns must be treated as missing instead of being matched by
their current names.
+ assertTrue(e.getMessage().contains("Missing required field:
MAN_ID"), e.getMessage());
Review Comment:
[P2] Make the V2 error the only passing outcome
A normal return from `qt_q02` still passes using the existing `463870`
golden, so this test also accepts the exact current-name behavior that the
patch is supposed to reject. Pin `enable_file_scanner_v2=true` and use the
repository's `test { sql; exception "Missing required field: MAN_ID" }` pattern
(with the stale q02 golden regenerated away); keep any intended V1 success
check separate.
##########
be/src/format_v2/table/iceberg_reader.h:
##########
@@ -73,6 +73,12 @@ class IcebergTableReader : public format::TableReader {
if (!_data_reader.file_schema.empty() && has_field_ids) {
return format::TableColumnMappingMode::BY_FIELD_ID;
}
+ if (!_data_reader.file_schema.empty() &&
supports_iceberg_scan_semantics_v2(_scan_params) &&
Review Comment:
[P1] Version this result-changing scan behavior
Scan-semantics version 2 already predates this PR: base FEs send it and base
BEs interpret a fully ID-less file with `BY_NAME`. This new v2 branch changes
the same plan to `BY_FIELD_ID`, so during a BE-first smooth upgrade an ordinary
optional-field scan can return physical values on old BEs and
NULL/default/error on upgraded BEs. `requiresCurrentScanSemantics()` does not
fence absent-mapping optional or always-present fields. Please introduce a new
capability/version for this rule and make new FEs fence pre-capability BEs (or
otherwise guarantee homogeneous scheduling), with a mixed-version regression.
##########
be/src/format_v2/table/iceberg_reader.h:
##########
@@ -73,6 +73,12 @@ class IcebergTableReader : public format::TableReader {
if (!_data_reader.file_schema.empty() && has_field_ids) {
return format::TableColumnMappingMode::BY_FIELD_ID;
}
+ if (!_data_reader.file_schema.empty() &&
supports_iceberg_scan_semantics_v2(_scan_params) &&
+ !schema_has_any_authoritative_name_mapping(_projected_columns)) {
Review Comment:
[P1] Keep the scan-level name-mapping signal for hidden keys
This infers a table-level property from visible `_projected_columns`, but
metadata-only projections contain only synthesized `_file`/`_pos` columns and
carry no schema annotation. When equality deletes apply, FE still sends the
full current/history schema with authoritative name mappings for hidden keys;
this branch nevertheless selects `BY_FIELD_ID` for an ID-less data file.
`_find_equality_delete_data_field()` then treats the mapped physical key as
missing, so a required key fails and an optional/defaulted key can leave rows
undeleted. Please derive mapping presence from the scan-level Iceberg schema
carrier (or an explicit scan-level bit), and test Parquet/ORC metadata-only
equality-delete scans.
##########
be/src/format_v2/column_mapper.cpp:
##########
@@ -2811,6 +2811,14 @@ Status TableColumnMapper::localize_filters(const
std::vector<TableFilter>& table
FileScanRequestBuilder builder(file_request);
RETURN_IF_ERROR(builder.add_non_predicate_column(std::move(demoted_projection)));
}
+ // Predicate demotion can widen a nested projection after mappings were
localized. Reapply the
+ // final shape so TableReader interprets the same child ordinals that
FileReader returns.
+ for (auto& mapping : _mappings) {
+ if (mapping.file_local_id.has_value() &&
+
file_request->local_positions.contains(LocalColumnId(*mapping.file_local_id))) {
+
RETURN_IF_ERROR(apply_scan_projection_to_mapping_file_type(*file_request,
&mapping));
Review Comment:
[P1] Reconcile mappings after Iceberg widens the request
This pass still runs before the actual final request shape:
`TableReader::open_reader()` calls `customize_file_scan_request()` afterward,
and Iceberg adds a nested equality-delete key by its root ID.
`add_predicate_column(LocalColumnId)` then lets a full-root projection dominate
the partial/demoted root, while the mapper and any localized nested predicate
retain the earlier shape. For a physical struct `[delete_key, a, b]` projected
as `a,b`, FileReader can return all three children but final materialization
indexes it as `[a,b]`, shifting the values. Please preserve the exact
nested-key projection, or reconcile both mapping projections and localized
expressions after customization, and cover this equality-delete case.
##########
regression-test/suites/external_table_p0/iceberg/test_iceberg_migrated_nested_without_name_mapping.groovy:
##########
@@ -0,0 +1,55 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+suite("test_iceberg_migrated_nested_without_name_mapping", "p0,external") {
+ String enabled = context.config.otherConfigs.get("enableIcebergTest")
+ if (enabled == null || !enabled.equalsIgnoreCase("true")) {
+ logger.info("disable iceberg test.")
+ return
+ }
+
+ String rest_port = context.config.otherConfigs.get("iceberg_rest_uri_port")
+ String minio_port = context.config.otherConfigs.get("iceberg_minio_port")
+ String externalEnvIp = context.config.otherConfigs.get("externalEnvIp")
+ String catalog_name = "test_iceberg_migrated_nested_without_name_mapping"
+
+ sql """drop catalog if exists ${catalog_name}"""
+ sql """
+ CREATE CATALOG ${catalog_name} PROPERTIES (
+ 'type'='iceberg',
+ 'iceberg.catalog.type'='rest',
+ 'uri' = 'http://${externalEnvIp}:${rest_port}',
+ 's3.access_key' = 'admin',
+ 's3.secret_key' = 'password',
+ 's3.endpoint' = 'http://${externalEnvIp}:${minio_port}',
+ 's3.region' = 'us-east-1'
+ )
+ """
+ sql """switch ${catalog_name}"""
+ sql """use multi_catalog"""
+ sql """set enable_fallback_to_original_planner=false"""
Review Comment:
[P1] Pin FileScannerV2 for this V2-only oracle
The external regression pipeline enables fuzzy session variables, and
`enable_file_scanner_v2` is randomized per session. When it is false, this
query uses V1, whose fully ID-less Iceberg path still matches the fixture's
current physical names and returns `1,10,11`; the new golden requires three
NULLs. This therefore fails nondeterministically and does not reliably exercise
the production branch changed here. Save the current value, set
`enable_file_scanner_v2=true` for the assertion, and restore it afterward.
--
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]
