github-actions[bot] commented on code in PR #65784:
URL: https://github.com/apache/doris/pull/65784#discussion_r3610668439
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergSnapshotCacheValue.java:
##########
@@ -17,14 +17,30 @@
package org.apache.doris.datasource.iceberg;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+
public class IcebergSnapshotCacheValue {
private final IcebergPartitionInfo partitionInfo;
private final IcebergSnapshot snapshot;
+ private final Optional<Map<Integer, List<String>>> nameMapping;
public IcebergSnapshotCacheValue(IcebergPartitionInfo partitionInfo,
IcebergSnapshot snapshot) {
+ this(partitionInfo, snapshot, Optional.empty());
+ }
+
+ public IcebergSnapshotCacheValue(IcebergPartitionInfo partitionInfo,
IcebergSnapshot snapshot,
+ Optional<Map<Integer, List<String>>> nameMapping) {
this.partitionInfo = partitionInfo;
this.snapshot = snapshot;
+ this.nameMapping = nameMapping.map(mapping -> {
+ Map<Integer, List<String>> copy = new HashMap<>();
+ mapping.forEach((id, names) -> copy.put(id, List.copyOf(names)));
+ return Map.copyOf(copy);
Review Comment:
[P1] Handle ID-less entries before making the mapping immutable
Iceberg's name-mapping parser deliberately accepts an entry without
`field-id` (`fieldFromJson` uses `getIntOrNull`, and `MappedField.id()` is
nullable). For a parser-supported property such as `[{"names":["legacy"]}]`,
`IcebergUtils.extractMappingsFromNameMapping` therefore puts a null key in its
`HashMap`; this new `Map.copyOf` then throws `NullPointerException`, so
snapshot loading fails before the table can be planned. The old scan-local
`HashMap` tolerated that unused key, making this a new regression. Please
ignore ID-less entries while still recursing into their nested mappings and
preserving `Optional.of(...)` for authoritative property presence, and add a
snapshot-cache test for this shape.
##########
be/src/format_v2/table/iceberg_reader.h:
##########
@@ -57,12 +57,16 @@ class IcebergTableReader : public format::TableReader {
Status prepare_split(const format::SplitReadOptions& options) override;
std::string debug_string() const override;
format::TableColumnMappingMode mapping_mode() const override {
- return !_data_reader.file_schema.empty() &&
_has_field_id(_data_reader.file_schema)
+ return !_data_reader.file_schema.empty() &&
_has_any_field_id(_data_reader.file_schema)
Review Comment:
[P1] Apply the existential ID rule to the V2 position-delete reader
This fixes mixed-ID selection for ordinary `IcebergTableReader`, but the
`position_deletes` system table instantiates a separate
`PositionDeleteFileTableReader`. Its `has_field_id()` still requires every root
and nested field to have an ID. For a delete file `file_path#..., pos#...,
row#2147483544<legacy_a#1, b(no id)>` after field 1 is renamed to `a`, that
reader selects `BY_NAME`, misses `legacy_a`, and can bind ID-less fields by
current name; Iceberg's file-wide existential rule must keep ID mode, read
field 1, and treat the ID-less sibling as absent. This is a distinct parallel
path from the earlier ordinary-reader thread because it never invokes
`_has_any_field_id()`. Please reuse the recursive any-ID predicate in
`PositionDeleteFileTableReader` and add Parquet/ORC `position_deletes.row`
result 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]
