morningman commented on code in PR #69711:
URL: https://github.com/apache/doris/pull/69711#discussion_r4397951416
##########
be/src/format_v2/table/iceberg_reader.cpp:
##########
@@ -974,6 +974,114 @@ static Status build_equality_delete_key_expr(
return build_nested_equality_delete_key_expr(path, std::move(root_expr),
key_expr);
}
+static int equality_delete_integer_width(PrimitiveType type) {
+ switch (type) {
+ case TYPE_TINYINT:
+ return 1;
+ case TYPE_SMALLINT:
+ return 2;
+ case TYPE_INT:
+ return 4;
+ case TYPE_BIGINT:
+ return 8;
+ case TYPE_LARGEINT:
+ return 16;
+ default:
+ return 0;
+ }
+}
+
+// Whether every `from` value converts exactly to a `to` value that compares
equal to the same
+// Iceberg value. These are the Iceberg promotions (INT -> LONG, FLOAT ->
DOUBLE, decimal
+// precision, DATE -> TIMESTAMP and DATE -> TIMESTAMP_NS), a wider fractional
scale of the same
+// date-time kind, and STRING -> VARBINARY: an Iceberg BINARY key decodes as
either one, depending
+// on enable_mapping_varbinary, and both hold the same bytes. TIMESTAMP ->
TIMESTAMP_NS is not an
+// Iceberg promotion, but a delete can be prepared under a nanosecond schema
that the table later
+// abandons. Scaling microseconds by 1000 is exact, whereas rounding the
tombstone
+// 12:00:00.000000500 to microseconds would delete the data key
12:00:00.000000.
+static bool is_equality_delete_widening(const DataTypePtr& from, const
DataTypePtr& to) {
+ const auto from_type = remove_nullable(from);
+ const auto to_type = remove_nullable(to);
+ if (from_type->equals(*to_type)) {
+ return true;
+ }
+ const auto from_primitive = from_type->get_primitive_type();
+ const auto to_primitive = to_type->get_primitive_type();
+ if (const int from_width = equality_delete_integer_width(from_primitive);
from_width > 0) {
+ return equality_delete_integer_width(to_primitive) >= from_width;
+ }
+ switch (from_primitive) {
+ case TYPE_FLOAT:
+ return to_primitive == TYPE_DOUBLE;
+ case TYPE_DECIMAL32:
+ case TYPE_DECIMAL64:
+ case TYPE_DECIMAL128I:
+ case TYPE_DECIMAL256:
+ return is_decimalv3(to_primitive) && to_type->get_scale() ==
from_type->get_scale() &&
+ to_type->get_precision() >= from_type->get_precision();
+ case TYPE_DATEV2:
+ return to_primitive == TYPE_DATETIMEV2 || to_primitive ==
TYPE_TIMESTAMP_NS;
+ case TYPE_DATETIMEV2:
Review Comment:
Confirmed, and fixed in `3e250364574`. Parquet INT96 carries no instant
annotation, so the V2 Parquet schema maps it to `DATETIMEV2(6)`
(`NativeFieldDescriptor::get_doris_type`), while an adjusted-UTC INT64 delete
key maps to `TIMESTAMPTZ(6)`; neither direction of the new widening table
accepts that pair, so the split failed with `NotSupported` where the previous
code compared both sides in the table's `TIMESTAMPTZ`. One correction: this is
not limited to id-only queries. Nothing re-types an INT96 column of an Iceberg
scan at open: `apply_projection_timestamp_semantics` acts only on
`timestamp_is_adjusted_to_utc`, which only the Paimon connector sets, and the
key is resolved from the reader's file schema in every case, so `SELECT ts`
failed the same way. `enable_mapping_timestamp_tz` is always true on master, so
this was the default configuration. The fix compares a `DATETIMEV2` /
`TIMESTAMPTZ` pair in `TIMESTAMPTZ`, at the larger of the two scales, when the
table field is `timest
amptz`, converting the `DATETIMEV2` side with the same session-time-zone cast
the projection applies to that column (strict, like the other comparison
casts), so a tombstone matches exactly the value a query returns.
`IcebergEqualityDeleteComparesInt96DataKeyAsTimestampTz` reads an INT96 data
file against an adjusted-UTC tombstone in Asia/Shanghai, with and without the
key projected.
##########
be/src/format_v2/table/iceberg_reader.cpp:
##########
@@ -1569,11 +1682,18 @@ Status
IcebergTableReader::_build_missing_equality_delete_key_expr(
const auto* table_leaf = missing_path.back();
DORIS_CHECK(table_leaf != nullptr);
DORIS_CHECK(table_leaf->type != nullptr);
- if (!table_leaf->type->equals(*filter.key_types[key_idx])) {
- auto cast_expr =
EvolutionCast::create_shared(filter.key_types[key_idx]);
- cast_expr->add_child(default_expr);
- default_expr = std::move(cast_expr);
+ if (missing_root.initial_default_value.has_value()) {
Review Comment:
Confirmed in the reader, and fixed in `3e250364574`. Under a struct initial
default `{}`, a sub-field without its own default extracts as NULL, so the key
should be compared in the delete file's type exactly like a missing root
without a default; the branch now checks the extracted leaf instead of the
root, walking the default value along the key's path with the same child
matching the key expression uses. On reachability: this history cannot reach
the reader through the current FE. Iceberg 1.12, which the FE uses, rejects any
non-null default on a nested type (`Types.NestedField.castDefault`: "Invalid
default value for struct ... (must be null)"), and `SchemaParser` cannot load
`"initial-default": {}` on a struct (`Expressions.lit` has no struct literal),
so such a table fails to load before planning and the FE never sends a struct
`initial_default_value`. The BE does implement the spec's `{}` default
(`IcebergMissingWholeStructEqualityKeyMaterializesDefault`), so the null-leaf c
ase is now covered by
`IcebergMissingWholeStructEqualityKeyComparesNullLeafInTombstoneType`.
##########
be/src/format_v2/table/iceberg_reader.cpp:
##########
@@ -1597,37 +1716,54 @@ Status
IcebergTableReader::_build_missing_equality_delete_key_expr(
Status
IcebergTableReader::_append_equality_delete_predicates(format::FileScanRequest*
request) {
DORIS_CHECK(request != nullptr);
for (const auto& filter : _equality_delete_filters) {
- auto delete_predicate = std::make_shared<EqualityDeletePredicate>(
- filter.delete_block, filter.field_ids, filter.hash_index);
DCHECK_EQ(filter.field_ids.size(), filter.key_types.size());
+ DCHECK_EQ(filter.field_ids.size(), filter.delete_key_types.size());
+ VExprSPtrs key_exprs;
+ DataTypes comparison_types;
bool has_missing_key = false;
for (size_t idx = 0; idx < filter.field_ids.size(); ++idx) {
EqualityDeleteColumnPath data_path;
bool complete_path = false;
RETURN_IF_ERROR(
_find_equality_delete_data_field(filter, idx, &data_path,
&complete_path));
+ VExprSPtr key_expr;
+ DataTypePtr comparison_type;
if (!complete_path) {
- VExprSPtr key_expr;
-
RETURN_IF_ERROR(_build_missing_equality_delete_key_expr(filter, idx, data_path,
-
request, &key_expr));
- delete_predicate->add_child(key_expr);
+ RETURN_IF_ERROR(_build_missing_equality_delete_key_expr(
+ filter, idx, data_path, request, &key_expr,
&comparison_type));
has_missing_key = true;
- continue;
- }
- const auto* root = data_path.front();
- const auto* field = data_path.back();
- const auto field_column_id =
format::LocalColumnId(root->file_local_id());
- _append_file_scan_column(request, field_column_id,
&request->predicate_columns);
- const auto block_position =
request->local_positions.at(field_column_id).value();
- VExprSPtr key_expr;
- RETURN_IF_ERROR(build_equality_delete_key_expr(data_path,
block_position, &key_expr));
- if (field->type->equals(*filter.key_types[idx])) {
- delete_predicate->add_child(key_expr);
} else {
- auto cast_expr =
EvolutionCast::create_shared(filter.key_types[idx]);
- cast_expr->add_child(key_expr);
- delete_predicate->add_child(cast_expr);
+ const auto* root = data_path.front();
+ const auto field_column_id =
format::LocalColumnId(root->file_local_id());
+ _append_file_scan_column(request, field_column_id,
&request->predicate_columns);
+ const auto block_position =
request->local_positions.at(field_column_id).value();
+ RETURN_IF_ERROR(
+ build_equality_delete_key_expr(data_path,
block_position, &key_expr));
+ RETURN_IF_ERROR(equality_delete_comparison_type(
+ filter.field_ids[idx], filter.delete_key_types[idx],
key_expr->data_type(),
+ _data_file_path(), &comparison_type));
+ key_expr = cast_equality_delete_key(std::move(key_expr),
comparison_type);
Review Comment:
Confirmed, and fixed in `3e250364574`: `_append_equality_delete_predicates`
now skips a loaded filter whose delete block has no rows before resolving its
keys, so an empty delete file adds no hidden key column, no comparison cast and
no predicate. Two notes on scope. Iceberg's own writers never commit such a
file: the rolling writers delete a file that received no rows
(`BaseTaskWriter`, `RollingFileWriter`), and with column statistics present
`DeleteFileIndex` prunes a zero-record equality delete against data files
without NULL keys, so this needs another writer that commits an empty file
without statistics, which `FileMetadata` allows. And the evaluation order
predates this PR: `EqualityDeletePredicate` has always evaluated its key
expressions before checking for an empty index, and on master a key whose table
type is `TIMESTAMP_NS` already failed this way on an empty delete file through
the DATE -> TIMESTAMP_NS evolution cast, which is strict in every session. The
skip covers b
oth. With a nonempty delete file, a key outside the `TIMESTAMP_NS` range still
fails as intended. `IcebergEqualityDeleteSkipsEmptyDeleteFileBeforeKeyCasts`
covers a present and a defaulted DATE key against an empty nanosecond delete
file.
--
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]
