refactor: change from serialising a u64 to i64

[JosephLenton]

Which issue does this PR close?

This is a part of the upgrade to Avro 0.22. We currently serialise the manifest first_row_id as a u64, and this is incorrect and out of spec.

From the Iceberg spec:

• first_row_id is defined as a long - see: https://iceberg.apache.org/spec/#manifest-lists
• a long is defined as a 64-bit signed integer - see: https://iceberg.apache.org/spec/#primitive-types

This is relevant to the Avro 0.22 upgrade because:

• in version 0.21, serialising a u64 as a long is allowed. It is silently coerced.

• in version 0.22, this fails and is rejected.

• Closes some of Upgrade to using apache-avro 0.22 #3063

I am making this change on it's own to help isolate changes (and to isolate discussion in case this is the wrong place, or changes elsewhere are needed).

What changes are included in this PR?

• Change from serialising first_row_id as a u64 to an i64.

Are these changes tested?

• Tests from main pass.
• I've also made this change on the Avro 0.22 branch I have, and the latest version works with this change.

AI Disclosure

The code changes and this PR comment is all made by a human.

Claude was used to debug the issue, and as a search. This included in code, and in spec.

[dannycjones]

Just noticed this too when reviewing some of the row lineage stuff! Thanks for the PR.

I think this change makes sense, although I think we should extend it to ManifestFile type too which should eliminate the TryInto calls.

[JosephLenton]

@dannycjones I can do that, however the PR will expand into a lot of places. The ManifestWriter interacts with many things, and they end up needing updating again or try_into everywhere.

For what it's worth I have a draft PR here where I moved everything across: #3068 . If that is preferable, I can close this PR and open that for review.

[dannycjones]

@dannycjones I can do that, however the PR will expand into a lot of places. The ManifestWriter interacts with many things, and they end up needing updating again or try_into everywhere.

For what it's worth I have a draft PR here where I moved everything across: #3068 . If that is preferable, I can close this PR and open that for review.

I think that's the right approach - to me, we just chose the wrong type here so we should move those things across.

[dannycjones]

@blackmwk and/or @CTTY, please can you check this is the right direction? I don't want to lead us on a larger change that we shouldn't make.

I see that there's a few types even for V2 (ManifestFile::existing_rows_count) that are unsigned in Rust, but in the spec are presumably signed assuming that its referring to Java types.

I am proposing to move from u64 to i64, which loses the guarantee in the library that these are positive (which they always should be) however the signed equivalent more closely represents what's described in the spec and serialized to Avro.