Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Replace &Option<T> with Option<&T> #5102

Merged
merged 4 commits into from
Jan 30, 2023
Merged

Replace &Option<T> with Option<&T> #5102

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
98ac06c
replace &Option<T> with Option<&T>
gaoxinge Jan 29, 2023
e117040
Merge remote-tracking branch 'origin/master' into issue4424
gaoxinge Jan 29, 2023
950c27b
replace Option<&usize> with Option<usize>
gaoxinge Jan 29, 2023
f3f65d2
fix clippy error
gaoxinge Jan 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions datafusion/core/src/physical_plan/metrics/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,8 +161,8 @@ impl Metric {
}

/// return a reference to the partition
pub fn partition(&self) -> &Option<usize> {
&self.partition
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`

}

Expand Down
2 changes: 1 addition & 1 deletion datafusion/physical-expr/src/aggregate/stats.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
// specific language governing permissions and limitations
// under the License.

/// Enum used for differenciating population and sample for statistical functions
/// Enum used for differentiating population and sample for statistical functions
#[derive(Debug, Clone, Copy)]
pub enum StatsType {
/// Population
Expand Down
4 changes: 2 additions & 2 deletions datafusion/physical-expr/src/expressions/case.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,8 +97,8 @@ impl CaseExpr {
}

/// Optional base expression that can be compared to literal values in the "when" expressions
pub fn expr(&self) -> &Option<Arc<dyn PhysicalExpr>> {
&self.expr
pub fn expr(&self) -> Option<&Arc<dyn PhysicalExpr>> {
self.expr.as_ref()
}

/// One or more when/then expressions
Expand Down
4 changes: 2 additions & 2 deletions datafusion/physical-expr/src/window/lead_lag.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,7 @@ impl PartitionEvaluator for WindowShiftEvaluator {
let dtype = array.data_type();
let idx = self.state.idx as i64 - self.shift_offset;
if idx < 0 || idx as usize >= array.len() {
get_default_value(&self.default_value, dtype)
get_default_value(self.default_value.as_ref(), dtype)
} else {
ScalarValue::try_from_array(array, idx as usize)
}
Expand All @@ -238,7 +238,7 @@ impl PartitionEvaluator for WindowShiftEvaluator {
}

fn get_default_value(
default_value: &Option<ScalarValue>,
default_value: Option<&ScalarValue>,
dtype: &DataType,
) -> Result<ScalarValue> {
if let Some(value) = default_value {
Expand Down
1 change: 0 additions & 1 deletion datafusion/proto/src/physical_plan/to_proto.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,6 @@ impl TryFrom<Arc<dyn PhysicalExpr>> for protobuf::PhysicalExprNode {
protobuf::PhysicalCaseNode {
expr: expr
.expr()
.as_ref()
.map(|exp| exp.clone().try_into().map(Box::new))
.transpose()?,
when_then_expr: expr
Expand Down