github-actions[bot] commented on code in PR #65675:
URL: https://github.com/apache/doris/pull/65675#discussion_r3589299994
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergUtils.java:
##########
@@ -700,8 +700,12 @@ public static Type
icebergTypeToDorisType(org.apache.iceberg.types.Type type, bo
icebergTypeToDorisType(map.valueType(),
enableMappingVarbinary, enableMappingTimestampTz));
case STRUCT:
Types.StructType struct = (Types.StructType) type;
+ // Preserve the original nested field-name case so DESCRIBE
stays consistent with the
+ // top-level columns (already case-preserved) and faithful to
the Iceberg schema
+ // (Spark/Trino compatibility). Field lookups remain
case-insensitive because
+ // StructType lowercases its map keys.
ArrayList<StructField> nestedTypes =
struct.fields().stream().map(
- x -> new StructField(x.name(),
+ x -> StructField.createPreservingCase(x.name(),
Review Comment:
This case-preserved old type is also fed into Iceberg complex-column MODIFY
validation. In `validateForModifyComplexColumn`, Doris converts the current
Iceberg type through this method, then compares it to the SQL-derived new type
with `ColumnType.checkSupportIcebergSchemaChangeForComplexType`. The new SQL
type still uses catalog `StructField` names that are lowercased by the normal
constructor, and `validateStructFieldCompatibility` compares `getName()` with
case-sensitive `equals`. So an existing Iceberg field like `CamelName` and a
user command that keeps `STRUCT<CamelName: BIGINT>` will be compared as old
`CamelName` versus new `camelname` and rejected as `Cannot rename struct field`
before the valid type update is applied. The validation path needs to compare
nested names with the same case-insensitive semantics used for lookup, or
otherwise normalize only the validation copy while preserving the Iceberg path
for the actual update.
##########
be/src/format/table/table_schema_change_helper.cpp:
##########
@@ -67,14 +67,20 @@ bool find_file_field_idx_by_name_mapping(
return true;
};
+ // When the table defines schema.name-mapping.default, field resolution
for files without field
+ // ids must go strictly through the mapping. A field that is not covered
by the mapping (empty
+ // name_mapping) or whose mapped names are absent from the file must
resolve to NULL, never fall
+ // back to physical-name matching.
if (table_field.__isset.name_mapping) {
for (const auto& mapped_name : table_field.name_mapping) {
if (try_match(mapped_name)) {
return true;
}
}
+ return false;
Review Comment:
This only fixes the legacy Iceberg schema-change helper. Query scans use
`FileScannerV2` by default, and that path builds
`format::iceberg::IcebergTableReader` with the same `history_schema_info`, but
`format_v2/table_reader.cpp::external_field_matches_name` still returns true on
`field.name` before looking at `field.name_mapping`. So for the new FE contract
where an unmapped field is sent as `name='b', name_mapping=[]`, the v2 path
still binds a legacy file's physical `b` column instead of producing NULL. The
strict name-mapping rule needs to be applied to the v2 schema-history matching
path as well; otherwise default query execution can keep returning the old
wrong value while these new tests only cover the legacy helper.
--
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]
