fix: Executor memory overhead overriding

[LukMRVC]

Which issue does this PR close?

Closes #1460 .

Rationale for this change

Even when using unified memory, Comet would override spark.executor.memoryOverhead using inconsistent byte units.
This situation should not have happened in the first place.

What changes are included in this PR?

Fixes for

• shouldOverrideMemoryConf
• Adds result of getCometShuffleMemorySizeInMiB to executor memoryOverhead

How are these changes tested?

Added CometPluginsNonOverrideUnifiedModeSuite to test that spark.executor.memoryOverhead is not overridden, which failed in previous versions.

[andygrove]

Thanks @LukMRVC. Could you fix the code formatting issue?

There are instructions in https://datafusion.apache.org/comet/contributor-guide/development.html#submitting-a-pull-request

[LukMRVC]

Yes, it should be fixed by now.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 57.82%. Comparing base (f09f8af) to head (6115fcc).
Report is 56 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #1462      +/-   ##
============================================
+ Coverage     56.12%   57.82%   +1.69%     
- Complexity      976      985       +9     
============================================
  Files           119      122       +3     
  Lines         11743    12178     +435     
  Branches       2251     2298      +47     
============================================
+ Hits           6591     7042     +451     
+ Misses         4012     3958      -54     
- Partials       1140     1178      +38     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[wForget]

Thank you for finding this issue, which was introduced in my previous PR #1379

[wForget]

getCometShuffleMemorySizeInMiB is only used for shuffle memory, we should also consider using getCometMemoryOverheadInMiB when unified memory manager is disabled

[wForget]

Thank you for finding this issue, which was introduced in my previous PR #1379

My initial proposal was:

• When unified memroy manager is disabled, we should use getCometMemoryOverheadInMiB override spark executor memory overhead.
• When unified memroy manager is enabled but shuffle unified memroy manager is disabled, we should use getCometShuffleMemorySizeInMiB override spark executor memory overhead.
• When unified memroy manager and shuffle unified memroy manager are enabled, we don't need to override spark executor memory overhead.

[LukMRVC]

Okay, @wForget I reverted some of my changes back to align with your proposal of overriding executor memory.

[andygrove]

PR builds are currently failing. This should be fixed by #1465

[wForget]

Okay, @wForget I reverted some of my changes back to align with your proposal of overriding executor memory.

Does newly added test case also need to be changed?

[LukMRVC]

That one should be fine, they passed with current changes to overriding spark executor memory, but fail without them.

[wForget]

That one should be fine, they passed with current changes to overriding spark executor memory, but fail without them.

Indeed, I successfully run this test locally, but this comment seems incorrect

[LukMRVC]

Yes, you are right. I overlooked it. I changed the comment.