Skip to content

Conversation

@alamb
Copy link
Contributor

@alamb alamb commented Sep 15, 2023

Which issue does this PR close?

Closes #.

Rationale for this change

https://crates.io/crates/chrono/0.4.31 was released recently and it appears to deprecated timestamp_nanos. This results in local clippy failures for me


error: use of deprecated method `chrono::DateTime::<Tz>::timestamp_nanos`: use `timestamp_nanos_opt()` instead
   --> datafusion/physical-expr/src/datetime_expressions.rs:171:30
    |
171 |     let now_ts = Some(now_ts.timestamp_nanos());
    |                              ^^^^^^^^^^^^^^^
    |
    = note: `-D deprecated` implied by `-D warnings`

What changes are included in this PR?

Use the non-deprecated API

Are these changes tested?

by existing tests

Are there any user-facing changes?

@github-actions github-actions bot added physical-expr Changes to the physical-expr crates optimizer Optimizer rules core Core DataFusion crate labels Sep 15, 2023
Copy link
Contributor Author

@alamb alamb left a comment

Choose a reason for hiding this comment

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

I think timestamp_nanos() was deprecated as it can panic internally -- timestamp_nanos_opt can not panic (and returns None if the value can not be represented)

@alamb alamb changed the title Minir: Fix clippy Use timestamp_nanos_opt instead of deprecated timestamp_nanos Minor: Fix clippy Use timestamp_nanos_opt instead of deprecated timestamp_nanos Sep 15, 2023
@alamb alamb changed the title Minor: Fix clippy Use timestamp_nanos_opt instead of deprecated timestamp_nanos Minor: Fix clippy by switching to timestamp_nanos_opt instead of (deprecated) timestamp_nanos Sep 15, 2023
Self::EndTimestamp(timestamp) => timestamp
.value()
.map(|ts| ts.timestamp_nanos() as usize)
.and_then(|ts| ts.timestamp_nanos_opt())
Copy link
Contributor

@tustvold tustvold Sep 15, 2023

Choose a reason for hiding this comment

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

Is this correct, it will convert a panic into 0?

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 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

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 one niggle

@alamb
Copy link
Contributor Author

alamb commented Sep 15, 2023

in order to get CI back healthy, I am going to merge this PR in. If anyone has other opinions about the 0 vs panic case that @tustvold points out on #7572 (comment) let me know and I can change it

@alamb alamb merged commit 4055c00 into apache:main Sep 15, 2023
@alamb alamb deleted the alamb/clippy branch September 15, 2023 16:36
@alamb
Copy link
Contributor Author

alamb commented Sep 15, 2023

I think this needs a follow on to pin chrono: #7575

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core Core DataFusion crate optimizer Optimizer rules physical-expr Changes to the physical-expr crates

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants