Revanth14 commented on PR #2164: URL: https://github.com/apache/iceberg-go/pull/2164#issuecomment-6091485481
> The fix lands in the right place: forcing full `file_path` and `pos` bounds at the writer means the existing bounds resolver in the conflict validator starts working for v2 position deletes without touching the read path, and it lines up exactly with Java's `MetricsConfig.forPositionDelete` (file_path and pos full, everything else none, table props ignored). Diagnosis and layer both check out. > > The regression test doesn't quite reproduce #2141 yet. It sets `file_path`/`pos` to `none`, which proves the override wins but isn't the reported scenario. The bug was default `truncate(16)` clipping long paths, and since the override ignores table props entirely, none and truncate pass for the same reason. A sub-case with default (or no) metrics props would pin the filed bug directly, and a small passthrough test would catch a future change that applied the pos-delete override to every content type. > > Two things worth a follow-up rather than this PR: v2 position delete files still write `referenced_data_file=nil` (Java sets it for single-target delete files), so the bounds path is the only resolver we have, and a tracking issue would be good. And this quietly stops `write.metadata.metrics.*` from affecting position delete metrics, which matches Java but is a user-visible change worth a line in the PR description or release notes. > > Left the doc-comment and test nits inline. Thanks for the review! addressed them - **#2141 reproduction:** the test now runs a table-defaults case next to the `none` override, for both partition specs. With the fix reverted, the table-defaults subtests fail with the `file_path` bound truncated to 16 bytes. - **Passthrough:** `TestMetricsPropsFor` checks that data and equality delete files still use the table properties, and position delete files use the fixed config. - **`referenced_data_file`:** tracked in #2167. - **`write.metadata.metrics.*` change:** noted under "Behavior change" in the PR description. One note on Java parity: as far as I can see, Java core's `PositionDeleteWriter` doesn't set `referenced_data_file` on v2 position deletes, and `ContentFileUtil.referencedDataFile` falls back to equal `file_path` bounds, which is the lookup #2164 fixes. Java's writer also drops the `file_path`/`pos` bounds when a delete file covers more than one data file. This PR keeps them, which is still correct because the lookup only resolves when the bounds are equal. Happy to match Java there in a follow-up if you'd prefer. @laskoviymishka -- 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]
