Baunsgaard commented on PR #16882: URL: https://github.com/apache/iceberg/pull/16882#issuecomment-4776980654
Apologies for the silence here, and thanks for driving this forward @kevinjqliu and @laskoviymishka, I was offline for the last few days. First off: I'm not attached to this solution. Reverting #16740 is fine with me, and lowering SCALE from 400k to something smaller is a perfectly valid alternative that would deliver similar benefits with less machinery. So I'm happy to go either way on the direction. One point I want to make sure doesn't get lost, though: the ~8% number is the reduction in the *entire core job duration*, not the saving on the single tests. In other words, caching the input writes of just this one class shaved ~8% off the whole Spark core suite (see the per-version table in #16740: -10.0% / -7.5% / -4.6%). That reframes the tradeoff a bit. It's not "a lot of machinery for 8% of one test", it's "8% of the whole job from one class's input writes." @amogh-jahagirdar is right that it only helps step 2, and only within a single JVM fork. That narrow scope was deliberate, I kept it intra-fork specifically to minimize the risk of handing back incorrectly cached data. If we broadened it across JVM forks (or to a GitHub Actions cache), the gains would be larger, but so would the correctness risk, which is exactly the burden being called out. On the root cause: the reason step 2 is so expensive is that we materialize the input through the full Spark write path (createDataFrame, repartition, sortWithinPartitions, the parquet + metadata write, the commit). That's a lot of Spark machinery just to produce deterministic input data that the test doesn't actually care about producing via Spark. I've been profiling and have some WIP code that writes that input data with native Iceberg writers instead of going through the Spark ecosystem. Just on my Machine not in a PR | spark | native | delta | |---|---|---| | 732.5 s (12:12) | 519.6 s (8:40) | -213 s (-29%) | That's a ~29% reduction on the tests themselves, every test, no shared state, no cache key to keep correct, and it still exercises the real rewrite path under test (step 3) unchanged. If that holds up across more reps, it's a bigger and cleaner win than either the cache or lowering SCALE, and it directly addresses the correctness/maintainability concerns raised here. So my proposal: I'm fine reverting #16740 now to unblock this (I can see it is already done), and I'll follow up with the native-write approach (plus the input-write vs rewrite vs Spark-overhead split) -- 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]
