Skip to content

Commit c225652

Browse files
fix: disable timestamp column for data skipping (delta-io#1003)
## What changes are proposed in this pull request? delta-kernel-rs assumes stats to be accurate as microseconds (like the timestamp type they are represented), but delta spec states that `add.stats` min/max values truncate strings/timestamps to milliseconds: > String columns are cut off at a fixed prefix length. Timestamp columns are truncated down to milliseconds While that's okay for min-stats, it's incorrect for max-stats: the file's _actual_ max-value is anywhere from 0-999µs larger than the recorded stats max-value. In the short-term we disable timestamp-based file skipping, and long term will explore a solution like adding 999us to our max stat (in follow-up issue). ## How was this change tested? New UT follow-up in delta-io#1002
1 parent 60d0944 commit c225652

2 files changed

Lines changed: 74 additions & 2 deletions

File tree

kernel/src/scan/data_skipping.rs

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -202,8 +202,13 @@ impl DataSkippingPredicateEvaluator for DataSkippingPredicateCreator {
202202
}
203203

204204
/// Retrieves the maximum value of a column, if it exists and has the requested type.
205-
fn get_max_stat(&self, col: &ColumnName, _data_type: &DataType) -> Option<Expr> {
206-
Some(joined_column_expr!("maxValues", col))
205+
// TODO(#1002): we currently don't support file skipping on timestamp columns' max stat since
206+
// they are truncated to milliseconds in add.stats.
207+
fn get_max_stat(&self, col: &ColumnName, data_type: &DataType) -> Option<Expr> {
208+
match data_type {
209+
&DataType::TIMESTAMP | &DataType::TIMESTAMP_NTZ => None,
210+
_ => Some(joined_column_expr!("maxValues", col)),
211+
}
207212
}
208213

209214
/// Retrieves the null count of a column, if it exists.

kernel/src/scan/data_skipping/tests.rs

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -339,3 +339,70 @@ fn test_sql_where() {
339339
do_test(ALL_NULL, pred, PRESENT, None, Some(false));
340340
do_test(ALL_NULL, pred, MISSING, None, None);
341341
}
342+
343+
// TODO(#1002): we currently don't support file skipping on timestamp columns' max stat since they
344+
// are truncated to milliseconds in add.stats.
345+
#[test]
346+
fn test_timestamp_skipping_disabled() {
347+
let creator = DataSkippingPredicateCreator;
348+
let col = &column_name!("timestamp_col");
349+
350+
assert!(
351+
creator.get_min_stat(col, &DataType::TIMESTAMP).is_some(),
352+
"get_min_stat should return Some: allow data skipping on timestamp minValues"
353+
);
354+
assert_eq!(
355+
creator.get_max_stat(col, &DataType::TIMESTAMP),
356+
None,
357+
"get_max_stat should return None: no data skipping on timestamp maxValues"
358+
);
359+
assert!(
360+
creator
361+
.get_min_stat(col, &DataType::TIMESTAMP_NTZ)
362+
.is_some(),
363+
"get_min_stat should return Some: allow data skipping on timestamp_ntz minValues"
364+
);
365+
assert_eq!(
366+
creator.get_max_stat(col, &DataType::TIMESTAMP_NTZ),
367+
None,
368+
"get_max_stat should return None: no data skipping on timestamp_ntz maxValues"
369+
);
370+
}
371+
372+
// TODO(#1002): we currently don't support file skipping on timestamp columns' max stat since they
373+
// are truncated to milliseconds in add.stats.
374+
#[test]
375+
fn test_timestamp_predicates_dont_data_skip() {
376+
let col = &column_expr!("ts_col");
377+
for timestamp in [&Scalar::Timestamp(1000000), &Scalar::TimestampNtz(1000000)] {
378+
// LT will do minValues -> OK
379+
let pred = Pred::lt(col.clone(), timestamp.clone());
380+
let skipping_pred = as_data_skipping_predicate(&pred);
381+
assert_eq!(
382+
skipping_pred.unwrap().to_string(),
383+
"Column(minValues.ts_col) < 1000000"
384+
);
385+
386+
// GT will do maxValues -> BLOCKED
387+
let pred = Pred::gt(col.clone(), timestamp.clone());
388+
let skipping_pred = as_data_skipping_predicate(&pred);
389+
assert!(
390+
skipping_pred.is_none(),
391+
"Expected no data skipping for timestamp predicate: {pred:#?}, got {skipping_pred:#?}"
392+
);
393+
394+
let pred = Pred::eq(col.clone(), timestamp.clone());
395+
let skipping_pred = as_data_skipping_predicate(&pred);
396+
assert_eq!(
397+
skipping_pred.unwrap().to_string(),
398+
"AND(NOT(Column(minValues.ts_col) > 1000000), null)"
399+
);
400+
401+
let pred = Pred::ne(col.clone(), timestamp.clone());
402+
let skipping_pred = as_data_skipping_predicate(&pred);
403+
assert_eq!(
404+
skipping_pred.unwrap().to_string(),
405+
"OR(NOT(Column(minValues.ts_col) = 1000000), null)"
406+
);
407+
}
408+
}

0 commit comments

Comments
 (0)