Make Precision<usize> copy to make it clear clones are not expensive
#11828
Conversation
| @@ -91,10 +91,10 @@ pub async fn get_statistics_with_limit( | |||
| // counts across all the files in question. If any file does not | |||
| // provide any information or provides an inexact value, we demote | |||
| // the statistic precision to inexact. | |||
| num_rows = add_row_stats(file_stats.num_rows.clone(), num_rows); | |||
| num_rows = add_row_stats(&file_stats.num_rows, &num_rows); | |||
There was a problem hiding this comment.
Are we able to have Precision to be Copy when T is Copy? That would simplify things.
There was a problem hiding this comment.
THank you @Dandandan -- that was a great idea -- I did it in in 7b71ea5 and it worked great
|
Yes, I check the clone cost of |
alamb
force-pushed
the
alamb/maybe_cleaner
branch
from
62fb091
to
8cf84ab
Compare
alamb
changed the title
addo_row_stats is not slow
alamb
changed the title
addo_row_stats is not slowPrecision<usize> copy to make it clear clones are not expensive
| @@ -91,10 +91,10 @@ pub async fn get_statistics_with_limit( | |||
| // counts across all the files in question. If any file does not | |||
| // provide any information or provides an inexact value, we demote | |||
| // the statistic precision to inexact. | |||
| num_rows = add_row_stats(file_stats.num_rows.clone(), num_rows); | |||
| num_rows = add_row_stats(&file_stats.num_rows, &num_rows); | |||
There was a problem hiding this comment.
THank you @Dandandan -- that was a great idea -- I did it in in 7b71ea5 and it worked great
| @@ -25,7 +25,7 @@ use arrow_schema::Schema; | |||
|
|
|||
| /// Represents a value with a degree of certainty. `Precision` is used to | |||
| /// propagate information the precision of statistical values. | |||
| #[derive(Clone, PartialEq, Eq, Default)] | |||
| #[derive(Clone, PartialEq, Eq, Default, Copy)] | |||
There was a problem hiding this comment.
This change only affects the type when the templated type is Copy
So in other words this doesn't make it easy to accidentally copy Precision<ScalarValue> only copies when the underlying T also supports copy
I found https://users.rust-lang.org/t/copy-bound-on-type-parameters/58928 helpful
There was a problem hiding this comment.
BTW: You can always manually implement the Copy trait if you want to have fine grained control about the bounds:
impl<T> Copy for Precision<T> where T: Debug + Clone + PartialEq + Eq + PartialOrd + Clone {}There was a problem hiding this comment.
👍 -- I think that is basically what #[derive(Debug)] does in this case, which is kind of cool
| let precision: Precision<ScalarValue> = | ||
| Precision::Exact(ScalarValue::Int64(Some(42))); | ||
| // Clippy would complain about this if it were Copy | ||
| let p2 = precision.clone(); |
There was a problem hiding this comment.
here is my test to ensure Precision<ScalarValue> is not copy
| assert_eq!(exact_precision.clone().to_inexact(), inexact_precision); | ||
| assert_eq!(inexact_precision.clone().to_inexact(), inexact_precision); | ||
| assert_eq!(absent_precision.clone().to_inexact(), absent_precision); | ||
| assert_eq!(exact_precision.to_inexact(), inexact_precision); |
There was a problem hiding this comment.
Once I derived Copy, clippy lead me to all the various other changes in this PR
Which issue does this PR close?
Builds on #11802
Rationale for this change
While reviewing #11802 from @Rachelint I had to double check that a call to
clone()was not copying anything large (specifically aScalarValue) and it turns it that it is notWhat changes are included in this PR?
Move theclone()(of ausize) into a function where the type is explictAdd
#[Derive(Copy)]toPrecisionwhich results in only calls toPrecision<ScalarValue>needing acloneAre these changes tested?
By existing CI
Are there any user-facing changes?
Precisionis nowCopyin many places, so clippy may tell you you need to removeclone()