github-actions[bot] commented on code in PR #65502:
URL: https://github.com/apache/doris/pull/65502#discussion_r3568578673
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/source/IcebergScanNode.java:
##########
@@ -454,20 +454,12 @@ public void createScanRangeLocations() throws
UserException {
// Extract name mapping from Iceberg table properties
Map<Integer, List<String>> nameMapping = extractNameMapping();
- boolean haveTopnLazyMatCol = false;
- for (SlotDescriptor slot : desc.getSlots()) {
- String colName = slot.getColumn().getName();
- if (colName.startsWith(Column.GLOBAL_ROWID_COL)) {
- haveTopnLazyMatCol = true;
- break;
- }
- }
- if (haveTopnLazyMatCol) {
- ExternalUtil.initSchemaInfoForAllColumn(params, -1L,
source.getTargetTable().getColumns(), nameMapping);
- } else {
- // Use new initSchemaInfo method that only includes needed columns
based on slots and pruned type
- ExternalUtil.initSchemaInfoForPrunedColumn(params, -1L,
desc.getSlots(), nameMapping);
- }
+ // Equality-delete keys are hidden scan dependencies and need not
appear in the query
+ // projection. Both scanners need the complete current schema to
resolve field ids,
+ // historical names, types, and initial defaults when an old data file
lacks such a key.
+ ExternalUtil.initSchemaInfoForAllColumn(
+ params, -1L, source.getTargetTable().getColumns(), nameMapping,
+
IcebergUtils.getBase64EncodedInitialDefaultFieldIds(icebergTable.schema()));
Review Comment:
This uses the selected scan columns from
`source.getTargetTable().getColumns()`, but computes the Base64-default
field-id set from `icebergTable.schema()`, which is the table's latest schema.
Time-travel/branch/tag scans load the requested snapshot into
`StatementContext`; `IcebergExternalTable.getFullSchema()` then comes from that
snapshot schema via `IcebergUtils.getIcebergSchema()`. If the selected snapshot
still has a UUID/BINARY/FIXED field with an initial default that is absent or
changed in the latest schema, `ExternalUtil` will send the default string
without `initial_default_value_base64_encoded`. BE then treats the Base64 text
as the literal string/VARBINARY default instead of decoding to the raw equality
key bytes, so missing-key equality deletes can be missed. Please derive the
Base64 field-id set from the same Iceberg schema/schema id used to build the
scan columns, or carry this encoding bit with each parsed `Column`, and add
snapshot/branch coverage for a binary-lik
e initial default.
##########
be/src/format/table/iceberg_reader_mixin.h:
##########
@@ -634,6 +663,82 @@ Status
IcebergReaderMixin<BaseReader>::_expand_block_if_need(Block* block) {
return Status::OK();
}
+template <typename BaseReader>
+Status
IcebergReaderMixin<BaseReader>::_register_missing_equality_delete_column(
+ int32_t field_id, const std::string& name, const DataTypePtr&
delete_key_type) {
+ DORIS_CHECK(delete_key_type != nullptr);
+ const schema::external::TField* table_field = nullptr;
+ const auto& scan_params = this->get_scan_params();
+ DORIS_CHECK(scan_params.__isset.history_schema_info);
+ DORIS_CHECK(!scan_params.history_schema_info.empty());
+ const auto* current_schema = &scan_params.history_schema_info.front();
+ if (scan_params.__isset.current_schema_id) {
+ const auto schema_it = std::ranges::find_if(
+ scan_params.history_schema_info, [&](const
schema::external::TSchema& schema) {
+ return schema.__isset.schema_id &&
+ schema.schema_id == scan_params.current_schema_id;
+ });
+ DORIS_CHECK(schema_it != scan_params.history_schema_info.end());
+ current_schema = &*schema_it;
+ }
+ DORIS_CHECK(current_schema->__isset.root_field);
+ for (const auto& field_ptr : current_schema->root_field.fields) {
+ if (field_ptr.__isset.field_ptr && field_ptr.field_ptr != nullptr &&
+ field_ptr.field_ptr->__isset.id && field_ptr.field_ptr->id ==
field_id) {
+ table_field = field_ptr.field_ptr.get();
+ break;
+ }
+ }
+ DORIS_CHECK(table_field != nullptr);
+
+ Field value;
+ if (table_field->__isset.initial_default_value) {
+ const auto nested_type = remove_nullable(delete_key_type);
+ if (table_field->__isset.initial_default_value_base64_encoded &&
+ table_field->initial_default_value_base64_encoded) {
+ std::string decoded_default;
+ if (!base64_decode(table_field->initial_default_value,
&decoded_default)) {
+ return Status::InvalidArgument(
+ "Invalid Base64 Iceberg initial default for field {}",
table_field->name);
+ }
+ if (nested_type->get_primitive_type() == TYPE_VARBINARY) {
+ value =
Field::create_field<TYPE_VARBINARY>(StringView(decoded_default));
+ } else {
+ DORIS_CHECK(nested_type->get_primitive_type() == TYPE_CHAR ||
+ nested_type->get_primitive_type() == TYPE_VARCHAR
||
+ nested_type->get_primitive_type() == TYPE_STRING);
+ value =
Field::create_field<TYPE_STRING>(std::move(decoded_default));
+ }
+ } else {
+ RETURN_IF_ERROR(nested_type->get_serde()->from_fe_string(
+ table_field->initial_default_value, value));
+ }
+ }
+ _missing_equality_delete_values.emplace(name,
delete_key_type->create_column_const(1, value));
+ return Status::OK();
+}
+
+template <typename BaseReader>
+Status
IcebergReaderMixin<BaseReader>::_materialize_missing_equality_delete_columns(Block*
block,
+
size_t rows) {
+ for (const auto& [name, value] : _missing_equality_delete_values) {
+ if (!this->col_name_to_block_idx_ref()->contains(name)) {
+ const auto expand_col = std::ranges::find_if(
+ _expand_columns,
+ [&](const ColumnWithTypeAndName& col) { return col.name ==
name; });
+ DORIS_CHECK(expand_col != _expand_columns.end());
+ (*this->col_name_to_block_idx_ref())[name] = block->columns();
+ block->insert({value->clone_resized(rows), expand_col->type,
name});
Review Comment:
This leaves the synthesized missing-key column as a `ColumnConst`. That
works for single-column equality deletes because `SimpleEqualityDelete` expands
consts before probing, but the V1 multi-key path hashes data columns directly
in `MultiEqualityDelete::filter_data_block()` and then compares them directly
in `_equal()`. `ColumnConst` does not implement the batch
`update_hashes_with_value()` method and its `compare_at()` expects the RHS to
also be const, while delete-file columns are materialized normal columns. A
delete file on `(id, added_key)` against an old data file where `added_key` is
logically the initial default will fail before it can remove matching rows.
Please insert a full column here (or normalize consts in `MultiEqualityDelete`)
and add a V1 multi-column equality-delete test with one absent/defaulted key.
##########
be/src/format/table/iceberg_reader.cpp:
##########
@@ -279,6 +279,12 @@ Status
IcebergParquetReader::on_before_init_reader(ReaderInitContext* ctx) {
_expand_columns[i].name = table_col_name;
}
+ if (it == field_id_to_file_col_name.end()) {
Review Comment:
At this point `ctx->table_info_node` has already been built with the Iceberg
name mapping and can resolve id-less files by alias, but the hidden
equality-key remap ignores it and only probes `field_id_to_file_col_name`. For
migrated Parquet/ORC files without field IDs, projected columns are still
readable through `*_field_id_with_name_mapping()`, while an unprojected
equality-delete key falls into this branch and gets materialized as
NULL/default. A delete on a renamed field such as table `current_id` mapped to
physical `legacy_id` can therefore leave matching rows visible. Please resolve
the hidden key through the same name-mapping node before treating it as
missing, and add V1 coverage for an unprojected equality key in an
id-less/name-mapped file.
##########
be/src/format/table/iceberg_reader_mixin.h:
##########
@@ -634,6 +663,82 @@ Status
IcebergReaderMixin<BaseReader>::_expand_block_if_need(Block* block) {
return Status::OK();
}
+template <typename BaseReader>
+Status
IcebergReaderMixin<BaseReader>::_register_missing_equality_delete_column(
+ int32_t field_id, const std::string& name, const DataTypePtr&
delete_key_type) {
+ DORIS_CHECK(delete_key_type != nullptr);
+ const schema::external::TField* table_field = nullptr;
+ const auto& scan_params = this->get_scan_params();
+ DORIS_CHECK(scan_params.__isset.history_schema_info);
+ DORIS_CHECK(!scan_params.history_schema_info.empty());
+ const auto* current_schema = &scan_params.history_schema_info.front();
+ if (scan_params.__isset.current_schema_id) {
+ const auto schema_it = std::ranges::find_if(
+ scan_params.history_schema_info, [&](const
schema::external::TSchema& schema) {
+ return schema.__isset.schema_id &&
+ schema.schema_id == scan_params.current_schema_id;
+ });
+ DORIS_CHECK(schema_it != scan_params.history_schema_info.end());
+ current_schema = &*schema_it;
+ }
+ DORIS_CHECK(current_schema->__isset.root_field);
+ for (const auto& field_ptr : current_schema->root_field.fields) {
+ if (field_ptr.__isset.field_ptr && field_ptr.field_ptr != nullptr &&
+ field_ptr.field_ptr->__isset.id && field_ptr.field_ptr->id ==
field_id) {
+ table_field = field_ptr.field_ptr.get();
+ break;
+ }
+ }
+ DORIS_CHECK(table_field != nullptr);
Review Comment:
This new path assumes the FE scan descriptor always carries the full current
schema. That is true for the new `IcebergScanNode`, but older/pruned
descriptors used `initSchemaInfoForPrunedColumn()` and only included projected
slots, so an unprojected equality-delete key can be absent from
`history_schema_info` during a rolling upgrade or with an older descriptor. V2
handles that case by falling back to projected metadata or the delete-file
name/type when full schema metadata is unavailable; V1 reaches this
`DORIS_CHECK(table_field != nullptr)` and aborts before it can apply even the
NULL/default-free semantics. Please add the same compatibility fallback or
return a controlled non-crashing error for missing schema metadata, and cover a
pruned-descriptor V1 missing-key case.
--
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]
