[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #7572: Minor: Fix `clippy` by switching to `timestamp_nanos_opt` instead of (deprecated) `timestamp_nanos`

["alamb (via GitHub)" <gi...@apache.org>]

alamb commented on code in PR #7572:
URL: https://github.com/apache/arrow-datafusion/pull/7572#discussion_r1327512545


##########
datafusion/core/src/physical_plan/metrics/value.rs:
##########
@@ -430,11 +430,13 @@ impl MetricValue {
             Self::Time { time, .. } => time.value(),
             Self::StartTimestamp(timestamp) => timestamp
                 .value()
-                .map(|ts| ts.timestamp_nanos() as usize)
+                .and_then(|ts| ts.timestamp_nanos_opt())
+                .map(|nanos| nanos as usize)
                 .unwrap_or(0),
             Self::EndTimestamp(timestamp) => timestamp
                 .value()
-                .map(|ts| ts.timestamp_nanos() as usize)
+                .and_then(|ts| ts.timestamp_nanos_opt())

Review Comment:
   I don't expect that the metrics would produce timestamps that can't be converted to nanoseconds so it is somehwhat a theoretical difference. 
   
   I could see the argument being made to preserve the existing behavior (`panic` if somehow the values can't be represented as a timestamp) but I also think returning 0 rather than panic'ing in that case (what this PR does) is also reasonable



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org