Skip to content

Optimize nth_value, remove first_value, last_value structs and use idiomatic rust style - #452

Merged
alamb merged 2 commits into
apache:masterfrom
jimexist:optimize-nth
Jun 1, 2021
Merged

Optimize nth_value, remove first_value, last_value structs and use idiomatic rust style#452
alamb merged 2 commits into
apache:masterfrom
jimexist:optimize-nth

Conversation

@jimexist

@jimexistjimexist commented May 31, 2021

Copy link
Copy Markdown
Member

Which issue does this PR close?

Closes#448 .

Rationale for this change

we can:

  1. reduce num of constructs in implementing nth_value
  2. further reuse code

What changes are included in this PR?

Are there any user-facing changes?

@jimexist
jimexist marked this pull request as draft May 31, 2021 01:52
@codecov-commenter

codecov-commenter commented May 31, 2021

Copy link
Copy Markdown

Codecov Report

Merging #452 (21b2be0) into master (c8ab5a4) will increase coverage by 0.06%.
The diff coverage is 96.87%.

Impacted file tree graph

@@ Coverage Diff @@## master #452 +/- ##
==========================================
+ Coverage 75.30% 75.36% +0.06% 
==========================================
Files 152 152 Lines 25275 25294 +19 ==========================================
+ Hits 19033 19063 +30 + Misses 6242 6231 -11 
Impacted FilesCoverage Δ
datafusion/src/physical_plan/expressions/mod.rs71.42% <ø> (ø)
...afusion/src/physical_plan/expressions/nth_value.rs90.47% <96.72%> (+19.70%)⬆️
datafusion/src/physical_plan/windows.rs87.12% <100.00%> (ø)

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 c8ab5a4...21b2be0. Read the comment docs.

@jimexist
jimexist marked this pull request as ready for review May 31, 2021 11:07
@jimexistjimexist changed the title Optimize nth_value, remove first_value, last_value structs and use idiomatic rust styleOptimize nth_value, remove first_value, last_value structs and use idiomatic rust styleMay 31, 2021
@jimexist

Copy link
Copy Markdown
MemberAuthor

@alamb as promised :-)

@alambalamb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The newly added tests are great. Thanks @jimexist -- and I think the code looks great too. 👍

fn test_i32_result(expr: Arc<NthValue>, expected: i32) -> Result<()> {
let arr: ArrayRef = Arc::new(Int32Array::from(vec![1, -2, 3, -4, 5, -6, 7, 8]));
let schema = Schema::new(vec![Field::new("arr", DataType::Int32, false)]);
let batch = RecordBatch::try_new(Arc::new(schema), vec![arr])?;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

FWIW for tests like this you can also use RecordBatch::try_from_iter to avoid having to construct the Schema directly.

This way is great too, I just figured I would point it out for the future

@alamb
alamb merged commit aab40f8 into apache:masterJun 1, 2021
@jimexist
jimexist deleted the optimize-nth branch June 1, 2021 23:49
HairstonE pushed a commit to HairstonE/datafusion that referenced this pull request Oct 7, 2025
Bumps [arduino/setup-protoc](https://github.com/arduino/setup-protoc) from 1 to 2.
- [Release notes](https://github.com/arduino/setup-protoc/releases)
- [Commits](arduino/setup-protoc@v1...v2)
---
updated-dependencies:
- dependency-name: arduino/setup-protoc
dependency-type: direct:production
update-type: version-update:semver-major
...
Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
HairstonE pushed a commit to HairstonE/datafusion that referenced this pull request Oct 7, 2025
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

use a more rust idiomatic way of handling nth_value

4 participants

@jimexist@codecov-commenter@alamb@houqp