search

apache / iceberg-rust

Apache Iceberg
https://rust.iceberg.apache.org/
Apache License 2.0
474 stars 97 forks source link

Empty snapshot ID should be `Null` instead of `-1` #352

Open Fokko opened 2 months ago

Fokko commented 2 months ago

This is an old bug from Java. Where the Snapshot was set to -1 instead of None:

https://github.com/apache/iceberg-rust/blob/aba620900e99423bbd3fed969618e67e58a03a7b/crates/iceberg/src/spec/table_metadata.rs#L44

From Java 1.5 and later this is fixed. For older version of Java, the current-snapshot-id is required. We solved this by setting a flag: https://github.com/apache/iceberg-python/pull/473

s-akhtar-baig commented 2 months ago

@Fokko, can you please assign this to me? Thanks!

Fokko commented 2 months ago

@s-akhtar-baig With pleasure 🙌

gupteaj commented 1 month ago

@Fokko , @liurenjie1024 what changes are we looking for ? Since rust is using option to hold snapshot id internally, but writing as -1 for V2 manifest files. Does it need similar changes ( some variable ) as iceberg-python or empty snapshot id as -1 looks valid for rust.

liurenjie1024 commented 1 month ago

I took a look the code and found that maybe we don't need to change anything since we already handled this case: https://github.com/apache/iceberg-rust/blob/6f8545618dbc666b7117870e08057960246d8812/crates/iceberg/src/spec/table_metadata.rs#L678

But it would be better to add some tests for it.