Skip to content

Conversation

@jimexist
Copy link
Member

@jimexist jimexist commented Jun 6, 2021

Which issue does this PR close?

to make way for #360 I'll need the combine batches fn, so i'll refactor the current sort exec stream and add some integration test by the way

this pull request is used by #516

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

@codecov-commenter
Copy link

codecov-commenter commented Jun 6, 2021

Codecov Report

Merging #515 (1c0dea8) into master (b84789a) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #515      +/-   ##
==========================================
+ Coverage   76.09%   76.11%   +0.02%     
==========================================
  Files         155      155              
  Lines       26575    26602      +27     
==========================================
+ Hits        20221    20249      +28     
+ Misses       6354     6353       -1     
Impacted Files Coverage Δ
datafusion/src/physical_plan/common.rs 91.66% <100.00%> (+7.45%) ⬆️
datafusion/src/physical_plan/sort.rs 91.95% <100.00%> (+0.21%) ⬆️

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 b84789a...1c0dea8. Read the comment docs.

@jimexist jimexist force-pushed the refactor-combine-batches branch 2 times, most recently from 49ddfd3 to a0430e1 Compare June 6, 2021 05:40
Copy link
Contributor

@Dandandan Dandandan left a comment

Choose a reason for hiding this comment

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

👍

let result = combined
.map(|batch| sort_batch(batch, schema, &expr))
.transpose()?;
sort_time.add(now.elapsed().as_nanos() as usize);
Copy link
Member Author

Choose a reason for hiding this comment

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

Note here the semantic is changed because on err it would no longer log a metric which shall be the correct behavior

@jimexist jimexist force-pushed the refactor-combine-batches branch from a0430e1 to 248ba2f Compare June 7, 2021 03:21
@jimexist jimexist force-pushed the refactor-combine-batches branch from 248ba2f to de4055b Compare June 7, 2021 12:00
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 @jimexist !

@alamb alamb merged commit 5773a03 into apache:master Jun 7, 2021
@jimexist jimexist deleted the refactor-combine-batches branch June 7, 2021 16:24
@houqp houqp added bug Something isn't working datafusion labels Jul 30, 2021
H0TB0X420 pushed a commit to H0TB0X420/datafusion that referenced this pull request Oct 7, 2025
* use DataFusion 32

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

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants