Skip to content

Commit

Permalink
chore: upgrade chrono and fix deprecation warnings (#2048)
Browse files Browse the repository at this point in the history
Bump chrono to 0.4.25 and fix warnings.
  • Loading branch information
eddyxu authored Mar 9, 2024
1 parent 855347d commit a6870c1
Show file tree
Hide file tree
Showing 6 changed files with 56 additions and 32 deletions.
5 changes: 4 additions & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,10 @@ half = { "version" = "=2.3.1", default-features = false, features = [
bytes = "1.4"
byteorder = "1.5"
clap = { version = "4", features = ["derive"] }
chrono = "0.4.23"
chrono = { version = "0.4.25", default-features = false, features = [
"std",
"now"
] }
criterion = { version = "0.5", features = ["async", "async_tokio"] }
datafusion = { version = "35.0.0", default-features = false, features = [
"regex_expressions",
Expand Down
4 changes: 2 additions & 2 deletions rust/lance-core/src/utils/testing.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
use crate::Result;
use async_trait::async_trait;
use bytes::Bytes;
use chrono::Duration;
use chrono::{Duration, TimeDelta};
use futures::stream::BoxStream;
use futures::{StreamExt, TryStreamExt};
use object_store::path::Path;
Expand Down Expand Up @@ -252,6 +252,6 @@ impl<'a> MockClock<'a> {
impl<'a> Drop for MockClock<'a> {
fn drop(&mut self) {
// Reset the clock to the epoch
mock_instant::MockClock::set_system_time(Duration::days(0).to_std().unwrap());
mock_instant::MockClock::set_system_time(TimeDelta::try_days(0).unwrap().to_std().unwrap());
}
}
4 changes: 2 additions & 2 deletions rust/lance-datagen/src/generator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1054,7 +1054,7 @@ pub mod array {
/// is a more common use pattern
pub fn rand_date32() -> Box<dyn ArrayGenerator> {
let now = chrono::Utc::now();
let one_year_ago = now - chrono::Duration::days(365);
let one_year_ago = now - chrono::TimeDelta::try_days(365).expect("TimeDelta try days");
rand_date32_in_range(one_year_ago, now)
}

Expand Down Expand Up @@ -1087,7 +1087,7 @@ pub mod array {
/// is a more common use pattern
pub fn rand_date64() -> Box<dyn ArrayGenerator> {
let now = chrono::Utc::now();
let one_year_ago = now - chrono::Duration::days(365);
let one_year_ago = now - chrono::TimeDelta::try_days(365).expect("TimeDelta try_days");
rand_date64_in_range(one_year_ago, now)
}

Expand Down
4 changes: 3 additions & 1 deletion rust/lance-table/src/format/manifest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,9 @@ impl Manifest {
let nanos = self.timestamp_nanos % 1_000_000_000;
let seconds = ((self.timestamp_nanos - nanos) / 1_000_000_000) as i64;
Utc.from_utc_datetime(
&NaiveDateTime::from_timestamp_opt(seconds, nanos as u32).unwrap_or(NaiveDateTime::MIN),
&DateTime::from_timestamp(seconds, nanos as u32)
.unwrap_or_default()
.naive_utc(),
)
}

Expand Down
64 changes: 41 additions & 23 deletions rust/lance/src/dataset/cleanup.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@
//! (which should only be done if the caller can guarantee there are no updates
//! happening at the same time)
use chrono::{DateTime, Duration, Utc};
use chrono::{DateTime, TimeDelta, Utc};
use futures::{stream, StreamExt, TryStreamExt};
use lance_core::{Error, Result};
use lance_io::object_store::ObjectStore;
Expand Down Expand Up @@ -217,7 +217,8 @@ impl<'a> CleanupTask<'a> {
inspection: CleanupInspection,
) -> Result<RemovalStats> {
let removal_stats = Mutex::new(RemovalStats::default());
let verification_threshold = utc_now() - Duration::days(UNVERIFIED_THRESHOLD_DAYS);
let verification_threshold = utc_now()
- TimeDelta::try_days(UNVERIFIED_THRESHOLD_DAYS).expect("TimeDelta::try_days");
let unreferenced_paths = self
.dataset
.object_store
Expand Down Expand Up @@ -455,7 +456,6 @@ mod tests {

use arrow::datatypes::{DataType, Field, Schema as ArrowSchema};
use arrow_array::{RecordBatchIterator, RecordBatchReader};
use chrono::Duration;
use lance_core::{
utils::testing::{MockClock, ProxyObjectStore, ProxyObjectStorePolicy},
Error, Result,
Expand Down Expand Up @@ -745,12 +745,14 @@ mod tests {
fixture.create_some_data().await.unwrap();
fixture.overwrite_some_data().await.unwrap();

fixture.clock.set_system_time(Duration::days(10));
fixture
.clock
.set_system_time(TimeDelta::try_days(10).unwrap());

let before_count = fixture.count_files().await.unwrap();

let removed = fixture
.run_cleanup(utc_now() - Duration::days(8))
.run_cleanup(utc_now() - TimeDelta::try_days(8).unwrap())
.await
.unwrap();

Expand Down Expand Up @@ -784,7 +786,9 @@ mod tests {
// remain if they are still referenced by newer manifests
let fixture = MockDatasetFixture::try_new().unwrap();
fixture.create_some_data().await.unwrap();
fixture.clock.set_system_time(Duration::days(10));
fixture
.clock
.set_system_time(TimeDelta::try_days(10).unwrap());
fixture.append_some_data().await.unwrap();
fixture.append_some_data().await.unwrap();

Expand All @@ -794,7 +798,7 @@ mod tests {
assert_eq!(before_count.num_data_files, 3);
assert_eq!(before_count.num_manifest_files, 4);

let before = utc_now() - Duration::days(7);
let before = utc_now() - TimeDelta::try_days(7).unwrap();
let removed = fixture.run_cleanup(before).await.unwrap();

let after_count = fixture.count_files().await.unwrap();
Expand All @@ -817,7 +821,9 @@ mod tests {
async fn cleanup_recent_verified_files() {
let fixture = MockDatasetFixture::try_new().unwrap();
fixture.create_some_data().await.unwrap();
fixture.clock.set_system_time(Duration::seconds(1));
fixture
.clock
.set_system_time(TimeDelta::try_seconds(1).unwrap());
fixture.overwrite_some_data().await.unwrap();

let before_count = fixture.count_files().await.unwrap();
Expand Down Expand Up @@ -854,9 +860,9 @@ mod tests {
assert!(fixture.append_some_data().await.is_err());

let age = if old_files {
Duration::days(UNVERIFIED_THRESHOLD_DAYS + 1)
TimeDelta::try_days(UNVERIFIED_THRESHOLD_DAYS + 1).unwrap()
} else {
Duration::days(UNVERIFIED_THRESHOLD_DAYS - 1)
TimeDelta::try_days(UNVERIFIED_THRESHOLD_DAYS - 1).unwrap()
};
fixture.clock.set_system_time(age);

Expand Down Expand Up @@ -896,7 +902,9 @@ mod tests {
let fixture = MockDatasetFixture::try_new().unwrap();
fixture.create_some_data().await.unwrap();
fixture.create_some_index().await.unwrap();
fixture.clock.set_system_time(Duration::days(10));
fixture
.clock
.set_system_time(TimeDelta::try_days(10).unwrap());
fixture.overwrite_some_data().await.unwrap();

let before_count = fixture.count_files().await.unwrap();
Expand All @@ -906,7 +914,7 @@ mod tests {
// Creating an index creates a new manifest so there are 4 total
assert_eq!(before_count.num_manifest_files, 4);

let before = utc_now() - Duration::days(8);
let before = utc_now() - TimeDelta::try_days(8).unwrap();
let removed = fixture.run_cleanup(before).await.unwrap();

let after_count = fixture.count_files().await.unwrap();
Expand Down Expand Up @@ -934,7 +942,9 @@ mod tests {
// This will keep some data from the appended file and should
// completely remove the first file
fixture.delete_data("filter_me < 20").await.unwrap();
fixture.clock.set_system_time(Duration::days(10));
fixture
.clock
.set_system_time(TimeDelta::try_days(10).unwrap());
fixture.overwrite_data(data_gen.batch(16)).await.unwrap();
// This will delete half of the last fragment
fixture.delete_data("filter_me >= 40").await.unwrap();
Expand All @@ -944,7 +954,7 @@ mod tests {
assert_eq!(before_count.num_delete_files, 2);
assert_eq!(before_count.num_manifest_files, 6);

let before = utc_now() - Duration::days(8);
let before = utc_now() - TimeDelta::try_days(8).unwrap();
let removed = fixture.run_cleanup(before).await.unwrap();

let after_count = fixture.count_files().await.unwrap();
Expand All @@ -970,12 +980,14 @@ mod tests {
// by any fragment. We need to make sure the cleanup routine
// doesn't over-zealously delete these
let fixture = MockDatasetFixture::try_new().unwrap();
fixture.clock.set_system_time(Duration::days(10));
fixture
.clock
.set_system_time(TimeDelta::try_days(10).unwrap());
fixture.create_some_data().await.unwrap();
fixture.create_some_index().await.unwrap();

let before_count = fixture.count_files().await.unwrap();
let before = utc_now() - Duration::days(8);
let before = utc_now() - TimeDelta::try_days(8).unwrap();
let removed = fixture.run_cleanup(before).await.unwrap();
assert_eq!(removed.old_versions, 0);
assert_eq!(removed.bytes_removed, 0);
Expand All @@ -994,7 +1006,9 @@ mod tests {
fixture.create_some_data().await.unwrap();
fixture.block_commits();
assert!(fixture.append_some_data().await.is_err());
fixture.clock.set_system_time(Duration::days(10));
fixture
.clock
.set_system_time(TimeDelta::try_days(10).unwrap());

let before_count = fixture.count_files().await.unwrap();
// This append will fail since the commit is blocked but it should have
Expand All @@ -1006,7 +1020,7 @@ mod tests {
// All of our manifests are newer than the threshold but temp files
// should still be deleted.
let removed = fixture
.run_cleanup(utc_now() - Duration::days(7))
.run_cleanup(utc_now() - TimeDelta::try_days(7).unwrap())
.await
.unwrap();

Expand All @@ -1031,15 +1045,17 @@ mod tests {
// but the cleanup routine has no way of detecting this. They should look
// just like an in-progress write.
let mut fixture = MockDatasetFixture::try_new().unwrap();
fixture.clock.set_system_time(Duration::days(10));
fixture
.clock
.set_system_time(TimeDelta::try_days(10).unwrap());
fixture.create_some_data().await.unwrap();
fixture.block_commits();
assert!(fixture.append_some_data().await.is_err());

let before_count = fixture.count_files().await.unwrap();

let removed = fixture
.run_cleanup(utc_now() - Duration::days(7))
.run_cleanup(utc_now() - TimeDelta::try_days(7).unwrap())
.await
.unwrap();

Expand All @@ -1056,7 +1072,9 @@ mod tests {
// prevent us from running cleanup again later.
let mut fixture = MockDatasetFixture::try_new().unwrap();
fixture.create_some_data().await.unwrap();
fixture.clock.set_system_time(Duration::days(10));
fixture
.clock
.set_system_time(TimeDelta::try_days(10).unwrap());
fixture.overwrite_some_data().await.unwrap();

// The delete operation should delete the first version and its
Expand All @@ -1069,7 +1087,7 @@ mod tests {
assert_eq!(before_count.num_manifest_files, 3);

assert!(fixture
.run_cleanup(utc_now() - Duration::days(7))
.run_cleanup(utc_now() - TimeDelta::try_days(7).unwrap())
.await
.is_err());

Expand All @@ -1085,7 +1103,7 @@ mod tests {
fixture.unblock_delete_manifest();

let removed = fixture
.run_cleanup(utc_now() - Duration::days(7))
.run_cleanup(utc_now() - TimeDelta::try_days(7).unwrap())
.await
.unwrap();

Expand Down
7 changes: 4 additions & 3 deletions rust/lance/src/utils/temporal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
//! unit tests. Anywhere in production code where we need to get the current time
//! we should use the below methods and types instead of the builtin methods and types
use chrono::{DateTime, NaiveDateTime, TimeZone, Utc};
use chrono::{DateTime, TimeZone, Utc};
#[cfg(test)]
use mock_instant::{SystemTime as NativeSystemTime, UNIX_EPOCH};

Expand All @@ -34,8 +34,9 @@ pub fn utc_now() -> DateTime<Utc> {
let now = SystemTime::now()
.duration_since(UNIX_EPOCH)
.expect("system time before Unix epoch");
let naive =
NaiveDateTime::from_timestamp_opt(now.as_secs() as i64, now.subsec_nanos()).unwrap();
let naive = DateTime::from_timestamp(now.as_secs() as i64, now.subsec_nanos())
.expect("DateTime::from_timestamp")
.naive_utc();
Utc.from_utc_datetime(&naive)
}

Expand Down

0 comments on commit a6870c1

Please sign in to comment.