twuebi commented on PR #1392: URL: https://github.com/apache/iceberg-go/pull/1392#issuecomment-4891921464
Thanks for picking up the follow-up from the code comment! I don't think the mixed state is reachable through spec-compliant metadata, though. The v3 spec ([Row Lineage](https://iceberg.apache.org/spec/#row-lineage)) requires that every data file on a committed v3 snapshot has an effective `first_row_id`: > Any null (unassigned) `first_row_id` must be assigned via inheritance, even if the data file is existing. This ensures that row IDs are assigned to existing data files in upgraded tables in the first commit after upgrading to v3. This repo implements that on both the write side (the v3 manifest-list writer assigns `first_row_id` to any data manifest lacking one) and the read side (inheritance during manifest reading), so `PlanFiles` always yields a non-nil `FirstRowID` on v3 — which is also why the new tests have to hand-mutate `tasks[1].FirstRowID = nil` to create the scenario. A mixed group would take a spec-violating writer, and for that the current conservative gate + warning feels like the safer behavior. Splitting also has a real cost: one group becomes two output streams, so we can end up with two undersized files instead of one target-size file. So my suggestion would be to instead update the stale "left as a follow-up" comment to note the mixed case is unreachable for spec-compliant metadata. Happy to be corrected if there's a real commit sequence that produces a mixed group! -- 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]
