edgarRd commented on PR #17157: URL: https://github.com/apache/iceberg/pull/17157#issuecomment-5642151484
> Thanks for your patience @edgarRd , the fix looks right to me. I do think at some point we need to refactor the tests a bit so we can better test the "conversion" part of ParquetConversions in a more general way vs the Filtering. The change does that by introducing 2 new test classes but we do have TestMetricsRowGroupFiltering already, and it'd be nice to see if we just need to change that to generalize a bit more. Maybe the ParquetConversions tests should standalone and test the matrix of data conversions. > > Either way, nothing blocking , I'll go ahead and merge. I agree on the test refactor as a follow-up. I’ll look into it and see if I can put together a separate PR with an approach that makes sense. Thanks for the review, @amogh-jahagirdar! -- 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]
