Skip to content

Conversation

@gaoxinge
Copy link
Contributor

Which issue does this PR close?

This pr partly fix #4424.

Rationale for this change

#4424

What changes are included in this PR?

Replace &Option<T> with Option<&T>.

Are these changes tested?

Pass the original test.

Are there any user-facing changes?

May change the api.

@github-actions github-actions bot added core Core DataFusion crate physical-expr Changes to the physical-expr crates labels Jan 29, 2023
@gaoxinge gaoxinge changed the title Issue4424 Replace &Option<T> with Option<&T> Jan 29, 2023
@jackwener jackwener requested a review from tustvold January 29, 2023 05:22
Copy link
Member

@jackwener jackwener left a comment

Choose a reason for hiding this comment

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

LGMT. Thanks you.

cc @tustvold.

Copy link
Contributor

@tustvold tustvold left a comment

Choose a reason for hiding this comment

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

Just a minor suggestion

Comment on lines 164 to 166
pub fn partition(&self) -> Option<&usize> {
self.partition.as_ref()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
pub fn partition(&self) -> Option<&usize> {
self.partition.as_ref()
}
pub fn partition(&self) -> Option<usize> {
self.partition.copied()
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I use .clone() instead of .copied() or .cloned(), because the compiler will raise error in last case:

error[E0599]: `Option<usize>` is not an iterator
   --> datafusion\core\src\physical_plan\metrics\mod.rs:165:24
    |
165 |         self.partition.copied()
    |                        ^^^^^^ `Option<usize>` is not an iterator
    |
   ::: C:\Users\gaoxinge\.rustup\toolchains\nightly-x86_64-pc-windows-gnu\lib/rustlib/src/rust\library\core\src\option.rs:518:1
    |
518 | pub enum Option<T> {
    | ------------------ doesn't satisfy `Option<usize>: Iterator`
    |
    = note: the following trait bounds were not satisfied:
            `Option<usize>: Iterator`
            which is required by `&mut Option<usize>: Iterator`

This error may refer to Strange .cloned() error message mentioning iterators.

Copy link
Contributor

Choose a reason for hiding this comment

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

Aah yes, got the various methods confused 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I remove the .clone() to pass the ci, because it will cause the clippy error:

error: using `clone` on type `std::option::Option<usize>` which implements the `Copy` trait
   --> datafusion/core/src/physical_plan/metrics/mod.rs:165:9
    |
165 |         self.partition.clone()
    |         ^^^^^^^^^^^^^^^^^^^^^^ help: try removing the `clone` call: `self.partition`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_copy
    = note: `-D clippy::clone-on-copy` implied by `-D warnings`

@tustvold tustvold merged commit 9c8bdfe into apache:master Jan 30, 2023
@tustvold
Copy link
Contributor

Thank you

@ursabot
Copy link

ursabot commented Jan 30, 2023

Benchmark runs are scheduled for baseline = 3133526 and contender = 9c8bdfe. 9c8bdfe is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

@gaoxinge gaoxinge deleted the issue4424 branch January 30, 2023 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core Core DataFusion crate physical-expr Changes to the physical-expr crates

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Replace &Option<T> with Option<&T>.

4 participants