xanderbailey commented on PR #3066: URL: https://github.com/apache/iceberg-rust/pull/3066#issuecomment-5840489111
Thanks for the review! Comments are addressed. I've chosen to leave off the `debug_assert` on `start_from`. The function only has `base` to check against, and `start_from > base.highest_field_id()` doesn't actually catch much I don't think. If the table's ids ran to 5 but the current schema only holds 1 and 3, a caller seeding from `base.highest_field_id() + 1 = 4` passes the assert and still hands a new column the dropped id 4. The real bound is `last_column_id`, which isn't in scope here. Instead I've documented the contract on the function and added `test_assign_fresh_ids_does_not_reuse_dropped_column_ids` to pin the behaviour, and I'll enforce the seed at the #3056 call site where `last_column_id` is available. Happy to add the assert as well if you'd still rather have it. Just wanted to note that it's a lower bound I think. I'll file a follow-up issue for factoring the three walkers together rather than growing this 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]
