alamb commented on code in PR #11828: URL: https://github.com/apache/datafusion/pull/11828#discussion_r1709834664
########## datafusion/common/src/stats.rs: ########## @@ -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)] Review 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 ########## datafusion/common/src/stats.rs: ########## @@ -417,9 +417,9 @@ mod tests { let inexact_precision = Precision::Inexact(42); let absent_precision = Precision::<i32>::Absent; - 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); Review Comment: Once I derived `Copy`, clippy lead me to all the various other changes in this PR ########## datafusion/common/src/stats.rs: ########## @@ -459,4 +459,19 @@ mod tests { assert_eq!(precision2.multiply(&precision3), Precision::Inexact(15)); assert_eq!(precision1.multiply(&absent_precision), Precision::Absent); } + + #[test] + fn test_precision_cloning() { + // Precision<usize> is copy + let precision: Precision<usize> = Precision::Exact(42); + let p2 = precision; + assert_eq!(precision, p2); + + // Precision<ScalarValue> is not copy (requires .clone()) + let precision: Precision<ScalarValue> = + Precision::Exact(ScalarValue::Int64(Some(42))); + // Clippy would complain about this if it were Copy + let p2 = precision.clone(); Review Comment: here is my test to ensure `Precision<ScalarValue>` is not copy ########## datafusion/core/src/datasource/statistics.rs: ########## @@ -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); Review Comment: THank you @Dandandan -- that was a great idea -- I did it in in 7b71ea5af and it worked great -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
