mbutrovich commented on code in PR #3483:
URL: https://github.com/apache/iceberg-rust/pull/3483#discussion_r4317782538
##########
crates/iceberg/src/delete_file_index.rs:
##########
@@ -386,9 +386,82 @@ mod tests {
use crate::ErrorKind;
use crate::spec::{
DataContentType, DataFileBuilder, DataFileFormat, Datum, Literal,
ManifestEntry,
- ManifestStatus, Struct,
+ ManifestStatus, NestedField, PrimitiveType, Struct, StructType, Type,
};
+ #[test]
+ fn promoted_data_file_partition_matches_delete_after_column_promotion() {
Review Comment:
Should these two tests move to the scan level? `delete_file_index.rs` itself
doesn't change in this PR, so they check `promoted_partition` plus `Struct`
equality, which the `data_file.rs` tests and
`test_scan_widens_old_partition_so_pruning_and_deletes_match` already cover.
Two cases from @JeonDaehong's Java run aren't covered end to end yet. One is
a v3 deletion vector written after the promotion against the older data file,
which is the case that hit the [partition
check](https://github.com/apache/iceberg-rust/blob/642d8dd66a7b75e0ba3f61bd74863e340e91d93d/crates/iceberg/src/delete_file_index.rs#L318-L324).
The other is a data file and a delete that were both written before the
promotion. Those match on main, and with this change both sides get widened, so
a test would show they still match. `TableTestFixture::setup_v3_manifest_files`
shows how to upgrade the fixture to v3 for the DV case.
##########
crates/iceberg/src/expr/visitors/inclusive_metrics_evaluator.rs:
##########
@@ -129,6 +130,16 @@ impl<'a> InclusiveMetricsEvaluator<'a> {
ROWS_MIGHT_MATCH
}
+
+ /// Widens a stored bound to the reference's type so a bound written under
a
+ /// narrower type (e.g. an `int` column later promoted to `long`) compares
+ /// against a predicate bound to the wider type.
+ fn promote_bound(bound: &Datum, reference: &BoundReference) -> Datum {
+ match reference.field().field_type.as_primitive_type() {
+ Some(primitive_type) => bound.promote_to(primitive_type),
+ None => bound.clone(),
+ }
+ }
Review Comment:
Could this borrow the bound when there's nothing to promote?
`bound.clone()`, and the `self.clone()` fallback in `Datum::promote_to`, copy
every bound the evaluator reads. A predicate on a `string` or `binary` column
now allocates a `String` or `Vec<u8>` per bound per data file during planning,
where main compares by reference. A `Cow` keeps the promoted case and borrows
otherwise:
```suggestion
fn promote_bound<'b>(bound: &'b Datum, reference: &BoundReference) ->
Cow<'b, Datum> {
reference
.field()
.field_type
.as_primitive_type()
.and_then(|primitive_type| bound.promote_to(primitive_type))
.map_or(Cow::Borrowed(bound), Cow::Owned)
}
```
That needs `use std::borrow::Cow;` and `Datum::promote_to` returning
`Option<Datum>` (with its doc comment updated):
```rust
pub(crate) fn promote_to(&self, target: &PrimitiveType) -> Option<Datum> {
self.literal
.promote_to(target)
.map(|literal| Datum::new(target.clone(), literal))
}
```
The call sites then bind `let bound = &*Self::promote_bound(bound,
reference);`, and the comparisons go back to `cmp_fn(bound, datum)`,
`datum.ge(lower_bound)` and `datum.le(upper_bound)` as on main.
##########
crates/iceberg/src/scan/context.rs:
##########
@@ -73,6 +73,19 @@ pub(crate) struct ManifestEntryContext {
pub sort_order: Option<SortOrderRef>,
}
+/// Returns whether `current` widens any field of `written` at the same
position
+/// (differing primitive types), i.e. partition values written with `written`
need
+/// widening to `current` before they compare against predicates bound to the
current
+/// schema.
+fn partition_types_differ(current: &StructType, written: &StructType) -> bool {
+ current.fields().len() != written.fields().len()
+ || current
+ .fields()
+ .iter()
+ .zip(written.fields())
+ .any(|(c, w)| c.field_type.as_primitive_type() !=
w.field_type.as_primitive_type())
+}
Review Comment:
Should this gate fire only for the type pairs that `promoted_partition`
changes? As written, any primitive difference deep-clones every `ManifestEntry`
in the manifest, bounds maps included, on every scan. A partition source
widened from `decimal(9, 2)` to `decimal(18, 2)` takes that path, but
`promote_partition_value` leaves every value as it is. So does a dropped
`string` source that now resolves to `unknown`.
The length check can't fire either. Both types come from the same spec, and
[`partition_type`](https://github.com/apache/iceberg-rust/blob/1be091a2135a5908537a5b45786ae7207a723aab/crates/iceberg/src/spec/partition.rs#L109-L133)
pushes one field per spec field. Something like this limits the clone to
manifests that need it (it also needs `PrimitiveType` in the `crate::spec`
import):
```suggestion
/// Returns whether any `int` or `float` field of `written` is widened in
`current` (to
/// `long` or `double`, or to `unknown` after its source column is dropped),
i.e. partition
/// values written with `written` need widening before they compare against
predicates
/// bound to the current schema.
fn partition_types_differ(current: &StructType, written: &StructType) ->
bool {
current.fields().iter().zip(written.fields()).any(|(c, w)| {
matches!(
(
w.field_type.as_primitive_type(),
c.field_type.as_primitive_type()
),
(
Some(PrimitiveType::Int),
Some(PrimitiveType::Long | PrimitiveType::Unknown)
) | (
Some(PrimitiveType::Float),
Some(PrimitiveType::Double | PrimitiveType::Unknown)
)
)
})
}
```
##########
crates/iceberg/src/spec/manifest/data_file.rs:
##########
@@ -184,7 +186,36 @@ pub struct DataFile {
pub(crate) content_size_in_bytes: Option<i64>,
}
+fn promote_partition_value(value: Option<&Literal>, field: &NestedFieldRef) ->
Option<Literal> {
+ match value {
+ Some(Literal::Primitive(primitive)) => {
+ let promoted = match field.field_type.as_primitive_type() {
+ // The partition source column was dropped, so there is no
target type to
+ // widen to; widen to the promotion ceiling instead so this
value and its
+ // deletes still normalize to the same type and the delete
index matches.
+ Some(PrimitiveType::Unknown) => primitive.widen(),
+ Some(target) => primitive.promote_to(target),
+ None => None,
+ };
+ promoted.map(Literal::Primitive).or_else(|| value.cloned())
+ }
+ other => other.cloned(),
+ }
+}
+
impl DataFile {
+ /// Returns this file's partition tuple widened to `partition_type` for
fields
+ /// promoted (`int` -> `long`, `float` -> `double`) since the file's
manifest
+ /// was written. Values whose type is unchanged are copied through. Column
+ /// bounds are widened separately, lazily, where they are compared.
+ pub(crate) fn promoted_partition(&self, partition_type: &StructType) ->
Struct {
+ self.partition
+ .iter()
+ .zip(partition_type.fields())
+ .map(|(value, field)| promote_partition_value(value, field))
+ .collect()
+ }
Review Comment:
What should happen when `self.partition` and `partition_type` have different
lengths? `zip` stops at the shorter one, so trailing partition values would be
dropped without an error, and the entry would then miss its partition-scoped
deletes. The two should always line up because they come from the same spec.
Should this return a `DataInvalid` error on a mismatch, the way
`get_deletes_for_data_file` does when a DV's partition disagrees with its data
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]
