feat: Require offHeap memory to be enabled (always use unified memory)

apache / datafusion-comet

Apache DataFusion Comet Spark Accelerator

https://datafusion.apache.org/comet

Apache License 2.0

823 stars 163 forks source link

feat: Require offHeap memory to be enabled (always use unified memory) #1062

Closed andygrove closed 1 week ago

andygrove commented 2 weeks ago

Which issue does this PR close?

Closes #949 Closes #1017

Rationale for this change

What changes are included in this PR?

[x] Fall back to Spark if offHeap is disabled
[x] Update tuning guide

How are these changes tested?

parthchandra commented 2 weeks ago

Is https://github.com/apache/datafusion-comet/issues/875 a blocker for this PR

andygrove commented 2 weeks ago

Is #875 a blocker for this PR

I don't think so. I have been using the unified memory approach for my local benchmarking and have not encountered any issues.

andygrove commented 2 weeks ago

All tests are passing, but this is misleading because the Spark tests are running with spark.memory.offHeap.enabled=false, so they are not using Comet for execution.

viirya commented 2 weeks ago

~~Okay. At least we know Spark tests can pass under off-heap. It is a good news for us. We don't need to update Spark tests. In #1063, we can restore it to test Comet execution.~~

Oh, I misread it.

viirya commented 2 weeks ago

Yea, we can try to restore it in #1063. As I commented there, if Spark tests cannot pass under off-heap, we can have a test-only setup to run Spark tests with on-heap + Comet execution.