mbutrovich commented on code in PR #3224:
URL: https://github.com/apache/iceberg-rust/pull/3224#discussion_r4259833708
##########
crates/iceberg/src/spec/manifest_list/mod.rs:
##########
@@ -59,25 +58,22 @@ pub struct ManifestList {
impl ManifestList {
/// Parse manifest list from bytes.
pub fn parse_with_version(bs: &[u8], version: FormatVersion) ->
Result<ManifestList> {
- match version {
- FormatVersion::V1 => {
- let reader = Reader::builder(bs)
- .reader_schema(&MANIFEST_LIST_AVRO_SCHEMA_V1)
- .build()?;
- let values =
Value::Array(reader.collect::<std::result::Result<Vec<Value>, _>>()?);
- from_value::<_serde::ManifestListV1>(&values)?.try_into()
- }
- FormatVersion::V2 => {
- let reader = Reader::new(bs)?;
- let values =
Value::Array(reader.collect::<std::result::Result<Vec<Value>, _>>()?);
- from_value::<_serde::ManifestListV2>(&values)?.try_into()
- }
- FormatVersion::V3 => {
- let reader = Reader::new(bs)?;
- let values =
Value::Array(reader.collect::<std::result::Result<Vec<Value>, _>>()?);
- from_value::<_serde::ManifestListV3>(&values)?.try_into()
- }
- }
+ let reader = Reader::new(bs)?;
+ let entries = match version {
+ FormatVersion::V1 => reader
+ .into_deser_iter::<Resolved<_serde::ManifestFileV1>>()
+ .map(|entry| entry?.0.try_into())
+ .collect::<Result<Vec<_>>>()?,
Review Comment:
Java writes V1 manifest lists with the field names `added_files_count`,
`existing_files_count` and `deleted_files_count` (see
[`V1Metadata.MANIFEST_LIST_SCHEMA`](https://github.com/apache/iceberg/blob/db43b88a6c0d470bd4f8acf08a18328750aeed20/core/src/main/java/org/apache/iceberg/V1Metadata.java#L32-L45)
and
[`ManifestFile`](https://github.com/apache/iceberg/blob/db43b88a6c0d470bd4f8acf08a18328750aeed20/api/src/main/java/org/apache/iceberg/ManifestFile.java#L56-L61)),
which are the names in the [spec's manifest list
fields](https://github.com/apache/iceberg/blob/db43b88a6c0d470bd4f8acf08a18328750aeed20/format/spec.md?plain=1#L1023-L1025).
[`ManifestFileV1`](https://github.com/apache/iceberg-rust/blob/3db218ec660658ca33acfebcf849006d727065c9/crates/iceberg/src/spec/manifest_list/_serde.rs#L97-L110)
only knows the older `added_data_files_count` names and has no aliases. So for
a V1 list written by current Java, those three counts parse as `None`.
I checked this by writing a one-entry list with Java's V1 Avro schema and
parsing it as V1. `added_files_count` and `existing_files_count` come back
`None`, while `added_rows_count` (same name in both) comes back `Some(9)`.
`main` at `b154603a` returns the same thing, so this isn't from this PR.
Before, resolution against `MANIFEST_LIST_AVRO_SCHEMA_V1` filled
`added_data_files_count` with its null default, so serde never saw Java's
names. Now that V1 reads against the writer schema, the `#[serde(alias = ...)]`
approach `ManifestFileV2` already uses works for V1 too. Adding the three
aliases to `ManifestFileV1` returns `Some(3)` and `Some(0)` for that list.
The spec treats a null count as non-zero, so planning stays correct, but the
counts are lost. As I read
[`existing_manifest`](https://github.com/apache/iceberg-rust/blob/3db218ec660658ca33acfebcf849006d727065c9/crates/iceberg/src/transaction/append.rs#L126-L151),
a fast append on such a table copies these entries into the new manifest list,
so the nulls get written back. Since this is a separate bug, could you open an
issue for it? The fix plus a test with Java's V1 names would make a small
follow-up PR.
--
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]
