search

apache / datafusion-comet

Apache DataFusion Comet Spark Accelerator
https://datafusion.apache.org/comet
Apache License 2.0
826 stars 164 forks source link

fix: Fix memory bloat caused by holding too many unclosed `ArrowReaderIterator`s #929

Closed Kontinuation closed 2 months ago

Kontinuation commented 2 months ago

Which issue does this PR close?

Closes #927.

Rationale for this change

Please refer to the comments of #927 for details.

What changes are included in this PR?

How are these changes tested?

It is pretty hard to add tests for this fix, so we manually tested this and relying on existing tests to make sure that it does not break anything.

viirya commented 2 months ago

This looks good to me.

@Kontinuation Could you rebase this? I would like to make sure this change can pass CI after latest CI change in main branch. Thank you.

Kontinuation commented 2 months ago

This looks good to me.

@Kontinuation Could you rebase this? I would like to make sure this change can pass CI after latest CI change in main branch. Thank you.

Rebased. One of the tests failed because of failing to download dependencies from maven central, it should be transient.

codecov-commenter commented 2 months ago

Codecov Report

Attention: Patch coverage is 0% with 15 lines in your changes missing coverage. Please review.

Project coverage is 54.87%. Comparing base (033fe6f) to head (86aee79). Report is 15 commits behind head on main.

Files with missing lines Patch % Lines
.../comet/execution/shuffle/ArrowReaderIterator.scala 0.00% 11 Missing :warning:
...ecution/shuffle/CometBlockStoreShuffleReader.scala 0.00% 4 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #929 +/- ## ============================================= + Coverage 34.03% 54.87% +20.83% + Complexity 883 853 -30 ============================================= Files 113 109 -4 Lines 43170 10707 -32463 Branches 9516 2053 -7463 ============================================= - Hits 14693 5875 -8818 + Misses 25471 3798 -21673 + Partials 3006 1034 -1972 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

viirya commented 2 months ago

Rebased. One of the tests failed because of failing to download dependencies from maven central, it should be transient.

Thanks. I re-triggered the test. Usually it will pass after that.

viirya commented 2 months ago

Thanks @Kontinuation