Skip to content

Commit

Permalink
chore: fix a bunch of clippy lints and re-enable tests
Browse files Browse the repository at this point in the history
Many of these tests were did not get correctly re-enabled after the
subcrate changes
  • Loading branch information
rtyler committed Aug 14, 2024
1 parent c446b12 commit 7b9868a
Show file tree
Hide file tree
Showing 12 changed files with 136 additions and 160 deletions.
1 change: 0 additions & 1 deletion crates/core/src/kernel/arrow/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ pub(crate) mod json;
const MAP_ROOT_DEFAULT: &str = "key_value";
const MAP_KEY_DEFAULT: &str = "key";
const MAP_VALUE_DEFAULT: &str = "value";
const LIST_ROOT_DEFAULT: &str = "item";

macro_rules! arrow_map {
($fieldname: ident, null) => {
Expand Down
6 changes: 3 additions & 3 deletions crates/core/src/kernel/snapshot/log_segment.rs
Original file line number Diff line number Diff line change
Expand Up @@ -785,12 +785,12 @@ pub(super) mod tests {
for i in 0..10 {
actions.push(Action::Add(Add {
path: format!("part-{}.parquet", i),
modification_time: chrono::Utc::now().timestamp_millis() as i64,
modification_time: chrono::Utc::now().timestamp_millis(),
..Default::default()
}));
}

let log_store = DeltaTableBuilder::from_uri("memory:///".to_string())
let log_store = DeltaTableBuilder::from_uri("memory:///")
.build_storage()
.unwrap();
let op = DeltaOperation::Write {
Expand All @@ -809,7 +809,7 @@ pub(super) mod tests {
for i in 0..9 {
actions.push(Action::Remove(Remove {
path: format!("part-{}.parquet", i),
deletion_timestamp: Some(chrono::Utc::now().timestamp_millis() as i64),
deletion_timestamp: Some(chrono::Utc::now().timestamp_millis()),
..Default::default()
}))
}
Expand Down
15 changes: 6 additions & 9 deletions crates/core/src/operations/cdc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -112,10 +112,7 @@ mod tests {
.expect("Failed to make a table");
table.load().await.expect("Failed to reload table");
let result = should_write_cdc(table.snapshot().unwrap()).expect("Failed to use table");
assert!(
result == false,
"A default table should not create CDC files"
);
assert!(!result, "A default table should not create CDC files");
}

///
Expand All @@ -140,7 +137,7 @@ mod tests {

let result = should_write_cdc(table.snapshot().unwrap()).expect("Failed to use table");
assert!(
result == true,
result,
"A table with the EnableChangeDataFeed should create CDC files"
);
}
Expand All @@ -166,7 +163,7 @@ mod tests {

let result = should_write_cdc(table.snapshot().unwrap()).expect("Failed to use table");
assert!(
result == false,
!result,
"A v7 table must not write CDC files unless the writer feature is set"
);
}
Expand Down Expand Up @@ -308,8 +305,8 @@ mod tests {
],
)
.unwrap();
let _ = arrow::util::pretty::print_batches(&vec![batch.clone()]);
let _ = arrow::util::pretty::print_batches(&vec![updated_batch.clone()]);
let _ = arrow::util::pretty::print_batches(&[batch.clone()]);
let _ = arrow::util::pretty::print_batches(&[updated_batch.clone()]);

let ctx = SessionContext::new();
let before = ctx.read_batch(batch).expect("Failed to make DataFrame");
Expand Down Expand Up @@ -398,7 +395,7 @@ mod tests {
match tracker.collect() {
Ok(df) => {
let batches = &df.collect().await.unwrap();
let _ = arrow::util::pretty::print_batches(&batches);
let _ = arrow::util::pretty::print_batches(batches);
assert_eq!(batches.len(), 2);
assert_batches_sorted_eq! {[
"+-------+--------------------------+------------------+",
Expand Down
5 changes: 1 addition & 4 deletions crates/core/src/operations/load_cdf.rs
Original file line number Diff line number Diff line change
Expand Up @@ -695,10 +695,7 @@ pub(crate) mod tests {

let cdc_actions = version_actions
.iter()
.filter(|action| match action {
&&Action::Cdc(_) => true,
_ => false,
})
.filter(|action| matches!(action, &&Action::Cdc(_)))
.collect_vec();
assert!(cdc_actions.is_empty());
Ok(())
Expand Down
8 changes: 4 additions & 4 deletions crates/core/src/operations/merge/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2800,8 +2800,8 @@ mod tests {

let captured_expressions = placeholders.into_iter().map(|p| p.expr).collect_vec();

assert!(captured_expressions.contains(&&source_id));
assert!(captured_expressions.contains(&&source_id.is_null()));
assert!(captured_expressions.contains(&source_id));
assert!(captured_expressions.contains(&source_id.is_null()));

assert_eq!(generalized, expected_filter);
}
Expand Down Expand Up @@ -2837,13 +2837,13 @@ mod tests {
assert_eq!(generalized, expected_filter);

assert_eq!(placeholders.len(), 1);
let placeholder_expr = placeholders.get(0).unwrap();
let placeholder_expr = placeholders.first().unwrap();

let expected_placeholder = col(Column::new(source.clone().into(), "id")).neg();

assert_eq!(placeholder_expr.expr, expected_placeholder);
assert_eq!(placeholder_expr.alias, "id_0");
assert_eq!(placeholder_expr.is_aggregate, false);
assert!(!placeholder_expr.is_aggregate);
}

#[tokio::test]
Expand Down
2 changes: 1 addition & 1 deletion crates/core/src/operations/transaction/conflict_checker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -700,7 +700,7 @@ mod tests {
) -> Result<(), CommitConflictError> {
use crate::table::state::DeltaTableState;

let setup_actions = setup.unwrap_or_else(|| init_table_actions());
let setup_actions = setup.unwrap_or_else(init_table_actions);
let state = DeltaTableState::from_actions(setup_actions).unwrap();
let snapshot = state.snapshot();
let transaction_info = TransactionInfo::new(snapshot, reads, &actions, read_whole_table);
Expand Down
15 changes: 3 additions & 12 deletions crates/core/src/operations/write.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1973,10 +1973,7 @@ mod tests {

let cdc_actions = version_actions
.iter()
.filter(|action| match action {
&&Action::Cdc(_) => true,
_ => false,
})
.filter(|action| matches!(action, &&Action::Cdc(_)))
.collect_vec();
assert!(cdc_actions.is_empty());
Ok(())
Expand Down Expand Up @@ -2042,10 +2039,7 @@ mod tests {

let cdc_actions = version_actions
.iter()
.filter(|action| match action {
&&Action::Cdc(_) => true,
_ => false,
})
.filter(|action| matches!(action, &&Action::Cdc(_)))
.collect_vec();
assert!(cdc_actions.is_empty());
Ok(())
Expand Down Expand Up @@ -2147,10 +2141,7 @@ mod tests {

let cdc_actions = version_actions
.iter()
.filter(|action| match action {
&&Action::Cdc(_) => true,
_ => false,
})
.filter(|action| matches!(action, &&Action::Cdc(_)))
.collect_vec();
assert!(!cdc_actions.is_empty());
Ok(())
Expand Down
112 changes: 112 additions & 0 deletions crates/core/src/schema/partitions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -374,4 +374,116 @@ mod tests {
assert_eq!(partition.key, "ds");
assert_eq!(partition.value, Scalar::String("2024-04-01".into()));
}

#[test]
fn test_create_delta_table_partition() {
let year = "2021".to_string();
let path = format!("year={year}");
assert_eq!(
DeltaTablePartition::try_from(path.as_ref()).unwrap(),
DeltaTablePartition {
key: "year".into(),
value: Scalar::String(year.into()),
}
);

let _wrong_path = "year=2021/month=";
assert!(matches!(
DeltaTablePartition::try_from(_wrong_path).unwrap_err(),
DeltaTableError::PartitionError {
partition: _wrong_path
},
))
}

#[test]
fn test_match_partition() {
let partition_2021 = DeltaTablePartition {
key: "year".into(),
value: Scalar::String("2021".into()),
};
let partition_2020 = DeltaTablePartition {
key: "year".into(),
value: Scalar::String("2020".into()),
};
let partition_2019 = DeltaTablePartition {
key: "year".into(),
value: Scalar::String("2019".into()),
};

let partition_year_2020_filter = PartitionFilter {
key: "year".to_string(),
value: PartitionValue::Equal("2020".to_string()),
};
let partition_month_12_filter = PartitionFilter {
key: "month".to_string(),
value: PartitionValue::Equal("12".to_string()),
};
let string_type = DataType::Primitive(PrimitiveType::String);

assert!(!partition_year_2020_filter.match_partition(&partition_2021, &string_type));
assert!(partition_year_2020_filter.match_partition(&partition_2020, &string_type));
assert!(!partition_year_2020_filter.match_partition(&partition_2019, &string_type));
assert!(!partition_month_12_filter.match_partition(&partition_2019, &string_type));

/* TODO: To be re-enabled at a future date, needs some type futzing
let partition_2020_12_31_23_59_59 = DeltaTablePartition {
key: "time".into(),
value: PrimitiveType::TimestampNtz.parse_scalar("2020-12-31 23:59:59").expect("Failed to parse timestamp"),
};
let partition_time_2020_12_31_23_59_59_filter = PartitionFilter {
key: "time".to_string(),
value: PartitionValue::Equal("2020-12-31 23:59:59.000000".into()),
};
assert!(partition_time_2020_12_31_23_59_59_filter.match_partition(
&partition_2020_12_31_23_59_59,
&DataType::Primitive(PrimitiveType::TimestampNtz)
));
assert!(!partition_time_2020_12_31_23_59_59_filter
.match_partition(&partition_2020_12_31_23_59_59, &string_type));
*/
}

#[test]
fn test_match_filters() {
let partitions = vec![
DeltaTablePartition {
key: "year".into(),
value: Scalar::String("2021".into()),
},
DeltaTablePartition {
key: "month".into(),
value: Scalar::String("12".into()),
},
];

let string_type = DataType::Primitive(PrimitiveType::String);
let partition_data_types: HashMap<&String, &DataType> = vec![
(&partitions[0].key, &string_type),
(&partitions[1].key, &string_type),
]
.into_iter()
.collect();

let valid_filters = PartitionFilter {
key: "year".to_string(),
value: PartitionValue::Equal("2021".to_string()),
};

let valid_filter_month = PartitionFilter {
key: "month".to_string(),
value: PartitionValue::Equal("12".to_string()),
};

let invalid_filter = PartitionFilter {
key: "year".to_string(),
value: PartitionValue::Equal("2020".to_string()),
};

assert!(valid_filters.match_partitions(&partitions, &partition_data_types),);
assert!(valid_filter_month.match_partitions(&partitions, &partition_data_types),);
assert!(!invalid_filter.match_partitions(&partitions, &partition_data_types),);
}
}
2 changes: 1 addition & 1 deletion crates/core/tests/command_merge.rs
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ async fn test_merge_concurrent_conflict() {
let tmp_dir = tempfile::tempdir().unwrap();
let table_uri = tmp_dir.path().to_str().to_owned().unwrap();

let table_ref1 = create_table(&table_uri.to_string(), Some(vec!["event_date"])).await;
let table_ref1 = create_table(table_uri, Some(vec!["event_date"])).await;
let table_ref2 = open_table(table_uri).await.unwrap();
let (df1, _df2) = create_test_data();

Expand Down
10 changes: 5 additions & 5 deletions crates/core/tests/integration_checkpoint.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
#![cfg(feature = "integration_test")]

use chrono::Utc;
use deltalake_core::checkpoints::{cleanup_expired_logs_for, create_checkpoint};
use deltalake_core::kernel::{DataType, PrimitiveType};
Expand All @@ -14,6 +12,8 @@ use tokio::time::sleep;

#[tokio::test]
#[serial]
// This test requires refactoring and a revisit
#[ignore]
async fn cleanup_metadata_fs_test() -> TestResult {
let storage = Box::new(LocalStorageIntegration::default());
let context = IntegrationContext::new(storage)?;
Expand All @@ -34,19 +34,19 @@ async fn cleanup_metadata_test(context: &IntegrationContext) -> TestResult {

// we don't need to actually populate files with content as cleanup works only with file's metadata
object_store
.put(&log_path(0), bytes::Bytes::from("foo"))
.put(&log_path(0), bytes::Bytes::from("foo").into())
.await?;

// since we cannot alter s3 object metadata, we mimic it with pauses
// also we forced to use 2 seconds since Last-Modified is stored in seconds
std::thread::sleep(Duration::from_secs(2));
object_store
.put(&log_path(1), bytes::Bytes::from("foo"))
.put(&log_path(1), bytes::Bytes::from("foo").into())
.await?;

std::thread::sleep(Duration::from_secs(3));
object_store
.put(&log_path(2), bytes::Bytes::from("foo"))
.put(&log_path(2), bytes::Bytes::from("foo").into())
.await?;

let v0time = object_store.head(&log_path(0)).await?.last_modified;
Expand Down
Loading

0 comments on commit 7b9868a

Please sign in to comment.