mbutrovich commented on code in PR #3424:
URL: https://github.com/apache/iceberg-rust/pull/3424#discussion_r4395046068
##########
crates/iceberg/src/scan/task.rs:
##########
@@ -760,6 +826,106 @@ impl From<FileScanTaskDeleteFile> for
Result<FileScanTaskDeleteFile> {
}
}
+/// Column statistics of a [`FileScanTask`]'s data file, keyed by field id and
limited to the
+/// columns requested for the scan.
+///
+/// A requested column missing from a map has unknown statistics of that kind,
for example when
+/// the table's metrics mode records no bounds. Bounds are inclusive even when
truncated, and are
+/// typed with the schema the manifest was written with, so after type
promotion an `int` bound
Review Comment:
> This promises that after type promotion an `int` bound can describe a
`long` column, which is a useful thing for callers to know, but no test pins it.
Should planning convert the retained bounds to the scan schema's type
instead, so that one scan returns one type per field id?
As I read it, a bound's type depends on which manifest holds the entry, not
only on when the data file was written. A file listed in a manifest written
before `y` was promoted gets `Datum::int`. A file listed in a manifest written
after the promotion gets `Datum::long`. Java's merge append writes old entries
into such manifests:
[`ManifestMergeManager`](https://github.com/apache/iceberg/blob/6275107e38cc87a7d416ef231019ceb51556f196/core/src/main/java/org/apache/iceberg/ManifestMergeManager.java#L195-L214)
copies them with `writer.existing(entry)` into a writer whose spec is [rebound
to the current
schema](https://github.com/apache/iceberg/blob/6275107e38cc87a7d416ef231019ceb51556f196/core/src/main/java/org/apache/iceberg/TableMetadata.java#L1105-L1107),
and the bound bytes stay 4 bytes. The type changes on read because
[`Datum::try_from_bytes`](https://github.com/apache/iceberg-rust/blob/de0a6ac8471da43f63382d66fac27d7e56fd273e/crates/iceberg/src/spec/values/datum.rs#L401-L408)
wi
dens 4-byte values when the manifest schema says `long`. So one scan can hand
out both types for the same column. [`Datum`'s
`PartialOrd`](https://github.com/apache/iceberg-rust/blob/de0a6ac8471da43f63382d66fac27d7e56fd273e/crates/iceberg/src/spec/values/datum.rs#L178-L294)
has no cross-type arms, so comparing them returns `None`. Ordering files by
their bounds is the comparison a sorted scan makes.
I checked this on fe1bf90 with a scratch test that writes two manifests, one
with `y: int` and one with `y: long`, and scans with
`with_column_stats(["y"])`. The lower bounds come back as `Datum { type: Int,
literal: Int(5) }` and `Datum { type: Long, literal: Long(6) }`, and
`partial_cmp` between them returns `None`.
The spec's [promotion
table](https://github.com/apache/iceberg/blob/6275107e38cc87a7d416ef231019ceb51556f196/format/spec.md?plain=1#L405-L417)
describes decoding a bound against the current type. `Datum::to` already
converts `int` to `long` and `float` to `double`, and decimals of different
precision compare correctly today. `date` to `timestamp` (v3) would need its
own conversion, or the bound could be dropped, since a missing bound already
means unknown. If keeping the manifest's type is intended, what do you think
about adding the two-manifest case to
`test_plan_files_keeps_bounds_typed_with_manifest_schema` and saying in the
docs that callers must convert bounds before comparing them across tasks?
--
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]
