ajiteshsingh commented on PR #2009: URL: https://github.com/apache/iceberg-go/pull/2009#issuecomment-5688998548
> Nice fix, this is tight and the scoping is exactly right. The reversal only touches TestRowGroup where raw Parquet stats first turn into bounds, and you left the data-file path alone since manifest bounds already come in big-endian. The synthetic-bytes test plus the real-Parquet-writer round-trip is a solid belt-and-suspenders setup, and asserting both a keep and a prune per bound is the right instinct. > > No blockers from me. The one thing I'd like before merge is the strictMetricsEval angle: the reversal lives only in inclusiveMetricsEval, and since strict shares the same bounds embedding, a future strict Parquet-stats path would silently reintroduce this exact bug, zero rows and no error. A small shared helper both sides call, or just a note on intBackedDecimal that any raw-Parquet-stats bound population has to run through it, would close that off. > > A few smaller things, all inline: the "Iceberg does not require the minimum number of bytes" comment reads backwards from the spec (the width-tolerance is a decoder property, not a relaxation), a both-negative test case would exercise the upper bound's sign-bit decode, and the bare type assertion in the real-Parquet test would read better as a require.True. > > Once the strict-path note is in, happy to merge. Thanks for the thorough review! Pulled the reversal into a `parquetStatsBounds` helper with a doc comment saying any raw-Parquet-stats bound population has to go through it, so a future strict or DV path can reuse it. Also reworded the min-width comment as a decoder property, added a both-negative case, switched to `require.True`, and made the real-Parquet assertions symmetric. -- 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]
