gaborkaszab commented on PR #14351: URL: https://github.com/apache/iceberg/pull/14351#issuecomment-3435589938
I took another look and I'm still hesitant about the testing part: 1) I did an experiment where I removed the changes from `FileMetadata` and apparently the tests still succeed. In other words there is no test exercising the new code changes even with adding the new test dimension for `format-version`. 2) This PR is about a fix for path rewrite for DVs. Introducing a new suite-level dimension seems overkill for me. Not sure if I miss anything, but I think `format-version` only matters for V2 vs V3 positional deletes atm in terms of path rewriting. However, only a small fraction of the tests have in fact delete files (not to mention that these are the ones now that don't run with the new test dimension, so only test V2), so for most of the tests this new dimension doesn't add any extra value. Not sure about what machine people use for development, but for me originally this suite runs for 4min55sec, with the new dimension it grows to 12min17sec, hence I'd be careful what dimensions we add. If we say that this new parameter is relevant for V4 with the new structure of metadata, I get that, but it's out of scope for this PR in my opinion. My proposal is to identify the subset of tests where the format version is relevant now, practically the ones having delete files, and add test param for them, but not for the entire suite. Unless I miss something :) -- 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]
