HeartSaVioR edited a comment on pull request #1487: URL: https://github.com/apache/iceberg/pull/1487#issuecomment-698617667
@rdblue Yes, but the rationalization looks to be a bit different. As I wrote in the PR title, the new test suite is intended to run E2E test and see whether Iceberg and Spark works correctly as we expected. It only uses Iceberg API on creating table and partition spec (because Spark 2.4 doesn't support DDL, probably also a good chance to go through DDL for Spark 3?), and for the remaining part we don't leverage anything on Iceberg internal. Due to the difference of rationalization there seems to be some differences on coverage: * TestFilteredScan only applies single partition criteria per test, while the new test suite apply all partition criteria on the table. New test suite can additionally check the Iceberg's benefit, partition pruning isn't affected by the order of partition criteria. * TestFilteredScan checks the tasks after planning input partitions, which is good to count how many data files are being read (if I understand correctly, and also assume planInputPartitions works as expected), but it doesn't provide the actual data files being read. The new test suite goes with hack to track the files being read, which is a bit more complicated, but is able to check the query doesn't read the data files outside of partition range. * TestFilteredScan has separate implementations between Spark 2.4 and 3 (I'm not sure whether it should be, or just a miss) where the new test suite simply works between two versions (with slight adjust on filter condition for timestamp filter pushdown). So while there're common parts between twos, there're also different parts (and rationalizations) between twos. I'm not sure which side to consolidate - I'm not sure we want to ensure Spark-Iceberg E2E query works perfectly in the Iceberg project side - that's up to the community's decision. If we would like to simply ensure Iceberg works as expected, probably just need to tweak a bit on TestFilteredScan to have `truncate`, and probably change the table partition spec to statically have all partitions for all tests instead of one partition per test. (My understanding is that tests should still work after the change. Do I understand correctly?) Once the decision has taken in this direction I might move the new test in our side, as I still need to ensure E2E query works nicely. Otherwise, probably we just need to add date partition on the new test suite and have a new test on date partition, and probably have tests for between on timestamp partitions to cover the tests on partitioned table in TestFilteredScan. Not sure we'd like to remove TestFilteredScan after enriching the new test suite, as it still has tests on unpartitioned table, and good to check whether the problem comes from Iceberg vs Spark - E2E test would fail on problems from any side. Please let me know what you think. Thanks! ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
