Skip to content

Conversation

@seddonm1
Copy link
Contributor

After #9523 RepartitionExec is blocking to pull all data into memory before starting the stream which crashes on large sets.

@github-actions
Copy link

@codecov-io
Copy link

Codecov Report

Merging #9580 (7a7d95c) into master (5bea624) will increase coverage by 0.07%.
The diff coverage is 86.16%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9580      +/-   ##
==========================================
+ Coverage   82.25%   82.33%   +0.07%     
==========================================
  Files         244      245       +1     
  Lines       55685    56252     +567     
==========================================
+ Hits        45806    46315     +509     
- Misses       9879     9937      +58     
Impacted Files Coverage Δ
rust/arrow/src/alloc/types.rs 0.00% <0.00%> (ø)
rust/datafusion/src/logical_plan/expr.rs 81.56% <ø> (+0.42%) ⬆️
...ion-testing/src/bin/arrow-json-integration-test.rs 0.00% <0.00%> (ø)
rust/arrow/src/ipc/writer.rs 87.23% <50.00%> (-0.59%) ⬇️
...datafusion/src/physical_plan/string_expressions.rs 77.00% <82.23%> (+7.37%) ⬆️
rust/parquet/src/arrow/array_reader.rs 77.61% <91.30%> (-0.02%) ⬇️
rust/datafusion/src/physical_plan/functions.rs 85.52% <91.42%> (+11.69%) ⬆️
rust/arrow/src/alloc/mod.rs 92.68% <92.68%> (ø)
rust/arrow/src/array/array_list.rs 94.05% <100.00%> (ø)
rust/arrow/src/array/builder.rs 84.94% <100.00%> (+0.01%) ⬆️
... and 27 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9a9baf6...7a7d95c. Read the comment docs.

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @seddonm1 -- I thought more about this last night and I still don't understand what is happening and am not sure this fixes the root cause of your hang. I think it is likely more of a workaround as yield_now simply lets other things run that are later in the stack get a chance to run.

However, as this mostly reverts the code back to where it was prior to #9523 (comment) I would be ok merging it.

Is it easy to reproduce the error / hang you are seeing locally? I would love to mess around with it and see if I could help debug further.

@edrevo does this change reduce the memory usage for you? I think it might but I don't think this is the root cause.

@alamb
Copy link
Contributor

alamb commented Feb 26, 2021

Update: I think @edrevo has identified the root cause of the hang -- see #9523 (comment). So my plan is to merge this PR (as a revert of #9523) to unblock @edrevo and then we'll proceed with the root cause fix in parallel

fyi @andygrove

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants