Decode manifest bounds after schema promotion

[osipovartem]

Summary

• Decode manifest bounds using the manifest's embedded schema, accepting old 4-byte INT/FLOAT bounds under promoted LONG/DOUBLE (and v3 DATE under TIMESTAMP) types.
• Promote bounds only where DataFusion consumes statistics or builds pruning arrays. This preserves historical bound types through ordinary manifest reads and rewrites.
• Decode manifest-list partition summaries using each entry's partition spec and snapshot schema. If an old source field is unavailable, discard only its optional summary; never discard the manifest. Mixed-spec manifests receive unknown pruning stats, while same-spec historical bounds retain pruning.
• Add checked DATE-to-TIMESTAMP and FLOAT-to-DOUBLE value promotion; preserve DECIMAL statistics with target precision validation.

This replaces the earlier selected-schema threading and removes the public ManifestFieldTypeIndex, extra entry constructors, and rewrite-specific reader mode from this PR. The nested-field index and writer error-propagation cleanup can be considered separately.

Validation

• cargo +1.95.0 test -q -p iceberg-rust-spec (174 active passed; upstream ignored tests unchanged)
• cargo +1.95.0 test -q -p iceberg-rust --lib (166 active passed)
• cargo +1.95.0 test -q -p datafusion_iceberg --lib (49 passed)
• cargo +1.95.0 clippy -p iceberg-rust-spec -p datafusion_iceberg --all-targets -- -D warnings
• Focused formatting and git diff --check
• Independent read-only review: approved after fixes for mixed partition specs, dropped source fields, decimal precision, and same-spec pruning performance.

Rust 1.98 Clippy currently reports pre-existing useless_borrows_in_formatting warnings in unrelated upstream files; the pinned validation above uses Rust 1.95.

[JanKaul]

Consider width-tolerant bound decoding instead of rewrite-time promotion

The bug this fixes is real, but there's likely a much smaller approach that also generalizes better.

Per the spec's binary single-value serialization, a bound's encoded width reflects the type it was written with (int = 4 bytes, long = 8), and int→long / float→double promotion is allowed without rewriting data. That means the decode side can absorb the mismatch directly: when decoding a long bound that is only 4 bytes, read an int and widen; likewise a double from 4 bytes reads a float. Roughly:

PrimitiveType::Long => {
    if bytes.len() == 4 {
        // evolved field: promoted int -> long
        PrimitiveLiteral::Long(i32::from_le_bytes(bytes.try_into()?) as i64)
    } else {
        PrimitiveLiteral::Long(i64::from_le_bytes(bytes.try_into()?))
    }
}
// Double: 4 bytes -> f32 as f64, else f64

This PR already carries the equivalent read-side logic in decode_bound. The key observation is that once the decoder is width-tolerant, the rewrite-time re-encoding is no longer needed: a rewritten manifest can keep the original 4-byte bounds and still be read correctly against the re-embedded (evolved) schema, since bounds are optional pruning statistics whose width the reader can always widen. That would let the PR drop ManifestFieldTypeIndex, promote_bound's re-serialization, and the _with_field_types / promote_bounds_ additions — i.e. most of the new public surface — while the writer simply copies bounds through unchanged.

Minimal shape: make the bound decoder width-tolerant (the decode_bound core you already have), and leave the writer path copying bounds as-is.

Two smaller notes:

• date → timestamp / nanosecond handling is different in kind and probably out of scope here. Unlike int→long/float→double (a width widening the decoder can absorb), converting a date bound to a timestamp is a value transformation (days × micros_per_day), so it can't come from width-tolerant decoding and isn't part of the standard promotion set. I'd split it into its own PR with an explicit spec reference for the promotion, so the widening fix stays small and self-evidently correct.
• into_value_map errors on complex field ids while it continues past removed ones. Given the docstring treats bounds as "optional pruning statistics … safe to discard," skipping a complex-typed id rather than erroring would be more consistent and removes an error path.

[osipovartem]

Thanks for the review. I reworked #410 around width-tolerant read-side decoding and removed the explicit rewrite-time bound promotion, date/timestamp conversion, and the unrelated DataFusion statistics changes. Complex/removed field IDs are now skipped.

One detail required more than just decode_bound: a manifest embeds its write-time schema, while a scan can use a newer (or historical) schema. The scan path now passes its selected schema to the reader; partition decoding still uses the embedded schema. The reader builds the field-ID lookup once per manifest, which is why the internal doc-hidden index remains instead of rescanning a potentially wide schema for every bound.

The first rewrite of an old manifest preserves its 4-byte numeric bounds. On a second rewrite, the typed entry has LONG/DOUBLE values and normal serialization may emit 8-byte bounds. The values remain valid and width-tolerant decoding handles both, but this is not byte-for-byte preservation through arbitrary rewrites. Do you require that stronger property? It would need a raw-bound sidecar or a raw Avro rewrite path; I left it out of this focused change pending your preference.

I added an Avro regression for old-manifest/current-schema reads and the first rewrite, and verified the local spec, core, and DataFusion test suites plus clippy.

[JanKaul]

Alternative: width-tolerant decode + promotion at the statistics boundary

The three requirements this PR's tests pin down are exactly right — (1) bounds must surface as the promoted type so pruning works against the scan schema, (2) historical snapshots must keep their own types, (3) a rewrite must not corrupt bounds for either reader. I'd like to propose a smaller architecture that meets all three without threading a schema into the manifest reader.

Core idea: keep decoding bounds against the manifest's embedded schema (unchanged reader), make that decode width-tolerant, and apply promotion at the two places that already hold the scan/snapshot schema — the DataFusion statistics conversion. Values then stay typed at their stored width everywhere in between, so re-serialization round-trips bytes exactly and the rewrite path needs no special mode.

Changes (4 files, roughly 45 lines of implementation):

1. iceberg-rust-spec/src/spec/manifest.rs — replace Value::try_from_bytes in into_value_map with a width-tolerant decode_bound (this is the same core your decode_bound already has):

   fn decode_bound(bytes: &[u8], data_type: &Type) -> Result<Value, Error> {
       match (data_type, bytes.len()) {
           (Type::Primitive(PrimitiveType::Long), 4) => Ok(Value::LongInt(i64::from(
               i32::from_le_bytes(bytes.try_into()?),
           ))),
           (Type::Primitive(PrimitiveType::Double), 4) => Ok(Value::Double(
               ordered_float::OrderedFloat(f64::from(f32::from_le_bytes(bytes.try_into()?))),
           )),
           _ => Value::try_from_bytes(bytes, data_type),
       }
   }

   This alone fixes the crash: a manifest whose re-embedded schema says long over 4-byte bounds decodes fine (spec: promotion is metadata-only, so narrow-width values under a wide type are expected).

2. iceberg-rust-spec/src/spec/values.rs — add the missing Float→Double arm to the existing Value::cast (it already has Int→Long for partition values). One existing test asserts Float→Double errors; it needs the same carve-out the Int cast test already has for its legal promotions.

3. datafusion_iceberg/src/statistics.rs — first line of convert_value_to_scalar_value: let value = value.cast(field_type)?;. The function already receives the field type from the scan schema, so an Int bound under a promoted long column becomes ScalarValue::Int64 — statistics now match the scan schema (requirement 1).

4. datafusion_iceberg/src/pruning_statistics.rs — in PruneDataFiles::{min,max}_values, cast the bound to field.field_type before into_any() (the field lookup is already there); drop the bound on cast failure — a missing statistic just disables pruning for that container.

Why the three requirements still hold:

• Promotion-aware pruning: the consumer casts to the scan schema's type (3, 4).
• Time travel: table_scan already selects the snapshot schema; the same cast then targets the snapshot-era type, so an int-era read stays Int. The manifest reader needs no schema parameter — the decision lives where the schema is already known.
• Rewrite safety: values decode at stored width (Int(42), 4 bytes) and re-serialize at stored width, byte-for-byte, with no width-preserving reader mode — it's the only representable behavior.

What this would remove from the PR: ManifestFieldTypeIndex as public API, the three try_from_vN_with_field_types variants, ManifestReader::new_for_rewrite / the Option<&Schema> split, datafiles_with_schema, and the Arc<Schema> threading through datafiles() and its call sites — the reader and all conversion signatures stay as on main.

One corner to be aware of: a manifest that already carries the wide-schema/narrow-bytes mismatch decodes wide (LongInt) and re-serializes wide on its next rewrite — self-consistent going forward. A time-travel read through that rewritten manifest against the int-era schema then hits a Long→Int cast failure at the consumer and drops that one bound (pruning disabled for that container, results still correct). If exact bounds under that double-corner matter, a checked narrowing arm in cast covers it — the value provably originated as an int.

Suggested test coverage (largely a relocation of what this PR already tests): narrow-under-promoted-schema decode incl. nested-skip and unknown-id skip; unknown-width rejection; stored-width round-trip in both directions; one serialized entry decoded via the old schema → Int(42) and via the promoted schema → LongInt(42); consumer conversion Int + long field → Int64.

Separately valuable from this PR regardless of direction: the nested-field-id index (top-level StructType::get still skips nested bounds — worth keeping as an internal improvement), and the writer-path error-propagation cleanup (filter_map(Result::ok) → proper ?).

[osipovartem]

Thanks for the simpler boundary-based approach. I replaced the PR with that design in 0e86a57: manifests decode against their embedded schema, and DataFusion promotes bounds when consuming statistics/pruning. Independent review also identified mixed partition specs and dropped historical partition source fields, so the patch conservatively disables only incompatible summaries rather than risking false pruning or failed scans. Focused tests and Rust 1.95 Clippy pass; the PR description has the exact results.

[JanKaul]

Looks great, thanks a lot!