github-actions[bot] commented on code in PR #65502:
URL: https://github.com/apache/doris/pull/65502#discussion_r3568056550
##########
be/src/format_v2/table/iceberg_reader.cpp:
##########
@@ -438,38 +482,125 @@ Status
IcebergTableReader::_append_row_position_output_column(format::FileScanRe
return Status::OK();
}
+const format::ColumnDefinition*
IcebergTableReader::_find_equality_delete_data_field(
+ const EqualityDeleteFilter& filter, size_t key_idx) const {
+ DORIS_CHECK(key_idx < filter.field_ids.size());
+ DORIS_CHECK(key_idx < filter.field_names.size());
+ const int field_id = filter.field_ids[key_idx];
+ auto field_it = std::ranges::find_if(_data_reader.file_schema,
+ [field_id](const
format::ColumnDefinition& field) {
+ return
field.has_identifier_field_id() &&
+
field.get_identifier_field_id() == field_id;
+ });
+ if (field_it != _data_reader.file_schema.end() ||
Review Comment:
This still lets a partial-id migrated file bypass BY_NAME binding.
`mapping_mode()` intentionally switches the split to BY_NAME when any data
column lacks an Iceberg field id, so normal scan columns are resolved by
`ColumnMapper`'s name/identifier/alias rules. This helper, however, first
accepts any file column whose field id equals the equality key id and returns
before it checks the mode. If an imported file has the real key column matched
only by `name_mapping` and some other physical column carrying the stale key
id, the delete predicate compares against that unrelated column and misses or
misapplies equality deletes. Please skip the id fast path when `mapping_mode()
== BY_NAME` and resolve hidden equality keys through the same BY_NAME matcher
as normal materialization.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergUtils.java:
##########
@@ -1198,6 +1204,27 @@ public static List<Column> parseSchema(Schema schema,
boolean enableMappingVarbi
return resSchema;
}
+ private static String
serializeInitialDefault(org.apache.iceberg.types.Type type, Object value,
+ boolean enableMappingVarbinary) {
+ String humanValue = Transforms.identity(type).toHumanString(type,
value);
+ if (type.typeId() == TypeID.TIMESTAMP) {
+ // Iceberg formats timestamps as ISO-8601 (for example
2024-01-01T00:00:00), while
+ // Doris' DATETIMEV2 default parser requires a space between the
date and time.
+ return humanValue.replace('T', ' ');
+ }
+ if (enableMappingVarbinary && type.typeId() == TypeID.UUID) {
+ // BINARY and FIXED are already Base64 in Iceberg's human
representation. UUID uses
+ // canonical text, so encode its 16-byte Iceberg representation
explicitly to keep the
+ // VARBINARY carrier consistent for all three mapped types.
+ UUID uuid = (UUID) value;
+ ByteBuffer bytes = ByteBuffer.allocate(16);
+ bytes.putLong(uuid.getMostSignificantBits());
+ bytes.putLong(uuid.getLeastSignificantBits());
+ return Base64.getEncoder().encodeToString(bytes.array());
+ }
+ return humanValue;
Review Comment:
This still sends the wrong representation for binary-like defaults when
varbinary mapping is disabled. In that mode UUID/BINARY become Doris STRING and
FIXED becomes CHAR, so the BE branch that Base64-decodes `TYPE_VARBINARY` never
runs; the STRING/CHAR serde keeps this `humanValue` text as-is. Equality delete
files, however, read BYTE_ARRAY/FIXED_LEN_BYTE_ARRAY keys as raw bytes into
string columns. For example an added BINARY default `00 01 02 ff` is carried as
`"AAEC/w=="`, but the delete key column contains raw bytes, so the missing-key
literal does not match the delete row. Please normalize UUID/BINARY/FIXED
defaults to the same raw-byte representation for the STRING/CHAR mapping as
well, or carry enough type metadata for BE to decode them before building the
literal.
##########
be/src/format/table/iceberg_reader.cpp:
##########
@@ -553,6 +559,11 @@ Status
IcebergOrcReader::on_before_init_reader(ReaderInitContext* ctx) {
if (i < _expand_columns.size()) {
_expand_columns[i].name = table_col_name;
}
+ if (it == field_id_to_file_col_name.end()) {
Review Comment:
This missing-key branch registers a constant for the equality key, but the
ORC path still treats the same synthetic column as physically present. After
this branch runs, the loop still appends `table_col_name` to
`ctx->column_names` and `add_children()` marks it with `exists=true`. ORC
therefore does not put it in `_fill_missing_cols`;
`_init_file_column_mapping()` adds it to `_read_file_cols`, and the batch read
later returns `Wrong read column` because the old ORC file has no such physical
column. Please leave the hidden equality key in the
block/default-materialization path without marking it as an existing ORC child,
and add an end-to-end V1 ORC missing-key equality-delete test.
##########
be/src/format_v2/table/iceberg_reader.cpp:
##########
@@ -632,12 +791,36 @@ Status
IcebergTableReader::_read_equality_delete_file(const TIcebergDeleteFileDe
return Status::NotSupported(
"Iceberg equality delete does not support complex column
{}", field_it->name);
}
- delete_fields.push_back(*field_it);
- delete_field_ids.push_back(field_id);
- delete_key_types.push_back(field_it->type);
+ delete_fields->push_back(*field_it);
+ result->field_ids.push_back(field_id);
+ result->field_names.push_back(field_it->name);
+ result->key_types.push_back(field_it->type);
}
+ return Status::OK();
+}
+
+Status IcebergTableReader::_load_equality_delete_file(const
TIcebergDeleteFileDesc& delete_file,
+ const
TFileScanRangeParams& scan_params,
+
IcebergDeleteFileIOContext* delete_io_ctx,
+ EqualityDeleteFilter*
result) {
+ DORIS_CHECK(result != nullptr);
+ std::unique_ptr<format::FileReader> reader;
+ RETURN_IF_ERROR(_create_delete_file_reader(delete_file, scan_params,
delete_io_ctx, &reader));
Review Comment:
The reader created here needs the same TIMESTAMPTZ mapping as the data
reader. Normal `TableReader` construction passes `enable_mapping_timestamp_tz`
into Parquet/ORC readers, but `_create_delete_file_reader()` uses those
constructors' default `false`, so adjusted-UTC timestamp keys in
equality-delete files are loaded as DATETIMEV2. The data side may still be
TIMESTAMPTZ and then gets cast to the delete key type through the session
timezone before `_equal()` compares raw column values. Around a DST fold, two
different instants can collapse to the same local DATETIMEV2 and an equality
delete for one instant can delete rows for the other. Please pass the effective
timestamp-tz mapping flag into V2 delete-file readers and add equality-delete
coverage with `enable_mapping_timestamp_tz=true` in a non-UTC timezone.
--
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]
