github-actions[bot] commented on code in PR #68004:
URL: https://github.com/apache/doris/pull/68004#discussion_r4013330464
##########
fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergSchemaUtils.java:
##########
@@ -214,8 +215,15 @@ static void applySchemaEvolution(TFileScanRangeParams
params, String encoded) {
* name-mapping property, and a present (possibly empty) map when it does
— the distinction #65784 relies on
* to make a table-level mapping AUTHORITATIVE (an unmapped field then
materializes its default/NULL instead
* of silently matching a physical column by its current name; see {@link
#buildField}). Port of legacy
- * {@code IcebergScanNode.extractNameMapping} + {@code
IcebergUtils.getNameMapping} (#65784). A malformed
- * property fails soft to a current-name mapping instead of becoming
indistinguishable from no property.
+ * {@code IcebergScanNode.extractNameMapping} + {@code
IcebergUtils.getNameMapping} (#65784).
Review Comment:
[P1] Reject an empty name-mapping value as malformed
Only `null` means absence upstream: Iceberg accepts an empty non-null
property value, while its readers pass every non-null value to
`NameMappingParser.fromJson`, which rejects `""`. The unchanged guard at
current line 230 therefore bypasses this new catch and leaves Doris without
authoritative aliases. For ID-less files after a rename, that can silently
materialize defaults/NULL for the renamed field or bind old bytes to a later
field that reuses the old name. Please treat only `null` as absent and cover
`""` in the malformed test while keeping `[]` as the valid authoritative empty
mapping.
##########
fe/fe-connector/fe-connector-iceberg/src/test/java/org/apache/doris/connector/iceberg/IcebergSchemaUtilsTest.java:
##########
@@ -715,21 +716,33 @@ public void extractNameMappingRecursesIntoNestedFields() {
}
@Test
- public void malformedNameMappingKeepsIdlessCurrentNameFallback() {
- // A malformed name-mapping property must not break the scan or look
identical to a genuinely absent
- // mapping. Required and optional fields both need current-name
aliases for ID-less legacy files.
+ public void malformedNameMappingFailsInsteadOfFallingBackToCurrentNames() {
+ // Iceberg refuses to read a table whose name mapping cannot be parsed
(Spark's BaseReader parses the
+ // property while constructing the file reader), so the connector must
surface the metadata fault.
+ // Rewriting the property into current-schema aliases would silently
return NULL for the renamed
+ // columns of ID-less files instead of reporting it.
Table table = createTable("t1", SCHEMA,
Collections.singletonMap(TableProperties.DEFAULT_NAME_MAPPING,
"{not valid json"));
- Map<Integer, List<String>> fallback =
IcebergSchemaUtils.extractNameMapping(table).orElseThrow();
- Assertions.assertEquals(Collections.singletonList("id"),
fallback.get(1));
- Assertions.assertEquals(Collections.singletonList("name"),
fallback.get(2));
+ DorisConnectorException exception =
Assertions.assertThrows(DorisConnectorException.class,
+ () -> IcebergSchemaUtils.extractNameMapping(table));
+
Assertions.assertTrue(exception.getMessage().contains(TableProperties.DEFAULT_NAME_MAPPING));
+ Assertions.assertTrue(exception.getMessage().contains("t1"));
+ }
+
+ @Test
+ public void validEmptyNameMappingStaysAuthoritative() {
+ // An explicitly empty mapping is NOT the same as an absent property:
it stays authoritative, so an
+ // ID-less file's columns resolve to their defaults/NULLs instead of
matching by current name.
+ Table table = createTable("t1", SCHEMA,
+ Collections.singletonMap(TableProperties.DEFAULT_NAME_MAPPING,
"[]"));
+
+ Map<Integer, List<String>> mapping =
IcebergSchemaUtils.extractNameMapping(table).orElseThrow();
+ Assertions.assertTrue(mapping.isEmpty());
Map<String, TField> fields = topFields(dict(table, "id", "name"));
Review Comment:
[P1] Fence authoritative no-fallback mappings during rolling upgrades
This assertion proves only the dictionary sent by the FE. With `[]`,
`hasProjectedNameAliasCollision` finds no alias and an ordinary optional-column
scan can omit `REQUIRED_CURRENT_BACKEND_SEMANTICS`. A smooth-upgrade-source BE
then ignores unknown thrift field 9, exhausts the empty legacy alias list, and
falls back to the current field name, returning stored data where a current BE
must materialize NULL/default. The same gap affects a projected field omitted
by a partial authoritative mapping. Please fence every projected authoritative
no-fallback mapping from old BEs and add a provider-level test for the
required-semantics property.
--
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]
