github-actions[bot] commented on code in PR #68003:
URL: https://github.com/apache/doris/pull/68003#discussion_r4021969996
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergExternalMetaCache.java:
##########
@@ -568,6 +569,9 @@ private IcebergSnapshotCacheValue loadSnapshotProjection(
retainedTable);
} catch (AnalysisException e) {
throw new RuntimeException(ExceptionUtils.getRootCauseMessage(e),
e);
+ } catch (UserException e) {
Review Comment:
[P1] Defer mapping validation until a data reader is planned
This propagates the parse failure from the shared snapshot projection, so it
now rejects operations that never consume the name mapping. On a cold/current
generation: native `$snapshots` and `$files` bind the source before the later
scan-node exemption; pure `INSERT ... VALUES` pins this projection even though
the append writer never reads existing files; and `DESCRIBE`/`SHOW COLUMNS`
obtains it only to resolve the logical schema. Iceberg likewise parses this
property in `BaseReader`, not `SparkWrite`. Please retain the metadata
generation independently (or keep a lazy validation state) and require parsing
only when a physical data scan consumes the mapping, without letting a
metadata/writer projection be reused by a data reader as a validated absent
mapping. Add cold-cache coverage for all three entry points.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergUtils.java:
##########
@@ -2337,9 +2342,19 @@ private static IcebergSchemaCacheValue
buildTableSchemaCacheValue(ExternalTable
/**
* Extract the Iceberg name mapping while retaining the distinction
between an absent property
- * and a valid empty mapping.
+ * and a valid (possibly empty) mapping.
+ *
+ * <p>A property that is present but cannot be parsed is a metadata fault
rather than an absent
+ * mapping. Iceberg readers refuse such tables outright (Spark's {@code
BaseReader} parses
+ * {@code schema.name-mapping.default} while constructing the file
reader), so Doris reports the
+ * fault instead of silently degrading to the current column names.
Degrading hides renamed
+ * columns behind NULLs when reading data files without field ids, and can
even return wrong
+ * values once a column name has been reused.
+ *
+ * @throws UserException if the property is present but cannot be parsed
as a name mapping
*/
- public static Optional<Map<Integer, List<String>>> getNameMapping(Table
icebergTable) {
+ public static Optional<Map<Integer, List<String>>> getNameMapping(Table
icebergTable)
+ throws UserException {
String nameMappingJson =
icebergTable.properties().get(TableProperties.DEFAULT_NAME_MAPPING);
if (nameMappingJson == null || nameMappingJson.isEmpty()) {
Review Comment:
[P1] Treat an empty property as malformed
This still maps a present `schema.name-mapping.default=""` to
`Optional.empty()`, so ID-less files fall back to current column names and can
still return NULL or the wrong column after renames—the exact failure this
change is intended to remove. Iceberg permits storing an empty property value,
while `NameMappingParser` requires a JSON array, so `""` is malformed rather
than absent. Please reserve `Optional.empty()` for `null` and add empty-string
coverage.
--
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]
