andygrove commented on PR #5724: URL: https://github.com/apache/datafusion-comet/pull/5724#issuecomment-5779115136
I caught up on everything since my last pass. Thanks for working through the earlier feedback. The Parquet 59 links, the zero-FPP guard, the unused max-bytes relaxation, the shared sizing table with the binary-search coverage, the explicit JVM wrapper and the overestimated-NDV test all look good. I've replied inline on the three things I think are still open: the name resolution on older runtimes, the folding opt-in and its docs, and the proto shape. A few other things: **Memory.** The native writers don't charge their buffers to any memory pool, and Bloom filters add a large up-front allocation per enabled column per open file. With fanout that multiplies by the number of open partitions, and folding doesn't return the memory because the vector keeps its capacity. That problem predates this PR and applies to both native writers, so I filed #6115 to charge write buffers to the shared off-heap pool. It doesn't need to be solved here. **Benchmarks.** On sunchao's benchmark request, I think a native write with filters on vs off at the default cap is enough, reporting write time and peak RSS. The pinned iceberg-rust scan doesn't load Bloom filters, so native-reader pruning numbers don't really apply to this PR. **Rebase and CI.** Please rebase. Right now the only conflict is in `CometIcebergWriteActionSuite.scala`. After that we need CI to run the Spark 3.4, 3.5 and 4.0 profiles, since no CI has run on any revision of this branch yet. -- 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]
