From efb4b6f870ec5cff7f20d5275655f581a7591f89 Mon Sep 17 00:00:00 2001 From: majidninja <143703501+majidninja@users.noreply.github.com> Date: Sat, 17 Oct 2026 10:10:43 +0300 Subject: [PATCH 01/16] feat(metrics): declare timeliness start/end pairs explicitly (#942) (#968) * feat(metrics): declare timeliness start/end pairs explicitly (#942) Timeliness paired columns only by role words in their names, so valid_from/valid_until, checkin/checkout or names in another language were never compared, and an ambiguous set had no way to say which pair was meant. temporal_pairs sits next to temporal_columns in Rust and Python. Declared pairs are compared as given; the other temporal columns still pair by name. An unknown column in a pair is reported like other unknown hint names. * feat(metrics): bind declared pair columns as temporal_pairs hints A pair over columns with no dates profiled fine and reported no pair checked. Each pair column now records a temporal_pair binding, so a pair proven inert raises naming temporal_pairs, as temporal_columns does. Regenerates the report schema for the new binding kind. --- .claude/skills/dataprof/reference/api.md | 5 +- crates/dataprof-core/src/semantic.rs | 59 ++++- crates/dataprof-db/src/lib.rs | 4 +- .../src/analysis/metrics/bounds.rs | 18 +- .../src/analysis/metrics/hint_binding.rs | 28 +- .../src/analysis/metrics/mod.rs | 21 +- .../src/analysis/metrics/timeliness.rs | 155 ++++++++++- .../src/record_batch_analyzer.rs | 21 +- crates/dataprof-python/src/config.rs | 28 ++ crates/dataprof-python/src/types.rs | 1 + crates/dataprof-runtime/src/hint_binding.rs | 14 +- crates/dataprof/src/profiler.rs | 12 + docs/guides/getting-started.md | 8 +- docs/python/README.md | 12 +- docs/schema/profile-report.v1.schema.json | 5 + python/dataprof/_api.py | 14 + python/dataprof/_api.pyi | 2 + python/dataprof/_dataprof.pyi | 3 + python/dataprof/_profiler.py | 5 + python/dataprof/_profiler.pyi | 1 + python/dataprof/_report.py | 10 +- python/tests/test_profiler_configuration.py | 1 + python/tests/test_python_api.py | 1 + .../tests/test_timeliness_declared_pairs.py | 200 ++++++++++++++ tests/timeliness_declared_pairs.rs | 243 ++++++++++++++++++ 25 files changed, 821 insertions(+), 50 deletions(-) create mode 100644 python/tests/test_timeliness_declared_pairs.py create mode 100644 tests/timeliness_declared_pairs.rs diff --git a/.claude/skills/dataprof/reference/api.md b/.claude/skills/dataprof/reference/api.md index 531f8d8b..56e759e6 100644 --- a/.claude/skills/dataprof/reference/api.md +++ b/.claude/skills/dataprof/reference/api.md @@ -37,7 +37,7 @@ Keyword arguments: `engine`, `chunk_size`, `memory_limit_mb`, `format`, `max_rows`, `name`, `csv_delimiter`, `csv_flexible`, `jsonl_on_error`, `sampling`, `stop_condition`, `on_progress`, `progress_interval_ms`, `quality_dimensions`, `metrics`, `locale`, `positive_columns`, -`identifier_columns`, `temporal_columns`. +`identifier_columns`, `temporal_columns`, `temporal_pairs`. ## Profiler builder (when keyword arguments are not enough) @@ -56,7 +56,8 @@ report = ( Every builder method returns the profiler, so calls chain. Other methods: `chunk_size`, `memory_limit_mb`, `format`, `name`, `csv_delimiter`, `csv_flexible`, `sampling`, `stop_condition`, `stop_when`, `on_progress`, -`progress_interval_ms`, `locale`, `positive_columns`, `temporal_columns`. +`progress_interval_ms`, `locale`, `positive_columns`, `temporal_columns`, +`temporal_pairs`. ## ProfileReport: execution metadata diff --git a/crates/dataprof-core/src/semantic.rs b/crates/dataprof-core/src/semantic.rs index ece6f83f..b547856f 100644 --- a/crates/dataprof-core/src/semantic.rs +++ b/crates/dataprof-core/src/semantic.rs @@ -12,6 +12,8 @@ pub enum SemanticHintKind { Identifier, /// A [`SemanticHints::temporal_columns`] entry. Temporal, + /// A column named by a [`SemanticHints::temporal_pairs`] entry. + TemporalPair, } impl SemanticHintKind { @@ -21,6 +23,7 @@ impl SemanticHintKind { Self::Positive => "positive_columns", Self::Identifier => "identifier_columns", Self::Temporal => "temporal_columns", + Self::TemporalPair => "temporal_pairs", } } } @@ -66,6 +69,8 @@ pub struct SemanticHints { pub positive_columns: Vec, pub identifier_columns: Vec, pub temporal_columns: Vec, + /// Start/end column pairs declared by the caller, compared as given. + pub temporal_pairs: Vec<(String, String)>, } impl SemanticHints { @@ -74,6 +79,7 @@ impl SemanticHints { positive_columns, identifier_columns, temporal_columns: Vec::new(), + temporal_pairs: Vec::new(), } } @@ -82,10 +88,18 @@ impl SemanticHints { self } + /// Declare start/end column pairs for timeliness to compare as given, + /// such as `valid_from`/`valid_until`, which no role word pairs by name. + pub fn with_temporal_pairs(mut self, temporal_pairs: Vec<(String, String)>) -> Self { + self.temporal_pairs = temporal_pairs; + self + } + pub fn is_empty(&self) -> bool { self.positive_columns.is_empty() && self.identifier_columns.is_empty() && self.temporal_columns.is_empty() + && self.temporal_pairs.is_empty() } pub fn is_identifier_column(&self, column_name: &str) -> bool { @@ -106,7 +120,21 @@ impl SemanticHints { .any(|candidate| candidate == column_name) } - /// Every `(kind, column_name)` pair across all three hint lists. + /// Each column named by a declared pair, once, in the order first named. + pub fn temporal_pair_columns(&self) -> Vec<&str> { + let mut columns: Vec<&str> = Vec::new(); + for (start, end) in &self.temporal_pairs { + for name in [start.as_str(), end.as_str()] { + if !columns.contains(&name) { + columns.push(name); + } + } + } + columns + } + + /// Every `(kind, column_name)` pair across the hint lists, with each column + /// of a declared pair listed once. pub fn iter(&self) -> impl Iterator { let positive = self .positive_columns @@ -120,7 +148,11 @@ impl SemanticHints { .temporal_columns .iter() .map(|c| (SemanticHintKind::Temporal, c.as_str())); - positive.chain(identifier).chain(temporal) + let pairs = self + .temporal_pair_columns() + .into_iter() + .map(|c| (SemanticHintKind::TemporalPair, c)); + positive.chain(identifier).chain(temporal).chain(pairs) } /// Fail loudly when a hint names a column that is not in the schema. @@ -129,6 +161,14 @@ impl SemanticHints { /// hint that cannot bind must not vanish: a typo'd or stale column name is a /// mistake the profiler should surface, not silently discard. pub fn validate_names(&self, column_names: &[&str]) -> Result<(), DataProfilerError> { + // A column compared with itself can never be out of order, so such a + // pair would report a clean check that never happened. + if let Some((name, _)) = self.temporal_pairs.iter().find(|(start, end)| start == end) { + return Err(DataProfilerError::InvalidSemanticHint { + message: format!("temporal_pairs names '{name}' as both start and end"), + suggestion: "Pair a start column with a different end column.".to_string(), + }); + } let mut unknown: Vec<(SemanticHintKind, &str)> = self .iter() .filter(|(_, name)| !column_names.contains(name)) @@ -167,14 +207,17 @@ impl SemanticHints { /// effective while producing neither metrics nor binding evidence. pub fn validate_quality_usage(&self, quality_computed: bool) -> Result<(), DataProfilerError> { if quality_computed - || (self.positive_columns.is_empty() && self.temporal_columns.is_empty()) + || (self.positive_columns.is_empty() + && self.temporal_columns.is_empty() + && self.temporal_pairs.is_empty()) { return Ok(()); } Err(DataProfilerError::InvalidSemanticHint { - message: "positive_columns and temporal_columns require the Quality metric pack" - .to_string(), + message: + "positive_columns, temporal_columns and temporal_pairs require the Quality metric pack" + .to_string(), suggestion: "Include MetricPack::Quality (\"quality\" in Python), or remove the value-driven hint. Identifier hints remain usable without quality because they affect column typing." .to_string(), }) @@ -210,9 +253,11 @@ impl SemanticHints { Err(DataProfilerError::InvalidSemanticHint { message: format!("semantic hint(s) bound to no matching value: {details}"), - suggestion: "positive_columns need numeric values; temporal_columns need date values. \ + suggestion: + "positive_columns need numeric values; temporal_columns and temporal_pairs \ + need date values. \ Fix the column choice or remove the hint." - .to_string(), + .to_string(), }) } } diff --git a/crates/dataprof-db/src/lib.rs b/crates/dataprof-db/src/lib.rs index 3a1de791..84275c8b 100644 --- a/crates/dataprof-db/src/lib.rs +++ b/crates/dataprof-db/src/lib.rs @@ -191,8 +191,8 @@ pub async fn analyze_database_with_options( // ignored. Checked before connecting so nothing is scanned first. if !options.semantic_hints().is_empty() { return Err(DataProfilerError::UnsupportedDataSource { - message: "positive_columns, identifier_columns, and temporal_columns are not \ - supported for database profiling yet" + message: "positive_columns, identifier_columns, temporal_columns, and temporal_pairs \ + are not supported for database profiling yet" .to_string(), }); } diff --git a/crates/dataprof-metrics/src/analysis/metrics/bounds.rs b/crates/dataprof-metrics/src/analysis/metrics/bounds.rs index b12e9404..667c6bec 100644 --- a/crates/dataprof-metrics/src/analysis/metrics/bounds.rs +++ b/crates/dataprof-metrics/src/analysis/metrics/bounds.rs @@ -121,7 +121,7 @@ pub(crate) fn score_bounds( timeliness, data, column_profiles, - &semantic_hints.positive_columns, + semantic_hints, temporal_columns, )?; @@ -420,10 +420,11 @@ fn collect_shares( timeliness: &TimelinessCalculator<'_>, data: &HashMap>, column_profiles: &[ColumnProfile], - positive_columns: &[String], + semantic_hints: &SemanticHints, temporal_columns: &[String], ) -> Result>>, DataProfilerError> { let wants = |dimension: QualityDimension| wanted.contains(&dimension); + let positive_columns = &semantic_hints.positive_columns; let accuracy = AccuracyCalculator::new(thresholds); let (mut types, mut formats, mut encodings) = (Vec::new(), Vec::new(), Vec::new()); let (mut outliers, mut violations) = (Vec::new(), Vec::new()); @@ -485,7 +486,7 @@ fn collect_shares( )); } if wants(QualityDimension::Timeliness) && temporal_columns.contains(&profile.name) { - let metrics = timeliness.calculate(&single, temporal_columns, column_profiles)?; + let metrics = timeliness.calculate(&single, temporal_columns, &[], column_profiles)?; stale.push(counts(metrics.stale_dates, metrics.valid_dates, 1)); // A value is either a date, which may be in the future, or invalid. date_failures.push(counts( @@ -510,7 +511,13 @@ fn collect_shares( let mut timeliness_shares = Some(vec![stale, date_failures]); if wants(QualityDimension::Timeliness) { - match temporal_pairs(timeliness, data, column_profiles, temporal_columns)? { + match temporal_pairs( + timeliness, + data, + column_profiles, + temporal_columns, + &semantic_hints.temporal_pairs, + )? { Pairs::None => {} Pairs::Aligned(pairs) => { if let Some(shares) = timeliness_shares.as_mut() { @@ -568,8 +575,9 @@ fn temporal_pairs( data: &HashMap>, column_profiles: &[ColumnProfile], temporal_columns: &[String], + declared_pairs: &[(String, String)], ) -> Result { - let metrics = calculator.calculate(data, temporal_columns, column_profiles)?; + let metrics = calculator.calculate(data, temporal_columns, declared_pairs, column_profiles)?; if metrics.temporal_pairs_checked == 0 { return Ok(Pairs::None); } diff --git a/crates/dataprof-metrics/src/analysis/metrics/hint_binding.rs b/crates/dataprof-metrics/src/analysis/metrics/hint_binding.rs index 5336eb42..c0128361 100644 --- a/crates/dataprof-metrics/src/analysis/metrics/hint_binding.rs +++ b/crates/dataprof-metrics/src/analysis/metrics/hint_binding.rs @@ -15,8 +15,8 @@ use dataprof_core::{SemanticHintBinding, SemanticHintKind, SemanticHints}; use super::utils::extract_year; use crate::analysis::inference::is_null_like_token; -/// Compute binding evidence for the value-driven hints (`positive_columns` and -/// `temporal_columns`) over `data`. +/// Compute binding evidence for the value-driven hints (`positive_columns`, +/// `temporal_columns` and the columns of `temporal_pairs`) over `data`. /// /// `exact` states whether `data` covers every row (`true`) or is a sample /// (`false`); it is copied onto each binding so a proven-inert hint can be told @@ -44,17 +44,19 @@ pub fn compute_value_hint_bindings( } } - for column in &hints.temporal_columns { + let temporal = hints + .temporal_columns + .iter() + .map(|column| (SemanticHintKind::Temporal, column.as_str())); + let pairs = hints + .temporal_pair_columns() + .into_iter() + .map(|column| (SemanticHintKind::TemporalPair, column)); + for (kind, column) in temporal.chain(pairs) { if let Some(values) = data.get(column) { // Mirror timeliness.rs: a value counts as a date if a year parses. - let (checked, matched) = count_matches(values, SemanticHintKind::Temporal); - bindings.push(binding( - column, - SemanticHintKind::Temporal, - checked, - matched, - exact, - )); + let (checked, matched) = count_matches(values, kind); + bindings.push(binding(column, kind, checked, matched, exact)); } } @@ -112,7 +114,9 @@ pub fn value_matches_hint(value: &str, kind: SemanticHintKind) -> bool { .parse::() .is_ok_and(|number| number.is_finite()), // Mirror timeliness.rs: it extracts a year from the original cell. - SemanticHintKind::Temporal => extract_year(value).is_some(), + SemanticHintKind::Temporal | SemanticHintKind::TemporalPair => { + extract_year(value).is_some() + } // Identifier binding is structural and is not value-driven. SemanticHintKind::Identifier => true, } diff --git a/crates/dataprof-metrics/src/analysis/metrics/mod.rs b/crates/dataprof-metrics/src/analysis/metrics/mod.rs index dcc4c03a..5d7f422b 100644 --- a/crates/dataprof-metrics/src/analysis/metrics/mod.rs +++ b/crates/dataprof-metrics/src/analysis/metrics/mod.rs @@ -147,14 +147,21 @@ impl MetricsCalculator { } } - /// Explicit temporal hints plus columns the profiler itself inferred as dates. - /// Hints come first; inferred columns are appended in stable profile order - /// and duplicates are removed. + /// Explicit temporal hints, then the columns of declared pairs, plus columns + /// the profiler itself inferred as dates. Hints come first; inferred columns + /// are appended in stable profile order and duplicates are removed. fn effective_temporal_columns( column_profiles: &[ColumnProfile], semantic_hints: &SemanticHints, ) -> Vec { let mut columns = semantic_hints.temporal_columns.clone(); + for (start, end) in &semantic_hints.temporal_pairs { + for name in [start, end] { + if !columns.contains(name) { + columns.push(name.clone()); + } + } + } for profile in column_profiles { if profile.data_type == DataType::Date && !columns.contains(&profile.name) { columns.push(profile.name.clone()); @@ -295,6 +302,7 @@ impl MetricsCalculator { let t = TimelinessCalculator::new(&self.thresholds).calculate( data, &temporal_columns, + &semantic_hints.temporal_pairs, column_profiles, )?; Some(TimelinessMetrics { @@ -613,7 +621,12 @@ impl MetricsCalculator { let timeliness = if Self::is_requested(requested, QualityDimension::Timeliness) { let t = if !data.is_empty() { - timeliness_calculator.calculate(data, &temporal_columns, column_profiles)? + timeliness_calculator.calculate( + data, + &temporal_columns, + &semantic_hints.temporal_pairs, + column_profiles, + )? } else { timeliness::TimelinessMetrics { future_dates_count: 0, diff --git a/crates/dataprof-metrics/src/analysis/metrics/timeliness.rs b/crates/dataprof-metrics/src/analysis/metrics/timeliness.rs index 3d0ea68e..4ca831f8 100644 --- a/crates/dataprof-metrics/src/analysis/metrics/timeliness.rs +++ b/crates/dataprof-metrics/src/analysis/metrics/timeliness.rs @@ -181,6 +181,7 @@ impl<'a> TimelinessCalculator<'a> { &self, data: &HashMap>, temporal_columns: &[String], + declared_pairs: &[(String, String)], column_profiles: &[ColumnProfile], ) -> Result { let summary = self.calculate_date_summary(data, temporal_columns); @@ -189,7 +190,12 @@ impl<'a> TimelinessCalculator<'a> { } else { (summary.stale_dates as f64 / summary.valid_dates as f64) * 100.0 }; - let ordering = Self::count_temporal_violations(data, temporal_columns, column_profiles)?; + let ordering = Self::count_temporal_violations( + data, + temporal_columns, + declared_pairs, + column_profiles, + )?; Ok(TimelinessMetrics { future_dates_count: summary.future_dates, @@ -282,9 +288,15 @@ impl<'a> TimelinessCalculator<'a> { /// left unpaired rather than guessed. Names that share nothing still pair /// when they are the only start and the only end, as /// `created_at`/`last_updated_at` are. + /// + /// Declared pairs come first and are compared as given (#942). A column + /// named in one takes no part in the pairing by name, which pairs the + /// rest: declaring `trip_start`/`trip_end` leaves `trip_start_date` free + /// to pair by its own name. fn column_pairs<'d>( data: &HashMap>, temporal_columns: &'d [String], + declared_pairs: &'d [(String, String)], ) -> Vec<(&'d str, &'d str)> { let temporal_pairs = [ ("start_date", "end_date"), @@ -302,6 +314,20 @@ impl<'a> TimelinessCalculator<'a> { let mut evaluated: HashSet<(&str, &str)> = HashSet::new(); let mut pairs = Vec::new(); + let declared: HashSet<&str> = declared_pairs + .iter() + .flat_map(|(start, end)| [start.as_str(), end.as_str()]) + .collect(); + for (start_name, end_name) in declared_pairs { + if start_name != end_name + && data.contains_key(start_name) + && data.contains_key(end_name) + && evaluated.insert((start_name, end_name)) + { + pairs.push((start_name.as_str(), end_name.as_str())); + } + } + for (start_col, end_col) in &temporal_pairs { // Walk `temporal_columns`, not `data`: the HashMap's iteration order // is randomized. A name the caller listed but the data does not @@ -309,7 +335,7 @@ impl<'a> TimelinessCalculator<'a> { let holders = |role: &str| -> Vec<(&'d str, Vec)> { temporal_columns .iter() - .filter(|name| data.contains_key(*name)) + .filter(|name| data.contains_key(*name) && !declared.contains(name.as_str())) .filter_map(|name| Some((name.as_str(), words_beside_role(name, role)?))) .collect() }; @@ -381,6 +407,7 @@ impl<'a> TimelinessCalculator<'a> { fn count_temporal_violations( data: &HashMap>, temporal_columns: &[String], + declared_pairs: &[(String, String)], column_profiles: &[ColumnProfile], ) -> Result { let mut violations = 0; @@ -388,7 +415,7 @@ impl<'a> TimelinessCalculator<'a> { let mut compared = Vec::new(); let profile = |name: &str| column_profiles.iter().find(|profile| profile.name == name); - for (start_name, end_name) in Self::column_pairs(data, temporal_columns) { + for (start_name, end_name) in Self::column_pairs(data, temporal_columns, declared_pairs) { let (Some(start_values), Some(end_values)) = (data.get(start_name), data.get(end_name)) else { continue; @@ -474,7 +501,7 @@ mod tests { data: &HashMap>, temporal_columns: &[String], ) -> Result { - self.calculate(data, temporal_columns, &whole(data)) + self.calculate(data, temporal_columns, &[], &whole(data)) } } @@ -757,6 +784,126 @@ mod tests { } } + /// Metrics with `declared` pairs, over `temporal_columns` as the metrics + /// pass them: hinted columns, then the columns of the declared pairs. + fn metrics_with_pairs( + data: &HashMap>, + temporal_columns: &[&str], + declared: &[(&str, &str)], + ) -> TimelinessMetrics { + let named: Vec = temporal_columns + .iter() + .map(|name| name.to_string()) + .collect(); + let declared: Vec<(String, String)> = declared + .iter() + .map(|(start, end)| (start.to_string(), end.to_string())) + .collect(); + TimelinessCalculator::with_reference_date(&config(), reference()) + .calculate(data, &named, &declared, &whole(data)) + .expect("timeliness metrics") + } + + fn sorted(pairs: &[(String, String)]) -> Vec<(&str, &str)> { + let mut pairs: Vec<(&str, &str)> = pairs + .iter() + .map(|(start, end)| (start.as_str(), end.as_str())) + .collect(); + pairs.sort(); + pairs + } + + #[test] + fn a_declared_pair_is_compared_whatever_its_names() { + // #942: no role word fits these names, so they were never compared. + let data = HashMap::from([ + ("valid_from".to_string(), vec!["2024-01-05".to_string()]), + ("valid_until".to_string(), vec!["2024-01-01".to_string()]), + ("checkin".to_string(), vec!["2024-03-01".to_string()]), + ("checkout".to_string(), vec!["2024-03-04".to_string()]), + ]); + let columns = ["valid_from", "valid_until", "checkin", "checkout"]; + + let metrics = metrics_with_pairs(&data, &columns, &[]); + assert!(metrics.compared_pairs.is_empty()); + + let declared = [("valid_from", "valid_until"), ("checkin", "checkout")]; + let metrics = metrics_with_pairs(&data, &columns, &declared); + assert_eq!(metrics.temporal_pairs_checked, 2); + assert_eq!(metrics.temporal_violations, 1); + assert_eq!( + sorted(&metrics.compared_pairs), + [("checkin", "checkout"), ("valid_from", "valid_until")] + ); + } + + #[test] + fn a_declared_pair_settles_names_that_are_ambiguous() { + // The #913 case, which pairs nothing by name. + let data = HashMap::from([ + ("primary_start".to_string(), vec!["2024-01-01".to_string()]), + ( + "secondary_start".to_string(), + vec!["2024-01-03".to_string()], + ), + ("end".to_string(), vec!["2024-01-02".to_string()]), + ]); + let metrics = metrics_with_pairs( + &data, + &["primary_start", "secondary_start", "end"], + &[("secondary_start", "end")], + ); + + assert_eq!(metrics.temporal_pairs_checked, 1); + assert_eq!(metrics.temporal_violations, 1); + assert_eq!( + sorted(&metrics.compared_pairs), + [("secondary_start", "end")] + ); + } + + #[test] + fn columns_outside_the_declared_pairs_still_pair_by_name() { + let data = HashMap::from([ + ("valid_from".to_string(), vec!["2024-01-01".to_string()]), + ("valid_until".to_string(), vec!["2024-01-05".to_string()]), + ("billing_start".to_string(), vec!["2024-06-30".to_string()]), + ("billing_end".to_string(), vec!["2024-06-01".to_string()]), + ]); + let metrics = metrics_with_pairs( + &data, + &["billing_start", "billing_end", "valid_from", "valid_until"], + &[("valid_from", "valid_until")], + ); + + assert_eq!(metrics.temporal_pairs_checked, 2); + assert_eq!(metrics.temporal_violations, 1); + assert_eq!( + sorted(&metrics.compared_pairs), + [ + ("billing_start", "billing_end"), + ("valid_from", "valid_until") + ] + ); + } + + #[test] + fn a_column_in_a_declared_pair_is_not_paired_again_by_name() { + // `start` and `end` would pair by name; declaring `start` with + // `finish` says which end it has. + let data = HashMap::from([ + ("start".to_string(), vec!["2024-01-03".to_string()]), + ("end".to_string(), vec!["2024-01-01".to_string()]), + ("finish".to_string(), vec!["2024-01-05".to_string()]), + ]); + let metrics = + metrics_with_pairs(&data, &["start", "end", "finish"], &[("start", "finish")]); + + assert_eq!(metrics.temporal_pairs_checked, 1); + assert_eq!(metrics.temporal_violations, 0); + assert_eq!(sorted(&metrics.compared_pairs), [("start", "finish")]); + } + // ---------------------------------------------------------------- #378 // Deterministic coverage for the timeliness calculator. Every case pins // "now" through `with_reference_date`, so no assertion here changes result diff --git a/crates/dataprof-parquet/src/record_batch_analyzer.rs b/crates/dataprof-parquet/src/record_batch_analyzer.rs index ab50464c..89065da5 100644 --- a/crates/dataprof-parquet/src/record_batch_analyzer.rs +++ b/crates/dataprof-parquet/src/record_batch_analyzer.rs @@ -506,7 +506,11 @@ impl RecordBatchAnalyzer { for field in schema.fields() { let column_name = field.name().to_string(); let positive_hint = self.semantic_hints.is_positive_column(&column_name); - let temporal_hint = self.semantic_hints.is_temporal_column(&column_name); + let temporal_hint = self.semantic_hints.is_temporal_column(&column_name) + || self + .semantic_hints + .temporal_pair_columns() + .contains(&column_name.as_str()); if let Entry::Vacant(entry) = self.column_analyzers.entry(column_name.clone()) { self.column_order.push(column_name); entry.insert(ColumnAnalyzer::with_value_hints( @@ -636,7 +640,16 @@ impl RecordBatchAnalyzer { .get(column) .map(|analyzer| analyzer.hint_binding(column, SemanticHintKind::Temporal)) }); - positive.chain(temporal).collect() + let pairs = self + .semantic_hints + .temporal_pair_columns() + .into_iter() + .filter_map(|column| { + self.column_analyzers + .get(column) + .map(|analyzer| analyzer.hint_binding(column, SemanticHintKind::TemporalPair)) + }); + positive.chain(temporal).chain(pairs).collect() } } @@ -755,7 +768,9 @@ impl ColumnAnalyzer { fn hint_binding(&self, column: &str, kind: SemanticHintKind) -> SemanticHintBinding { let matched_values = match kind { SemanticHintKind::Positive => self.positive_matched_values, - SemanticHintKind::Temporal => self.temporal_matched_values, + SemanticHintKind::Temporal | SemanticHintKind::TemporalPair => { + self.temporal_matched_values + } SemanticHintKind::Identifier => self.hint_checked_values, }; SemanticHintBinding { diff --git a/crates/dataprof-python/src/config.rs b/crates/dataprof-python/src/config.rs index 536f289a..031fd45b 100644 --- a/crates/dataprof-python/src/config.rs +++ b/crates/dataprof-python/src/config.rs @@ -38,6 +38,7 @@ pub struct PyProfilerConfig { pub(crate) positive_columns: Vec, pub(crate) identifier_columns: Vec, pub(crate) temporal_columns: Vec, + pub(crate) temporal_pairs: Vec<(String, String)>, } #[pymethods] @@ -63,6 +64,7 @@ impl PyProfilerConfig { identifier_columns = None, temporal_columns = None, columns = None, + temporal_pairs = None, ))] #[allow(clippy::too_many_arguments)] fn new( @@ -85,6 +87,7 @@ impl PyProfilerConfig { identifier_columns: Option>, temporal_columns: Option>, columns: Option>, + temporal_pairs: Option>>, ) -> PyResult { let engine = parse_engine(engine)?; let format_override = format.map(parse_format).transpose()?; @@ -135,6 +138,20 @@ impl PyProfilerConfig { // return a plausible, emptier report with nothing saying why. let locale = Locale::parse_optional(locale.as_deref()).map_err(PyValueError::new_err)?; + // Taken as sequences, so a pair read from JSON as a list works like a + // tuple; anything but two names is refused rather than cut to fit. + let temporal_pairs = temporal_pairs + .unwrap_or_default() + .into_iter() + .map(|pair| match <[String; 2]>::try_from(pair) { + Ok([start, end]) => Ok((start, end)), + Err(pair) => Err(PyValueError::new_err(format!( + "temporal_pairs entries must be (start, end) column pairs, got {} name(s)", + pair.len() + ))), + }) + .collect::>>()?; + // Parse metrics strings into MetricPack enums let metric_packs = metrics .map(|packs| { @@ -165,6 +182,7 @@ impl PyProfilerConfig { positive_columns: positive_columns.unwrap_or_default(), identifier_columns: identifier_columns.unwrap_or_default(), temporal_columns: temporal_columns.unwrap_or_default(), + temporal_pairs, }) } @@ -253,6 +271,12 @@ impl PyProfilerConfig { self.temporal_columns.clone() } + /// Start/end column pairs timeliness compares as given + #[getter] + fn temporal_pairs(&self) -> Vec<(String, String)> { + self.temporal_pairs.clone() + } + fn __repr__(&self) -> String { format!( "ProfilerConfig(engine='{}', chunk_size={}, memory_limit_mb={}, format={}, max_rows={})", @@ -330,6 +354,9 @@ impl PyProfilerConfig { if !self.temporal_columns.is_empty() { profiler = profiler.temporal_columns(self.temporal_columns.clone()); } + if !self.temporal_pairs.is_empty() { + profiler = profiler.temporal_pairs(self.temporal_pairs.clone()); + } profiler } @@ -340,6 +367,7 @@ impl PyProfilerConfig { self.identifier_columns.clone(), ) .with_temporal_columns(self.temporal_columns.clone()) + .with_temporal_pairs(self.temporal_pairs.clone()) } /// The analysis selection this config asks for. diff --git a/crates/dataprof-python/src/types.rs b/crates/dataprof-python/src/types.rs index d816efe5..c60c15ef 100644 --- a/crates/dataprof-python/src/types.rs +++ b/crates/dataprof-python/src/types.rs @@ -1223,6 +1223,7 @@ impl PyProfileReport { SemanticHintKind::Positive => "positive", SemanticHintKind::Identifier => "identifier", SemanticHintKind::Temporal => "temporal", + SemanticHintKind::TemporalPair => "temporal_pair", }, )?; dict.set_item("checked_values", b.checked_values)?; diff --git a/crates/dataprof-runtime/src/hint_binding.rs b/crates/dataprof-runtime/src/hint_binding.rs index 3c72d911..87d2a45e 100644 --- a/crates/dataprof-runtime/src/hint_binding.rs +++ b/crates/dataprof-runtime/src/hint_binding.rs @@ -36,8 +36,20 @@ impl ValueHintBindingAccumulator { exact: true, }, }); + let pairs = hints + .temporal_pair_columns() + .into_iter() + .map(|column| BindingCounter { + binding: SemanticHintBinding { + column: column.to_string(), + kind: SemanticHintKind::TemporalPair, + checked_values: 0, + matched_values: 0, + exact: true, + }, + }); Self { - counters: positive.chain(temporal).collect(), + counters: positive.chain(temporal).chain(pairs).collect(), } } diff --git a/crates/dataprof/src/profiler.rs b/crates/dataprof/src/profiler.rs index 5fda963e..15ef10a0 100644 --- a/crates/dataprof/src/profiler.rs +++ b/crates/dataprof/src/profiler.rs @@ -67,6 +67,8 @@ pub struct ProfilerConfig { pub identifier_columns: Vec, /// Columns whose values should contribute to timeliness quality metrics. pub temporal_columns: Vec, + /// Start/end column pairs timeliness compares as given. + pub temporal_pairs: Vec<(String, String)>, /// Database connection configuration. Required for `analyze_query()`. #[cfg(feature = "database")] pub database_config: Option, @@ -92,6 +94,7 @@ impl Default for ProfilerConfig { positive_columns: Vec::new(), identifier_columns: Vec::new(), temporal_columns: Vec::new(), + temporal_pairs: Vec::new(), #[cfg(feature = "database")] database_config: None, } @@ -311,6 +314,14 @@ impl Profiler { self } + /// Declare start/end column pairs for timeliness to compare, such as + /// `valid_from`/`valid_until`. Declared pairs are compared as given; the + /// other temporal columns are still paired by name. + pub fn temporal_pairs(mut self, pairs: Vec<(String, String)>) -> Self { + self.config.temporal_pairs = pairs; + self + } + /// Set the progress update interval (default: 500ms) pub fn progress_interval(mut self, interval: Duration) -> Self { self.config.progress_interval = interval; @@ -543,6 +554,7 @@ impl Profiler { self.config.identifier_columns.clone(), ) .with_temporal_columns(self.config.temporal_columns.clone()) + .with_temporal_pairs(self.config.temporal_pairs.clone()) } /// Reject semantic hints that could not bind to the profiled data. diff --git a/docs/guides/getting-started.md b/docs/guides/getting-started.md index 837f0c3d..151dcd11 100644 --- a/docs/guides/getting-started.md +++ b/docs/guides/getting-started.md @@ -212,7 +212,10 @@ Measures whether confidently inferred date/time values are current and consistent. Inferred date columns participate automatically. Pass `temporal_columns=["created_at", "updated_at"]` in Python or call `.temporal_columns(...)` on the Rust profiler to add columns whose dates cannot -be inferred confidently, such as mixed-format strings. +be inferred confidently, such as mixed-format strings. Pass +`temporal_pairs=[("valid_from", "valid_until")]` in Python or call +`.temporal_pairs(...)` on the Rust profiler to compare start/end pairs that no +role word below pairs by name. - `future_dates_count` -- dates that are in the future - `stale_data_ratio` -- fraction of temporal data that appears outdated @@ -223,7 +226,8 @@ be inferred confidently, such as mixed-format strings. With several start or end columns, a start pairs with the end whose other words match (`trip_start` with `trip_end`), or failing that, whose other words match apart from a trailing date word (`trip_start_date` with - `trip_end`); when that is ambiguous, no pair is formed + `trip_end`); when that is ambiguous, no pair is formed unless it is declared + in `temporal_pairs` ### Validity diff --git a/docs/python/README.md b/docs/python/README.md index d281e6f9..cc165ef0 100644 --- a/docs/python/README.md +++ b/docs/python/README.md @@ -110,6 +110,7 @@ dp.profile( positive_columns=None, # list[str] -- columns expected to be non-negative identifier_columns=None, # list[str] -- semantic IDs, not measures temporal_columns=None, # list[str] -- columns assessed for timeliness + temporal_pairs=None, # list[tuple[str, str]] -- start/end pairs compared as given ) -> ProfileReport ``` @@ -652,15 +653,20 @@ marks numeric-looking IDs as semantic strings so numeric stats and outlier metrics do not treat them as measures. Timeliness scoring assesses confidently inferred date columns by default. `temporal_columns` adds columns that inference cannot identify confidently, such -as mixed-format date strings. +as mixed-format date strings. `temporal_pairs=[("valid_from", "valid_until")]` +declares start/end pairs that timeliness compares row by row, for names no role +word pairs; the remaining temporal columns are still paired by name. Hints are validated, never silently dropped. A hint that names a missing column raises `ValueError` listing the unmatched names and the available columns; a `positive_columns` hint on a column with no numeric values, or a -`temporal_columns` hint on a column with no dates, is likewise rejected. +`temporal_columns` hint on a column with no dates, is likewise rejected. A +`temporal_pairs` entry is checked the same way: a missing column, or a column +with no dates, raises `ValueError` naming `temporal_pairs`. `report.semantic_hint_bindings` records how each hint bound — `column`, `kind`, `checked_values`, `matched_values`, and `exact` (whether the counts covered -every row or a sample): +every row or a sample). Each column of a declared pair is recorded once, with +kind `temporal_pair`: ```python report = dp.profile("readings.csv", positive_columns=["pressure"]) diff --git a/docs/schema/profile-report.v1.schema.json b/docs/schema/profile-report.v1.schema.json index cea100d2..b3c7f031 100644 --- a/docs/schema/profile-report.v1.schema.json +++ b/docs/schema/profile-report.v1.schema.json @@ -2466,6 +2466,11 @@ "const": "temporal", "description": "A [`SemanticHints::temporal_columns`] entry.", "type": "string" + }, + { + "const": "temporal_pair", + "description": "A column named by a [`SemanticHints::temporal_pairs`] entry.", + "type": "string" } ] }, diff --git a/python/dataprof/_api.py b/python/dataprof/_api.py index 8deabb91..1cc50250 100644 --- a/python/dataprof/_api.py +++ b/python/dataprof/_api.py @@ -104,6 +104,7 @@ def profile_file( positive_columns: list[str] | None = None, identifier_columns: list[str] | None = None, temporal_columns: list[str] | None = None, + temporal_pairs: list[tuple[str, str]] | None = None, ) -> ProfileReport: """Profile a file path and return a report. @@ -181,6 +182,10 @@ def profile_file( identifiers instead of measures. temporal_columns: Columns whose values should contribute to timeliness quality metrics. + temporal_pairs: ``(start, end)`` column pairs whose dates timeliness + compares row by row, such as ``("valid_from", "valid_until")``. + Declared pairs are compared as given; the remaining temporal + columns are still paired by name. Returns: ProfileReport with analysis results and quality metrics. @@ -208,6 +213,7 @@ def profile_file( positive_columns=positive_columns, identifier_columns=identifier_columns, temporal_columns=temporal_columns, + temporal_pairs=temporal_pairs, ) rust_report = _analyze_file(normalized_path, config) return ProfileReport(rust_report) @@ -236,6 +242,7 @@ def profile( positive_columns: list[str] | None = None, identifier_columns: list[str] | None = None, temporal_columns: list[str] | None = None, + temporal_pairs: list[tuple[str, str]] | None = None, ) -> ProfileReport: """Profile a data source and return a report. @@ -323,6 +330,10 @@ def profile( identifiers instead of measures. temporal_columns: Columns whose values should contribute to timeliness quality metrics. + temporal_pairs: ``(start, end)`` column pairs whose dates timeliness + compares row by row, such as ``("valid_from", "valid_until")``. + Declared pairs are compared as given; the remaining temporal + columns are still paired by name. Returns: ProfileReport with analysis results and quality metrics. @@ -353,6 +364,7 @@ def profile( positive_columns=positive_columns, identifier_columns=identifier_columns, temporal_columns=temporal_columns, + temporal_pairs=temporal_pairs, ) # DataFrame/Arrow paths — build config for metric packs + quality dims + locale @@ -369,6 +381,7 @@ def _df_config() -> ProfilerConfig | None: positive_columns, identifier_columns, temporal_columns, + temporal_pairs, ) ): return ProfilerConfig( @@ -380,6 +393,7 @@ def _df_config() -> ProfilerConfig | None: positive_columns=positive_columns, identifier_columns=identifier_columns, temporal_columns=temporal_columns, + temporal_pairs=temporal_pairs, ) return None diff --git a/python/dataprof/_api.pyi b/python/dataprof/_api.pyi index 2e9e254c..ce1cea21 100644 --- a/python/dataprof/_api.pyi +++ b/python/dataprof/_api.pyi @@ -39,6 +39,7 @@ def profile( positive_columns: list[str] | None = None, identifier_columns: list[str] | None = None, temporal_columns: list[str] | None = None, + temporal_pairs: list[tuple[str, str]] | None = None, ) -> ProfileReport: """Profile a data source (file path, DataFrame, or Arrow object).""" ... @@ -65,6 +66,7 @@ def profile_file( positive_columns: list[str] | None = None, identifier_columns: list[str] | None = None, temporal_columns: list[str] | None = None, + temporal_pairs: list[tuple[str, str]] | None = None, ) -> ProfileReport: """Profile a file path with file-oriented options.""" ... diff --git a/python/dataprof/_dataprof.pyi b/python/dataprof/_dataprof.pyi index 09c69778..f08ea72d 100644 --- a/python/dataprof/_dataprof.pyi +++ b/python/dataprof/_dataprof.pyi @@ -39,6 +39,7 @@ class ProfilerConfig: identifier_columns: list[str] | None = None, temporal_columns: list[str] | None = None, columns: list[str] | None = None, + temporal_pairs: list[tuple[str, str]] | None = None, ) -> None: ... @property def engine(self) -> str: ... @@ -61,6 +62,8 @@ class ProfilerConfig: @property def temporal_columns(self) -> list[str]: ... @property + def temporal_pairs(self) -> list[tuple[str, str]]: ... + @property def jsonl_on_error(self) -> str: ... # --- Sampling --- diff --git a/python/dataprof/_profiler.py b/python/dataprof/_profiler.py index 154c975e..c41a7bc0 100644 --- a/python/dataprof/_profiler.py +++ b/python/dataprof/_profiler.py @@ -161,6 +161,11 @@ def temporal_columns(self, columns: list[str]) -> Profiler: self._kwargs["temporal_columns"] = columns return self + def temporal_pairs(self, pairs: list[tuple[str, str]]) -> Profiler: + """Declare start/end column pairs for timeliness to compare as given.""" + self._kwargs["temporal_pairs"] = pairs + return self + def metrics(self, packs: list[str]) -> Profiler: """Select metric packs to compute. diff --git a/python/dataprof/_profiler.pyi b/python/dataprof/_profiler.pyi index 28c6598e..03c8cf5d 100644 --- a/python/dataprof/_profiler.pyi +++ b/python/dataprof/_profiler.pyi @@ -39,4 +39,5 @@ class Profiler: def positive_columns(self, columns: list[str]) -> Profiler: ... def identifier_columns(self, columns: list[str]) -> Profiler: ... def temporal_columns(self, columns: list[str]) -> Profiler: ... + def temporal_pairs(self, pairs: list[tuple[str, str]]) -> Profiler: ... def profile(self, source: Any) -> ProfileReport: ... diff --git a/python/dataprof/_report.py b/python/dataprof/_report.py index 0ef75213..04a5cf68 100644 --- a/python/dataprof/_report.py +++ b/python/dataprof/_report.py @@ -259,11 +259,11 @@ def semantic_hint_bindings(self) -> list[dict[str, _Any]]: """Per-column evidence of how each semantic hint bound to the data. Empty unless ``positive_columns``/``identifier_columns``/ - ``temporal_columns`` were supplied. Each entry has ``column``, ``kind``, - ``checked_values``, ``matched_values``, and ``exact``. A hint proven - inert over the full data raises before a report is returned, so a report - only ever carries bindings that matched something or whose evidence was - sampled. + ``temporal_columns``/``temporal_pairs`` were supplied. Each entry has + ``column``, ``kind``, ``checked_values``, ``matched_values``, and + ``exact``. A hint proven inert over the full data raises before a report + is returned, so a report only ever carries bindings that matched + something or whose evidence was sampled. """ return list(self._report.semantic_hint_bindings) diff --git a/python/tests/test_profiler_configuration.py b/python/tests/test_profiler_configuration.py index eda1cd82..c5001bb9 100644 --- a/python/tests/test_profiler_configuration.py +++ b/python/tests/test_profiler_configuration.py @@ -298,6 +298,7 @@ def test_returns_self(self): assert p.positive_columns(["pressure"]) is p assert p.identifier_columns(["order_id"]) is p assert p.temporal_columns(["observed_on"]) is p + assert p.temporal_pairs([("valid_from", "valid_until")]) is p def test_csv_delimiter(self): if not os.path.exists(SEMICOLON_FILE): diff --git a/python/tests/test_python_api.py b/python/tests/test_python_api.py index cb53bbf3..97b8affc 100644 --- a/python/tests/test_python_api.py +++ b/python/tests/test_python_api.py @@ -702,6 +702,7 @@ def test_profile_signature(self): "positive_columns", "identifier_columns", "temporal_columns", + "temporal_pairs", } actual_params = set(sig.parameters.keys()) assert actual_params == expected_params, ( diff --git a/python/tests/test_timeliness_declared_pairs.py b/python/tests/test_timeliness_declared_pairs.py new file mode 100644 index 00000000..7337575b --- /dev/null +++ b/python/tests/test_timeliness_declared_pairs.py @@ -0,0 +1,200 @@ +"""Start/end pairs declared next to ``temporal_columns`` (#942). + +Timeliness found pairs only from role words in the names, so +``valid_from``/``valid_until``, ``checkin``/``checkout`` or names in another +language were never compared. A declared pair is compared as given on every +engine and input path, and an unknown name in one is reported like any other +hint. + +``tests/timeliness_declared_pairs.rs`` is the Rust twin. +""" + +from __future__ import annotations + +import datetime as dt +import importlib.util +import json +from collections.abc import Callable +from pathlib import Path +from typing import Any + +import pytest + +try: + import dataprof as dp +except ImportError: + pytest.skip( + "dataprof native extension not built. Run: maturin develop", + allow_module_level=True, + ) + +START, END = "valid_from", "valid_until" +#: The start of every row. +STARTS = ["2024-03-15", "2024-04-20", "2024-05-10"] +#: The end, before the start on every row, so the pair counts three violations. +ENDS = ["2024-02-01", "2024-03-01", "2024-03-20"] + +Route = Callable[..., Any] + + +def _dates(values: list[str]) -> list[dt.date]: + return [dt.date.fromisoformat(value) for value in values] + + +def _records() -> list[dict[str, str]]: + return [{START: start, END: end} for start, end in zip(STARTS, ENDS, strict=True)] + + +def _csv(tmp_path: Path) -> Path: + path = tmp_path / "validity.csv" + rows = [f"{START},{END}", *(f"{a},{b}" for a, b in zip(STARTS, ENDS, strict=True))] + path.write_text("\n".join(rows) + "\n", encoding="utf-8") + return path + + +def _routes(tmp_path: Path) -> dict[str, Route]: + """Every input route, each profiling the two date columns with ``**hints``.""" + + def csv_file(engine: str) -> Route: + return lambda **hints: dp.profile(str(_csv(tmp_path)), engine=engine, **hints) + + def json_file(**hints: Any) -> Any: + path = tmp_path / "validity.json" + path.write_text(json.dumps(_records()), encoding="utf-8") + return dp.profile(str(path), **hints) + + def jsonl_file(**hints: Any) -> Any: + path = tmp_path / "validity.jsonl" + path.write_text("\n".join(json.dumps(r) for r in _records()) + "\n", encoding="utf-8") + return dp.profile(str(path), **hints) + + def arrow_table() -> Any: + import pyarrow as pa + + return pa.table( + { + START: pa.array(_dates(STARTS), pa.date32()), + END: pa.array(_dates(ENDS), pa.date32()), + } + ) + + def parquet_file(**hints: Any) -> Any: + import pyarrow.parquet as pq + + path = tmp_path / "validity.parquet" + pq.write_table(arrow_table(), path) + return dp.profile(str(path), **hints) + + def arrow(**hints: Any) -> Any: + return dp.profile(arrow_table(), **hints) + + def pandas(**hints: Any) -> Any: + import pandas as pd + + return dp.profile(pd.DataFrame({START: STARTS, END: ENDS}), **hints) + + def polars(**hints: Any) -> Any: + import polars as pl + + return dp.profile(pl.DataFrame({START: STARTS, END: ENDS}), **hints) + + def builder(**hints: Any) -> Any: + profiler = dp.Profiler().temporal_pairs(hints["temporal_pairs"]) + return profiler.profile(str(_csv(tmp_path))) + + routes: dict[str, Route] = { + "auto": csv_file("auto"), + "incremental": csv_file("incremental"), + "columnar": csv_file("columnar"), + "json": json_file, + "jsonl": jsonl_file, + "dict": lambda **hints: dp.profile({START: STARTS, END: ENDS}, **hints), + "rows": lambda **hints: dp.profile(_records(), **hints), + "profiler": builder, + } + # An optional library drops only its own routes; skipping inside a route + # would skip the whole test and every route it had not reached yet. + optional = {"parquet": ("pyarrow", parquet_file), "arrow": ("pyarrow", arrow)} + optional |= {"pandas": ("pandas", pandas), "polars": ("polars", polars)} + for label, (module, route) in optional.items(): + if importlib.util.find_spec(module) is not None: + routes[label] = route + return routes + + +def _ordering(report: Any) -> tuple[int, int]: + timeliness = report.to_dict()["quality"]["timeliness"] + return timeliness["temporal_pairs_checked"], timeliness["temporal_violations"] + + +def test_a_declared_pair_is_compared_on_every_path(tmp_path: Path): + wrong = [] + for label, route in _routes(tmp_path).items(): + for pairs, expected in (([], (0, 0)), ([(START, END)], (3, 3))): + report = route(temporal_pairs=pairs) + found = _ordering(report) + if found != expected: + wrong.append(f"[{label}] declared {pairs}: {found}, expected {expected}") + # Each pair column binds once, as the option the caller used. + bound = [ + (binding["column"], binding["matched_values"]) + for binding in report.semantic_hint_bindings + if binding["kind"] == "temporal_pair" + ] + expected_bound = [(START, 3), (END, 3)] if pairs else [] + if bound != expected_bound: + wrong.append( + f"[{label}] declared {pairs}: bindings {bound}, expected {expected_bound}" + ) + assert not wrong, "\n".join(wrong) + + +def test_a_declared_pair_over_columns_without_dates_is_rejected(tmp_path: Path): + # The clean check that never happened: two text columns used to profile + # fine and report ``temporal_pairs_checked == 0``. + columns = {"name": ["Ada", "Linus", "Grace"], "city": ["Turin", "Helsinki", "Arlington"]} + path = tmp_path / "people.csv" + rows = [ + "name,city", + *(f"{n},{c}" for n, c in zip(columns["name"], columns["city"], strict=True)), + ] + path.write_text("\n".join(rows) + "\n", encoding="utf-8") + sources: dict[str, Any] = { + engine: (str(path), {"engine": engine}) for engine in ("auto", "incremental", "columnar") + } + sources["dict"] = (columns, {}) + if importlib.util.find_spec("pyarrow") is not None: + import pyarrow as pa + + sources["arrow"] = (pa.table(columns), {}) + for label, (source, options) in sources.items(): + with pytest.raises(ValueError) as caught: + dp.profile(source, temporal_pairs=[("name", "city")], **options) + message = str(caught.value) + assert "'name' (temporal_pairs)" in message, label + assert "'city' (temporal_pairs)" in message, label + + +def test_a_pair_given_as_a_list_works_like_a_tuple(tmp_path: Path): + report = dp.profile(str(_csv(tmp_path)), temporal_pairs=[[START, END]]) # ty: ignore[invalid-argument-type] + assert _ordering(report) == (3, 3) + + +def test_an_unknown_column_in_a_declared_pair_is_rejected(tmp_path: Path): + for label, route in _routes(tmp_path).items(): + with pytest.raises(ValueError) as caught: + route(temporal_pairs=[(START, "valid_untill")]) + message = str(caught.value) + assert "'valid_untill'" in message, label + assert "temporal_pairs" in message, label + + +@pytest.mark.parametrize("pair", [[START], [START, END, END]]) +def test_an_entry_that_is_not_two_names_is_rejected(tmp_path: Path, pair: list[str]): + with pytest.raises(ValueError, match="temporal_pairs entries must be"): + dp.profile(str(_csv(tmp_path)), temporal_pairs=[pair]) # ty: ignore[invalid-argument-type] + + +def test_config_reports_the_declared_pairs(): + config = dp.ProfilerConfig(temporal_pairs=[(START, END)]) + assert config.temporal_pairs == [(START, END)] diff --git a/tests/timeliness_declared_pairs.rs b/tests/timeliness_declared_pairs.rs new file mode 100644 index 00000000..843e899a --- /dev/null +++ b/tests/timeliness_declared_pairs.rs @@ -0,0 +1,243 @@ +//! Start/end pairs declared next to `temporal_columns` (#942). +//! +//! Timeliness found pairs only from role words in the names, so +//! `valid_from`/`valid_until`, `checkin`/`checkout` or names in another +//! language were never compared. A declared pair is compared as given on +//! every engine and input format, and an unknown name in one is reported like +//! any other hint. +//! +//! `python/tests/test_timeliness_declared_pairs.py` is the Python twin. + +use std::io::Write; + +use dataprof::{DataProfilerError, EngineType, ProfileReport, Profiler, SemanticHintKind}; +use tempfile::NamedTempFile; + +const START: &str = "valid_from"; +const END: &str = "valid_until"; +/// The start of every row. +const STARTS: [&str; 3] = ["2024-03-15", "2024-04-20", "2024-05-10"]; +/// The end, before the start on every row, so the pair counts three violations. +const ENDS: [&str; 3] = ["2024-02-01", "2024-03-01", "2024-03-20"]; + +fn text_fixture(suffix: &str, body: String) -> NamedTempFile { + let mut file = NamedTempFile::with_suffix(suffix).unwrap(); + write!(file, "{body}").unwrap(); + file.flush().unwrap(); + file +} + +fn csv_fixture() -> NamedTempFile { + let rows = STARTS + .iter() + .zip(ENDS) + .map(|(start, end)| format!("{start},{end}")); + let body = std::iter::once(format!("{START},{END}")) + .chain(rows) + .collect::>() + .join("\n") + + "\n"; + text_fixture(".csv", body) +} + +#[cfg(feature = "parquet")] +fn parquet_fixture() -> NamedTempFile { + use std::sync::Arc; + + use arrow::array::{ArrayRef, StringArray}; + use arrow::compute::cast; + use arrow::datatypes::{DataType, Field, Schema}; + use arrow::record_batch::RecordBatch; + use parquet::arrow::ArrowWriter; + + let days = |values: [&str; 3]| -> ArrayRef { + cast(&StringArray::from(values.to_vec()), &DataType::Date32).unwrap() + }; + let schema = Arc::new(Schema::new(vec![ + Field::new(START, DataType::Date32, false), + Field::new(END, DataType::Date32, false), + ])); + let batch = RecordBatch::try_new(schema.clone(), vec![days(STARTS), days(ENDS)]).unwrap(); + + let file = NamedTempFile::with_suffix(".parquet").unwrap(); + let mut writer = ArrowWriter::try_new(file.reopen().unwrap(), schema, None).unwrap(); + writer.write(&batch).unwrap(); + writer.close().unwrap(); + file +} + +fn declared(start: &str, end: &str) -> Vec<(String, String)> { + vec![(start.to_string(), end.to_string())] +} + +/// Every engine over the CSV, then every other format, each profiled with +/// `pairs` declared. +fn reports(pairs: &[(String, String)]) -> Vec<(String, ProfileReport)> { + let csv = csv_fixture(); + let mut reports = Vec::new(); + for engine in [ + EngineType::Auto, + EngineType::Incremental, + EngineType::Columnar, + ] { + let report = Profiler::new() + .engine(engine) + .temporal_pairs(pairs.to_vec()) + .analyze_file(csv.path()) + .unwrap_or_else(|e| panic!("[{engine:?}] {e}")); + reports.push((format!("{engine:?}"), report)); + } + + let records: Vec = STARTS + .iter() + .zip(ENDS) + .map(|(start, end)| format!(r#"{{"{START}":"{start}","{END}":"{end}"}}"#)) + .collect(); + let mut files = vec![ + ( + "json", + text_fixture(".json", format!("[{}]", records.join(","))), + ), + ("jsonl", text_fixture(".jsonl", records.join("\n") + "\n")), + ]; + #[cfg(feature = "parquet")] + files.push(("parquet", parquet_fixture())); + for (label, file) in files { + let report = Profiler::new() + .temporal_pairs(pairs.to_vec()) + .analyze_file(file.path()) + .unwrap_or_else(|e| panic!("[{label}] {e}")); + reports.push((label.to_string(), report)); + } + reports +} + +/// `(temporal_pairs_checked, temporal_violations)`. +fn ordering(report: &ProfileReport, label: &str) -> (usize, usize) { + let timeliness = report + .quality + .as_ref() + .and_then(|quality| quality.metrics.timeliness.as_ref()) + .unwrap_or_else(|| panic!("[{label}] timeliness assessed")); + ( + timeliness.temporal_pairs_checked, + timeliness.temporal_violations, + ) +} + +/// Collects every wrong path before failing, so a regression names all the +/// paths it reaches rather than the first. +#[test] +fn a_declared_pair_is_compared_on_every_path() { + let mut wrong = Vec::new(); + for (pairs, expected) in [(Vec::new(), (0, 0)), (declared(START, END), (3, 3))] { + for (label, report) in reports(&pairs) { + let found = ordering(&report, &label); + if found != expected { + wrong.push(format!( + "[{label}] declared {pairs:?}: (pairs_checked, violations) {found:?}, expected {expected:?}" + )); + } + // Each pair column binds once, as the option the caller used. + let bound: Vec<(&str, usize)> = report + .semantic_hint_bindings + .iter() + .filter(|binding| binding.kind == SemanticHintKind::TemporalPair) + .map(|binding| (binding.column.as_str(), binding.matched_values)) + .collect(); + let expected_bound = if pairs.is_empty() { + vec![] + } else { + vec![(START, 3), (END, 3)] + }; + if bound != expected_bound { + wrong.push(format!( + "[{label}] declared {pairs:?}: bindings {bound:?}, expected {expected_bound:?}" + )); + } + } + } + assert!(wrong.is_empty(), "{}", wrong.join("\n")); +} + +#[track_caller] +fn assert_hint_error(result: Result, needles: &[&str]) { + match result { + Err(DataProfilerError::InvalidSemanticHint { + message, + suggestion, + }) => { + let text = format!("{message}\n{suggestion}"); + for needle in needles { + assert!(text.contains(needle), "expected {needle:?} in: {text}"); + } + } + Err(other) => panic!("expected InvalidSemanticHint, got: {other}"), + Ok(_) => panic!("expected InvalidSemanticHint, but profiling succeeded"), + } +} + +#[test] +fn an_unknown_column_in_a_declared_pair_is_rejected() { + let csv = csv_fixture(); + for engine in [EngineType::Incremental, EngineType::Columnar] { + let result = Profiler::new() + .engine(engine) + .temporal_pairs(declared(START, "valid_untill")) + .analyze_file(csv.path()); + assert_hint_error( + result, + &["'valid_untill'", "temporal_pairs", "'valid_until'"], + ); + } +} + +#[test] +fn a_pair_of_one_column_with_itself_is_rejected() { + let csv = csv_fixture(); + let result = Profiler::new() + .temporal_pairs(declared(START, START)) + .analyze_file(csv.path()); + assert_hint_error(result, &["'valid_from'", "temporal_pairs"]); +} + +#[test] +fn a_declared_pair_over_columns_without_dates_is_rejected() { + // The clean check that never happened: two text columns used to profile + // fine and report `temporal_pairs_checked == 0`. + let body = "name,city\nAda,Turin\nLinus,Helsinki\nGrace,Arlington\n"; + let csv = text_fixture(".csv", body.to_string()); + let records = r#"{"name":"Ada","city":"Turin"} +{"name":"Linus","city":"Helsinki"}"#; + let jsonl = text_fixture(".jsonl", records.to_string() + "\n"); + let mut runs = vec![ + ("Auto", Profiler::new().engine(EngineType::Auto), csv.path()), + ( + "Incremental", + Profiler::new().engine(EngineType::Incremental), + csv.path(), + ), + ( + "Columnar", + Profiler::new().engine(EngineType::Columnar), + csv.path(), + ), + ]; + runs.push(("jsonl", Profiler::new(), jsonl.path())); + for (label, profiler, path) in runs { + let result = profiler + .temporal_pairs(declared("name", "city")) + .analyze_file(path); + match result { + Err(DataProfilerError::InvalidSemanticHint { message, .. }) => { + for needle in ["'name' (temporal_pairs)", "'city' (temporal_pairs)"] { + assert!( + message.contains(needle), + "[{label}] {needle:?} in: {message}" + ); + } + } + other => panic!("[{label}] expected InvalidSemanticHint, got {other:?}"), + } + } +} From 2d644d7b10cc2e4a484e11eccc65f19ce47517f8 Mon Sep 17 00:00:00 2001 From: 1932819331hy-debug <1932819331hy@gmail.com> Date: Sat, 17 Oct 2026 16:25:36 +0900 Subject: [PATCH 02/16] fix(streaming): reserve channel capacity for terminal progress event (#975) * fix(streaming): await channel capacity when emitting finished progress event Intermediate progress events (such as ProgressEvent::ChunkProcessed) are best-effort and dropped via try_send when a progress channel is full to prevent blocking ingestion. However, when profiling ends, dropping ProgressEvent::Finished on a full channel causes consumers draining the channel to never learn the profile completed or how many rows were counted. This change: - Adds ProgressSink::emit_async to await channel capacity via tx.send(event).await. - Adds ProgressTracker::emit_finished_async and calls it at the end of AsyncStreamingProfiler::profile_stream. - Updates Profiler::progress_channel documentation explaining intermediate best-effort dropping versus terminal event delivery. - Adds regression test test_progress_channel_capacity_one_receives_finished_event verifying that a capacity-1 channel on a current-thread runtime reliably receives Finished with no stop condition, MaxRows, and MaxBytes. Fixes #974. * fix(streaming): emit finished progress event only after reader checks succeed * style: apply rustfmt formatting to async reader and test suite * fix(streaming): reserve Finished event permit up front with try_reserve_owned * fix(core): flatten finished permit extraction to satisfy clippy collapsible_if * fix(streaming): reserve terminal capacity per profile run --- crates/dataprof-core/src/progress.rs | 11 + .../dataprof-engines/src/progress_tracker.rs | 16 +- .../src/streaming/async_reader.rs | 8 +- crates/dataprof/src/profiler.rs | 24 +- tests/async_streaming.rs | 274 ++++++++++++++++++ 5 files changed, 327 insertions(+), 6 deletions(-) diff --git a/crates/dataprof-core/src/progress.rs b/crates/dataprof-core/src/progress.rs index 7bc9a7d1..7b07a40b 100644 --- a/crates/dataprof-core/src/progress.rs +++ b/crates/dataprof-core/src/progress.rs @@ -52,6 +52,17 @@ impl ProgressSink { !matches!(self, Self::None) } + /// Reserve channel capacity for a terminal event without changing the + /// public shape of the Channel variant. + #[doc(hidden)] + #[cfg(feature = "async-streaming")] + pub fn reserve_terminal(&self) -> Option> { + match self { + Self::Channel(tx) => tx.clone().try_reserve_owned().ok(), + _ => None, + } + } + /// Deliver one event. A full channel drops it rather than block profiling. pub fn emit(&self, event: ProgressEvent) { match self { diff --git a/crates/dataprof-engines/src/progress_tracker.rs b/crates/dataprof-engines/src/progress_tracker.rs index 1b046294..3b1ca294 100644 --- a/crates/dataprof-engines/src/progress_tracker.rs +++ b/crates/dataprof-engines/src/progress_tracker.rs @@ -5,6 +5,8 @@ use dataprof_core::{ProgressEvent, ProgressSink}; /// Engine-internal tracker that rate-limits event emission. pub(crate) struct ProgressTracker { sink: ProgressSink, + #[cfg(feature = "async-streaming")] + terminal_permit: Option>, start_time: Instant, last_update: Instant, update_interval: Duration, @@ -16,8 +18,12 @@ pub(crate) struct ProgressTracker { impl ProgressTracker { pub(crate) fn new(sink: ProgressSink, interval: Duration) -> Self { let now = Instant::now(); + #[cfg(feature = "async-streaming")] + let terminal_permit = sink.reserve_terminal(); Self { sink, + #[cfg(feature = "async-streaming")] + terminal_permit, start_time: now, last_update: now, update_interval: interval, @@ -82,12 +88,18 @@ impl ProgressTracker { pub(crate) fn emit_finished(&mut self, truncated: bool) { let elapsed = Instant::now().duration_since(self.start_time); - self.emit(ProgressEvent::Finished { + let event = ProgressEvent::Finished { total_rows: self.total_rows, total_bytes: self.total_bytes, elapsed, truncated, - }); + }; + #[cfg(feature = "async-streaming")] + if let Some(permit) = self.terminal_permit.take() { + permit.send(event); + return; + } + self.emit(event); } fn emit(&self, event: ProgressEvent) { diff --git a/crates/dataprof-engines/src/streaming/async_reader.rs b/crates/dataprof-engines/src/streaming/async_reader.rs index 4d6635f8..5b955d8f 100644 --- a/crates/dataprof-engines/src/streaming/async_reader.rs +++ b/crates/dataprof-engines/src/streaming/async_reader.rs @@ -334,6 +334,7 @@ impl AsyncStreamingProfiler { let ( _headers, mut column_stats, + mut progress_tracker, total_rows, sampled_rows, total_bytes, @@ -434,6 +435,7 @@ impl AsyncStreamingProfiler { execution = execution.with_unterminated_quote(ended_inside_quotes); } + let is_truncated = truncation_reason.is_some(); if let Some(reason) = truncation_reason { execution = execution.with_truncation(reason); } else if total_rows > 0 && sampled_rows < total_rows { @@ -444,6 +446,8 @@ impl AsyncStreamingProfiler { execution = execution.with_sampling(sampled_rows as f64 / total_rows as f64); } + progress_tracker.emit_finished(is_truncated); + Ok(ReportAssembler::new(data_source, execution) .columns(column_profiles) .with_quality_data(sample_columns) @@ -1055,6 +1059,7 @@ impl AsyncStreamingProfiler { ( Vec, StreamingColumnCollection, + ProgressTracker, usize, usize, u64, @@ -1277,11 +1282,10 @@ impl AsyncStreamingProfiler { sampled_rows += 1; } - progress_tracker.emit_finished(truncation_reason.is_some()); - Ok(( headers, column_stats, + progress_tracker, total_rows, sampled_rows, total_bytes, diff --git a/crates/dataprof/src/profiler.rs b/crates/dataprof/src/profiler.rs index 15ef10a0..4a7ec92e 100644 --- a/crates/dataprof/src/profiler.rs +++ b/crates/dataprof/src/profiler.rs @@ -353,6 +353,12 @@ impl Profiler { } /// Set an async channel for progress events (requires `async-streaming` feature). + /// + /// Intermediate events (such as [`ProgressEvent::ChunkProcessed`]) are best-effort + /// and dropped if the channel is full rather than blocking profiling. + /// The terminal [`ProgressEvent::Finished`] event uses a slot reserved up front via + /// `try_reserve_owned()`, guaranteeing delivery without blocking even when the consumer + /// drains the channel only after profiling completes. #[cfg(feature = "async-streaming")] pub fn progress_channel(mut self, tx: tokio::sync::mpsc::Sender) -> Self { self.progress_sink = ProgressSink::Channel(tx); @@ -396,6 +402,12 @@ impl Profiler { // finish. A sink on `Auto` routes CSV to the incremental engine. let bracket = self.progress_sink.is_active() && !(is_csv && !matches!(self.config.engine, EngineType::Columnar)); + #[cfg(feature = "async-streaming")] + let terminal_permit = if bracket { + self.progress_sink.reserve_terminal() + } else { + None + }; let source_bytes = std::fs::metadata(path).ok().map(|meta| meta.len()); let started = std::time::Instant::now(); if bracket { @@ -430,12 +442,20 @@ impl Profiler { } else { 0 }); - self.progress_sink.emit(ProgressEvent::Finished { + let event = ProgressEvent::Finished { total_rows: execution.rows_processed, total_bytes, elapsed: started.elapsed(), truncated: !execution.source_exhausted, - }); + }; + #[cfg(feature = "async-streaming")] + if let Some(permit) = terminal_permit { + permit.send(event); + } else { + self.progress_sink.emit(event); + } + #[cfg(not(feature = "async-streaming"))] + self.progress_sink.emit(event); } Ok(report) } diff --git a/tests/async_streaming.rs b/tests/async_streaming.rs index 70a77454..a458d8a0 100644 --- a/tests/async_streaming.rs +++ b/tests/async_streaming.rs @@ -545,3 +545,277 @@ async fn test_profiler_profile_stream_finished_event_counts_bytes_stop_condition "finished event total_rows must match execution.rows_processed" ); } + +/// A progress channel that is full when profiling ends must still deliver the +/// `Finished` event to a consumer draining the channel (gh #974). +#[tokio::test(flavor = "current_thread")] +async fn test_progress_channel_capacity_one_receives_finished_event() { + for stop_cond in [ + None, + Some(StopCondition::MaxRows(150)), + Some(StopCondition::MaxBytes(50_000)), + ] { + let mut csv_data = String::from("id,value\n"); + for i in 0..10_000 { + csv_data.push_str(&format!("{},val_{}\n", i, i)); + } + + let source = BytesSource::new( + bytes::Bytes::from(csv_data.clone()), + AsyncSourceInfo::new("bytes-csv", FileFormat::Csv) + .size_hint(Some(csv_data.len() as u64)), + ); + + let (tx, mut rx) = tokio::sync::mpsc::channel(1); + let consumer_task = tokio::spawn(async move { + let mut finished_received = false; + while let Some(event) = rx.recv().await { + if let dataprof::ProgressEvent::Finished { .. } = event { + finished_received = true; + break; + } + } + finished_received + }); + + let mut profiler = Profiler::new() + .progress_interval(Duration::ZERO) + .progress_channel(tx); + + if let Some(ref cond) = stop_cond { + profiler = profiler.stop_when(cond.clone()); + } + + let report = profiler.profile_stream(source).await.unwrap(); + assert!(report.execution.rows_processed > 0); + + drop(profiler); + + let finished_received = consumer_task.await.unwrap(); + assert!( + finished_received, + "Finished event must be received by channel consumer for stop_condition {:?}", + stop_cond + ); + } +} + +/// A stream that fails during profiling must never emit `ProgressEvent::Finished`. +#[tokio::test] +async fn test_progress_channel_does_not_emit_finished_on_error() { + // 1. Duplicate CSV headers error + { + let csv = b"x,x\n1,2\n"; + let source = BytesSource::new( + bytes::Bytes::from_static(csv), + AsyncSourceInfo::new("duplicate-header-csv", FileFormat::Csv), + ); + + let (tx, mut rx) = tokio::sync::mpsc::channel(10); + let consumer = tokio::spawn(async move { + let mut got_finished = false; + while let Some(event) = rx.recv().await { + if let dataprof::ProgressEvent::Finished { .. } = event { + got_finished = true; + } + } + got_finished + }); + + let res = Profiler::new() + .progress_channel(tx) + .profile_stream(source) + .await; + + assert!( + res.is_err(), + "profiling should fail due to duplicate headers" + ); + let got_finished = consumer.await.unwrap(); + assert!( + !got_finished, + "Finished event must not be emitted when profiling fails on duplicate headers" + ); + } + + // 2. Unterminated quote error in strict CSV mode + { + let csv = b"id,val\n1,\"unterminated\n2,normal\n"; + let source = BytesSource::new( + bytes::Bytes::from_static(csv), + AsyncSourceInfo::new("unterminated-quote-csv", FileFormat::Csv), + ); + + let (tx, mut rx) = tokio::sync::mpsc::channel(10); + let consumer = tokio::spawn(async move { + let mut got_finished = false; + while let Some(event) = rx.recv().await { + if let dataprof::ProgressEvent::Finished { .. } = event { + got_finished = true; + } + } + got_finished + }); + + let res = Profiler::new() + .csv_flexible(false) + .progress_channel(tx) + .profile_stream(source) + .await; + + assert!( + res.is_err(), + "profiling should fail due to unterminated quote in strict mode" + ); + let got_finished = consumer.await.unwrap(); + assert!( + !got_finished, + "Finished event must not be emitted when profiling fails on unterminated quote" + ); + } +} + +/// A progress channel whose capacity is 1 must deliver `Finished` without blocking +/// or timing out even when the caller drains the channel only after profiling completes (gh #974). +#[tokio::test] +async fn test_progress_channel_drained_after_profile_under_timeout() { + let mut csv_data = String::from("id,value\n"); + for i in 0..10_000 { + csv_data.push_str(&format!("{},val_{}\n", i, i)); + } + + let source = BytesSource::new( + bytes::Bytes::from(csv_data.clone()), + AsyncSourceInfo::new("drained-after-csv", FileFormat::Csv) + .size_hint(Some(csv_data.len() as u64)), + ); + + let (tx, mut rx) = tokio::sync::mpsc::channel(1); + let profiler = Profiler::new() + .progress_interval(Duration::ZERO) + .progress_channel(tx); + + let report = tokio::time::timeout(Duration::from_secs(2), profiler.profile_stream(source)) + .await + .expect("profiling must not deadlock or time out while channel is not yet drained") + .unwrap(); + + assert!(report.execution.rows_processed > 0); + + let mut finished_received = false; + while let Ok(event) = rx.try_recv() { + if let dataprof::ProgressEvent::Finished { .. } = event { + finished_received = true; + break; + } + } + assert!( + finished_received, + "Finished event must be received by consumer draining after profiling completes" + ); +} + +/// A facade-bracketed columnar file route must also deliver `Finished` using the reserved permit. +#[test] +fn test_progress_channel_file_route_receives_finished_event() { + use std::io::Write; + + let mut tmp = tempfile::NamedTempFile::with_suffix(".csv").unwrap(); + writeln!(tmp, "id,value").unwrap(); + for i in 0..100 { + writeln!(tmp, "{},val_{}", i, i).unwrap(); + } + tmp.flush().unwrap(); + + let (tx, mut rx) = tokio::sync::mpsc::channel(1); + let report = Profiler::new() + .engine(dataprof::EngineType::Columnar) + .progress_interval(Duration::ZERO) + .progress_channel(tx) + .analyze_file(tmp.path()) + .unwrap(); + + assert_eq!(report.execution.rows_processed, 100); + + let mut finished_received = false; + while let Ok(event) = rx.try_recv() { + if let dataprof::ProgressEvent::Finished { .. } = event { + finished_received = true; + break; + } + } + assert!( + finished_received, + "Finished event must be delivered on file routes via reserved permit" + ); +} + +/// Reusing a profiler must reserve fresh terminal capacity for every run. +#[test] +fn test_progress_channel_reused_profiler_receives_finished_each_run() { + use std::io::Write; + + let mut tmp = tempfile::NamedTempFile::with_suffix(".csv").unwrap(); + writeln!(tmp, "id,value").unwrap(); + for i in 0..100 { + writeln!(tmp, "{},val_{}", i, i).unwrap(); + } + tmp.flush().unwrap(); + + let (tx, mut rx) = tokio::sync::mpsc::channel(1); + let profiler = Profiler::new() + .progress_interval(Duration::ZERO) + .progress_channel(tx); + + for run in 1..=2 { + let report = profiler.analyze_file(tmp.path()).unwrap(); + assert_eq!(report.execution.rows_processed, 100); + + let events = std::iter::from_fn(|| rx.try_recv().ok()).collect::>(); + assert!( + events + .iter() + .any(|event| matches!(event, dataprof::ProgressEvent::Finished { .. })), + "run {run} must receive a freshly reserved Finished event; got {events:?}" + ); + } +} + +/// Reusing a facade-bracketed file route must reserve fresh terminal capacity for every run. +#[test] +fn test_progress_channel_reused_columnar_file_route_receives_finished_each_run() { + use std::io::Write; + + let mut tmp = tempfile::NamedTempFile::with_suffix(".csv").unwrap(); + writeln!(tmp, "id,value").unwrap(); + for i in 0..100 { + writeln!(tmp, "{},val_{}", i, i).unwrap(); + } + tmp.flush().unwrap(); + + let (tx, mut rx) = tokio::sync::mpsc::channel(1); + let profiler = Profiler::new() + .engine(dataprof::EngineType::Columnar) + .progress_interval(Duration::ZERO) + .progress_channel(tx); + + for run in 1..=2 { + let report = profiler.analyze_file(tmp.path()).unwrap(); + assert_eq!(report.execution.rows_processed, 100); + + let events = std::iter::from_fn(|| rx.try_recv().ok()).collect::>(); + assert!( + events + .iter() + .any(|event| matches!(event, dataprof::ProgressEvent::Finished { .. })), + "columnar run {run} must receive a freshly reserved Finished event; got {events:?}" + ); + } +} + +/// Preserve the public enum constructor accepted before gh #974. +#[test] +fn test_progress_sink_channel_variant_accepts_sender() { + let (tx, _rx) = tokio::sync::mpsc::channel(1); + let _sink = dataprof::ProgressSink::Channel(tx); +} From 1808c4b57f7a8a61d9014c1a9eca5e3d6774d0e7 Mon Sep 17 00:00:00 2001 From: Andrea Bozzo Date: Sat, 17 Oct 2026 09:36:24 +0200 Subject: [PATCH 03/16] fix: tell a JSON string from a container that spells it in duplicate-row signatures (#945) (#977) --- crates/dataprof-db/src/lib.rs | 10 ++- crates/dataprof-db/src/query_columns.rs | 23 +++-- .../tests/structured_column_decoding.rs | 89 +++++++++++++++++-- .../src/streaming/async_reader.rs | 4 +- crates/dataprof-json/src/lib.rs | 8 +- crates/dataprof-python/src/columns.rs | 9 +- .../dataprof-runtime/src/streaming_stats.rs | 40 ++++++++- docs/guides/getting-started.md | 3 +- python/tests/test_nested_duplicate_rows.py | 23 ++++- tests/nested_duplicate_rows.rs | 26 +++++- 10 files changed, 207 insertions(+), 28 deletions(-) diff --git a/crates/dataprof-db/src/lib.rs b/crates/dataprof-db/src/lib.rs index 84275c8b..70985a52 100644 --- a/crates/dataprof-db/src/lib.rs +++ b/crates/dataprof-db/src/lib.rs @@ -280,10 +280,16 @@ pub async fn analyze_database_with_options( // from value-level checks, and the columns no longer line up. let mut uniqueness = RowUniquenessTracker::default(); if !aligned.is_empty() { + let container_rows: Vec<&[usize]> = columns.container_rows().collect(); for row_index in 0..actual_rows_processed { let mut signature = RowSignature::default(); - for cells in &aligned { - signature.push_field(&cells[row_index]); + for (cells, containers) in aligned.iter().zip(&container_rows) { + // Ascending by construction: rows are pushed in order. + if containers.binary_search(&row_index).is_ok() { + signature.push_container_field(&cells[row_index]); + } else { + signature.push_field(&cells[row_index]); + } } uniqueness.observe(signature.finish()); } diff --git a/crates/dataprof-db/src/query_columns.rs b/crates/dataprof-db/src/query_columns.rs index 0accbe83..c6e03d88 100644 --- a/crates/dataprof-db/src/query_columns.rs +++ b/crates/dataprof-db/src/query_columns.rs @@ -18,9 +18,9 @@ use std::ops::Index; #[derive(Debug, Clone, Default, PartialEq, Eq)] pub struct QueryColumns { columns: Vec<(String, Vec)>, - /// Per column, by position, how many values were a JSON object or array or - /// a SQL array. Always as long as `columns`. - containers: Vec, + /// Per column, by position, the rows whose value was a JSON object or array + /// or a SQL array, ascending. Always as long as `columns`. + containers: Vec>, } impl QueryColumns { @@ -58,8 +58,10 @@ impl QueryColumns { /// container: a JSON object or array, or a SQL array. pub fn push_decoded(&mut self, index: usize, value: String, container: bool) { if let Some((_, data)) = self.columns.get_mut(index) { + if container { + self.containers[index].push(data.len()); + } data.push(value); - self.containers[index] += usize::from(container); } } @@ -69,11 +71,18 @@ impl QueryColumns { self.columns .iter() .zip(&self.containers) - .filter(|(_, count)| **count > 0) - .map(|((name, _), count)| (name.clone(), *count)) + .filter(|(_, rows)| !rows.is_empty()) + .map(|((name, _), rows)| (name.clone(), rows.len())) .collect() } + /// Per column, in query order, the rows whose value was a container, + /// ascending. Row signatures tag those values apart from a string that + /// spells the same text (#945). + pub fn container_rows(&self) -> impl Iterator { + self.containers.iter().map(Vec::as_slice) + } + /// Number of columns. pub fn len(&self) -> usize { self.columns.len() @@ -155,7 +164,7 @@ impl Index<&str> for QueryColumns { impl FromIterator<(String, Vec)> for QueryColumns { fn from_iter)>>(iter: I) -> Self { let columns: Vec<_> = iter.into_iter().collect(); - let containers = vec![0; columns.len()]; + let containers = vec![Vec::new(); columns.len()]; Self { columns, containers, diff --git a/crates/dataprof-db/tests/structured_column_decoding.rs b/crates/dataprof-db/tests/structured_column_decoding.rs index 41c83bbd..ae1dd44b 100644 --- a/crates/dataprof-db/tests/structured_column_decoding.rs +++ b/crates/dataprof-db/tests/structured_column_decoding.rs @@ -93,6 +93,39 @@ fn profile_wrongs(report: &ProfileReport, expected: &[(&str, DataType)]) -> Vec< wrong } +fn duplicate_rows(report: &ProfileReport) -> Option { + report + .quality + .as_ref() + .and_then(|quality| quality.metrics.uniqueness.as_ref()) + .map(|uniqueness| uniqueness.duplicate_rows) +} + +/// Rows of the column `name` whose value was a container, per path. +fn container_rows_wrongs( + paths: &[(&'static str, QueryColumns)], + name: &str, + expected: &[usize], +) -> Vec { + let mut wrong = Vec::new(); + for (path, columns) in paths { + let position = columns + .names() + .position(|column| column == name) + .expect("column in the result"); + let rows = columns + .container_rows() + .nth(position) + .expect("container rows"); + if rows != expected { + wrong.push(format!( + "{path} {name}: container rows {rows:?}, expected {expected:?}" + )); + } + } + wrong +} + #[cfg(feature = "postgres")] mod postgres { use super::*; @@ -100,14 +133,6 @@ mod postgres { const TABLE: &str = "SELECT * FROM structured_decoding_pg ORDER BY id"; - fn duplicate_rows(report: &ProfileReport) -> Option { - report - .quality - .as_ref() - .and_then(|quality| quality.metrics.uniqueness.as_ref()) - .map(|uniqueness| uniqueness.duplicate_rows) - } - /// Created once per process; the tests only read it. static FIXTURE: tokio::sync::OnceCell> = tokio::sync::OnceCell::const_new(); @@ -216,6 +241,30 @@ mod postgres { assert!(wrong.is_empty(), "{wrong:#?}"); } + /// A `jsonb` string holding an object's text renders the same text as the + /// object, so the row signature must tell them apart by the container + /// flag (#945). Rows 2 and 3 hold the object and are one duplicate; row 1 + /// holds the string and is not. + #[tokio::test] + async fn a_json_string_spelling_an_object_is_not_the_object() { + let Some(url) = url().await else { + return; + }; + let query = r#"SELECT id, j FROM (VALUES + (1, 1, '"{\"x\":1}"'::jsonb), + (2, 1, '{"x":1}'::jsonb), + (3, 1, '{"x":1}'::jsonb)) AS t(n, id, j) ORDER BY n"#; + + let mut wrong = container_rows_wrongs(&both_paths(&url, query).await, "j", &[1, 2]); + let duplicates = duplicate_rows(&profile(&url, query).await); + if duplicates != Some(1) { + wrong.push(format!( + "profile: {duplicates:?} duplicate rows, expected 1" + )); + } + assert!(wrong.is_empty(), "{wrong:#?}"); + } + /// PostgreSQL accepts numbers serde_json cannot hold. Such a document keeps /// the database's digits instead of falling back to null, and its keys are /// still sorted: the two rows differ only in key order, whitespace and @@ -388,4 +437,28 @@ mod mysql { ); assert!(wrong.is_empty(), "{wrong:#?}"); } + + /// A JSON string holding an object's text renders the same text as the + /// object, so the row signature must tell them apart by the container + /// flag (#945). Rows 2 and 3 hold the object and are one duplicate. + #[tokio::test] + async fn a_json_string_spelling_an_object_is_not_the_object() { + let Some(url) = url().await else { + return; + }; + // The streaming path refuses `UNION`, so the rows come from `VALUES`. + let query = r#"SELECT id, j FROM (VALUES + ROW(1, 1, CAST('"{\\"x\\":1}"' AS JSON)), + ROW(2, 1, CAST('{"x":1}' AS JSON)), + ROW(3, 1, CAST('{"x":1}' AS JSON))) AS t(n, id, j) ORDER BY n"#; + + let mut wrong = container_rows_wrongs(&both_paths(&url, query).await, "j", &[1, 2]); + let duplicates = duplicate_rows(&profile(&url, query).await); + if duplicates != Some(1) { + wrong.push(format!( + "profile: {duplicates:?} duplicate rows, expected 1" + )); + } + assert!(wrong.is_empty(), "{wrong:#?}"); + } } diff --git a/crates/dataprof-engines/src/streaming/async_reader.rs b/crates/dataprof-engines/src/streaming/async_reader.rs index 5b955d8f..e28e9fd0 100644 --- a/crates/dataprof-engines/src/streaming/async_reader.rs +++ b/crates/dataprof-engines/src/streaming/async_reader.rs @@ -1175,7 +1175,7 @@ impl AsyncStreamingProfiler { sampler.offer((values, containers)); } else { count_containers(&headers, &containers, &mut container_counts); - column_stats.process_record(&headers, values); + column_stats.process_record_with_containers(&headers, values, &containers); sampled_rows += 1; } @@ -1278,7 +1278,7 @@ impl AsyncStreamingProfiler { // in here rather than row by row. for (values, containers) in sampler.take_sample() { count_containers(&headers, &containers, &mut container_counts); - column_stats.process_record(&headers, values); + column_stats.process_record_with_containers(&headers, values, &containers); sampled_rows += 1; } diff --git a/crates/dataprof-json/src/lib.rs b/crates/dataprof-json/src/lib.rs index e4c7f15e..006e9327 100644 --- a/crates/dataprof-json/src/lib.rs +++ b/crates/dataprof-json/src/lib.rs @@ -714,8 +714,14 @@ fn feed_json_object( obj.get(column).map(json_value_text).unwrap_or_default() }) .collect(); + let containers: Vec = known_columns + .iter() + .enumerate() + .filter(|(_, column)| matches!(obj.get(*column), Some(Value::Array(_) | Value::Object(_)))) + .map(|(position, _)| position) + .collect(); - column_stats.process_record(known_columns, values); + column_stats.process_record_with_containers(known_columns, values, &containers); } /// Analyze JSON/JSONL data from a buffered reader using streaming statistics. diff --git a/crates/dataprof-python/src/columns.rs b/crates/dataprof-python/src/columns.rs index aa3a195c..750a0dbc 100644 --- a/crates/dataprof-python/src/columns.rs +++ b/crates/dataprof-python/src/columns.rs @@ -172,9 +172,14 @@ pub fn profile_columns( for row_index in 0..num_rows { let mut row_signature = RowSignature::default(); let mut row_has_null = false; - for (_, cells, _) in &columns { + for (_, cells, containers) in &columns { let value = cells[row_index].as_deref().unwrap_or(""); - row_signature.push_field(value); + // Validated above as ascending, so a binary search holds. + if containers.binary_search(&row_index).is_ok() { + row_signature.push_container_field(value); + } else { + row_signature.push_field(value); + } // A missing cell renders empty, which `is_null_like_token` // already treats as null — the same rule that produces // `null_count` below. diff --git a/crates/dataprof-runtime/src/streaming_stats.rs b/crates/dataprof-runtime/src/streaming_stats.rs index 18e83139..6963a401 100644 --- a/crates/dataprof-runtime/src/streaming_stats.rs +++ b/crates/dataprof-runtime/src/streaming_stats.rs @@ -462,7 +462,21 @@ impl RowSignature { /// ending in one of those against a row that ends in no field at all, /// inventing a duplicate. pub fn push_field(&mut self, value: &str) { - let _ = write!(self.buffer, "{}:", value.len()); + self.push_tagged(value, ':'); + } + + /// Append one field holding a JSON object or array, given as its text. + /// + /// A container crosses as its compact JSON, which a string can spell, so + /// it is tagged apart from a scalar: `"{\"x\":1}"` and `{"x": 1}` are two + /// values and must not sign alike (#945). + pub fn push_container_field(&mut self, text: &str) { + self.push_tagged(text, '#'); + } + + /// Append one field as its length, `tag`, then the text. + fn push_tagged(&mut self, value: &str, tag: char) { + let _ = write!(self.buffer, "{}{tag}", value.len()); self.buffer.push_str(value); if !value.is_empty() { self.len_through_last_present = self.buffer.len(); @@ -702,6 +716,22 @@ impl StreamingColumnCollection { pub fn process_record(&mut self, headers: &[String], values: I) where I: IntoIterator, + { + self.process_record_with_containers(headers, values, &[]); + } + + /// [`process_record`](Self::process_record) for a record whose fields at + /// `containers` (positions in `headers`) were JSON objects or arrays, + /// given as their text. Their statistics read the text like any other + /// value; only the row signature tells them apart from a string that + /// spells the same text (#945). + pub fn process_record_with_containers( + &mut self, + headers: &[String], + values: I, + containers: &[usize], + ) where + I: IntoIterator, { let mut row_signature = RowSignature::default(); let mut row_has_null = false; @@ -710,9 +740,13 @@ impl StreamingColumnCollection { // Headers define the row schema. Normalize a missing trailing field to // the profiler's empty/null representation so ragged flexible records // update every column and hash identically to an explicit empty field. - for header in headers { + for (position, header) in headers.iter().enumerate() { let value = values.next().unwrap_or_default(); - row_signature.push_field(&value); + if containers.contains(&position) { + row_signature.push_container_field(&value); + } else { + row_signature.push_field(&value); + } if let Some(stats) = self.columns.get_mut(header) { stats.update(&value); diff --git a/docs/guides/getting-started.md b/docs/guides/getting-started.md index 151dcd11..24ad6480 100644 --- a/docs/guides/getting-started.md +++ b/docs/guides/getting-started.md @@ -188,7 +188,8 @@ Measures duplication in the data. - `duplicate_rows` -- number of exact duplicate rows. A nested value compares as a value, so two objects, or two Arrow maps, with the same entries in a - different order are equal + different order are equal, and a string never equals an object or array, + even one that spells its JSON - `key_uniqueness` -- ratio of distinct values in likely-key columns - `high_cardinality_warning` -- flagged when a column has suspiciously many unique values diff --git a/python/tests/test_nested_duplicate_rows.py b/python/tests/test_nested_duplicate_rows.py index 4c9c5f94..3ba10b72 100644 --- a/python/tests/test_nested_duplicate_rows.py +++ b/python/tests/test_nested_duplicate_rows.py @@ -3,7 +3,9 @@ The JSON readers rendered an object in its source key order, so equal records with their keys in a different order were not duplicates. Arrow and Parquet rendered a nested value as its display string, which matches ``["p, q"]`` with -``["p", "q"]`` and ``[None]`` with ``[]``. +``["p", "q"]`` and ``[None]`` with ``[]``. A container's text is its compact +JSON, which a string can spell, so a string equal to it signed the same row as +the container (#945). ``tests/nested_duplicate_rows.rs`` is the Rust twin. """ @@ -66,6 +68,25 @@ 0, False, ), + # #945: a container crosses as its compact JSON, which a string can spell. + ( + "a string spelling an object against the object", + [{"id": 1, "a": '{"x":1,"y":2}'}, {"id": 1, "a": {"y": 2, "x": 1}}], + 0, + False, + ), + ( + "a string spelling an array against the array", + [{"id": 1, "a": "[1,2]"}, {"id": 1, "a": [1, 2]}], + 0, + False, + ), + ( + "equal strings spelling an object", + [{"id": 1, "a": '{"x":1}'}, {"id": 1, "a": '{"x":1}'}], + 1, + True, + ), ] diff --git a/tests/nested_duplicate_rows.rs b/tests/nested_duplicate_rows.rs index ce604b22..719d0e7d 100644 --- a/tests/nested_duplicate_rows.rs +++ b/tests/nested_duplicate_rows.rs @@ -5,6 +5,8 @@ //! different order were not duplicates. Arrow and Parquet rendered a nested //! value as its display string, which does not quote strings or show a null //! list element, so `["p, q"]` matched `["p", "q"]` and `[null]` matched `[]`. +//! A container's text is its compact JSON, which a string can spell, so a +//! string equal to it signed the same row as the container (#945). //! //! `python/tests/test_nested_duplicate_rows.py` is the Python twin. @@ -22,7 +24,7 @@ struct Case { arrow: bool, } -const CASES: [Case; 6] = [ +const CASES: [Case; 9] = [ Case { name: "key order", records: r#"{"id": 1, "a": {"x": 1, "y": 2}} @@ -67,6 +69,28 @@ const CASES: [Case; 6] = [ duplicates: 0, arrow: false, }, + // #945: a container crosses as its compact JSON, which a string can spell. + Case { + name: "a string spelling an object against the object", + records: r#"{"id": 1, "a": "{\"x\":1,\"y\":2}"} +{"id": 1, "a": {"y": 2, "x": 1}}"#, + duplicates: 0, + arrow: false, + }, + Case { + name: "a string spelling an array against the array", + records: r#"{"id": 1, "a": "[1,2]"} +{"id": 1, "a": [1, 2]}"#, + duplicates: 0, + arrow: false, + }, + Case { + name: "equal strings spelling an object", + records: r#"{"id": 1, "a": "{\"x\":1}"} +{"id": 1, "a": "{\"x\":1}"}"#, + duplicates: 1, + arrow: true, + }, ]; fn file(suffix: &str, contents: &[u8]) -> NamedTempFile { From c1364f23e9fdaee8a5a83915b47d4df1f79870ce Mon Sep 17 00:00:00 2001 From: 1932819331hy-debug <1932819331hy@gmail.com> Date: Sat, 17 Oct 2026 16:47:35 +0900 Subject: [PATCH 04/16] fix(python): refuse malformed values inside quality and type_homogeneity in from_dict (#980) * fix(python): refuse malformed values inside quality and type_homogeneity in from_dict (#950) * fix(python): handle overflowing ints in score validation and allow additive homogeneity properties * fix(python): add _LEXICAL_CLASS_ORDER to stub and narrow report.quality in tests --- python/dataprof/_columns.pyi | 2 + python/dataprof/_report_backing.py | 79 +++++++++++++++---- .../tests/test_canonical_report_document.py | 4 + .../test_flat_loader_malformed_fields.py | 48 ++++++++++- python/tests/test_type_homogeneity.py | 6 +- 5 files changed, 119 insertions(+), 20 deletions(-) diff --git a/python/dataprof/_columns.pyi b/python/dataprof/_columns.pyi index cd49ddc3..0b8a9a57 100644 --- a/python/dataprof/_columns.pyi +++ b/python/dataprof/_columns.pyi @@ -11,6 +11,8 @@ def column_to_dict(col: ColumnProfile | _NativeColumn) -> dict[str, Any]: """Convert a ColumnProfile to the nested dict layout used in ``report.to_dict()['columns']``.""" ... +_LEXICAL_CLASS_ORDER: tuple[str, ...] + def _homogeneity_counts(value: _Any) -> dict[str, int] | None: ... def _type_mixture(col: ColumnProfile) -> list[tuple[str, int, float]]: ... def _column_record(col: ColumnProfile) -> dict[str, _Any]: ... diff --git a/python/dataprof/_report_backing.py b/python/dataprof/_report_backing.py index 14a5a0fe..0e44abe1 100644 --- a/python/dataprof/_report_backing.py +++ b/python/dataprof/_report_backing.py @@ -10,7 +10,7 @@ from typing import Any from ._accessors import ColumnProfile, _Field, _MappingAccessor, _ReportView -from ._columns import _homogeneity_counts +from ._columns import _LEXICAL_CLASS_ORDER, _homogeneity_counts from ._report_schema import _QUALITY_DIMENSIONS # The states a `quality_status` document may declare, and whether each one @@ -121,17 +121,30 @@ def _is_count(value: Any) -> bool: def _is_finite_number(value: Any) -> bool: # bool before int: bool is an int subclass, and True == 1. - return ( - isinstance(value, int | float) - and not isinstance(value, bool) - and math.isfinite(float(value)) - ) + if not isinstance(value, int | float) or isinstance(value, bool): + return False + try: + return math.isfinite(float(value)) + except (OverflowError, ValueError): + return False def _is_list_of_mappings(value: Any) -> bool: return isinstance(value, list) and all(isinstance(entry, dict) for entry in value) +def _is_list_of_dimension_names(value: Any) -> bool: + return isinstance(value, list) and all( + isinstance(entry, str) and entry in _QUALITY_DIMENSIONS for entry in value + ) + + +def _is_homogeneity_counts(value: Any) -> bool: + if not isinstance(value, dict): + return False + return all(name in value and _is_count(value[name]) for name in _LEXICAL_CLASS_ORDER) + + def _column_accessor(document: dict[str, Any], path: str) -> _MappingAccessor: values = dict(document) # `null` is the schema's "not analyzed", the same answer as a missing key. @@ -148,7 +161,17 @@ def _column_accessor(document: dict[str, Any], path: str) -> _MappingAccessor: for name, field in vars(ColumnProfile).items(): if isinstance(field, _Field) and field._document_section == "stats": values[name] = stats.get(name) if stats is not None else None - values["type_homogeneity"] = _homogeneity_counts(document.get("type_homogeneity")) + homogeneity = _read_field( + document, + "type_homogeneity", + f"{path}.type_homogeneity", + "a four-count mapping or null", + lambda value: value is None or _is_homogeneity_counts(value), + None, + ) + values["type_homogeneity"] = ( + _homogeneity_counts(homogeneity) if homogeneity is not None else None + ) patterns = _read_field( document, "patterns", @@ -182,7 +205,16 @@ def _column_accessor(document: dict[str, Any], path: str) -> _MappingAccessor: def _quality_accessor(document: dict[str, Any]) -> _MappingAccessor: values = dict(document) - values["overall_quality_score"] = document.get("overall_score") + overall_score = _read_field( + document, + "overall_score", + "quality.overall_score", + "a finite number or null", + lambda value: value is None or _is_finite_number(value), + None, + ) + values["overall_score"] = overall_score + values["overall_quality_score"] = float(overall_score) if overall_score is not None else None values["low_sample_warning"] = _read_field( document, "low_sample_warning", @@ -191,15 +223,17 @@ def _quality_accessor(document: dict[str, Any]) -> _MappingAccessor: _is_bool, False, ) - values["assessed_dimensions"] = _read_field( - document, - "assessed_dimensions", - "quality.assessed_dimensions", - "a list", - lambda value: isinstance(value, list), - [], + values["assessed_dimensions"] = list( + _read_field( + document, + "assessed_dimensions", + "quality.assessed_dimensions", + "a list of known dimension names", + _is_list_of_dimension_names, + [], + ) ) - values["dimension_scores"] = _read_field( + dimension_scores = _read_field( document, "dimension_scores", "quality.dimension_scores", @@ -207,6 +241,15 @@ def _quality_accessor(document: dict[str, Any]) -> _MappingAccessor: lambda value: isinstance(value, dict), dict.fromkeys(_QUALITY_DIMENSIONS), ) + read_scores = dict(dimension_scores) + for dim, score in dimension_scores.items(): + if score is not None and not _is_finite_number(score): + raise ValueError( + f"from_dict(): 'quality.dimension_scores.{dim}' must be a finite number or null, " + f"got {score!r}." + ) + read_scores[dim] = float(score) if score is not None else None + values["dimension_scores"] = read_scores values["score_weights"] = _read_score_weights(document) return _MappingAccessor(values) @@ -326,7 +369,9 @@ def _report_from_dict(document: dict[str, Any]) -> _ReportView: None, ) values["quality"] = _quality_accessor(quality) if quality is not None else None - values["quality_score"] = quality.get("overall_score") if quality is not None else None + values["quality_score"] = ( + values["quality"].get("overall_quality_score") if values["quality"] is not None else None + ) values["quality_sampled_dimensions"] = ( quality.get("sampled_dimensions") if quality is not None else None ) diff --git a/python/tests/test_canonical_report_document.py b/python/tests/test_canonical_report_document.py index b6065020..9876714e 100644 --- a/python/tests/test_canonical_report_document.py +++ b/python/tests/test_canonical_report_document.py @@ -168,6 +168,10 @@ def test_restored_quality_prints_the_score_it_returns(): def test_legacy_summary_export_agrees_with_what_was_loaded(): summary = dataprof.profile({"x": ["a", "1", None]}).to_dict() summary["columns"][0]["type_homogeneity"] = {"numeric": 1, "text": 1} + with pytest.raises(ValueError, match=r"'columns\[0\]\.type_homogeneity' must be"): + dataprof.ProfileReport.from_dict(summary) + + summary["columns"][0]["type_homogeneity"] = None restored = dataprof.ProfileReport.from_dict(summary) assert restored["x"].type_homogeneity is None for exported in (restored.to_dict(), json.loads(restored.to_json())): diff --git a/python/tests/test_flat_loader_malformed_fields.py b/python/tests/test_flat_loader_malformed_fields.py index 9293c940..0777bcc3 100644 --- a/python/tests/test_flat_loader_malformed_fields.py +++ b/python/tests/test_flat_loader_malformed_fields.py @@ -64,6 +64,18 @@ def _no_quality_status(document: dict[str, Any], value: Any) -> None: #: (field path in the error, how to place the value, malformed values) MALFORMED = [ ("columns[0].stats", _set("columns", 0, "stats"), [[1], "x", 1]), + ( + "columns[0].type_homogeneity", + _set("columns", 0, "type_homogeneity"), + [ + [1], + "x", + 1, + {"numeric": 1}, + {"boolean": "x", "date": 0, "numeric": 0, "text": 0}, + {"boolean": -1, "date": 0, "numeric": 0, "text": 0}, + ], + ), ("columns[1].patterns", _pattern_entry, [1, "x", None]), ("columns[1].patterns", _set("columns", 1, "patterns"), ["x", {}]), ( @@ -72,8 +84,22 @@ def _no_quality_status(document: dict[str, Any], value: Any) -> None: ["x", None, True, float("nan")], ), ("quality.low_sample_warning", _set("quality", "low_sample_warning"), ["false", 0, None]), - ("quality.assessed_dimensions", _set("quality", "assessed_dimensions"), [None, {}, "x"]), + ( + "quality.overall_score", + _set("quality", "overall_score"), + ["x", [1], {}, True, float("nan"), 10**309], + ), + ( + "quality.assessed_dimensions", + _set("quality", "assessed_dimensions"), + [None, {}, "x", [1], ["unknown"]], + ), ("quality.dimension_scores", _set("quality", "dimension_scores"), [[1], None, "x"]), + ( + "quality.dimension_scores.completeness", + _set("quality", "dimension_scores", "completeness"), + ["x", [1], {}, True, float("nan"), 10**309], + ), ("quality.score_bounds", _set("quality", "score_bounds"), ["x", [1]]), ("quality", _no_quality_status, ["x", [1]]), ("execution.ragged_row_count", _set("execution", "ragged_row_count"), [None, -1, "0", True]), @@ -101,7 +127,9 @@ def test_a_malformed_field_is_refused_with_its_path(summary, path, place, value) ((), "semantic_hint_bindings", lambda r: r.semantic_hint_bindings, []), (("quality",), "low_sample_warning", lambda r: r.quality.low_sample_warning, False), (("quality",), "assessed_dimensions", lambda r: r.quality.assessed_dimensions(), []), + (("quality",), "overall_score", lambda r: r.quality.overall_quality_score(), None), (("columns", 0), "stats", lambda r: r["id"].min, None), + (("columns", 0), "type_homogeneity", lambda r: r["id"].type_homogeneity, None), (("columns", 1), "patterns", lambda r: r["city"].patterns, None), ] @@ -121,12 +149,30 @@ def test_a_missing_key_keeps_its_compatibility_default(summary, section, key, re def test_null_still_means_not_analyzed_where_the_schema_allows_it(summary): document = copy.deepcopy(summary) document["columns"][0]["stats"] = None + document["columns"][0]["type_homogeneity"] = None document["columns"][1]["patterns"] = None document["quality"]["score_bounds"] = None + document["quality"]["overall_score"] = None + document["quality"]["dimension_scores"]["completeness"] = None report = dp.ProfileReport.from_dict(document) assert report["id"].min is None + assert report["id"].type_homogeneity is None assert report["city"].patterns is None + assert report.quality is not None + assert report.quality.overall_quality_score() is None + assert report.quality.dimension_scores()["completeness"] is None def test_a_well_formed_summary_round_trips(summary): assert dp.ProfileReport.from_dict(copy.deepcopy(summary)).to_dict() == summary + + +def test_unknown_additive_keys_in_type_homogeneity_are_ignored(summary): + document = copy.deepcopy(summary) + document["columns"][0]["type_homogeneity"]["classifier_provenance"] = "v2" + report = dp.ProfileReport.from_dict(document) + assert report["id"].type_homogeneity is not None + assert ( + report["id"].type_homogeneity["numeric"] + == summary["columns"][0]["type_homogeneity"]["numeric"] + ) diff --git a/python/tests/test_type_homogeneity.py b/python/tests/test_type_homogeneity.py index 053495c4..1e682aad 100644 --- a/python/tests/test_type_homogeneity.py +++ b/python/tests/test_type_homogeneity.py @@ -163,9 +163,10 @@ def test_the_published_schema_rejects_what_the_loader_discards(self): "a partial count map must not validate, because the loader drops it" ) - def test_a_malformed_mapping_reads_as_absence(self): + def test_a_malformed_mapping_is_refused(self): # An invented count is worse than a gap: a partial mapping cannot be # completed with zeros, because zero is a claim about the data. + # It is refused with ValueError naming the field path (#950). document = { "schema_version": dp.REPORT_SCHEMA_VERSION, "source": "hand-edited.csv", @@ -183,7 +184,8 @@ def test_a_malformed_mapping_reads_as_absence(self): "quality": None, } - assert dp.ProfileReport.from_dict(document)["v"].type_homogeneity is None + with pytest.raises(ValueError, match=r"'columns\[0\]\.type_homogeneity' must be"): + dp.ProfileReport.from_dict(document) def test_the_flat_exports_carry_the_derived_scalars(self, tmp_path): # to_dataframe()/save(".csv") are flat and cannot hold the count map, so From 422197cc8d50a4287d9ddd3934604ffa5c9bbabe Mon Sep 17 00:00:00 2001 From: 1932819331hy-debug <1932819331hy@gmail.com> Date: Sat, 17 Oct 2026 18:33:58 +0900 Subject: [PATCH 05/16] fix(python): handle mixed object columns in pandas DataFrames without Arrow errors (#979) * fix(python): handle mixed object columns in pandas DataFrames without Arrow errors * style: format test_pandas_mixed_columns and wrap long lines in _inputs * fix(python): narrow Arrow conversion exception handling and support MultiIndex columns * fix(python): preserve pd.NA and pd.NaT as Arrow nulls in mixed object fallback * fix(python): address review feedback on mixed columns * fix(python): include mixed categorical columns in fallback --- crates/dataprof-python/src/arrow_export.rs | 9 +- python/dataprof/_inputs.py | 63 +++++++ python/tests/test_pandas_mixed_columns.py | 209 +++++++++++++++++++++ 3 files changed, 277 insertions(+), 4 deletions(-) create mode 100644 python/tests/test_pandas_mixed_columns.py diff --git a/crates/dataprof-python/src/arrow_export.rs b/crates/dataprof-python/src/arrow_export.rs index 03ea6374..aacc7f2c 100644 --- a/crates/dataprof-python/src/arrow_export.rs +++ b/crates/dataprof-python/src/arrow_export.rs @@ -954,10 +954,11 @@ fn convert_pandas_to_batches(py: Python<'_>, df: &Bound<'_, PyAny>) -> PyResult< ) })?; - // Convert pandas to pyarrow Table - let pa_table = pyarrow - .getattr("Table")? - .call_method1("from_pandas", (df,))?; + // Convert pandas to pyarrow Table, falling back to string arrays for mixed object columns (#976) + let inputs_mod = py.import("dataprof._inputs")?; + let pa_table = inputs_mod + .getattr("_pandas_to_pyarrow_table")? + .call1((df, &pyarrow))?; import_batches_from_pyarrow(py, &pa_table) } diff --git a/python/dataprof/_inputs.py b/python/dataprof/_inputs.py index 91ebc3b5..31868119 100644 --- a/python/dataprof/_inputs.py +++ b/python/dataprof/_inputs.py @@ -87,6 +87,69 @@ def _cell_to_str(value: object) -> str | None: return str(value) +def _pandas_to_pyarrow_table(df: _Any, pyarrow: _Any) -> _Any: + """Convert a pandas DataFrame to a pyarrow Table, handling mixed object columns. + + Fast path: delegates directly to ``pyarrow.Table.from_pandas(df)``. + Fallback path: for object and categorical columns whose values Arrow cannot + cast into a single type (e.g. numeric with placeholder text or mixed + booleans/dates), converts their cells via ``_cell_to_str`` into string arrays + so they can be profiled with ``type_homogeneity`` rather than aborting (#976). + """ + try: + return pyarrow.Table.from_pandas(df) + except (pyarrow.ArrowInvalid, pyarrow.ArrowTypeError): + # If columns have duplicate labels, pyarrow Table.from_pandas fails with + # ValueError; duplicate column frames cannot be converted. + if hasattr(df.columns, "is_unique") and not df.columns.is_unique: + raise + + failing_cols: set[_Any] = set() + for i in range(len(df.columns)): + col = df.iloc[:, i] + # CategoricalDtype is an extension dtype whose name is "category". + # It can also hold mixed Python values that Arrow cannot cast. + if col.dtype != object and getattr(col.dtype, "name", None) != "category": + continue + name = df.columns[i] + try: + pyarrow.Array.from_pandas(col) + except (pyarrow.ArrowInvalid, pyarrow.ArrowTypeError): + failing_cols.add(name) + + if not failing_cols: + raise + + df_fixed = df.copy(deep=False) + for name in failing_cols: + col = df[name] + try: + if hasattr(col, "isna"): + missing = col.isna() + df_fixed[name] = [ + None if is_m else _cell_to_str(v) for is_m, v in zip(missing, col) + ] + else: + df_fixed[name] = [_cell_to_str(v) for v in col] + except Exception as exc: + raise ValueError( + f"pandas DataFrame: column {name!r} could not be converted for profiling: {exc}" + ) from exc + + try: + return pyarrow.Table.from_pandas(df_fixed) + except Exception as exc: + for name in failing_cols: + try: + pyarrow.Array.from_pandas(df_fixed[name]) + except (pyarrow.ArrowInvalid, pyarrow.ArrowTypeError) as col_exc: + raise ValueError( + f"pandas DataFrame: column {name!r} could not be converted for profiling: " + f"{col_exc}" + ) from col_exc + raise ValueError(f"pandas DataFrame conversion failed: {exc}") from exc + + def _columns_from_dict(source: dict[_Any, _Any]) -> list[_Column]: """Convert a dict of equal-length sequences into columns.""" columns: list[_Column] = [] diff --git a/python/tests/test_pandas_mixed_columns.py b/python/tests/test_pandas_mixed_columns.py new file mode 100644 index 00000000..188415e7 --- /dev/null +++ b/python/tests/test_pandas_mixed_columns.py @@ -0,0 +1,209 @@ +"""A pandas object column with mixed Python types must profile cleanly (#976). + +When a pandas DataFrame contains object columns with mixed Python types +(such as numeric columns with placeholder strings, or mixed booleans/dates), +pyarrow cannot cast them into a single physical Arrow type and fails with +``ArrowInvalid`` or ``ArrowTypeError``. + +Rather than aborting the entire profile, such columns are profiled from +their values' text (as ``list[dict]`` input does), with ``type_homogeneity`` +recording the mix, while the other typed columns in the frame keep their +native Arrow-derived profiles unchanged. +""" + +from __future__ import annotations + +import datetime +import decimal + +import dataprof as dp +import pytest + +pd = pytest.importorskip("pandas", reason="pandas is required for pandas DataFrame tests") +pa = pytest.importorskip("pyarrow", reason="pyarrow is required for Arrow import tests") + + +def test_pandas_mixed_object_columns_profile_cleanly(): + """Match list-of-dicts profiles for each mixed-column case from #976.""" + cases = [ + [1, "2", 3], + [1.5, "n/a", 2.0], + [True, "yes", False], + [datetime.date(2024, 1, 1), "unknown", datetime.date(2024, 1, 2)], + ["a", None, "b"], + [1, decimal.Decimal("2.5"), 3], + ] + + for values in cases: + df = pd.DataFrame({"c": pd.Series(values, dtype=object)}) + dataframe_column = dp.profile(df)["c"] + records_column = dp.profile([{"c": value} for value in values])["c"] + + assert dataframe_column.data_type == records_column.data_type + assert dataframe_column.type_homogeneity == records_column.type_homogeneity + + +def test_pandas_mixed_categorical_column_matches_records_route(): + """Mixed categorical values use the same profile as list-of-dicts input.""" + values = [1.5, "n/a", 2.0] + df = pd.DataFrame({"c": pd.Categorical(values)}) + original = df.copy(deep=True) + + dataframe_column = dp.profile(df)["c"] + records_column = dp.profile([{"c": value} for value in values])["c"] + + assert dataframe_column.data_type == records_column.data_type + assert dataframe_column.type_homogeneity == records_column.type_homogeneity + assert df.equals(original) + + +def test_frame_with_mixed_column_and_typed_columns_preserves_typed_profiles(): + """Typed columns keep their native Arrow-derived profiles alongside a mixed column.""" + df_clean = pd.DataFrame( + { + "id": [1, 2, 3], + "score": [95.5, 82.0, 77.5], + "name": ["Alice", "Bob", "Charlie"], + } + ) + clean_report = dp.profile(df_clean) + + df_mixed = pd.DataFrame( + { + "id": [1, 2, 3], + "score": [95.5, 82.0, 77.5], + "name": ["Alice", "Bob", "Charlie"], + "notes": pd.Series([1.5, "n/a", 2.0], dtype=object), + } + ) + mixed_report = dp.profile(df_mixed) + + for col in ("id", "score", "name"): + assert mixed_report[col].data_type == clean_report[col].data_type + assert mixed_report[col].null_count == clean_report[col].null_count + assert mixed_report[col].total_count == clean_report[col].total_count + + assert mixed_report["notes"].data_type == "float" + assert mixed_report["notes"].type_homogeneity == { + "boolean": 0, + "date": 0, + "numeric": 2, + "text": 0, + } + + +def test_unconvertible_column_raises_value_error_naming_column(): + """If a column genuinely cannot be converted, raise ValueError naming the column.""" + + class Unconvertible: + def __str__(self): + raise RuntimeError("cannot stringify object") + + df = pd.DataFrame({"bad_column": [Unconvertible()]}) + with pytest.raises(ValueError, match="column 'bad_column'"): + dp.profile(df) + + +def test_interop_profile_dataframe_mixed_columns(): + """Verify that lower-level interop.profile_dataframe also handles mixed columns.""" + from dataprof.interop import profile_dataframe + + df = pd.DataFrame( + { + "c": pd.Series([1.5, "unknown", 2.0], dtype=object), + } + ) + raw_report = profile_dataframe(df) + report = dp.ProfileReport(raw_report) + assert report["c"].data_type == "string" + assert report["c"].type_homogeneity == { + "boolean": 0, + "date": 0, + "numeric": 2, + "text": 1, + } + + +def test_pandas_multiindex_columns_with_mixed_objects(): + """Verify that DataFrames with tuple/MultiIndex columns and mixed objects profile.""" + df = pd.DataFrame( + { + ("metrics", "score"): pd.Series([1.5, "n/a", 2.0], dtype=object), + ("metrics", "count"): [10, 20, 30], + } + ) + report = dp.profile(df) + score_col = "('metrics', 'score')" + assert report[score_col].data_type == "float" + assert report[score_col].type_homogeneity == { + "boolean": 0, + "date": 0, + "numeric": 2, + "text": 0, + } + + +def test_pandas_duplicate_columns_raises_clear_error(): + """Duplicate column labels fail cleanly and are not misclassified as mixed objects.""" + df = pd.DataFrame([[1, 2]], columns=["a", "a"]) + with pytest.raises(Exception) as exc_info: + dp.profile(df) + assert "Duplicate column names found" in str(exc_info.value) + + +def test_pandas_mixed_columns_preserve_missing_values_as_null(): + """Verify that pd.NA and pd.NaT in mixed object columns become Arrow nulls, + not string literals. + """ + from dataprof._inputs import _pandas_to_pyarrow_table + + df = pd.DataFrame( + { + "id": [10, 20, 30], + "mixed_na": pd.Series([1, "two", pd.NA], dtype=object), + "mixed_nat": pd.Series([1, "two", pd.NaT], dtype=object), + } + ) + original = df.copy(deep=True) + + tbl = _pandas_to_pyarrow_table(df, pa) + assert df.equals(original) + assert tbl["mixed_na"].to_pylist() == ["1", "two", None] + assert tbl["mixed_na"].null_count == 1 + assert tbl["mixed_nat"].to_pylist() == ["1", "two", None] + assert tbl["mixed_nat"].null_count == 1 + + # Neighboring typed column retains native Arrow integer type and values + assert pa.types.is_integer(tbl["id"].type) + assert tbl["id"].to_pylist() == [10, 20, 30] + + report = dp.profile(df) + assert report["mixed_na"].null_count == 1 + assert report["mixed_na"].total_count == 3 + assert report["mixed_na"].type_homogeneity == { + "boolean": 0, + "date": 0, + "numeric": 1, + "text": 1, + } + assert report["mixed_nat"].null_count == 1 + assert report["mixed_nat"].total_count == 3 + assert report["id"].data_type == "integer" + assert report["id"].null_count == 0 + + +def test_pandas_mixed_columns_do_not_misclassify_containers_as_null(): + """Verify container objects like dict and list in mixed columns are not + misclassified as null. + """ + from dataprof._inputs import _pandas_to_pyarrow_table + + df = pd.DataFrame( + { + "mixed_containers": pd.Series([1, "two", {"key": "val"}, [1, 2]], dtype=object), + } + ) + + tbl = _pandas_to_pyarrow_table(df, pa) + assert tbl["mixed_containers"].to_pylist() == ["1", "two", '{"key":"val"}', "[1,2]"] + assert tbl["mixed_containers"].null_count == 0 From 0d8d6ad0aa7d343334b487349076bca96b97d83f Mon Sep 17 00:00:00 2001 From: Andrea Bozzo Date: Sat, 17 Oct 2026 11:41:07 +0200 Subject: [PATCH 06/16] feat: compute compare() with a typed Rust report delta (#713) (#981) * feat: compute compare() with a typed Rust report delta (#713) ReportDelta compares two reports in Rust, and ProfileReport.compare() returns its document. The keys compare() already returned keep their names and values; each score gains per-side evidence and the result gains report references. ReportSide reads a flat summary, so reports loaded from to_dict() output or pre-0.12 files still compare. * fix: compare legacy summaries and repeated column names in the report delta (#713) A flat summary that does not record its scan, or the weights behind its overall score, reads as coverage_unrecorded instead of failing or being certified. A repeated column name is listed once with its first profile, as the quality gate reads it. * fix: leave an overall score with an unknown sampled component unrecorded (#713) A dimension a later release added carries weight in the stored overall score; when its component was sampled, the delta no longer certifies the score as complete on either the native or the flat-summary route. --- .../dataprof/reference/interpretation.md | 3 +- crates/dataprof-python/src/lib.rs | 3 +- crates/dataprof-python/src/types.rs | 31 + crates/dataprof-runtime/src/lib.rs | 5 + crates/dataprof-runtime/src/profile_report.rs | 17 + crates/dataprof-runtime/src/quality_gate.rs | 58 +- crates/dataprof-runtime/src/report_delta.rs | 632 ++++++++++++++++++ crates/dataprof/src/lib.rs | 4 + docs/python/README.md | 55 +- examples/before_after_cleaning.rs | 50 +- python/dataprof/_dataprof.pyi | 1 + python/dataprof/_report.py | 136 ++-- python/tests/test_report_delta.py | 154 +++++ tests/public_api_facade.rs | 95 +++ tests/report_delta.rs | 397 +++++++++++ 15 files changed, 1487 insertions(+), 154 deletions(-) create mode 100644 crates/dataprof-runtime/src/report_delta.rs create mode 100644 python/tests/test_report_delta.py create mode 100644 tests/report_delta.rs diff --git a/.claude/skills/dataprof/reference/interpretation.md b/.claude/skills/dataprof/reference/interpretation.md index f2363940..f598dbf0 100644 --- a/.claude/skills/dataprof/reference/interpretation.md +++ b/.claude/skills/dataprof/reference/interpretation.md @@ -181,7 +181,8 @@ with every locale-specific pattern suppressed. `before.compare(after)` returns deltas. Three caveats: - Comparing a sampled profile to a full one measures your sampling, not drift. - Check `sampling_applied` on both sides first. + Each score's `evidence` and `reports.a`/`reports.b` `evidence` say whether a + side was truncated, sampled or scored from a sample; read them first. - Comparing profiles with different `metrics` selections produces absence, not change. Narrow both sides the same way. - A stored report can predate a change in how something is measured: text diff --git a/crates/dataprof-python/src/lib.rs b/crates/dataprof-python/src/lib.rs index 1e71bb25..bff25eac 100644 --- a/crates/dataprof-python/src/lib.rs +++ b/crates/dataprof-python/src/lib.rs @@ -32,7 +32,7 @@ pub use partial::{ pub use progress::PyProgressEvent; pub use sampling::PySamplingStrategy; pub use stop_condition::PyStopCondition; -pub use types::{PyColumnProfile, PyDataQualityMetrics, PyProfileReport}; +pub use types::{PyColumnProfile, PyDataQualityMetrics, PyProfileReport, report_delta_json}; use pyo3::prelude::*; use pyo3::types::{PyDict, PyModule}; @@ -97,6 +97,7 @@ pub fn dataprof(m: &Bound<'_, PyModule>) -> PyResult<()> { m.add_function(wrap_pyfunction!(analyze_file, m)?)?; m.add_function(wrap_pyfunction!(profile_parquet_bytes, m)?)?; m.add_function(wrap_pyfunction!(list_patterns, m)?)?; + m.add_function(wrap_pyfunction!(report_delta_json, m)?)?; // DataFrame/Arrow profiling m.add_function(wrap_pyfunction!(profile_dataframe, m)?)?; diff --git a/crates/dataprof-python/src/types.rs b/crates/dataprof-python/src/types.rs index c60c15ef..d79a3458 100644 --- a/crates/dataprof-python/src/types.rs +++ b/crates/dataprof-python/src/types.rs @@ -1330,3 +1330,34 @@ impl PyProfileReport { ) } } + +/// One side of `report_delta_json`: a native report, or the flat summary of +/// a report loaded from one, as JSON. +#[derive(FromPyObject)] +pub enum DeltaInput<'py> { + Native(PyRef<'py, PyProfileReport>), + Summary(String), +} + +impl DeltaInput<'_> { + fn side(&self) -> PyResult { + match self { + Self::Native(report) => Ok(dataprof::ReportSide::from_report(&report.inner)), + Self::Summary(text) => dataprof::ReportSide::from_summary_json(text).map_err(|e| { + pyo3::exceptions::PyValueError::new_err(format!( + "compare(): the flat summary cannot be compared: {e}" + )) + }), + } + } +} + +/// Compare two reports with the runtime delta and return its document as +/// JSON. `ProfileReport.compare()` converts it into the dict it returns. +#[pyfunction] +pub fn report_delta_json(a: DeltaInput<'_>, b: DeltaInput<'_>) -> PyResult { + let delta = dataprof::ReportDelta::between_sides(&a.side()?, &b.side()?); + serde_json::to_string(&delta).map_err(|e| { + pyo3::exceptions::PyRuntimeError::new_err(format!("JSON serialization failed: {e}")) + }) +} diff --git a/crates/dataprof-runtime/src/lib.rs b/crates/dataprof-runtime/src/lib.rs index d809b1c3..d4f373dd 100644 --- a/crates/dataprof-runtime/src/lib.rs +++ b/crates/dataprof-runtime/src/lib.rs @@ -8,6 +8,7 @@ pub mod profile_builder; pub mod profile_report; pub mod quality_gate; pub mod report_assembler; +pub mod report_delta; pub mod streaming_stats; #[cfg(feature = "async-streaming")] @@ -37,6 +38,10 @@ pub use quality_gate::{ MetricValue, NotEvaluated, PolicyError, PolicyScope, QualityPolicy, RequiredMetric, Verdict, }; pub use report_assembler::ReportAssembler; +pub use report_delta::{ + ColumnChange, DimensionChange, ReportDelta, ReportReference, ReportSide, SchemaChange, + ScoreChange, SemanticsComparison, Sides, +}; pub use streaming_stats::{ RowCompletenessTracker, RowSignature, RowUniquenessTracker, StreamReservoirSampler, StreamingColumnCollection, StreamingStatistics, TextLengthStats, diff --git a/crates/dataprof-runtime/src/profile_report.rs b/crates/dataprof-runtime/src/profile_report.rs index 66a1af53..54d9437b 100644 --- a/crates/dataprof-runtime/src/profile_report.rs +++ b/crates/dataprof-runtime/src/profile_report.rs @@ -144,6 +144,23 @@ impl MetricSemantics { shape_moments: Some(ShapeMomentEstimator::AdjustedSample), } } + + /// Whether every definition this build knows is recorded. A comparison + /// can only vouch for definitions both reports declare, so an unrecorded + /// one leaves it unknown. Destructured, so a new field fails to compile + /// here until it is counted. + pub(crate) fn is_fully_recorded(&self) -> bool { + let Self { + text_length_unit, + null_tokens, + pattern_match_count, + shape_moments, + } = self; + text_length_unit.is_some() + && null_tokens.is_some() + && pattern_match_count.is_some() + && shape_moments.is_some() + } } /// Complete profiling report for a data source. diff --git a/crates/dataprof-runtime/src/quality_gate.rs b/crates/dataprof-runtime/src/quality_gate.rs index 7673f211..771d6a8b 100644 --- a/crates/dataprof-runtime/src/quality_gate.rs +++ b/crates/dataprof-runtime/src/quality_gate.rs @@ -169,10 +169,11 @@ pub enum EvidenceGap { /// Every row was scanned, but this metric was computed from a retained /// sample of them. QualitySampled, - /// The report does not record how its quality numbers were obtained, so - /// whether they cover every scanned row is unknown. Only reachable for a - /// report read back from a document written before dataprof recorded it; - /// a profiling run always records it. Unknown coverage is not full + /// The report does not record how its numbers were obtained: which quality + /// components were sampled, or, in a flat summary from an older release, + /// whether the scan read the whole source. Only reachable for a report + /// read back from a document written before dataprof recorded it; a + /// profiling run always records it. Unknown coverage is not full /// coverage, so a full-source requirement is left unevaluated. CoverageUnrecorded, } @@ -221,7 +222,7 @@ impl Evidence { /// The weaker of two evidence statements: a number is only as complete as /// the least complete input behind it. The left-hand gap wins a tie, so /// the scan's gap is named ahead of the quality sample's. - fn and(self, other: Self) -> Self { + pub(crate) fn and(self, other: Self) -> Self { match self { Self::Complete => other, incomplete => incomplete, @@ -1172,12 +1173,22 @@ fn null_percentage(profile: &ColumnProfile) -> Option { /// /// Truncation is named ahead of sampling and sampling ahead of skipped /// records, so the gap reported is the largest one. -fn scan_evidence(execution: &ExecutionMetadata) -> Evidence { - let reason = if !execution.source_exhausted || execution.truncation_reason.is_some() { +pub(crate) fn scan_evidence(execution: &ExecutionMetadata) -> Evidence { + scan_coverage( + !execution.source_exhausted || execution.truncation_reason.is_some(), + execution.sampling_applied, + execution.error_count > 0, + ) +} + +/// [`scan_evidence`] over the three recorded facts it reads, for a report +/// delta reading them from a flat summary. +pub(crate) fn scan_coverage(truncated: bool, sampled: bool, records_skipped: bool) -> Evidence { + let reason = if truncated { Some(EvidenceGap::Truncated) - } else if execution.sampling_applied { + } else if sampled { Some(EvidenceGap::Sampled) - } else if execution.error_count > 0 { + } else if records_skipped { Some(EvidenceGap::RecordsSkipped) } else { None @@ -1191,7 +1202,7 @@ fn scan_evidence(execution: &ExecutionMetadata) -> Evidence { /// Which part of the assessment a check reads, and so whose provenance /// decides whether its number covers every scanned row. #[derive(Debug, Clone, Copy)] -enum Provenance { +pub(crate) enum Provenance { /// The aggregate over every dimension. Overall, /// One dimension, through whichever components it is built from. @@ -1205,7 +1216,7 @@ enum Provenance { /// Uniqueness is the one dimension with two, and they can differ in /// provenance: a full-stream row tracker counts duplicates over every row /// while the key scan reads the retained sample. -fn dimension_components(dimension: QualityDimension) -> &'static [&'static str] { +pub(crate) fn dimension_components(dimension: QualityDimension) -> &'static [&'static str] { match dimension { QualityDimension::Completeness => &["completeness"], QualityDimension::Consistency => &["consistency"], @@ -1228,7 +1239,23 @@ fn quality_evidence(quality: Option<&QualityAssessment>, provenance: Provenance) // absence itself, and there is no number for evidence to describe. return Evidence::Complete; }; - let Some(sampled) = quality.sampled_dimensions() else { + sampled_component_evidence( + quality.sampled_dimensions().as_deref(), + &quality.metrics.assessed_dimensions(), + provenance, + ) +} + +/// [`quality_evidence`] over the recorded facts it reads: the sampled +/// component labels (`None` when the report did not record them) and the +/// dimensions the overall score weighs. A report delta reads the same facts +/// from a flat summary, which has no [`QualityAssessment`] behind it. +pub(crate) fn sampled_component_evidence( + sampled: Option<&[String]>, + assessed: &[QualityDimension], + provenance: Provenance, +) -> Evidence { + let Some(sampled) = sampled else { return Evidence::Incomplete { reason: EvidenceGap::CoverageUnrecorded, }; @@ -1239,10 +1266,9 @@ fn quality_evidence(quality: Option<&QualityAssessment>, provenance: Provenance) // dimensions, so a sampled dimension the weights exclude does not // reach it. Reporting the aggregate as sampled because of one would // withhold a verdict the number does not depend on. - Provenance::Overall => quality - .metrics - .assessed_dimensions() - .into_iter() + Provenance::Overall => assessed + .iter() + .copied() .flat_map(dimension_components) .any(|component| contains(component)), Provenance::Dimension(dimension) => dimension_components(dimension) diff --git a/crates/dataprof-runtime/src/report_delta.rs b/crates/dataprof-runtime/src/report_delta.rs new file mode 100644 index 00000000..8958d121 --- /dev/null +++ b/crates/dataprof-runtime/src/report_delta.rs @@ -0,0 +1,632 @@ +//! What changed between two reports. +//! +//! A pipeline that keeps a baseline report asks the same questions of every +//! new one: did quality move, did a column appear or vanish, did a column's +//! null rate change. [`ReportDelta`] answers them in one typed value, so the +//! answer does not depend on which language computed it, and so a policy over +//! drift can be written against a contract rather than a dictionary. +//! +//! # The contract +//! +//! The two reports are `a` and `b`, compared in that order. The serialized +//! shape is the one `ProfileReport.compare()` returns in Python, and it is +//! stable: later releases may add keys, never remove or rename one. +//! +//! - **Signs.** Every change is `b - a`. A positive `abs` on a score is an +//! improvement, because a score is higher when quality is better. A positive +//! `null_pct_delta` is a degradation, because it is more nulls. +//! - **Precision.** Each side's number is taken at its serialized precision +//! (2 decimals on a 0-100 scale), and the change is rounded to it too, so a +//! report and its saved document compare alike. +//! - **No invented numbers.** A number one side does not carry, an +//! unassessed dimension, an empty column or an empty report, leaves the +//! change `None`. `rel_pct` is `None` as well when `a` is zero, where a +//! relative change is undefined. +//! - **Provenance.** Each number carries the [`Evidence`] behind it on both +//! sides, so an exact baseline compared with a sampled or truncated run says +//! so instead of reading a sampling difference as a change in the data. +//! - **Definitions.** `metric_semantics.comparable` is `Some(true)` only when +//! both reports record every definition this build knows, with the same +//! values. A difference in a measurement whose definition changed between +//! releases is not a difference in the data. +//! +//! Nothing here decides whether a change is acceptable; that is a policy's +//! job. A window-over-window comparison reuses the same vocabulary: `a` and +//! `b` name the two sides without saying which came first. +//! +//! # Example +//! +//! ```no_run +//! use dataprof_runtime::{ProfileReport, ReportDelta}; +//! +//! fn drift(baseline: &ProfileReport, current: &ProfileReport) { +//! let delta = ReportDelta::between(baseline, current); +//! if let Some(change) = delta.quality_score.abs { +//! println!("quality moved {change:+.2} points"); +//! } +//! for name in &delta.schema.removed { +//! println!("column {name} is gone"); +//! } +//! } +//! ``` + +use std::collections::{BTreeMap, HashSet}; + +use dataprof_core::QualityDimension; +use dataprof_core::serde_helpers::rounded_2; +use serde::ser::{SerializeMap, Serializer}; + +use crate::profile_report::{MetricSemantics, ProfileReport}; +use crate::quality_gate::{ + Evidence, EvidenceGap, Provenance, dimension_components, sampled_component_evidence, + scan_coverage, scan_evidence, +}; + +/// The same fact about each side of a comparison. +#[derive(Debug, Clone, PartialEq, Eq, serde::Serialize)] +#[non_exhaustive] +pub struct Sides { + /// The first report. + pub a: T, + /// The second report. + pub b: T, +} + +/// Which report a side is, and how much of its source it read. +#[derive(Debug, Clone, PartialEq, Eq, serde::Serialize)] +#[non_exhaustive] +pub struct ReportReference { + /// The report's identifier. `None` for a flat summary, which does not + /// record one. + pub id: Option, + /// When the report was generated. `None` for a flat summary. + pub timestamp: Option, + /// The source identifier: a path, a query, or an in-memory label. + pub source: String, + /// Rows the scan read. `None` for a flat summary that did not record it. + pub rows_processed: Option, + /// Whether the scan covered the whole source. + pub evidence: Evidence, +} + +/// One score on both sides and how it moved. +#[derive(Debug, Clone, PartialEq, serde::Serialize)] +#[non_exhaustive] +pub struct ScoreChange { + /// The score in `a`, or `None` when `a` did not assess it. + pub a: Option, + /// The score in `b`, or `None` when `b` did not assess it. + pub b: Option, + /// `b - a` in score points, when both sides have a score. + pub abs: Option, + /// `b - a` as a percentage of `a`, when both have a score and `a` is not + /// zero. + pub rel_pct: Option, + /// The evidence behind each side's score: the scan's coverage, and + /// whether the score was computed from a retained sample of the rows. + pub evidence: Sides, +} + +impl ScoreChange { + fn new(a: Option, b: Option, evidence: Sides) -> Self { + let (abs, rel_pct) = match (a, b) { + (Some(a), Some(b)) => ( + Some(rounded_2(b - a)), + (a != 0.0).then(|| rounded_2((b - a) / a.abs() * 100.0)), + ), + _ => (None, None), + }; + Self { + a, + b, + abs, + rel_pct, + evidence, + } + } +} + +/// One quality dimension's change. Serialized as an entry of the +/// `dimensions` object, keyed by the dimension's name. +#[derive(Debug, Clone, PartialEq)] +#[non_exhaustive] +pub struct DimensionChange { + /// The dimension. + pub dimension: QualityDimension, + /// Its score on both sides. + pub score: ScoreChange, +} + +/// One column's change. Serialized as an entry of the `columns` object, keyed +/// by the column's name. Each side's number covers what that side's scan +/// read; see [`ReportReference::evidence`]. +#[derive(Debug, Clone, PartialEq, serde::Serialize)] +#[non_exhaustive] +pub struct ColumnChange { + /// The column's name. + #[serde(skip)] + pub name: String, + /// Null percentage in `a`, or `None` when `a` has no such column or the + /// column holds no values. + pub null_pct_a: Option, + /// Null percentage in `b`, under the same rules. + pub null_pct_b: Option, + /// `null_pct_b - null_pct_a`, when both are present. Positive means more + /// nulls. + pub null_pct_delta: Option, +} + +/// Each side's recorded measurement definitions, and whether they agree. +#[derive(Debug, Clone, PartialEq, Eq, serde::Serialize)] +#[non_exhaustive] +pub struct SemanticsComparison { + /// What `a` recorded, or `None` when it predates the record. + pub a: Option, + /// What `b` recorded. + pub b: Option, + /// `Some(true)` when both record every definition this build knows and + /// they match; `Some(false)` when they record different ones; `None` + /// when either side leaves a definition unrecorded, which includes every + /// report written before 0.12. + pub comparable: Option, +} + +/// Which columns each side has. +#[derive(Debug, Clone, PartialEq, Eq, serde::Serialize)] +#[non_exhaustive] +pub struct SchemaChange { + /// In `b` only, in `b`'s order. + pub added: Vec, + /// In `a` only, in `a`'s order. + pub removed: Vec, + /// In both, in `a`'s order. + pub common: Vec, +} + +/// What changed between two reports. See the [module docs](self) for the +/// contract. +#[derive(Debug, Clone, PartialEq, serde::Serialize)] +#[non_exhaustive] +pub struct ReportDelta { + /// The overall quality score. + pub quality_score: ScoreChange, + /// Every quality dimension this build knows, in [`QualityDimension::all`] + /// order, assessed or not. + #[serde(serialize_with = "serialize_dimensions")] + pub dimensions: Vec, + /// The union of both sides' columns: `a`'s in its order, then the ones + /// only `b` has, in `b`'s order. + #[serde(serialize_with = "serialize_columns")] + pub columns: Vec, + /// Whether the two reports measured the same way. + pub metric_semantics: SemanticsComparison, + /// Columns added, removed and shared. + pub schema: SchemaChange, + /// Which two reports were compared. + pub reports: Sides, +} + +impl ReportDelta { + /// Compare two reports, `a` first. + pub fn between(a: &ProfileReport, b: &ProfileReport) -> Self { + Self::between_sides(&ReportSide::from_report(a), &ReportSide::from_report(b)) + } + + /// Compare two sides, either of which may come from a flat summary. + pub fn between_sides(a: &ReportSide, b: &ReportSide) -> Self { + let pair = |a: Evidence, b: Evidence| Sides { a, b }; + let quality_score = ScoreChange::new( + a.quality_score, + b.quality_score, + pair(a.overall_evidence, b.overall_evidence), + ); + let dimensions = a + .dimensions + .iter() + .zip(&b.dimensions) + .map(|(left, right)| { + debug_assert_eq!(left.dimension, right.dimension); + DimensionChange { + dimension: left.dimension, + score: ScoreChange::new( + left.score, + right.score, + pair(left.evidence, right.evidence), + ), + } + }) + .collect(); + + let a_names: HashSet<&str> = a.columns.iter().map(|(name, _)| name.as_str()).collect(); + let b_names: HashSet<&str> = b.columns.iter().map(|(name, _)| name.as_str()).collect(); + let a_nulls: BTreeMap<&str, Option> = a + .columns + .iter() + .map(|(name, pct)| (name.as_str(), *pct)) + .collect(); + let b_nulls: BTreeMap<&str, Option> = b + .columns + .iter() + .map(|(name, pct)| (name.as_str(), *pct)) + .collect(); + let added: Vec = b + .columns + .iter() + .filter(|(name, _)| !a_names.contains(name.as_str())) + .map(|(name, _)| name.clone()) + .collect(); + let columns = a + .columns + .iter() + .map(|(name, _)| name) + .chain(&added) + .map(|name| { + let null_pct_a = a_nulls.get(name.as_str()).copied().flatten(); + let null_pct_b = b_nulls.get(name.as_str()).copied().flatten(); + ColumnChange { + name: name.clone(), + null_pct_a, + null_pct_b, + null_pct_delta: null_pct_a.zip(null_pct_b).map(|(a, b)| rounded_2(b - a)), + } + }) + .collect(); + let (removed, common) = a + .columns + .iter() + .map(|(name, _)| name.clone()) + .partition(|name| !b_names.contains(name.as_str())); + + let comparable = match (&a.metric_semantics, &b.metric_semantics) { + (Some(left), Some(right)) if left.is_fully_recorded() && right.is_fully_recorded() => { + Some(left == right) + } + _ => None, + }; + + Self { + quality_score, + dimensions, + columns, + metric_semantics: SemanticsComparison { + a: a.metric_semantics.clone(), + b: b.metric_semantics.clone(), + comparable, + }, + schema: SchemaChange { + added, + removed, + common, + }, + reports: Sides { + a: a.reference.clone(), + b: b.reference.clone(), + }, + } + } +} + +fn serialize_dimensions( + dimensions: &[DimensionChange], + serializer: S, +) -> Result { + let mut map = serializer.serialize_map(Some(dimensions.len()))?; + for change in dimensions { + map.serialize_entry(&change.dimension.to_string(), &change.score)?; + } + map.end() +} + +fn serialize_columns( + columns: &[ColumnChange], + serializer: S, +) -> Result { + let mut map = serializer.serialize_map(Some(columns.len()))?; + for change in columns { + map.serialize_entry(&change.name, change)?; + } + map.end() +} + +/// What a comparison reads from one report. +/// +/// Built from a [`ProfileReport`], or from a flat summary (the document +/// [`ProfileReport::summary_json`] writes, and the one Python's `to_dict()` +/// and pre-0.12 `save()` produced). A flat summary never becomes a +/// `ProfileReport`, because it cannot acquire provenance it never recorded, +/// but it carries everything a comparison reads, and what it did not record +/// is read as unrecorded. +#[derive(Debug, Clone, PartialEq)] +#[non_exhaustive] +pub struct ReportSide { + reference: ReportReference, + quality_score: Option, + overall_evidence: Evidence, + dimensions: Vec, + columns: Vec<(String, Option)>, + metric_semantics: Option, +} + +#[derive(Debug, Clone, PartialEq)] +struct DimensionSide { + dimension: QualityDimension, + score: Option, + evidence: Evidence, +} + +/// One entry per column name, in order, holding the first profile of a +/// repeated name. Most input paths refuse duplicate names, but a report +/// rebuilt from a document can carry them, and the quality gate reads the +/// first profile too, so the two never disagree about which one a name means. +fn first_of_each_name( + columns: impl Iterator)>, +) -> Vec<(String, Option)> { + let mut seen = HashSet::new(); + columns + .filter(|(name, _)| seen.insert(name.clone())) + .collect() +} + +/// The overall score's coverage when `sampled` names a component this build +/// does not know: a dimension a later release added, whose weight is in the +/// stored overall score. Nothing rules out that it reaches the score, so the +/// score is not certified; `None` when every sampled label is known. +fn unknown_component_coverage(sampled: Option<&[String]>) -> Option { + let known = |label: &String| { + QualityDimension::all() + .into_iter() + .flat_map(dimension_components) + .any(|component| component == label) + }; + sampled + .is_some_and(|labels| !labels.iter().all(known)) + .then_some(Evidence::Incomplete { + reason: EvidenceGap::CoverageUnrecorded, + }) +} + +/// A number at its serialized precision; a non-finite one serializes as +/// null, so it is no number at all. +fn at_precision(value: Option) -> Option { + value.filter(|value| value.is_finite()).map(rounded_2) +} + +impl ReportSide { + /// Read a profiled or loaded report. + pub fn from_report(report: &ProfileReport) -> Self { + let scan = scan_evidence(&report.execution); + let quality = report.quality.as_ref(); + let scores = quality.map(|assessment| assessment.scores()); + let sampled = quality.map(|assessment| assessment.sampled_dimensions()); + let assessed = quality + .map(|assessment| assessment.metrics.assessed_dimensions()) + .unwrap_or_default(); + // No assessment means no score for evidence to describe; the scan's + // coverage is all there is to say. + let evidence = |provenance| match &sampled { + Some(sampled) => scan.and(sampled_component_evidence( + sampled.as_deref(), + &assessed, + provenance, + )), + None => scan, + }; + let dimensions = QualityDimension::all() + .into_iter() + .map(|dimension| DimensionSide { + dimension, + score: at_precision( + scores + .as_ref() + .and_then(|scores| scores.dimension_scores.get(&dimension.to_string())) + .copied() + .flatten(), + ), + evidence: evidence(Provenance::Dimension(dimension)), + }) + .collect(); + Self { + reference: ReportReference { + id: Some(report.id.clone()), + timestamp: Some(report.timestamp.clone()), + source: report.source_identifier(), + rows_processed: Some(report.execution.rows_processed), + evidence: scan, + }, + quality_score: at_precision(scores.as_ref().and_then(|scores| scores.overall_score)), + overall_evidence: unknown_component_coverage(sampled.clone().flatten().as_deref()) + .map_or_else(|| evidence(Provenance::Overall), |gap| scan.and(gap)), + dimensions, + columns: first_of_each_name(report.column_profiles.iter().map(|column| { + let pct = (column.total_count > 0) + .then(|| column.null_count as f64 / column.total_count as f64 * 100.0); + (column.name.clone(), at_precision(pct)) + })), + metric_semantics: report.metric_semantics.clone(), + } + } + + /// Read a flat summary document. + /// + /// A field an older release did not write reads as unrecorded: scan + /// coverage it does not state is `coverage_unrecorded`, never complete. + /// Fails when the document lacks `source` or `columns`, or holds a + /// field of the wrong type. + pub fn from_summary_json(text: &str) -> serde_json::Result { + let summary: Summary = serde_json::from_str(text)?; + let execution = &summary.execution; + let known_gap = scan_coverage( + execution.source_exhausted == Some(false) || execution.truncation_reason.is_some(), + execution.sampling_applied == Some(true), + execution.error_count.is_some_and(|count| count > 0), + ); + let recorded = execution.source_exhausted.is_some() + && execution.sampling_applied.is_some() + && execution.error_count.is_some(); + // A recorded gap is named; otherwise an unrecorded fact is a gap of + // its own, because unknown coverage is not full coverage. + let scan = match known_gap { + Evidence::Complete if !recorded => Evidence::Incomplete { + reason: EvidenceGap::CoverageUnrecorded, + }, + known => known, + }; + let quality = summary.quality.as_ref(); + // The dimensions the overall score weighs. An overall score always + // weighs at least one, so an empty list beside a score was not + // recorded: a missing list reads as empty here, as it does in the + // Python loader. + let assessed: Option> = quality.and_then(|quality| { + let parsed: Vec = quality + .assessed_dimensions + .iter() + // A dimension a later release added is dropped here: when it + // was sampled, its component label is unknown and + // `unknown_component_coverage` leaves the overall score + // unrecorded; when it was exact it changes no coverage. + .filter_map(|name| name.parse().ok()) + .collect(); + (!(parsed.is_empty() && quality.overall_score.is_some())).then_some(parsed) + }); + let evidence = |provenance| { + let Some(quality) = quality else { + return scan; + }; + let sampled = quality.sampled_dimensions.as_deref(); + let coverage = match (provenance, &assessed) { + // Without the weights, a sampled component may or may not + // reach the overall score; only "nothing was sampled" settles it. + (Provenance::Overall, None) if sampled.is_some_and(|s| s.is_empty()) => { + Evidence::Complete + } + (Provenance::Overall, None) => Evidence::Incomplete { + reason: EvidenceGap::CoverageUnrecorded, + }, + (_, assessed) => sampled_component_evidence( + sampled, + assessed.as_deref().unwrap_or_default(), + provenance, + ), + }; + scan.and(coverage) + }; + let dimensions = QualityDimension::all() + .into_iter() + .map(|dimension| DimensionSide { + dimension, + score: at_precision( + quality + .and_then(|quality| quality.dimension_scores.get(&dimension.to_string())) + .copied() + .flatten(), + ), + evidence: evidence(Provenance::Dimension(dimension)), + }) + .collect(); + Ok(Self { + reference: ReportReference { + id: None, + timestamp: None, + source: summary.source, + rows_processed: execution.rows_processed, + evidence: scan, + }, + quality_score: at_precision(quality.and_then(|quality| quality.overall_score)), + overall_evidence: unknown_component_coverage( + quality.and_then(|quality| quality.sampled_dimensions.as_deref()), + ) + .map_or_else(|| evidence(Provenance::Overall), |gap| scan.and(gap)), + dimensions, + columns: first_of_each_name( + summary + .columns + .into_iter() + .map(|column| (column.name, at_precision(column.null_percentage))), + ), + metric_semantics: summary.metric_semantics, + }) + } +} + +/// The fields of a flat summary a comparison reads. Everything else in the +/// document is ignored. +#[derive(serde::Deserialize)] +struct Summary { + source: String, + execution: SummaryExecution, + columns: Vec, + quality: Option, + #[serde(default)] + metric_semantics: Option, +} + +#[derive(serde::Deserialize)] +struct SummaryExecution { + rows_processed: Option, + source_exhausted: Option, + truncation_reason: Option, + sampling_applied: Option, + error_count: Option, +} + +#[derive(serde::Deserialize)] +struct SummaryColumn { + name: String, + null_percentage: Option, +} + +#[derive(serde::Deserialize)] +struct SummaryQuality { + overall_score: Option, + #[serde(default)] + dimension_scores: BTreeMap>, + #[serde(default)] + assessed_dimensions: Vec, + /// Absent in a summary written before dataprof recorded it, which reads + /// as unrecorded coverage, never as "nothing was sampled". + sampled_dimensions: Option>, +} + +#[cfg(test)] +mod tests { + use super::*; + + fn complete() -> Sides { + Sides { + a: Evidence::Complete, + b: Evidence::Complete, + } + } + + #[test] + fn a_zero_baseline_has_an_absolute_change_and_no_relative_one() { + let change = ScoreChange::new(Some(0.0), Some(50.0), complete()); + assert_eq!(change.abs, Some(50.0)); + assert_eq!(change.rel_pct, None); + } + + #[test] + fn a_change_is_rounded_to_the_serialized_precision() { + // 80.1 - 80.0 is 0.0999... in binary; the contract states 0.1. The + // relative change divides that same unrounded difference, as the + // Python implementation that shipped did: 0.1249..., so 0.12. + let change = ScoreChange::new(Some(80.0), Some(80.1), complete()); + assert_eq!(change.abs, Some(0.1)); + assert_eq!(change.rel_pct, Some(0.12)); + } + + #[test] + fn a_missing_side_leaves_both_changes_absent() { + for (a, b) in [(None, Some(1.0)), (Some(1.0), None), (None, None)] { + let change = ScoreChange::new(a, b, complete()); + assert_eq!((change.abs, change.rel_pct), (None, None)); + } + } + + #[test] + fn a_non_finite_input_is_no_number() { + assert_eq!(at_precision(Some(f64::NAN)), None); + assert_eq!(at_precision(Some(f64::INFINITY)), None); + assert_eq!(at_precision(Some(12.345)), Some(12.35)); + } +} diff --git a/crates/dataprof/src/lib.rs b/crates/dataprof/src/lib.rs index 5c494373..0f41ae2f 100644 --- a/crates/dataprof/src/lib.rs +++ b/crates/dataprof/src/lib.rs @@ -65,6 +65,10 @@ pub use dataprof_runtime::{ QualityAnalysisStatus, QualityPolicy, REPORT_SCHEMA_VERSION, RequiredMetric, Severity, UnevaluatedRule, Verdict, }; +pub use dataprof_runtime::{ + ColumnChange, DimensionChange, ReportDelta, ReportReference, ReportSide, SchemaChange, + ScoreChange, SemanticsComparison, Sides, +}; #[cfg(feature = "async-streaming")] pub use dataprof_engines::streaming::ReqwestSource; diff --git a/docs/python/README.md b/docs/python/README.md index cc165ef0..0516ec88 100644 --- a/docs/python/README.md +++ b/docs/python/README.md @@ -1195,30 +1195,45 @@ checkout with `uv sync` and `uv run maturin develop`, then invoke the module wit ### `compare()` -Detect quality drift or schema changes between two profiles (e.g. the same -dataset before and after a pipeline run): +`a.compare(b)` returns what changed from `a` to `b`, such as a saved baseline +and today's run. It is computed by the Rust `ReportDelta`, so both languages +return the same document: ```python -before = dp.profile("data_v1.csv") -after = dp.profile("data_v2.csv") -delta = before.compare(after) -# { -# "quality_score": {"a": 92.3, "b": 88.1, "abs": -4.2, "rel_pct": -4.55}, -# "dimensions": {"completeness": {...}, "consistency": {...}, ...}, -# "columns": {"email": {"null_pct_a": 1.0, "null_pct_b": 6.5, "null_pct_delta": 5.5}, ...}, -# "schema": {"added": ["phone"], "removed": [], "common": ["id", "email", ...]}, -# "metric_semantics": {"a": {...}, "b": {...}, "comparable": True}, -# } +baseline = dp.ProfileReport.load("baseline.json") +delta = baseline.compare(dp.profile("orders.csv")) + +delta["quality_score"] # {"a": 91.2, "b": 88.75, "abs": -2.45, "rel_pct": -2.69, "evidence": {...}} +delta["dimensions"]["completeness"] # the same shape, for each of the seven dimensions +delta["columns"]["email"] # {"null_pct_a": 1.0, "null_pct_b": 4.5, "null_pct_delta": 3.5} +delta["schema"] # {"added": [...], "removed": [...], "common": [...]} +delta["metric_semantics"]["comparable"] # True, False, or None when either side predates the record +delta["reports"]["b"] # {"id", "timestamp", "source", "rows_processed", "evidence"} ``` -`metric_semantics.comparable` is `True` when both reports record every -measurement definition this release knows, with the same values, and `None` -when either side does not, which includes every report written before 0.12. Text lengths counted UTF-8 bytes through 0.11, -so across that boundary a changed `max_length` on non-ASCII text can be the unit -rather than the data. - -> The `compare()` result shape is provisional and will align with the Rust-side -> `QualityDelta` type once it lands. +- **Signs.** Every change is `b - a`. A positive score change is an + improvement; a positive `null_pct_delta` is more nulls. +- **Precision.** Both sides are compared at their serialized 2-decimal + precision, so a report and its saved file compare alike. +- **No invented numbers.** A score either side did not assess, or a column one + side lacks or that holds no values, leaves the change `None`. `rel_pct` is + `None` when `a` is 0. +- **Evidence.** Each score carries the coverage behind it on both sides, with + the reasons `check()` uses (`truncated`, `sampled`, `records_skipped`, + `quality_sampled`, `coverage_unrecorded`). A change between an exact + baseline and a sampled or truncated run can be a sampling difference. +- **Definitions.** Only `comparable: True` means a change in a measurement + is a change in the data. Text lengths counted UTF-8 bytes through 0.11, so + across that boundary a changed `max_length` on non-ASCII text can be the + unit rather than the data. `comparable` covers how dataprof measures, not + how each report was configured: profile both sides with the same options + (`metrics`, `positive_columns`, `temporal_columns`, ...) before reading a + change as drift. + +Either side may be a report loaded from a flat `to_dict()` summary, including +one saved before 0.12; its `id` and `timestamp` are `None`, and coverage it +did not record reads as `coverage_unrecorded`. The shape is stable: later +releases may add keys, but do not remove or rename them. ### `save()` diff --git a/examples/before_after_cleaning.rs b/examples/before_after_cleaning.rs index 17d12b5a..9a575cc0 100644 --- a/examples/before_after_cleaning.rs +++ b/examples/before_after_cleaning.rs @@ -17,7 +17,7 @@ use std::io::Write; use std::path::Path; use anyhow::Result; -use dataprof::{ColumnProfile, ProfileReport, Profiler}; +use dataprof::{ColumnProfile, ProfileReport, Profiler, ReportDelta}; const RAW: &str = "\ signup_id,email,plan,monthly_eur @@ -48,10 +48,6 @@ fn profile(path: &Path) -> Result { .analyze_file(path)?) } -fn null_percentage(col: &ColumnProfile) -> f64 { - 100.0 * col.null_count as f64 / col.total_count.max(1) as f64 -} - fn by_name(report: &ProfileReport) -> BTreeMap<&str, &ColumnProfile> { report .column_profiles @@ -78,36 +74,50 @@ fn main() -> Result<()> { let before: ProfileReport = serde_json::from_reader(std::fs::File::open(&baseline_path)?)?; let after = profile(&clean_path)?; + // The delta never invents a number: a score either side did not assess + // stays `None`, so print it as such instead of as zero. + let delta = ReportDelta::between(&before, &after); + let show = |value: Option| value.map_or("n/a".to_string(), |v| format!("{v:.1}")); + let quality = &delta.quality_score; + println!("## Cleaning summary\n"); println!( - "| | rows | quality |\n|---|---:|---:|\n| before | {} | {:.1} |\n| after | {} | {:.1} |", + "| | rows | quality |\n|---|---:|---:|\n| before | {} | {} |\n| after | {} | {} |", before.execution.rows_processed, - before.quality_score().unwrap_or(0.0), + show(quality.a), after.execution.rows_processed, - after.quality_score().unwrap_or(0.0), + show(quality.b), ); let dropped = before.execution.rows_processed - after.execution.rows_processed; - let gained = after.quality_score().unwrap_or(0.0) - before.quality_score().unwrap_or(0.0); - println!("\n{dropped} row(s) dropped, {gained:+.1} quality points\n"); + let gained = quality + .abs + .map_or("n/a".to_string(), |change| format!("{change:+.1}")); + println!("\n{dropped} row(s) dropped, {gained} quality points\n"); println!("## Per-column null rate\n"); println!("| column | before | after | delta |"); println!("|---|---:|---:|---:|"); - let (before_cols, after_cols) = (by_name(&before), by_name(&after)); - for (name, before_col) in &before_cols { - let Some(after_col) = after_cols.get(name) else { - println!( - "| {name} | {:.0}% | *dropped* | |", - null_percentage(before_col) - ); + let pct = |value: Option| value.map_or("n/a".to_string(), |v| format!("{v:.0}%")); + for column in &delta.columns { + let name = &column.name; + if delta.schema.removed.contains(name) { + println!("| {name} | {} | *dropped* | |", pct(column.null_pct_a)); continue; - }; - let (b, a) = (null_percentage(before_col), null_percentage(after_col)); - println!("| {name} | {b:.0}% | {a:.0}% | {:+.0}pp |", a - b); + } + let change = column + .null_pct_delta + .map_or(String::new(), |change| format!("{change:+.0}pp")); + println!( + "| {name} | {} | {} | {change} |", + pct(column.null_pct_a), + pct(column.null_pct_b) + ); } + let after_cols = by_name(&after); + // The headline numbers can improve while a specific defect survives. Check the // defects you set out to fix, not just the score. println!("\n## Did the defects go away?\n"); diff --git a/python/dataprof/_dataprof.pyi b/python/dataprof/_dataprof.pyi index f08ea72d..71682a4b 100644 --- a/python/dataprof/_dataprof.pyi +++ b/python/dataprof/_dataprof.pyi @@ -414,6 +414,7 @@ class ProfileReport: def analyze_file(path: str, config: ProfilerConfig | None) -> ProfileReport: ... def list_patterns(locale: str | None = None) -> list[dict[str, Any]]: ... +def report_delta_json(a: ProfileReport | str, b: ProfileReport | str) -> str: ... def profile_dataframe( df: Any, name: str, max_rows: int | None, config: ProfilerConfig | None ) -> ProfileReport: ... diff --git a/python/dataprof/_report.py b/python/dataprof/_report.py index 04a5cf68..0e0393d3 100644 --- a/python/dataprof/_report.py +++ b/python/dataprof/_report.py @@ -18,7 +18,7 @@ from ._accessors import ColumnProfile, DataQualityMetrics, _NativeAccessor, _ReportView from ._columns import _column_record, _dominant_pattern, column_to_dict -from ._dataprof import ProfileReport as _RustProfileReport +from ._dataprof import ProfileReport as _RustProfileReport, report_delta_json as _report_delta_json from ._findings import FindingsResult as _FindingsResult, _FindingPolicy from ._gate import QualityGateResult as _QualityGateResult, _Policy from ._paths import _normalize_pathlike @@ -34,7 +34,7 @@ _section_min_cost, _stats_cell, ) -from ._report_backing import _DEFAULT_SCORE_WEIGHTS, _METRIC_SEMANTICS, _report_from_dict +from ._report_backing import _DEFAULT_SCORE_WEIGHTS, _report_from_dict from ._report_schema import _QUALITY_DIMENSIONS, REPORT_SCHEMA_VERSION from ._rounding import _r2, _r4, _round_dimension, _round_quartiles @@ -955,18 +955,21 @@ def _to_llm_context(self, *, source: str, max_tokens: int, include_samples: bool return "\n".join([*header, *body]) def compare(self, other: ProfileReport) -> dict[str, _Any]: - """Compare this report with another and return a dict of deltas. + """Compare this report (``a``) with ``other`` (``b``) and return the deltas. The result captures quality drift and schema differences between two - profiles (e.g. the same dataset before and after a pipeline change): - - - ``quality_score``: overall score for each side plus absolute and - relative-percent change. - - ``dimensions``: the same a/b/abs/rel_pct shape per ISO 25012 - dimension (completeness, consistency, uniqueness, accuracy, - timeliness, validity, precision), sourced from :meth:`quality_summary`. - - ``columns``: per-column null-percentage drift over the union of - column names (missing on one side → ``None``). + profiles, such as a saved baseline and today's run. It is computed by + the Rust ``ReportDelta``, so Rust and Python return the same document: + + - ``quality_score``: ``a``, ``b``, ``abs`` (``b - a`` in points) and + ``rel_pct`` (``b - a`` as a percentage of ``a``), plus ``evidence``, + the coverage behind each side's score. + - ``dimensions``: the same shape for each ISO 25012 dimension + (completeness, consistency, uniqueness, accuracy, timeliness, + validity, precision), as :meth:`quality_summary` reports them. + - ``columns``: per-column ``null_pct_a``, ``null_pct_b`` and + ``null_pct_delta`` over the union of column names, this report's + order first. - ``schema``: column names ``added`` / ``removed`` / ``common``. - ``metric_semantics``: each side's :attr:`metric_semantics` and ``comparable``: ``True`` when both record every definition this @@ -974,92 +977,33 @@ def compare(self, other: ProfileReport) -> dict[str, _Any]: does not, which includes every report written before 0.12. Only ``True`` means a difference in a measurement, such as a text length, is a difference in the data. - - .. note:: - The exact shape is provisional and will align with the Rust-side - ``QualityDelta`` type (#310) once it lands. + - ``reports``: for each side, its ``id`` and ``timestamp`` (``None`` + for a flat summary), ``source``, ``rows_processed`` and the scan's + ``evidence``. + + Every change is ``b - a``: a positive score change is an improvement, + a positive ``null_pct_delta`` is more nulls. Numbers are compared at + their serialized 2-decimal precision. A number either side lacks + leaves the change ``None``, and so does ``rel_pct`` when ``a`` is 0. + + Each ``evidence`` is ``{"coverage": "complete"}``, or + ``{"coverage": "incomplete", "reason": ...}`` with the reason + ``check()`` uses: ``truncated``, ``sampled``, ``records_skipped``, + ``quality_sampled`` or ``coverage_unrecorded``. A change between an + exact and a sampled side can be a sampling difference. + + The shape is stable: later releases may add keys, but do not remove + or rename them. Either report may be loaded from a flat ``to_dict()`` + summary, including one saved before 0.12. """ + return _json.loads(_report_delta_json(self._delta_input(), other._delta_input())) - def _delta(a: float | None, b: float | None) -> dict[str, float | None]: - abs_change = None if a is None or b is None else _r2(b - a) - if a is None or b is None or a == 0: - rel_pct = None - else: - rel_pct = _r2((b - a) / abs(a) * 100.0) - return {"a": a, "b": b, "abs": abs_change, "rel_pct": rel_pct} - - a_summary = self.quality_summary() - b_summary = other.quality_summary() - - dimensions = { - dim: _delta(a_summary.get(dim), b_summary.get(dim)) - for dim in ( - "completeness", - "consistency", - "uniqueness", - "accuracy", - "timeliness", - "validity", - "precision", - ) - } - - a_cols = self.column_profiles - b_cols = other.column_profiles - a_order = list(a_cols) - b_order = list(b_cols) - a_names = set(a_order) - b_names = set(b_order) - # Preserve the left report's schema order, then append right-only - # columns in their source order. - all_names = [*a_order, *(name for name in b_order if name not in a_names)] - - def _null_pct(cols: dict[str, ColumnProfile], name: str) -> float | None: - col = cols.get(name) - return _r2(col.null_percentage) if col is not None else None - - columns: dict[str, dict[str, float | None]] = {} - for name in all_names: - null_a = _null_pct(a_cols, name) - null_b = _null_pct(b_cols, name) - null_delta = None if null_a is None or null_b is None else _r2(null_b - null_a) - columns[name] = { - "null_pct_a": null_a, - "null_pct_b": null_b, - "null_pct_delta": null_delta, - } - - a_semantics = self.metric_semantics - b_semantics = other.metric_semantics - # Both sides must record every definition this build knows; an empty - # or partial record is as unknown as a missing one. Loaded values are - # validated to the one value each definition has today, so a `False` - # needs a second recorded value to become reachable. - known = _METRIC_SEMANTICS.keys() - comparable = ( - a_semantics == b_semantics - if a_semantics is not None - and b_semantics is not None - and a_semantics.keys() == known - and b_semantics.keys() == known - else None - ) - - return { - "quality_score": _delta(self.quality_score, other.quality_score), - "dimensions": dimensions, - "columns": columns, - "metric_semantics": { - "a": a_semantics, - "b": b_semantics, - "comparable": comparable, - }, - "schema": { - "added": [name for name in b_order if name not in a_names], - "removed": [name for name in a_order if name not in b_names], - "common": [name for name in a_order if name in b_names], - }, - } + def _delta_input(self) -> _RustProfileReport | str: + # A flat summary never becomes a native report (it lacks the + # provenance one records), so it crosses as the summary it exports. + if self._native_report is not None: + return self._native_report + return _json.dumps(self._legacy_summary()) @classmethod def from_dict(cls, data: dict[str, _Any]) -> ProfileReport: diff --git a/python/tests/test_report_delta.py b/python/tests/test_report_delta.py new file mode 100644 index 00000000..4e2b6870 --- /dev/null +++ b/python/tests/test_report_delta.py @@ -0,0 +1,154 @@ +"""ProfileReport.compare() returns the Rust report delta (#713). + +`tests/report_delta.rs` is the Rust twin. Here the extra routes are the +Python ones: a report loaded from a flat `to_dict()` summary, including a +summary written by 0.11, compares through the same delta as a profiled one. +""" + +import json +from pathlib import Path + +import dataprof as dp + +FIXTURES = Path(__file__).parents[2] / "tests/fixtures" +CLEAN = {"id": [1, 2, 3, 4], "amount": [10, 20, 30, 40]} +WITH_NULLS = {"id": [1, 2, 3, 4], "amount": [10, None, None, 40]} +SCORE_KEYS = {"a", "b", "abs", "rel_pct", "evidence"} + + +def _flat(report: dp.ProfileReport) -> dp.ProfileReport: + return dp.ProfileReport.from_dict(report.to_dict()) + + +def test_the_shape_is_the_frozen_contract(): + delta = dp.profile(WITH_NULLS).compare(dp.profile(CLEAN)) + assert list(delta) == [ + "quality_score", + "dimensions", + "columns", + "metric_semantics", + "schema", + "reports", + ] + assert set(delta["quality_score"]) == SCORE_KEYS + assert list(delta["dimensions"]) == [ + "completeness", + "consistency", + "uniqueness", + "accuracy", + "timeliness", + "validity", + "precision", + ] + for change in delta["dimensions"].values(): + assert set(change) == SCORE_KEYS + assert set(change["evidence"]) == {"a", "b"} + assert delta["columns"]["amount"] == { + "null_pct_a": 50.0, + "null_pct_b": 0.0, + "null_pct_delta": -50.0, + } + assert set(delta["metric_semantics"]) == {"a", "b", "comparable"} + assert set(delta["schema"]) == {"added", "removed", "common"} + for side in delta["reports"].values(): + assert set(side) == {"id", "timestamp", "source", "rows_processed", "evidence"} + + +def test_every_change_is_b_minus_a(): + delta = dp.profile(WITH_NULLS).compare(dp.profile(CLEAN)) + completeness = delta["dimensions"]["completeness"] + assert completeness["b"] > completeness["a"] + assert completeness["abs"] == round(completeness["b"] - completeness["a"], 2) + assert completeness["abs"] > 0 + assert delta["columns"]["amount"]["null_pct_delta"] < 0 + + +def test_a_flat_summary_compares_like_the_report_it_came_from(): + a, b = dp.profile(WITH_NULLS), dp.profile(CLEAN) + native = a.compare(b) + flat = _flat(a).compare(_flat(b)) + mixed = _flat(a).compare(b) + + assert native["reports"]["a"]["id"] == json.loads(a.to_json())["id"] + for delta in (flat, mixed): + assert delta["reports"]["a"]["id"] is None + assert delta["reports"]["a"]["timestamp"] is None + masked = json.loads(json.dumps(native)) + for side in ("a", "b"): + masked["reports"][side]["id"] = None + masked["reports"][side]["timestamp"] = None + assert flat == masked + masked["reports"]["b"] = native["reports"]["b"] + assert mixed == masked + + +def test_a_0_11_baseline_reads_as_unrecorded(tmp_path: Path): + text = (FIXTURES / "legacy_0_11_non_ascii.summary.json").read_text(encoding="utf-8") + baseline = dp.ProfileReport.from_dict(json.loads(text)) + path = tmp_path / "cities.csv" + path.write_text("city\n東京\nZürich\nOslo\n\n", encoding="utf-8") + current = dp.profile(str(path)) + delta = baseline.compare(current) + + assert delta["metric_semantics"]["comparable"] is None + # 0.11 did not record which quality components were sampled. + assert delta["quality_score"]["evidence"]["a"] == { + "coverage": "incomplete", + "reason": "coverage_unrecorded", + } + assert delta["quality_score"]["evidence"]["b"] == {"coverage": "complete"} + assert delta["reports"]["a"]["evidence"] == {"coverage": "complete"} + + +def test_a_truncated_side_says_so(tmp_path: Path): + path = tmp_path / "rows.csv" + path.write_text("id,amount\n" + "".join(f"{i},{i * 2}\n" for i in range(200))) + exact = dp.profile(str(path)) + truncated = dp.profile(str(path), max_rows=50) + delta = exact.compare(truncated) + + truncated_evidence = {"coverage": "incomplete", "reason": "truncated"} + assert delta["reports"]["a"]["evidence"] == {"coverage": "complete"} + assert delta["reports"]["b"]["evidence"] == truncated_evidence + assert delta["reports"]["b"]["rows_processed"] == 50 + assert delta["quality_score"]["evidence"]["b"] == truncated_evidence + for change in delta["dimensions"].values(): + assert change["evidence"]["b"] == truncated_evidence + + +def test_nothing_assessed_invents_no_number(): + empty = dp.profile({"id": [], "amount": []}) + for delta in (empty.compare(dp.profile(CLEAN)), empty.compare(empty)): + assert delta["quality_score"]["abs"] is None + assert delta["quality_score"]["rel_pct"] is None + assert all(change["abs"] is None for change in delta["dimensions"].values()) + assert all(change["null_pct_delta"] is None for change in delta["columns"].values()) + + +def test_a_summary_that_records_only_its_row_count_still_compares(): + """The oldest accepted flat summary records `rows_processed` and nothing + else about its scan, so its coverage is unrecorded, never complete.""" + legacy = dp.ProfileReport.from_dict( + {"source": "old.csv", "columns": [], "execution": {"rows_processed": 5}} + ) + unrecorded = {"coverage": "incomplete", "reason": "coverage_unrecorded"} + for delta in (legacy.compare(legacy), legacy.compare(dp.profile(CLEAN))): + assert delta["reports"]["a"]["rows_processed"] == 5 + assert delta["reports"]["a"]["evidence"] == unrecorded + assert delta["quality_score"]["abs"] is None + + +def test_a_repeated_column_name_reads_its_first_profile_as_check_does(): + report = dp.profile(CLEAN) + document = json.loads(report.to_json()) + first, second = dict(document["column_profiles"][1]), dict(document["column_profiles"][1]) + first["null_count"] = 0 + second["null_count"] = second["total_count"] + document["column_profiles"] = [document["column_profiles"][0], first, second] + rebuilt = dp.ProfileReport.from_dict(document) + + delta = rebuilt.compare(rebuilt) + assert delta["schema"]["common"] == ["id", "amount"] + assert delta["columns"]["amount"]["null_pct_a"] == 0.0 + observed = rebuilt.check(max_null_percentage={"amount": 50}).checks + assert [c.observed for c in observed if c.column == "amount"] == [0.0] diff --git a/tests/public_api_facade.rs b/tests/public_api_facade.rs index e461cb89..f325d0a2 100644 --- a/tests/public_api_facade.rs +++ b/tests/public_api_facade.rs @@ -623,3 +623,98 @@ fn quality_gate_types_are_matched_with_a_wildcard() { let empty = QualityPolicy::new().evaluate(&report).unwrap_err(); assert_eq!(policy_error(&empty), 1); } + +/// The report-delta types are `#[non_exhaustive]` so #749 and #310 can add +/// fields without breaking callers. This names every field through the facade +/// in code that must compile, so a rename fails here. +#[test] +fn report_delta_fields_are_reachable_through_the_facade() { + use dataprof::{ + ColumnChange, DimensionChange, ReportDelta, ReportReference, SchemaChange, ScoreChange, + SemanticsComparison, Sides, + }; + + fn score(change: &ScoreChange) -> usize { + let ScoreChange { + a, + b, + abs, + rel_pct, + evidence: Sides { + a: left, b: right, .. + }, + .. + } = change; + let _ = (a, b, abs, rel_pct, left, right); + 1 + } + + fn reference(side: &ReportReference) -> usize { + let ReportReference { + id, + timestamp, + source, + rows_processed, + evidence, + .. + } = side; + let _ = (id, timestamp, evidence); + source.len() + rows_processed.unwrap_or(0) + } + + fn delta(delta: &ReportDelta) -> usize { + let ReportDelta { + quality_score, + dimensions, + columns, + metric_semantics: + SemanticsComparison { + a: semantics_a, + b: semantics_b, + comparable, + .. + }, + schema: + SchemaChange { + added, + removed, + common, + .. + }, + reports: Sides { a, b, .. }, + .. + } = delta; + let _ = (semantics_a, semantics_b, comparable, added, removed, common); + let dimension_total: usize = dimensions + .iter() + .map( + |DimensionChange { + dimension, + score: change, + .. + }| { + let _ = dimension; + score(change) + }, + ) + .sum(); + let column_total: usize = columns + .iter() + .map( + |ColumnChange { + name, + null_pct_a, + null_pct_b, + null_pct_delta, + .. + }| { + let _ = (null_pct_a, null_pct_b, null_pct_delta); + name.len() + }, + ) + .sum(); + score(quality_score) + dimension_total + column_total + reference(a) + reference(b) + } + + let _ = delta; +} diff --git a/tests/report_delta.rs b/tests/report_delta.rs new file mode 100644 index 00000000..22bd6d9c --- /dev/null +++ b/tests/report_delta.rs @@ -0,0 +1,397 @@ +//! The typed report-to-report delta (#713). +//! +//! `python/tests/test_report_delta.py` is the Python twin, and also checks a +//! report loaded from a flat summary against the report it came from. + +use std::io::Write; + +use dataprof::{EvidenceGap, ProfileReport, Profiler, ReportDelta, ReportSide, StopCondition}; +use serde_json::{Value, json}; + +fn csv(contents: &str) -> tempfile::NamedTempFile { + let mut file = tempfile::Builder::new().suffix(".csv").tempfile().unwrap(); + file.write_all(contents.as_bytes()).unwrap(); + file +} + +fn profile(contents: &str) -> ProfileReport { + let file = csv(contents); + Profiler::new().analyze_file(file.path()).unwrap() +} + +fn document(delta: &ReportDelta) -> Value { + serde_json::to_value(delta).unwrap() +} + +const CLEAN: &str = "id,amount\n1,10\n2,20\n3,30\n4,40\n"; +const WITH_NULLS: &str = "id,amount\n1,10\n2,\n3,\n4,40\n"; + +#[test] +fn identical_reports_give_explicit_zeros_for_what_was_assessed() { + let report = profile(CLEAN); + let delta = document(&ReportDelta::between(&report, &report)); + + assert_eq!(delta["quality_score"]["abs"], json!(0.0)); + assert_eq!(delta["quality_score"]["rel_pct"], json!(0.0)); + for (name, change) in delta["dimensions"].as_object().unwrap() { + if change["a"].is_null() { + // Unassessed on both sides: no number to subtract. + assert!(change["abs"].is_null(), "{name}"); + } else { + assert_eq!(change["abs"], json!(0.0), "{name}"); + } + } + for change in delta["columns"].as_object().unwrap().values() { + assert_eq!(change["null_pct_delta"], json!(0.0)); + } + assert_eq!(delta["metric_semantics"]["comparable"], json!(true)); + assert_eq!(delta["schema"]["common"], json!(["id", "amount"])); +} + +#[test] +fn every_change_is_b_minus_a() { + let worse = profile(WITH_NULLS); + let better = profile(CLEAN); + let delta = document(&ReportDelta::between(&worse, &better)); + + let completeness = &delta["dimensions"]["completeness"]; + let (a, b) = ( + completeness["a"].as_f64().unwrap(), + completeness["b"].as_f64().unwrap(), + ); + assert!(b > a, "the clean side scores higher"); + assert!( + completeness["abs"].as_f64().unwrap() > 0.0, + "improvement is positive" + ); + assert_eq!(delta["columns"]["amount"]["null_pct_a"], json!(50.0)); + assert_eq!(delta["columns"]["amount"]["null_pct_b"], json!(0.0)); + assert_eq!( + delta["columns"]["amount"]["null_pct_delta"], + json!(-50.0), + "fewer nulls is negative" + ); +} + +#[test] +fn an_all_null_column_is_a_share_and_a_null_free_one_is_zero() { + let empty_column = profile("id,note\n1,\n2,\n"); + let filled = profile("id,note\n1,a\n2,b\n"); + let delta = document(&ReportDelta::between(&empty_column, &filled)); + + // The note column is all null: its share is 100%, a real number. + assert_eq!(delta["columns"]["note"]["null_pct_a"], json!(100.0)); + assert_eq!(delta["columns"]["note"]["null_pct_delta"], json!(-100.0)); + + let reversed = document(&ReportDelta::between(&filled, &empty_column)); + assert_eq!(reversed["columns"]["note"]["null_pct_a"], json!(0.0)); + assert_eq!(reversed["columns"]["note"]["null_pct_delta"], json!(100.0)); +} + +#[test] +fn a_report_with_nothing_assessed_invents_no_number() { + let header_only = profile("id,amount\n"); + let report = profile(CLEAN); + for delta in [ + document(&ReportDelta::between(&header_only, &report)), + document(&ReportDelta::between(&report, &header_only)), + document(&ReportDelta::between(&header_only, &header_only)), + ] { + assert!(delta["quality_score"]["abs"].is_null()); + assert!(delta["quality_score"]["rel_pct"].is_null()); + for change in delta["dimensions"].as_object().unwrap().values() { + assert!(change["abs"].is_null()); + assert!(change["rel_pct"].is_null()); + } + for change in delta["columns"].as_object().unwrap().values() { + assert!( + change["null_pct_delta"].is_null(), + "an empty column has no null share" + ); + } + } +} + +#[test] +fn schema_changes_keep_each_side_order() { + let a = profile("id,old,shared\n1,x,y\n"); + let b = profile("shared,new,id\ny,z,1\n"); + let delta = document(&ReportDelta::between(&a, &b)); + + assert_eq!(delta["schema"]["added"], json!(["new"])); + assert_eq!(delta["schema"]["removed"], json!(["old"])); + assert_eq!(delta["schema"]["common"], json!(["id", "shared"])); + let columns: Vec<&String> = delta["columns"].as_object().unwrap().keys().collect(); + assert_eq!(columns, ["id", "old", "shared", "new"]); + assert!(delta["columns"]["old"]["null_pct_b"].is_null()); + assert!(delta["columns"]["old"]["null_pct_delta"].is_null()); + assert!(delta["columns"]["new"]["null_pct_a"].is_null()); +} + +#[test] +fn a_truncated_side_says_so_on_every_number() { + let rows: String = (0..200).map(|i| format!("{i},{}\n", i * 2)).collect(); + let file = csv(&format!("id,amount\n{rows}")); + let exact = Profiler::new().analyze_file(file.path()).unwrap(); + let truncated = Profiler::new() + .stop_when(StopCondition::MaxRows(50)) + .analyze_file(file.path()) + .unwrap(); + + let delta = ReportDelta::between(&exact, &truncated); + assert!(delta.reports.a.evidence.is_complete()); + assert_eq!(delta.reports.b.evidence.gap(), Some(EvidenceGap::Truncated)); + assert_eq!(delta.reports.b.rows_processed, Some(50)); + assert!(delta.quality_score.evidence.a.is_complete()); + assert_eq!( + delta.quality_score.evidence.b.gap(), + Some(EvidenceGap::Truncated) + ); + for change in &delta.dimensions { + assert_eq!(change.score.evidence.b.gap(), Some(EvidenceGap::Truncated)); + } +} + +#[test] +fn a_summary_compares_like_the_report_it_came_from() { + let a = profile(WITH_NULLS); + let b = profile(CLEAN); + let native = document(&ReportDelta::between(&a, &b)); + let side = |report: &ProfileReport| { + ReportSide::from_summary_json(&report.summary_json().unwrap()).unwrap() + }; + let flat = document(&ReportDelta::between_sides(&side(&a), &side(&b))); + + let mut masked = native.clone(); + for name in ["a", "b"] { + // A flat summary records neither identity field. + masked["reports"][name]["id"] = Value::Null; + masked["reports"][name]["timestamp"] = Value::Null; + } + assert_eq!(flat, masked); + assert_eq!(native["reports"]["a"]["id"], json!(a.id)); +} + +#[test] +fn a_summary_from_before_0_12_reads_as_unrecorded() { + let legacy = std::fs::read_to_string( + std::path::Path::new(env!("CARGO_MANIFEST_DIR")) + .join("tests/fixtures/legacy_0_11_non_ascii.summary.json"), + ) + .unwrap(); + let legacy = ReportSide::from_summary_json(&legacy).unwrap(); + let current = ReportSide::from_report(&profile("city\nOslo\n")); + let delta = ReportDelta::between_sides(&legacy, ¤t); + + assert_eq!(delta.metric_semantics.comparable, None); + assert!(delta.metric_semantics.a.is_none()); + // 0.11 did not record which quality components were sampled. + assert_eq!( + delta.quality_score.evidence.a.gap(), + Some(EvidenceGap::CoverageUnrecorded) + ); + assert!(delta.quality_score.evidence.b.is_complete()); +} + +#[test] +fn a_summary_without_columns_is_an_error() { + let report = profile(CLEAN); + let mut summary: Value = serde_json::from_str(&report.summary_json().unwrap()).unwrap(); + summary.as_object_mut().unwrap().remove("columns"); + let error = ReportSide::from_summary_json(&summary.to_string()).unwrap_err(); + assert!(error.to_string().contains("columns"), "{error}"); +} + +/// The oldest flat summaries record only some of the execution fields. Python +/// writes the rest as null; a summary from elsewhere can omit them. +#[test] +fn a_summary_that_did_not_record_its_scan_reads_as_unrecorded() { + let unrecorded = || Some(EvidenceGap::CoverageUnrecorded); + for execution in [ + json!({"rows_processed": 5}), + json!({"rows_processed": 5, "source_exhausted": null, "sampling_applied": null, "error_count": null}), + json!({}), + ] { + let summary = json!({"source": "old.csv", "columns": [], "execution": execution}); + let side = ReportSide::from_summary_json(&summary.to_string()).unwrap(); + let delta = ReportDelta::between_sides(&side, &side); + assert_eq!(delta.reports.a.evidence.gap(), unrecorded(), "{execution}"); + assert_eq!(delta.quality_score.evidence.a.gap(), unrecorded()); + assert!(delta.quality_score.abs.is_none()); + } + + let rows_only = json!({"source": "old.csv", "columns": [], "execution": {"rows_processed": 5}}); + let side = ReportSide::from_summary_json(&rows_only.to_string()).unwrap(); + assert_eq!( + ReportDelta::between_sides(&side, &side) + .reports + .a + .rows_processed, + Some(5) + ); + + // A recorded gap is still named over an unrecorded fact. + let truncated = json!({"source": "x", "columns": [], "execution": {"source_exhausted": false}}); + let side = ReportSide::from_summary_json(&truncated.to_string()).unwrap(); + assert_eq!( + ReportDelta::between_sides(&side, &side) + .reports + .a + .evidence + .gap(), + Some(EvidenceGap::Truncated) + ); +} + +/// Without the weights behind the overall score, a sampled component may +/// reach it, so the score is not certified exact. +#[test] +fn an_overall_score_without_its_weights_is_not_certified() { + let report = profile(CLEAN); + let mut summary: Value = serde_json::from_str(&report.summary_json().unwrap()).unwrap(); + summary["quality"]["sampled_dimensions"] = json!(["consistency"]); + for assessed in [None, Some(json!([]))] { + let quality = summary["quality"].as_object_mut().unwrap(); + match &assessed { + None => { + quality.remove("assessed_dimensions"); + } + Some(list) => { + quality.insert("assessed_dimensions".into(), list.clone()); + } + } + let side = ReportSide::from_summary_json(&summary.to_string()).unwrap(); + let delta = ReportDelta::between_sides(&side, &side); + assert_eq!( + delta.quality_score.evidence.a.gap(), + Some(EvidenceGap::CoverageUnrecorded), + "assessed_dimensions = {assessed:?}" + ); + } + + // Nothing sampled settles it without the weights. + summary["quality"]["sampled_dimensions"] = json!([]); + let side = ReportSide::from_summary_json(&summary.to_string()).unwrap(); + assert!( + ReportDelta::between_sides(&side, &side) + .quality_score + .evidence + .a + .is_complete() + ); +} + +/// A rebuilt report can repeat a column name. The delta lists it once and +/// reads its first profile, as `QualityPolicy` does. +#[test] +fn a_repeated_column_name_is_listed_once_with_its_first_profile() { + let mut report = profile(CLEAN); + let mut first = report.column_profiles[1].clone(); + let mut second = first.clone(); + first.null_count = 0; + second.null_count = second.total_count; + report.column_profiles = vec![report.column_profiles[0].clone(), first, second]; + + let delta = document(&ReportDelta::between(&report, &report)); + assert_eq!(delta["schema"]["common"], json!(["id", "amount"])); + let columns: Vec<&String> = delta["columns"].as_object().unwrap().keys().collect(); + assert_eq!(columns, ["id", "amount"]); + assert_eq!(delta["columns"]["amount"]["null_pct_a"], json!(0.0)); + + let side = ReportSide::from_summary_json(&report.summary_json().unwrap()).unwrap(); + let flat = document(&ReportDelta::between_sides(&side, &side)); + assert_eq!(flat["schema"], delta["schema"]); + assert_eq!(flat["columns"], delta["columns"]); +} + +#[test] +fn serialization_is_deterministic_and_ordered() { + let a = profile(WITH_NULLS); + let b = profile(CLEAN); + let first = serde_json::to_string(&ReportDelta::between(&a, &b)).unwrap(); + let second = serde_json::to_string(&ReportDelta::between(&a, &b)).unwrap(); + assert_eq!(first, second); + + let delta: Value = serde_json::from_str(&first).unwrap(); + let keys: Vec<&String> = delta.as_object().unwrap().keys().collect(); + assert_eq!( + keys, + [ + "quality_score", + "dimensions", + "columns", + "metric_semantics", + "schema", + "reports" + ] + ); + let dimensions: Vec<&String> = delta["dimensions"].as_object().unwrap().keys().collect(); + assert_eq!( + dimensions, + [ + "completeness", + "consistency", + "uniqueness", + "accuracy", + "timeliness", + "validity", + "precision" + ] + ); +} + +#[test] +fn a_partly_recorded_side_leaves_comparability_unknown() { + let report = profile(CLEAN); + let mut summary: Value = serde_json::from_str(&report.summary_json().unwrap()).unwrap(); + // 0.12 recorded only its text-length unit. + summary["metric_semantics"] = json!({"text_length_unit": "unicode_scalar"}); + let partial = ReportSide::from_summary_json(&summary.to_string()).unwrap(); + let current = ReportSide::from_report(&report); + + for delta in [ + ReportDelta::between_sides(&partial, ¤t), + ReportDelta::between_sides(¤t, &partial), + ReportDelta::between_sides(&partial, &partial), + ] { + assert_eq!(delta.metric_semantics.comparable, None); + } + let both = ReportDelta::between_sides(¤t, ¤t); + assert_eq!(both.metric_semantics.comparable, Some(true)); +} + +/// A dimension a later release added carries weight in the stored overall +/// score. When its component was sampled, nothing this build knows rules out +/// that the sample reaches the score, so the score is not certified exact. +#[test] +fn a_sampled_component_this_build_does_not_know_leaves_the_overall_unrecorded() { + let report = profile(CLEAN); + let mut canonical = serde_json::to_value(&report).unwrap(); + canonical["quality"]["confidence"] = json!({"Mixed": { + "exact_dimensions": ["completeness"], + "sample_size": 4, + "sampled_dimensions": ["future_dimension"], + }}); + let native: ProfileReport = serde_json::from_value(canonical).unwrap(); + let delta = ReportDelta::between(&native, &native); + assert_eq!( + delta.quality_score.evidence.a.gap(), + Some(EvidenceGap::CoverageUnrecorded) + ); + // A known dimension's own score is unaffected. + let completeness = &delta.dimensions[0]; + assert!(completeness.score.evidence.a.is_complete()); + + let mut summary: Value = serde_json::from_str(&report.summary_json().unwrap()).unwrap(); + summary["quality"]["sampled_dimensions"] = json!(["future_dimension"]); + summary["quality"]["assessed_dimensions"] = json!(["completeness", "future_dimension"]); + let side = ReportSide::from_summary_json(&summary.to_string()).unwrap(); + assert_eq!( + ReportDelta::between_sides(&side, &side) + .quality_score + .evidence + .a + .gap(), + Some(EvidenceGap::CoverageUnrecorded) + ); +} From ca8a85e867d96b364d27cb3eb596c412be47f651 Mon Sep 17 00:00:00 2001 From: Denis Le Thanh Date: Sat, 17 Oct 2026 12:17:42 +0200 Subject: [PATCH 07/16] fix(report): atomic report saving (#978) * fix: make report saves atomic * fix: preserve permissions and symlinks on atomic save * fix: address atomic save platform edge cases --------- Co-authored-by: Andrea Bozzo --- python/dataprof/_report.py | 85 +++++++++-- python/tests/test_report_exports.py | 212 ++++++++++++++++++++++++++-- 2 files changed, 275 insertions(+), 22 deletions(-) diff --git a/python/dataprof/_report.py b/python/dataprof/_report.py index 0e0393d3..9820f6e7 100644 --- a/python/dataprof/_report.py +++ b/python/dataprof/_report.py @@ -8,8 +8,10 @@ import html as _html import json as _json import os as _os +import tempfile as _tempfile import warnings as _warnings from collections.abc import ( + Callable as _Callable, Iterator as _Iterator, Mapping as _Mapping, Sequence as _Sequence, @@ -51,6 +53,57 @@ def _quality_status_document(state: str, error: str | None) -> dict[str, _Any]: return document +def _atomic_write(path: str, writer: _Callable[[str], None]) -> None: + path = _os.path.realpath(path) + directory = _os.path.dirname(_os.path.abspath(path)) + basename = _os.path.basename(path) + + fd, temp_path = _tempfile.mkstemp( + dir=directory, + prefix=f".{basename}.", + suffix=".tmp", + ) + _os.close(fd) + + try: + writer(temp_path) + + try: + mode = _os.stat(path).st_mode & 0o777 + except FileNotFoundError: + mode_path = f"{temp_path}.mode" + mode_fd = _os.open( + mode_path, + _os.O_WRONLY | _os.O_CREAT | _os.O_EXCL, + 0o666, + ) + try: + mode = _os.fstat(mode_fd).st_mode & 0o777 + finally: + _os.close(mode_fd) + _os.unlink(mode_path) + + _os.chmod(temp_path, mode) + _os.replace(temp_path, path) + finally: + try: + _os.unlink(temp_path) + except FileNotFoundError: + pass + + +def _atomic_write_text( + path: str, + content: str, + newline: str | None = None, +) -> None: + def write(temp_path: str) -> None: + with open(temp_path, "w", encoding="utf-8", newline=newline) as f: + f.write(content) + + _atomic_write(path, write) + + class ProfileReport: """Report access and export methods shared by native and restored backings. @@ -739,30 +792,38 @@ def save(self, path: str | _os.PathLike[str]) -> ProfileReport: """ path = _normalize_pathlike(path) lower_path = path.lower() + if lower_path.endswith(".json"): - with open(path, "w", encoding="utf-8", newline="\n") as f: - f.write(self.to_json()) + _atomic_write_text(path, self.to_json(), newline="\n") + elif lower_path.endswith(".csv"): records = self._records() + # Opening unconditionally matters for a zero-column report: save() # must either create the requested artifact or fail, never return # success while leaving no file behind. - with open(path, "w", encoding="utf-8", newline="") as f: - if records: - writer = _csv.DictWriter(f, fieldnames=records[0].keys()) - writer.writeheader() - writer.writerows(records) + def write_csv(temp_path: str) -> None: + with open(temp_path, "w", encoding="utf-8", newline="") as f: + if records: + writer = _csv.DictWriter(f, fieldnames=records[0].keys()) + writer.writeheader() + writer.writerows(records) + + _atomic_write(path, write_csv) elif lower_path.endswith(".parquet"): table = self.to_arrow() import pyarrow.parquet as pq - pq.write_table(table, path) + def write_parquet(temp_path: str) -> None: + pq.write_table(table, temp_path) + + _atomic_write(path, write_parquet) + elif lower_path.endswith(".html"): - with open(path, "w", encoding="utf-8") as f: - f.write(self.to_html()) + _atomic_write_text(path, self.to_html()) + elif lower_path.endswith((".md", ".markdown")): - with open(path, "w", encoding="utf-8") as f: - f.write(self.to_markdown()) + _atomic_write_text(path, self.to_markdown()) else: raise ValueError( f"Unsupported format for '{path}'. " diff --git a/python/tests/test_report_exports.py b/python/tests/test_report_exports.py index 8065bb24..c744d809 100644 --- a/python/tests/test_report_exports.py +++ b/python/tests/test_report_exports.py @@ -410,6 +410,199 @@ def test_save_accepts_pathlike(self, report, tmp_path): assert result is report assert dataprof.ProfileReport.load(path).to_dict() == report.to_dict() + def test_failed_json_save_preserves_existing_file(self, report, tmp_path, monkeypatch): + path = tmp_path / "report.json" + original = b"previous good report\n" + path.write_bytes(original) + + def fail_to_json(): + raise RuntimeError("simulated serialization failure") + + monkeypatch.setattr(report, "to_json", fail_to_json) + + with pytest.raises(RuntimeError, match="simulated serialization failure"): + report.save(path) + + assert path.read_bytes() == original + + def test_failed_json_write_preserves_existing_file_and_cleans_temp( + self, report, tmp_path, monkeypatch + ): + import builtins + + path = tmp_path / "report.json" + original = b"previous good report\n" + path.write_bytes(original) + + real_open = builtins.open + + class FailingFile: + def __enter__(self): + return self + + def __exit__(self, exc_type, exc_value, traceback): + return False + + def write(self, data): + raise OSError("simulated write failure") + + def failing_open(file, *args, **kwargs): + name = Path(file).name + + if name.startswith(".report.json.") and name.endswith(".tmp"): + return FailingFile() + + return real_open(file, *args, **kwargs) + + monkeypatch.setattr(builtins, "open", failing_open) + + with pytest.raises(OSError, match="simulated write failure"): + report.save(path) + + monkeypatch.undo() + + assert path.read_bytes() == original + assert list(tmp_path.glob(".report.json.*.tmp")) == [] + + @pytest.mark.parametrize( + ("suffix", "method_name"), + [ + (".json", "to_json"), + (".csv", "_records"), + (".parquet", "to_arrow"), + (".html", "to_html"), + (".md", "to_markdown"), + (".markdown", "to_markdown"), + ], + ) + def test_failed_serialization_preserves_existing_file( + self, report, tmp_path, monkeypatch, suffix, method_name + ): + path = tmp_path / f"report{suffix}" + original = b"previous good report\n" + path.write_bytes(original) + + def fail(): + raise RuntimeError("simulated serialization failure") + + monkeypatch.setattr(report, method_name, fail) + + with pytest.raises(RuntimeError, match="simulated serialization failure"): + report.save(path) + + assert path.read_bytes() == original + + @pytest.mark.parametrize( + "suffix", + [".csv", ".html", ".md", ".markdown"], + ) + def test_failed_write_preserves_existing_file_and_cleans_temp( + self, report, tmp_path, monkeypatch, suffix + ): + import builtins + + path = tmp_path / f"report{suffix}" + original = b"previous good report\n" + path.write_bytes(original) + + real_open = builtins.open + + class FailingFile: + def __init__(self, inner): + self.inner = inner + + def __enter__(self): + return self + + def __exit__(self, exc_type, exc_value, traceback): + self.inner.close() + return False + + def write(self, data): + # Attempt to simulate a real partial write before the failure. + partial = data[: max(1, len(data) // 2)] + self.inner.write(partial) + self.inner.flush() + raise OSError("simulated write failure") + + def failing_open(file, *args, **kwargs): + name = Path(file).name + + if name.startswith(f".report{suffix}.") and name.endswith(".tmp"): + return FailingFile(real_open(file, *args, **kwargs)) + + return real_open(file, *args, **kwargs) + + monkeypatch.setattr(builtins, "open", failing_open) + + with pytest.raises(OSError, match="simulated write failure"): + report.save(path) + + monkeypatch.undo() + + assert path.read_bytes() == original + assert list(tmp_path.glob(f".report{suffix}.*.tmp")) == [] + + @pytest.mark.skipif( + os.name == "nt", + reason="POSIX file permissions are not available on Windows", + ) + def test_save_preserves_existing_file_permissions(self, report, tmp_path): + path = tmp_path / "report.json" + path.write_text("old report", encoding="utf-8") + path.chmod(0o640) + + report.save(path) + + assert path.stat().st_mode & 0o777 == 0o640 + + def test_new_save_uses_default_file_permissions(self, report, tmp_path): + reference = tmp_path / "reference.txt" + reference.write_text("reference", encoding="utf-8") + + path = tmp_path / "report.json" + report.save(path) + + assert path.stat().st_mode & 0o777 == reference.stat().st_mode & 0o777 + + def test_save_through_symlink_preserves_symlink(self, report, tmp_path): + target = tmp_path / "target.json" + target.write_text("old report", encoding="utf-8") + + path = tmp_path / "report.json" + + try: + path.symlink_to(target) + except OSError as exc: + pytest.skip(f"symlinks are not available: {exc}") + + report.save(path) + + assert path.is_symlink() + assert dataprof.ProfileReport.load(path).to_dict() == report.to_dict() + + def test_failed_parquet_write_preserves_existing_file_and_cleans_temp( + self, report, tmp_path, monkeypatch + ): + pq = pytest.importorskip("pyarrow.parquet") + + path = tmp_path / "report.parquet" + original = b"previous good report\n" + path.write_bytes(original) + + def failing_write_table(table, destination, *args, **kwargs): + temp_path = Path(destination) + temp_path.write_bytes(b"partial parquet data") + raise OSError("simulated parquet write failure") + + monkeypatch.setattr(pq, "write_table", failing_write_table) + + with pytest.raises(OSError, match="simulated parquet write failure"): + report.save(path) + + assert path.read_bytes() == original + assert list(tmp_path.glob(".report.parquet.*.tmp")) == [] + def test_save_empty_report_csv_creates_artifact(self, tmp_path): report = dataprof.profile({}) path = tmp_path / "empty.csv" @@ -424,18 +617,17 @@ def test_save_and_load_extensions_are_case_insensitive(self, report, tmp_path): assert report.save(path) is report assert dataprof.ProfileReport.load(path).to_dict() == report.to_dict() - def test_save_parquet(self, report): + def test_save_parquet(self, report, tmp_path): pytest.importorskip("pyarrow") pq = pytest.importorskip("pyarrow.parquet") - with tempfile.NamedTemporaryFile(suffix=".parquet", delete=False) as f: - path = f.name - try: - result = report.save(path) - assert result is report - table = pq.read_table(path) - assert table.num_rows == report.columns - finally: - os.unlink(path) + + path = tmp_path / "report.parquet" + + result = report.save(path) + + assert result is report + table = pq.read_table(path) + assert table.num_rows == report.columns def test_to_json_is_byte_stable_for_unchanged_input(tmp_path): From df2feae839a96fbf359782ea355a27e56f3e7fcf Mon Sep 17 00:00:00 2001 From: Andrea Bozzo Date: Sat, 17 Oct 2026 13:10:11 +0200 Subject: [PATCH 08/16] test: check every input route against an independent oracle (#829) (#984) * test: check every input route against an independent oracle (#829) tests/fixtures/oracle-v1.json holds 20 cases whose expected values were computed with exact rational arithmetic, without dataprof. The Python test runs each through every route that carries it, the Rust twin through the file engines, and both compare at the serialized precision. A strict table lists the disagreements open issues own (#877, #982, #983). * test: bound wide order statistics by an ulp and check known entries per route (#829) Medians, modes and quartiles past 2^53 are compared within half an ulp of the exact value, as the mean was, instead of a relative tolerance that accepted errors of millions. A wildcard known entry must still disagree on every route it covers. The Rust twin reads JSON integers exactly. --- python/tests/test_oracle.py | 277 ++++++++++ tests/fixtures/oracle-v1.json | 938 ++++++++++++++++++++++++++++++++++ tests/oracle.rs | 474 +++++++++++++++++ 3 files changed, 1689 insertions(+) create mode 100644 python/tests/test_oracle.py create mode 100644 tests/fixtures/oracle-v1.json create mode 100644 tests/oracle.rs diff --git a/python/tests/test_oracle.py b/python/tests/test_oracle.py new file mode 100644 index 00000000..f1062f75 --- /dev/null +++ b/python/tests/test_oracle.py @@ -0,0 +1,277 @@ +"""Profile the oracle-v1 matrix through every input route and check it against +answers computed without dataprof (#829). + +Parity tests compare routes with each other, so they pass when every route is +wrong the same way. `tests/fixtures/oracle-v1.json` holds 20 cases whose +expected values were computed with exact rational arithmetic and 100-digit +decimals, independently of this code: numeric edges (integers past 2^53, the +uint64 boundary, the 64-value SIMD threshold, cancellation, a variance whose +intermediate sum of squares overflows), nested values, and the null +vocabulary. Each case runs through every route that can carry it, and every +disagreement is collected before asserting, so one failing run shows which +routes and fields are wrong. + +`KNOWN` lists the disagreements an open issue owns. It is strict both ways: a +new disagreement fails, and so does a listed one that stops disagreeing, so the +table cannot outlive the fix that empties it. + +`tests/oracle.rs` is the Rust twin for the file engines. +""" + +from __future__ import annotations + +import csv +import hashlib +import io +import json +import math +from collections.abc import Callable, Iterator +from fractions import Fraction +from pathlib import Path +from typing import Any + +import dataprof as dp +import pytest + +pd = pytest.importorskip("pandas") +pa = pytest.importorskip("pyarrow") +pq = pytest.importorskip("pyarrow.parquet") +pl = pytest.importorskip("polars") + +FIXTURE = Path(__file__).parents[2] / "tests/fixtures/oracle-v1.json" +#: The hash published with the fixture on #829, so an edit to an expected +#: value cannot pass unnoticed. +FIXTURE_SHA256 = "fe75498f65bc0b7375d7c0cb09e8c98bbf8999662132f5aff54868a95a495409" +CASES = json.loads(FIXTURE.read_text(encoding="utf-8"))["cases"] + +#: Half a unit of the 4dp serialized precision, plus room for the binary +#: representation of the rounded value. +ABS_TOLERANCE = 0.000051 +#: For magnitudes where 4dp carries no information (around 1e307). +REL_TOLERANCE = 1e-12 + +#: (case, route, field) -> the issue that owns the disagreement. A route of +#: "*" covers every route the case runs on. +KNOWN: dict[tuple[str, str, str], str] = { + # 64-bit integer extrema and order statistics are taken in f64, so values + # past 2^53 collapse: the extremes round, distinct values tie for a mode, + # and the quartiles of a narrow spread coincide. + **{ + (case, "*", field): "#877" + for case in ("above_2_pow_53", "epoch_nanoseconds", "uint64_edge") + for field in ("min", "max", "mode") + }, + ("epoch_nanoseconds", "*", "quartiles.iqr"): "#877", + ("uint64_edge", "*", "quartiles.iqr"): "#877", + # An all-null text column reports lengths of 0 rather than none. + ("all_null", "*", "stats"): "#983", + # The pandas mixed-object fallback renders a container as the text a + # string can spell, so the two sign one row. + ("container_vs_its_text", "pandas", "duplicate_rows"): "#982", + ("container_and_text_repeats", "pandas", "duplicate_rows"): "#982", +} + +Route = tuple[str, Callable[[], dp.ProfileReport]] +Mismatch = tuple[str, str, str, Any, Any] + + +def _values(case: dict[str, Any]) -> list[Any]: + if case["kind"] != "numeric": + return case["values"] + cast = float if case["dtype"] == "float64" else int + return [cast(value) for value in case["values"]] + + +def _routes(case: dict[str, Any], directory: Path) -> Iterator[Route]: + """Every public route that carries the case's logical data.""" + values = _values(case) + records = [{"v": value} for value in values] + wanted = set(case.get("routes", [])) + + def allowed(route: str) -> bool: + return not wanted or route in wanted + + if allowed("dict"): + yield "dict", lambda: dp.profile({"v": values}) + if allowed("records"): + yield "records", lambda: dp.profile(records) + document = json.dumps(records, allow_nan=False) + if allowed("json"): + path = directory / f"{case['id']}.json" + path.write_text(document, encoding="utf-8") + yield "json", lambda path=path: dp.profile(str(path)) + yield "json_bytes", lambda: dp.profile(document.encode(), format="json") + if allowed("jsonl"): + path = directory / f"{case['id']}.jsonl" + path.write_text("".join(json.dumps(row) + "\n" for row in records), encoding="utf-8") + yield "jsonl", lambda path=path: dp.profile(str(path)) + + # A container in a CSV cell is text, not the same logical value. + if case["kind"] != "nested": + buffer = io.StringIO(newline="") + writer = csv.writer(buffer, quoting=csv.QUOTE_ALL) + writer.writerow(["v"]) + writer.writerows([[value] for value in values]) + text = buffer.getvalue() + path = directory / f"{case['id']}.csv" + path.write_text(text, encoding="utf-8", newline="") + for engine in ("incremental", "columnar"): + yield ( + f"csv_{engine}", + lambda engine=engine, path=path: dp.profile(str(path), engine=engine), + ) + yield "csv_bytes", lambda: dp.profile(text.encode(), format="csv") + + if case["kind"] == "numeric": + dtype = case["dtype"] + frame = pd.DataFrame({"v": pd.Series(values, dtype=dtype)}) + table = pa.table({"v": pa.array(values, type=getattr(pa, dtype)())}) + polars_dtype = {"int64": pl.Int64, "uint64": pl.UInt64, "float64": pl.Float64}[dtype] + polars_frame = pl.DataFrame({"v": pl.Series(values, dtype=polars_dtype)}) + else: + frame = pd.DataFrame({"v": pd.Series(values, dtype=object)}) + table = pa.table({"v": values}) if allowed("arrow") else None + polars_frame = pl.from_arrow(table) if table is not None else None + + if allowed("pandas"): + yield "pandas", lambda: dp.profile(frame) + if table is not None: + if allowed("polars"): + yield "polars", lambda: dp.profile(polars_frame) + if allowed("arrow"): + yield "arrow", lambda: dp.profile(table) + if allowed("parquet"): + path = directory / f"{case['id']}.parquet" + pq.write_table(table, path) + yield "parquet", lambda path=path: dp.profile(str(path)) + + +def _faithful(actual: Any, wanted: str) -> bool: + """Whether `actual` is a faithful binary64 rendering of the exact `wanted`. + + Past 2^53 adjacent floats are hundreds apart, so neither a 4dp nor a + relative tolerance says anything there: the bound is half an ulp of the + exact value, the error rounding to the nearest float may carry. + """ + if actual is None: + return False + exact = Fraction(wanted) + return abs(Fraction(actual) - exact) <= Fraction(math.ulp(float(exact))) / 2 + + +def _close(actual: Any, wanted: str, relative: bool) -> bool: + if actual is None: + return False + target = float(Fraction(wanted)) + if relative: + return math.isclose(float(actual), target, rel_tol=REL_TOLERANCE, abs_tol=0.0) + return math.isclose(float(actual), target, rel_tol=0.0, abs_tol=ABS_TOLERANCE) + + +def _check(case: dict[str, Any], route: str, report: dp.ProfileReport) -> list[Mismatch]: + """Every field of the case's expectation that `report` gets wrong.""" + column = report.to_dict()["columns"][0] + quality = report.to_dict().get("quality") or {} + expected = case.get("expected") or case["expected_common_markers"] + found: list[Mismatch] = [] + + def compare( + field: str, actual: Any, wanted: Any, relative: bool = False, faithful: bool = False + ) -> None: + if wanted is None: + good = actual is None + elif isinstance(wanted, str) and field != "data_type": + good = _faithful(actual, wanted) if faithful else _close(actual, wanted, relative) + else: + good = actual == wanted + if not good: + found.append((case["id"], route, field, wanted, actual)) + + for field in ("total_count", "null_count", "unique_count", "data_type", "stats"): + if field in expected: + compare(field, column.get(field), expected[field]) + if "null_percentage" in expected: + compare("null_percentage", column.get("null_percentage"), expected["null_percentage"]) + if "duplicate_rows_single_column" in expected: + uniqueness = quality.get("uniqueness") or {} + compare( + "duplicate_rows", + uniqueness.get("duplicate_rows"), + expected["duplicate_rows_single_column"], + ) + if case["kind"] != "numeric": + return found + + stats = column.get("stats") or {} + # Past 2^53 an exact integer mean can fall between two binary64 values; + # either neighbour is a faithful float, so the bound is half an ulp. + wide = case["dtype"] != "float64" and any(abs(int(v)) > 2**53 for v in case["values"]) + # Around 1e307 the 4dp rounding carries no information; compare relatively. + extreme = case["id"] == "variance_fits_m2_does_not" + variance = expected["sample_variance_binary64"] if extreme else expected["variance_4dp"] + compare("variance", stats.get("variance"), variance, relative=extreme) + compare("std_dev", stats.get("std_dev"), expected["std_dev_4dp"], relative=extreme) + if wide: + compare("mean", stats.get("mean"), expected["mean_exact"], faithful=True) + else: + compare("mean", stats.get("mean"), expected["mean_4dp"], relative=extreme) + for name in ("min", "max"): + exact = Fraction(expected[f"{name}_exact"]) + actual = stats.get(name) + if wide: + # An exact integer extreme needs an integer-preserving value; + # comparing after float() would erase the defect (#877). + if actual is None or Fraction(actual) != exact: + found.append((case["id"], route, name, str(exact), actual)) + else: + compare(name, actual, str(float(exact)), relative=extreme) + if extreme: + return found + compare("skewness", stats.get("skewness"), expected["adjusted_sample_skewness_4dp"]) + compare("kurtosis", stats.get("kurtosis"), expected["adjusted_sample_excess_kurtosis_4dp"]) + compare("median", stats.get("median"), expected["median_exact"], faithful=wide) + compare("mode", stats.get("mode"), expected["mode_exact"], faithful=wide) + compare("outlier_count", stats.get("outlier_count"), expected["outlier_count"]) + quartiles = stats.get("quartiles") + if expected["quartiles_4dp"] is None: + compare("quartiles", quartiles, None) + else: + for name, value in expected["quartiles_4dp"].items(): + compare(f"quartiles.{name}", (quartiles or {}).get(name), value, faithful=wide) + return found + + +def _owner(mismatch: Mismatch) -> str | None: + case, route, field = mismatch[:3] + return KNOWN.get((case, route, field)) or KNOWN.get((case, "*", field)) + + +def test_the_fixture_is_the_published_one(): + assert hashlib.sha256(FIXTURE.read_bytes()).hexdigest() == FIXTURE_SHA256 + + +@pytest.mark.parametrize("case", CASES, ids=[case["id"] for case in CASES]) +def test_every_route_matches_the_independent_answer(case: dict[str, Any], tmp_path: Path): + seen: list[tuple[str, str]] = [] + mismatches: list[Mismatch] = [] + for route, run in _routes(case, tmp_path): + seen.append((case["id"], route)) + mismatches.extend(_check(case, route, run())) + + unexplained = [m for m in mismatches if _owner(m) is None] + explained = {(m[0], m[1], m[2]) for m in mismatches if _owner(m) is not None} + # A "*" entry claims every route, so each route it covers must still + # disagree: a fix on one route narrows the entry instead of hiding there. + stale = [ + (case["id"], route, key[2]) + for key in KNOWN + if key[0] == case["id"] + for _, route in seen + if key[1] in ("*", route) and (case["id"], route, key[2]) not in explained + ] + report = "\n".join( + f" {route}: {field} expected {wanted!r}, got {actual!r}" + for _, route, field, wanted, actual in unexplained + ) + assert not unexplained, f"{case['id']} disagrees with the oracle:\n{report}" + assert not stale, f"{case['id']}: known disagreements that no longer occur: {stale}" diff --git a/tests/fixtures/oracle-v1.json b/tests/fixtures/oracle-v1.json new file mode 100644 index 00000000..d9b1c646 --- /dev/null +++ b/tests/fixtures/oracle-v1.json @@ -0,0 +1,938 @@ +{ + "fixture_version": 1, + "audited_commit": "0d8d6ad0aa7d343334b487349076bca96b97d83f", + "oracle": "fractions.Fraction + 100-digit decimal.Decimal; no dataprof imports", + "conventions": { + "variance": "sample, ddof=1", + "quartiles": "Hyndman-Fan type 7; absent below four observations", + "mode": "absent when all unique; smallest tied modal value", + "numeric_strings": "parse according to dtype before constructing typed inputs", + "undefined": "null, never zero except the documented constant shape convention", + "float_input": "exact rational value of supplied IEEE binary64", + "precision": "4dp ordinary statistics, 2dp percentages; exact integer targets remain strings" + }, + "cases": [ + { + "id": "ordinary", + "kind": "numeric", + "column": "v", + "dtype": "int64", + "values": [ + "1", + "2", + "3", + "4" + ], + "expected": { + "total_count": 4, + "null_count": 0, + "unique_count": 4, + "duplicate_rows_single_column": 0, + "min_exact": "1", + "max_exact": "4", + "mean_exact": "5/2", + "sample_variance_exact": "5/3", + "mean_binary64": "2.5", + "sample_variance_binary64": "1.6666666666666667", + "mean_4dp": "2.5000", + "variance_4dp": "1.6667", + "std_dev_4dp": "1.2910", + "median_exact": "5/2", + "mode_exact": null, + "quartiles_4dp": { + "q1": "1.7500", + "q2": "2.5000", + "q3": "3.2500", + "iqr": "1.5000" + }, + "outlier_count": 0, + "adjusted_sample_skewness_4dp": "0.0000", + "adjusted_sample_excess_kurtosis_4dp": "-1.2000" + }, + "notes": [] + }, + { + "id": "mixed_sign_zero_mean", + "kind": "numeric", + "column": "v", + "dtype": "int64", + "values": [ + "-10", + "-2", + "0", + "4", + "8" + ], + "expected": { + "total_count": 5, + "null_count": 0, + "unique_count": 5, + "duplicate_rows_single_column": 0, + "min_exact": "-10", + "max_exact": "8", + "mean_exact": "0", + "sample_variance_exact": "46", + "mean_binary64": "0.0", + "sample_variance_binary64": "46.0", + "mean_4dp": "0.0000", + "variance_4dp": "46.0000", + "std_dev_4dp": "6.7823", + "median_exact": "0", + "mode_exact": null, + "quartiles_4dp": { + "q1": "-2.0000", + "q2": "0.0000", + "q3": "4.0000", + "iqr": "6.0000" + }, + "outlier_count": 0, + "adjusted_sample_skewness_4dp": "-0.5769", + "adjusted_sample_excess_kurtosis_4dp": "0.4877" + }, + "notes": [] + }, + { + "id": "constant", + "kind": "numeric", + "column": "v", + "dtype": "int64", + "values": [ + "7", + "7", + "7", + "7" + ], + "expected": { + "total_count": 4, + "null_count": 0, + "unique_count": 1, + "duplicate_rows_single_column": 3, + "min_exact": "7", + "max_exact": "7", + "mean_exact": "7", + "sample_variance_exact": "0", + "mean_binary64": "7.0", + "sample_variance_binary64": "0.0", + "mean_4dp": "7.0000", + "variance_4dp": "0.0000", + "std_dev_4dp": "0.0000", + "median_exact": "7", + "mode_exact": "7", + "quartiles_4dp": { + "q1": "7.0000", + "q2": "7.0000", + "q3": "7.0000", + "iqr": "0.0000" + }, + "outlier_count": 0, + "adjusted_sample_skewness_4dp": "0.0000", + "adjusted_sample_excess_kurtosis_4dp": "0.0000" + }, + "notes": [ + "Zero shape moments are dataprof's explicit constant-column convention; mathematically standardized moments are undefined." + ] + }, + { + "id": "two_values", + "kind": "numeric", + "column": "v", + "dtype": "int64", + "values": [ + "1", + "2" + ], + "expected": { + "total_count": 2, + "null_count": 0, + "unique_count": 2, + "duplicate_rows_single_column": 0, + "min_exact": "1", + "max_exact": "2", + "mean_exact": "3/2", + "sample_variance_exact": "1/2", + "mean_binary64": "1.5", + "sample_variance_binary64": "0.5", + "mean_4dp": "1.5000", + "variance_4dp": "0.5000", + "std_dev_4dp": "0.7071", + "median_exact": "3/2", + "mode_exact": null, + "quartiles_4dp": null, + "outlier_count": null, + "adjusted_sample_skewness_4dp": null, + "adjusted_sample_excess_kurtosis_4dp": null + }, + "notes": [] + }, + { + "id": "asymmetric", + "kind": "numeric", + "column": "v", + "dtype": "int64", + "values": [ + "1", + "2", + "3", + "10", + "4", + "5" + ], + "expected": { + "total_count": 6, + "null_count": 0, + "unique_count": 6, + "duplicate_rows_single_column": 0, + "min_exact": "1", + "max_exact": "10", + "mean_exact": "25/6", + "sample_variance_exact": "61/6", + "mean_binary64": "4.166666666666667", + "sample_variance_binary64": "10.166666666666666", + "mean_4dp": "4.1667", + "variance_4dp": "10.1667", + "std_dev_4dp": "3.1885", + "median_exact": "7/2", + "mode_exact": null, + "quartiles_4dp": { + "q1": "2.2500", + "q2": "3.5000", + "q3": "4.7500", + "iqr": "2.5000" + }, + "outlier_count": 1, + "adjusted_sample_skewness_4dp": "1.4396", + "adjusted_sample_excess_kurtosis_4dp": "2.4377" + }, + "notes": [] + }, + { + "id": "above_2_pow_53", + "kind": "numeric", + "column": "v", + "dtype": "int64", + "values": [ + "9007199254740993", + "9007199254740995", + "9007199254740997" + ], + "expected": { + "total_count": 3, + "null_count": 0, + "unique_count": 3, + "duplicate_rows_single_column": 0, + "min_exact": "9007199254740993", + "max_exact": "9007199254740997", + "mean_exact": "9007199254740995", + "sample_variance_exact": "4", + "mean_binary64": "9007199254740996.0", + "sample_variance_binary64": "4.0", + "mean_4dp": "9007199254740995.0000", + "variance_4dp": "4.0000", + "std_dev_4dp": "2.0000", + "median_exact": "9007199254740995", + "mode_exact": null, + "quartiles_4dp": null, + "outlier_count": null, + "adjusted_sample_skewness_4dp": "0.0000", + "adjusted_sample_excess_kurtosis_4dp": null + }, + "notes": [ + "#877: exact extrema/order statistics remain known limitations. Mean may be nearest binary64; do not demand an unrepresentable exact mean from a float field." + ] + }, + { + "id": "epoch_nanoseconds", + "kind": "numeric", + "column": "v", + "dtype": "int64", + "values": [ + "1759700000123456789", + "1759700000123456791", + "1759700000123456795", + "1759700000123456800" + ], + "expected": { + "total_count": 4, + "null_count": 0, + "unique_count": 4, + "duplicate_rows_single_column": 0, + "min_exact": "1759700000123456789", + "max_exact": "1759700000123456800", + "mean_exact": "7038800000493827175/4", + "sample_variance_exact": "283/12", + "mean_binary64": "1.7597000001234568e+18", + "sample_variance_binary64": "23.583333333333332", + "mean_4dp": "1759700000123456793.7500", + "variance_4dp": "23.5833", + "std_dev_4dp": "4.8563", + "median_exact": "1759700000123456793", + "mode_exact": null, + "quartiles_4dp": { + "q1": "1759700000123456790.5000", + "q2": "1759700000123456793.0000", + "q3": "1759700000123456796.2500", + "iqr": "5.7500" + }, + "outlier_count": 0, + "adjusted_sample_skewness_4dp": "0.6876", + "adjusted_sample_excess_kurtosis_4dp": "-0.9465" + }, + "notes": [ + "#877: extrema, mode and IQR are expected failures; variance and shape are mandatory now." + ] + }, + { + "id": "uint64_edge", + "kind": "numeric", + "column": "v", + "dtype": "uint64", + "values": [ + "18446744073709551612", + "18446744073709551613", + "18446744073709551614", + "18446744073709551615" + ], + "expected": { + "total_count": 4, + "null_count": 0, + "unique_count": 4, + "duplicate_rows_single_column": 0, + "min_exact": "18446744073709551612", + "max_exact": "18446744073709551615", + "mean_exact": "36893488147419103227/2", + "sample_variance_exact": "5/3", + "mean_binary64": "1.8446744073709552e+19", + "sample_variance_binary64": "1.6666666666666667", + "mean_4dp": "18446744073709551613.5000", + "variance_4dp": "1.6667", + "std_dev_4dp": "1.2910", + "median_exact": "36893488147419103227/2", + "mode_exact": null, + "quartiles_4dp": { + "q1": "18446744073709551612.7500", + "q2": "18446744073709551613.5000", + "q3": "18446744073709551614.2500", + "iqr": "1.5000" + }, + "outlier_count": 0, + "adjusted_sample_skewness_4dp": "0.0000", + "adjusted_sample_excess_kurtosis_4dp": "-1.2000" + }, + "notes": [ + "Preserve as uint64/decimal strings; never build a pandas float64 series first. #877 order-statistic limitation applies." + ] + }, + { + "id": "simd_64", + "kind": "numeric", + "column": "v", + "dtype": "int64", + "values": [ + "0", + "1", + "2", + "3", + "0", + "1", + "2", + "3", + "0", + "1", + "2", + "3", + "0", + "1", + "2", + "3", + "0", + "1", + "2", + "3", + "0", + "1", + "2", + "3", + "0", + "1", + "2", + "3", + "0", + "1", + "2", + "3", + "0", + "1", + "2", + "3", + "0", + "1", + "2", + "3", + "0", + "1", + "2", + "3", + "0", + "1", + "2", + "3", + "0", + "1", + "2", + "3", + "0", + "1", + "2", + "3", + "0", + "1", + "2", + "3", + "0", + "1", + "2", + "3" + ], + "expected": { + "total_count": 64, + "null_count": 0, + "unique_count": 4, + "duplicate_rows_single_column": 60, + "min_exact": "0", + "max_exact": "3", + "mean_exact": "3/2", + "sample_variance_exact": "80/63", + "mean_binary64": "1.5", + "sample_variance_binary64": "1.2698412698412698", + "mean_4dp": "1.5000", + "variance_4dp": "1.2698", + "std_dev_4dp": "1.1269", + "median_exact": "3/2", + "mode_exact": "0", + "quartiles_4dp": { + "q1": "0.7500", + "q2": "1.5000", + "q3": "2.2500", + "iqr": "1.5000" + }, + "outlier_count": 0, + "adjusted_sample_skewness_4dp": "0.0000", + "adjusted_sample_excess_kurtosis_4dp": "-1.3726" + }, + "notes": [] + }, + { + "id": "simd_65", + "kind": "numeric", + "column": "v", + "dtype": "int64", + "values": [ + "0", + "1", + "2", + "3", + "0", + "1", + "2", + "3", + "0", + "1", + "2", + "3", + "0", + "1", + "2", + "3", + "0", + "1", + "2", + "3", + "0", + "1", + "2", + "3", + "0", + "1", + "2", + "3", + "0", + "1", + "2", + "3", + "0", + "1", + "2", + "3", + "0", + "1", + "2", + "3", + "0", + "1", + "2", + "3", + "0", + "1", + "2", + "3", + "0", + "1", + "2", + "3", + "0", + "1", + "2", + "3", + "0", + "1", + "2", + "3", + "0", + "1", + "2", + "3", + "0" + ], + "expected": { + "total_count": 65, + "null_count": 0, + "unique_count": 4, + "duplicate_rows_single_column": 61, + "min_exact": "0", + "max_exact": "3", + "mean_exact": "96/65", + "sample_variance_exact": "167/130", + "mean_binary64": "1.476923076923077", + "sample_variance_binary64": "1.2846153846153847", + "mean_4dp": "1.4769", + "variance_4dp": "1.2846", + "std_dev_4dp": "1.1334", + "median_exact": "1", + "mode_exact": "0", + "quartiles_4dp": { + "q1": "0.0000", + "q2": "1.0000", + "q3": "2.0000", + "iqr": "2.0000" + }, + "outlier_count": 0, + "adjusted_sample_skewness_4dp": "0.0257", + "adjusted_sample_excess_kurtosis_4dp": "-1.3883" + }, + "notes": [] + }, + { + "id": "large_offset_64", + "kind": "numeric", + "column": "v", + "dtype": "int64", + "values": [ + "1000000000000", + "1000000000001", + "1000000000002", + "1000000000003", + "1000000000000", + "1000000000001", + "1000000000002", + "1000000000003", + "1000000000000", + "1000000000001", + "1000000000002", + "1000000000003", + "1000000000000", + "1000000000001", + "1000000000002", + "1000000000003", + "1000000000000", + "1000000000001", + "1000000000002", + "1000000000003", + "1000000000000", + "1000000000001", + "1000000000002", + "1000000000003", + "1000000000000", + "1000000000001", + "1000000000002", + "1000000000003", + "1000000000000", + "1000000000001", + "1000000000002", + "1000000000003", + "1000000000000", + "1000000000001", + "1000000000002", + "1000000000003", + "1000000000000", + "1000000000001", + "1000000000002", + "1000000000003", + "1000000000000", + "1000000000001", + "1000000000002", + "1000000000003", + "1000000000000", + "1000000000001", + "1000000000002", + "1000000000003", + "1000000000000", + "1000000000001", + "1000000000002", + "1000000000003", + "1000000000000", + "1000000000001", + "1000000000002", + "1000000000003", + "1000000000000", + "1000000000001", + "1000000000002", + "1000000000003", + "1000000000000", + "1000000000001", + "1000000000002", + "1000000000003" + ], + "expected": { + "total_count": 64, + "null_count": 0, + "unique_count": 4, + "duplicate_rows_single_column": 60, + "min_exact": "1000000000000", + "max_exact": "1000000000003", + "mean_exact": "2000000000003/2", + "sample_variance_exact": "80/63", + "mean_binary64": "1000000000001.5", + "sample_variance_binary64": "1.2698412698412698", + "mean_4dp": "1000000000001.5000", + "variance_4dp": "1.2698", + "std_dev_4dp": "1.1269", + "median_exact": "2000000000003/2", + "mode_exact": "1000000000000", + "quartiles_4dp": { + "q1": "1000000000000.7500", + "q2": "1000000000001.5000", + "q3": "1000000000002.2500", + "iqr": "1.5000" + }, + "outlier_count": 0, + "adjusted_sample_skewness_4dp": "0.0000", + "adjusted_sample_excess_kurtosis_4dp": "-1.3726" + }, + "notes": [] + }, + { + "id": "cancellation", + "kind": "numeric", + "column": "v", + "dtype": "float64", + "values": [ + "-1000000000000.0", + "1.0", + "1000000000000.0" + ], + "expected": { + "total_count": 3, + "null_count": 0, + "unique_count": 3, + "duplicate_rows_single_column": 0, + "min_exact": "-1000000000000", + "max_exact": "1000000000000", + "mean_exact": "1/3", + "sample_variance_exact": "3000000000000000000000001/3", + "mean_binary64": "0.3333333333333333", + "sample_variance_binary64": "1e+24", + "mean_4dp": "0.3333", + "variance_4dp": "1000000000000000000000000.3333", + "std_dev_4dp": "1000000000000.0000", + "median_exact": "1", + "mode_exact": null, + "quartiles_4dp": null, + "outlier_count": null, + "adjusted_sample_skewness_4dp": "-0.0000", + "adjusted_sample_excess_kurtosis_4dp": null + }, + "notes": [] + }, + { + "id": "variance_fits_m2_does_not", + "kind": "numeric", + "column": "v", + "dtype": "float64", + "values": [ + "-7e+153", + "7e+153", + "-7e+153", + "7e+153" + ], + "expected": { + "total_count": 4, + "null_count": 0, + "unique_count": 2, + "duplicate_rows_single_column": 2, + "min_exact": "-6999999999999999812063107609186712233361878314499896009468692980217164324595475341466780559922101737600776669044728186587542627454761660556073094092947456", + "max_exact": "6999999999999999812063107609186712233361878314499896009468692980217164324595475341466780559922101737600776669044728186587542627454761660556073094092947456", + "mean_exact": "0", + "sample_variance_exact": "195999999999999989475534026114456026349367271676528028779069413094081833701602300025803936559401972368386088681890464802872128308312907921015659788480645324324307119799838030887033365081000266559657524758864226094648549510952190699937329163951435954071576762323756507534959814884274963111659533347830307487744/3", + "mean_binary64": "0.0", + "sample_variance_binary64": "6.533333333333333e+307", + "mean_4dp": "0.0000", + "variance_4dp": "65333333333333329825178008704818675449789090558842676259689804364693944567200766675267978853133990790000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000.0000", + "std_dev_4dp": "8082903768654760486117252102031840939217375263803738313097158955132786283991214063878752744551741607000000000000000000000000000000000000000000000000000000.0000", + "median_exact": "0", + "mode_exact": "-6999999999999999812063107609186712233361878314499896009468692980217164324595475341466780559922101737600776669044728186587542627454761660556073094092947456", + "quartiles_4dp": { + "q1": "-6999999999999999812063107609186712233361878314499896009468692980217164324595475341466780559922101738000000000000000000000000000000000000000000000000000000.0000", + "q2": "0.0000", + "q3": "6999999999999999812063107609186712233361878314499896009468692980217164324595475341466780559922101738000000000000000000000000000000000000000000000000000000.0000", + "iqr": "13999999999999999624126215218373424466723756628999792018937385960434328649190950682933561119844203480000000000000000000000000000000000000000000000000000000.0000" + }, + "outlier_count": 0, + "adjusted_sample_skewness_4dp": "0.0000", + "adjusted_sample_excess_kurtosis_4dp": "-6.0000" + }, + "notes": [ + "Use sample_variance_binary64, relative tolerance 1e-12; a fixed decimal-place tolerance is meaningless at this magnitude. Check not-null. Higher moments/outlier fences are outside this first extreme-float assertion set." + ] + }, + { + "id": "object_key_order", + "kind": "nested", + "column": "v", + "values": [ + { + "a": 1, + "b": 2 + }, + { + "b": 2, + "a": 1 + }, + { + "a": 2, + "b": 2 + }, + null + ], + "routes": [ + "dict", + "records", + "json", + "jsonl", + "pandas", + "arrow", + "parquet", + "polars" + ], + "expected": { + "total_count": 4, + "null_count": 1, + "duplicate_rows_single_column": 1, + "data_type": "nested", + "stats": null, + "unique_count": null + } + }, + { + "id": "list_order", + "kind": "nested", + "column": "v", + "values": [ + [ + 1, + 2 + ], + [ + 2, + 1 + ], + [ + 1, + 2 + ], + [], + null + ], + "routes": [ + "dict", + "records", + "json", + "jsonl", + "pandas", + "arrow", + "parquet", + "polars" + ], + "expected": { + "total_count": 5, + "null_count": 1, + "duplicate_rows_single_column": 1, + "data_type": "nested", + "stats": null, + "unique_count": null + } + }, + { + "id": "inner_null_is_not_outer_null", + "kind": "nested", + "column": "v", + "values": [ + { + "x": null + }, + { + "x": 1 + }, + null, + { + "x": null + } + ], + "routes": [ + "dict", + "records", + "json", + "jsonl", + "pandas", + "arrow", + "parquet", + "polars" + ], + "expected": { + "total_count": 4, + "null_count": 1, + "duplicate_rows_single_column": 1, + "data_type": "nested", + "stats": null, + "unique_count": null + } + }, + { + "id": "container_vs_its_text", + "kind": "nested", + "column": "v", + "values": [ + { + "x": 1 + }, + "{\"x\":1}", + [ + 1, + 2 + ], + "[1,2]" + ], + "routes": [ + "dict", + "records", + "json", + "jsonl", + "pandas" + ], + "expected": { + "total_count": 4, + "null_count": 0, + "duplicate_rows_single_column": 0 + } + }, + { + "id": "container_and_text_repeats", + "kind": "nested", + "column": "v", + "values": [ + { + "x": 1 + }, + "{\"x\":1}", + [ + 1, + 2 + ], + "[1,2]", + { + "x": 1 + }, + "{\"x\":1}" + ], + "routes": [ + "dict", + "records", + "json", + "jsonl", + "pandas" + ], + "expected": { + "total_count": 6, + "null_count": 0, + "duplicate_rows_single_column": 2 + } + }, + { + "id": "null_tokens_and_near_misses", + "kind": "nulls", + "column": "v", + "values": [ + null, + "", + " ", + "\t", + "null", + "NULL", + "NaN", + "NA", + "N/A", + "n/a", + "#N/A", + "\\N", + "None", + "Na", + "na", + "none", + "NONE", + "#n/a", + "\\n", + "N/a", + "-", + "?", + "n.d.", + "0" + ], + "expected_common_markers": { + "total_count": 24, + "null_count": 13, + "null_percentage": "54.17", + "unique_count": 11, + "duplicate_rows_single_column": 1 + }, + "expected_typed_only": { + "null_count": 1, + "null_percentage": "4.17" + }, + "notes": [ + "Duplicate-row identity is raw lexical identity, not equality after the null vocabulary: only None and the empty string collide here. The other null-like spellings remain different row signatures by the documented RowSignature contract.", + "Typed-only mode is a proposed #846 setting, not an option on this commit. CSV carries an explicit sentinel for typed-only nulls; quoted empty fields alone are not a universal null protocol." + ] + }, + { + "id": "all_null", + "kind": "nulls", + "column": "v", + "values": [ + null, + null, + null, + null + ], + "expected_common_markers": { + "total_count": 4, + "null_count": 4, + "null_percentage": "100.00", + "unique_count": 0, + "stats": null, + "duplicate_rows_single_column": 3 + }, + "notes": [ + "The absence of text extrema/mean is the mathematical target. Master returns empty TextStats with zero lengths: record as an explicit absence-contract decision rather than silently changing the expected answer to match implementation." + ] + } + ] +} diff --git a/tests/oracle.rs b/tests/oracle.rs new file mode 100644 index 00000000..92762689 --- /dev/null +++ b/tests/oracle.rs @@ -0,0 +1,474 @@ +//! Profile the oracle-v1 matrix through the file engines and check it against +//! answers computed without dataprof (#829). +//! +//! `tests/fixtures/oracle-v1.json` holds 20 cases whose expected values were +//! computed with exact rational arithmetic, independently of this code. This +//! runs each case as CSV through the incremental and columnar engines and as +//! JSON and JSONL, compares at the serialized precision, and collects every +//! disagreement before asserting. `KNOWN` lists the ones an open issue owns, +//! strictly: a listed disagreement that stops occurring fails too. +//! +//! `python/tests/test_oracle.py` is the Python twin, and adds the in-memory +//! routes (dicts, records, pandas, polars, Arrow) and Parquet. + +use std::path::Path; + +use dataprof::{EngineType, ProfileReport, Profiler}; +use serde_json::{Value, json}; + +const ROUTES: [&str; 4] = ["csv_incremental", "csv_columnar", "json", "jsonl"]; + +/// Half a unit of the 4dp serialized precision, plus room for the binary +/// representation of the rounded value. +const ABS_TOLERANCE: f64 = 0.000051; +/// For magnitudes where 4dp carries no information (around 1e307). +const REL_TOLERANCE: f64 = 1e-12; + +/// (case, route, field) and the issue that owns the disagreement. A route of +/// "*" covers every route the case runs on. The same entries as the Python +/// twin, for the routes this file runs. +const KNOWN: &[(&str, &str, &str, &str)] = &[ + // 64-bit integer extrema and order statistics are taken in f64, so values + // past 2^53 collapse: the extremes round, distinct values tie for a mode, + // and the quartiles of a narrow spread coincide. + ("above_2_pow_53", "*", "min", "#877"), + ("above_2_pow_53", "*", "max", "#877"), + ("above_2_pow_53", "*", "mode", "#877"), + ("epoch_nanoseconds", "*", "min", "#877"), + ("epoch_nanoseconds", "*", "max", "#877"), + ("epoch_nanoseconds", "*", "mode", "#877"), + ("epoch_nanoseconds", "*", "quartiles.iqr", "#877"), + ("uint64_edge", "*", "min", "#877"), + ("uint64_edge", "*", "max", "#877"), + ("uint64_edge", "*", "mode", "#877"), + ("uint64_edge", "*", "quartiles.iqr", "#877"), + // An all-null text column reports lengths of 0 rather than none. + ("all_null", "*", "stats", "#983"), +]; + +#[derive(Debug, Clone, PartialEq)] +struct Mismatch { + case: String, + route: &'static str, + field: String, + expected: String, + actual: String, +} + +fn fixture() -> Value { + let path = Path::new(env!("CARGO_MANIFEST_DIR")).join("tests/fixtures/oracle-v1.json"); + serde_json::from_str(&std::fs::read_to_string(path).unwrap()).unwrap() +} + +/// The case's values as JSON, typed by its dtype: numeric values are strings +/// in the fixture so no JSON tool can round them before profiling. +fn values(case: &Value) -> Vec { + let raw = case["values"].as_array().unwrap(); + if case["kind"] != "numeric" { + return raw.clone(); + } + raw.iter() + .map(|value| { + let text = value.as_str().unwrap(); + // serde_json keeps an integer exact up to u64; a float parses as f64. + serde_json::from_str::(text).unwrap_or_else(|_| json!(text)) + }) + .collect() +} + +fn csv_text(values: &[Value]) -> String { + let mut text = String::from("\"v\"\n"); + for value in values { + let cell = match value { + Value::Null => String::new(), + Value::String(text) => text.replace('"', "\"\""), + // Python's csv writer prints a float with repr; the fixture's + // float strings are already that form. + other => other.to_string(), + }; + text.push_str(&format!("\"{cell}\"\n")); + } + text +} + +fn profile(case: &Value, route: &str, dir: &Path) -> Option { + let values = values(case); + let records: Vec = values.iter().map(|value| json!({"v": value})).collect(); + let id = case["id"].as_str().unwrap(); + let wanted = case["routes"].as_array(); + let allowed = |name: &str| wanted.is_none_or(|routes| routes.iter().any(|r| r == name)); + let path = match route { + // A container in a CSV cell is text, not the same logical value. + "csv_incremental" | "csv_columnar" if case["kind"] == "nested" => return None, + "csv_incremental" | "csv_columnar" => { + let path = dir.join(format!("{id}.csv")); + std::fs::write(&path, csv_text(&values)).unwrap(); + path + } + "json" if allowed("json") => { + let path = dir.join(format!("{id}.json")); + std::fs::write(&path, serde_json::to_string(&records).unwrap()).unwrap(); + path + } + "jsonl" if allowed("jsonl") => { + let path = dir.join(format!("{id}.jsonl")); + let lines: String = records.iter().map(|row| format!("{row}\n")).collect(); + std::fs::write(&path, lines).unwrap(); + path + } + _ => return None, + }; + let profiler = match route { + "csv_incremental" => Profiler::new().engine(EngineType::Incremental), + "csv_columnar" => Profiler::new().engine(EngineType::Columnar), + _ => Profiler::new(), + }; + Some( + profiler + .analyze_file(&path) + .unwrap_or_else(|e| panic!("{id} via {route}: {e}")), + ) +} + +/// A fraction such as `"5/3"`, or a decimal, as the nearest f64. +fn number(text: &str) -> f64 { + match text.split_once('/') { + Some((num, den)) => num.parse::().unwrap() / den.parse::().unwrap(), + None => text.parse().unwrap(), + } +} + +/// An exact rational over i128, for integer cases past 2^53: an integer, a +/// fraction such as `"36893488147419103227/2"`, or a decimal such as +/// `"18446744073709551612.7500"`. +fn rational(text: &str) -> (i128, i128) { + if let Some((num, den)) = text.split_once('/') { + return (num.parse().unwrap(), den.parse().unwrap()); + } + match text.split_once('.') { + Some((whole, fraction)) => ( + format!("{whole}{fraction}").parse().unwrap(), + 10_i128.pow(fraction.len() as u32), + ), + None => (text.parse().unwrap(), 1), + } +} + +/// A reported number as an exact integer when it is one. A JSON integer is +/// read as an integer: `as_f64` would round `9007199254740993` to a +/// neighbour, and a fixed #877 would then still read as wrong. +fn exact_integer(value: &Value) -> Option { + value + .as_i64() + .map(i128::from) + .or_else(|| value.as_u64().map(i128::from)) + .or_else(|| { + value + .as_f64() + .filter(|f| f.fract() == 0.0 && f.abs() < 1e38) + .map(|f| f as i128) + }) +} + +/// Whether `actual` is a faithful binary64 rendering of the exact `wanted`. +/// +/// Past 2^53 adjacent floats are hundreds apart, so neither a 4dp nor a +/// relative tolerance says anything there: the bound is half an ulp of the +/// exact value, the error rounding to the nearest float may carry. +fn faithful(actual: &Value, wanted: &str) -> bool { + let (num, den) = rational(wanted); + let target = num as f64 / den as f64; + match exact_integer(actual) { + Some(a) => (a * den - num).abs() * 2 <= (ulp(target) as i128) * den, + None => actual + .as_f64() + .is_some_and(|a| (a - target).abs() <= ulp(target)), + } +} + +/// Collects the fields of one (case, route) run that disagree with the oracle. +struct Checker<'a> { + case: &'a str, + route: &'static str, + found: Vec, +} + +impl Checker<'_> { + fn record(&mut self, field: &str, wanted: impl ToString, actual: &Value) { + self.found.push(Mismatch { + case: self.case.to_string(), + route: self.route, + field: field.to_string(), + expected: wanted.to_string(), + actual: actual.to_string(), + }); + } + + /// Compare at the serialized precision; `relative` for magnitudes where + /// 4dp carries no information. + fn compare(&mut self, field: &str, actual: &Value, wanted: &Value, relative: bool) { + let good = match wanted { + Value::Null => actual.is_null(), + Value::String(text) if field != "data_type" => actual.as_f64().is_some_and(|a| { + let target = number(text); + if relative { + (a - target).abs() <= REL_TOLERANCE * target.abs() + } else { + (a - target).abs() <= ABS_TOLERANCE + } + }), + other => actual == other, + }; + if !good { + self.record(field, wanted, actual); + } + } + + /// Compare against an exact value within half an ulp; for values past + /// 2^53, where 4dp and relative tolerances say nothing. + fn compare_faithful(&mut self, field: &str, actual: &Value, wanted: &Value) { + let good = match wanted { + Value::Null => actual.is_null(), + Value::String(text) => faithful(actual, text), + other => actual == other, + }; + if !good { + self.record(field, wanted, actual); + } + } +} + +/// The gap from `value` to the next f64 away from zero. +fn ulp(value: f64) -> f64 { + let magnitude = value.abs(); + f64::from_bits(magnitude.to_bits() + 1) - magnitude +} + +fn check(case: &Value, route: &'static str, report: &ProfileReport) -> Vec { + let summary: Value = serde_json::from_str(&report.summary_json().unwrap()).unwrap(); + let column = &summary["columns"][0]; + let expected = if case["expected"].is_object() { + &case["expected"] + } else { + &case["expected_common_markers"] + }; + let id = case["id"].as_str().unwrap(); + let mut checker = Checker { + case: id, + route, + found: Vec::new(), + }; + + for field in [ + "total_count", + "null_count", + "unique_count", + "data_type", + "stats", + "null_percentage", + ] { + if let Some(wanted) = expected.get(field) { + checker.compare(field, &column[field], wanted, false); + } + } + if let Some(wanted) = expected.get("duplicate_rows_single_column") { + checker.compare( + "duplicate_rows", + &summary["quality"]["uniqueness"]["duplicate_rows"], + wanted, + false, + ); + } + if case["kind"] != "numeric" { + return checker.found; + } + + let stats = &column["stats"]; + let wide = case["dtype"] != "float64" + && case["values"] + .as_array() + .unwrap() + .iter() + .any(|v| rational(v.as_str().unwrap()).0.unsigned_abs() > 1 << 53); + let extreme = id == "variance_fits_m2_does_not"; + let variance = if extreme { + &expected["sample_variance_binary64"] + } else { + &expected["variance_4dp"] + }; + checker.compare("variance", &stats["variance"], variance, extreme); + checker.compare( + "std_dev", + &stats["std_dev"], + &expected["std_dev_4dp"], + extreme, + ); + + if wide { + // Past 2^53 every f64 is an integer, and an exact mean can fall between + // two of them; either neighbour is faithful, so the bound is half an ulp. + checker.compare_faithful("mean", &stats["mean"], &expected["mean_exact"]); + // An exact integer extreme needs an integer-preserving value; + // comparing after a float conversion would erase the defect (#877). + for name in ["min", "max"] { + let wanted = expected[format!("{name}_exact")].as_str().unwrap(); + let actual = &stats[name]; + if exact_integer(actual) != Some(rational(wanted).0) { + checker.record(name, wanted, actual); + } + } + } else { + checker.compare("mean", &stats["mean"], &expected["mean_4dp"], extreme); + for name in ["min", "max"] { + let exact = number(expected[format!("{name}_exact")].as_str().unwrap()); + checker.compare(name, &stats[name], &json!(exact.to_string()), extreme); + } + } + if extreme { + return checker.found; + } + checker.compare( + "skewness", + &stats["skewness"], + &expected["adjusted_sample_skewness_4dp"], + false, + ); + checker.compare( + "kurtosis", + &stats["kurtosis"], + &expected["adjusted_sample_excess_kurtosis_4dp"], + false, + ); + let mode = stats.get("mode").cloned().unwrap_or(Value::Null); + if wide { + checker.compare_faithful("median", &stats["median"], &expected["median_exact"]); + checker.compare_faithful("mode", &mode, &expected["mode_exact"]); + } else { + checker.compare("median", &stats["median"], &expected["median_exact"], false); + checker.compare("mode", &mode, &expected["mode_exact"], false); + } + checker.compare( + "outlier_count", + &stats["outlier_count"], + &expected["outlier_count"], + false, + ); + match expected["quartiles_4dp"].as_object() { + None => checker.compare( + "quartiles", + stats.get("quartiles").unwrap_or(&Value::Null), + &Value::Null, + false, + ), + Some(quartiles) => { + for (name, wanted) in quartiles { + let field = format!("quartiles.{name}"); + let actual = &stats["quartiles"][name]; + if wide { + checker.compare_faithful(&field, actual, wanted); + } else { + checker.compare(&field, actual, wanted, false); + } + } + } + } + checker.found +} + +fn owner(m: &Mismatch) -> Option<&'static str> { + KNOWN + .iter() + .find(|(case, route, field, _)| { + *case == m.case && (*route == "*" || *route == m.route) && *field == m.field + }) + .map(|(.., issue)| *issue) +} + +#[test] +fn every_file_engine_matches_the_independent_answer() { + let dir = tempfile::tempdir().unwrap(); + let mut runs = Vec::new(); + let mut mismatches = Vec::new(); + for case in fixture()["cases"].as_array().unwrap() { + for route in ROUTES { + if let Some(report) = profile(case, route, dir.path()) { + runs.push((case["id"].as_str().unwrap().to_string(), route)); + mismatches.extend(check(case, route, &report)); + } + } + } + + let unexplained: Vec = mismatches + .iter() + .filter(|m| owner(m).is_none()) + .map(|m| { + format!( + " {} via {}: {} expected {}, got {}", + m.case, m.route, m.field, m.expected, m.actual + ) + }) + .collect(); + // A "*" entry claims every route, so each route it covers must still + // disagree: a fix on one route narrows the entry instead of hiding there. + let stale: Vec = KNOWN + .iter() + .flat_map(|(case, route, field, issue)| { + runs.iter() + .filter(move |(c, r)| c == case && (*route == "*" || route == r)) + .filter(|(c, r)| { + !mismatches + .iter() + .any(|m| m.case == *c && m.route == *r && m.field == *field) + }) + .map(move |(_, r)| format!(" {case} via {r}: {field} ({issue})")) + }) + .collect(); + + assert!( + unexplained.is_empty(), + "disagreements with the oracle:\n{}", + unexplained.join("\n") + ); + assert!( + stale.is_empty(), + "known disagreements that no longer occur:\n{}", + stale.join("\n") + ); + // Every case ran through at least one route. + assert_eq!( + runs.iter() + .map(|(case, _)| case.as_str()) + .collect::>() + .len(), + 20 + ); +} + +/// The comparison helpers for values past 2^53, on inputs the current data +/// cannot reach: an exact JSON integer (what a fixed #877 would report), and a +/// median a million away from the truth. +#[test] +fn the_wide_value_helpers_are_exact() { + let exact = json!(9_007_199_254_740_993_u64); + assert_eq!(exact_integer(&exact), Some(9_007_199_254_740_993)); + assert!(faithful(&exact, "9007199254740993")); + // Its nearest float is a faithful rendering too; a neighbour two ulps off is not. + assert!(faithful( + &json!(9_007_199_254_740_992.0), + "9007199254740993" + )); + assert!(!faithful( + &json!(9_007_199_254_740_996.0), + "9007199254740993" + )); + + let median = "1759700000123456794"; + assert!(faithful(&json!(1.759_700_000_123_456_8e18), median)); + assert!(!faithful(&json!(1.759_700_000_123_456_8e18 + 1e6), median)); + + assert_eq!( + rational("18446744073709551612.7500"), + (184_467_440_737_095_516_127_500, 10_000) + ); + assert_eq!(rational("-5/3"), (-5, 3)); +} From 4232d9211afa4ab7ce2b0781258065cabaac92b9 Mon Sep 17 00:00:00 2001 From: Andrea Bozzo Date: Sat, 17 Oct 2026 15:42:23 +0200 Subject: [PATCH 09/16] fix: report no statistics for a column without a non-null value (#983) (#985) An all-null text column reported text lengths of 0, the same as a column of real values, where an all-null numeric column reported none. One rule, stats_over_values, now gives every column without a value no statistics, on the profile builder and on column analysis. --- .../dataprof-metrics/src/analysis/column.rs | 39 ++++++ .../dataprof-runtime/src/profile_builder.rs | 27 ++--- docs/python/README.md | 5 +- python/tests/test_all_null_columns.py | 64 ++++++++++ python/tests/test_oracle.py | 2 - tests/all_null_columns.rs | 112 ++++++++++++++++++ tests/cross_engine_consistency.rs | 15 ++- tests/oracle.rs | 2 - 8 files changed, 241 insertions(+), 25 deletions(-) create mode 100644 python/tests/test_all_null_columns.py create mode 100644 tests/all_null_columns.rs diff --git a/crates/dataprof-metrics/src/analysis/column.rs b/crates/dataprof-metrics/src/analysis/column.rs index 52255125..ef3a22fe 100644 --- a/crates/dataprof-metrics/src/analysis/column.rs +++ b/crates/dataprof-metrics/src/analysis/column.rs @@ -26,6 +26,22 @@ struct ColumnAnalysis { skip_unique_count: bool, } +/// The statistics a column carries: none when it holds no non-null value. +/// +/// Every statistic is an aggregate over values, and over no values it is +/// undefined, which the repo reports as absent, not zero. Each type's arm can +/// still produce one: a text column of nulls only reported lengths of 0, the +/// same as a column of real zero-length values (#983). Shared by this path and +/// `profile_builder::build_column_profile`, so every engine draws the line in +/// one place; a column with no rows is the case where both counts are 0. +pub fn stats_over_values(stats: ColumnStats, total_count: usize, null_count: usize) -> ColumnStats { + if null_count >= total_count { + ColumnStats::None + } else { + stats + } +} + /// Analyze a column with full profiling (includes pattern detection and unique counts) pub fn analyze_column(name: &str, data: &[String]) -> ColumnProfile { analyze_column_with_options( @@ -176,6 +192,7 @@ fn analyze_column_with_options( DataType::Nested => unreachable!("infer_type never answers Nested"), } }; + let stats = stats_over_values(stats, total_count, null_count); let patterns = if analysis.skip_patterns { None @@ -221,6 +238,28 @@ fn analyze_column_with_options( mod tests { use super::*; + /// The database connectors profile through this path. A column holding + /// only nulls and null tokens has no values, so no statistics, where it + /// used to report text lengths of 0 (#983); one value restores them. + #[test] + fn a_column_without_a_value_has_no_statistics() { + let nulls: Vec = ["", "NA", "null", " "].map(String::from).to_vec(); + let profile = analyze_column("empty", &nulls); + assert_eq!((profile.null_count, profile.total_count), (4, 4)); + assert!( + matches!(profile.stats, ColumnStats::None), + "{:?}", + profile.stats + ); + + let mut one = nulls.clone(); + one.push("ab".to_string()); + match analyze_column("one", &one).stats { + ColumnStats::Text(text) => assert_eq!((text.min_length, text.max_length), (2, 2)), + other => panic!("expected text statistics, got {other:?}"), + } + } + #[test] fn non_finite_numeric_column_has_no_statistics() { let data = ["Infinity", "-inf"].map(String::from); diff --git a/crates/dataprof-runtime/src/profile_builder.rs b/crates/dataprof-runtime/src/profile_builder.rs index e10f77dd..ec69ac67 100644 --- a/crates/dataprof-runtime/src/profile_builder.rs +++ b/crates/dataprof-runtime/src/profile_builder.rs @@ -12,6 +12,7 @@ use dataprof_core::{ TextStats, }; use dataprof_metrics::{ + analysis::column::stats_over_values, analysis::inference::{ classify_lexical_forms, count_locale_numbers, holds_digit_codes, is_inferred_date_token, is_integer_token, is_null_like_token, parse_strict_boolean_token, @@ -215,26 +216,18 @@ pub fn build_column_profile(input: ColumnProfileInput<'_>) -> ColumnProfile { } }; - // A column with no rows has counts, but no statistics: every aggregate over - // zero values is undefined, and the repo's convention for undefined is - // absent, not zero. Text and date columns already landed on - // `ColumnStats::None` here through their empty-sample arms; numeric and - // boolean did not, and reported a min, max, mean, std_dev and variance of - // 0.0, and a true_ratio of 0.0 -- plausible numbers describing rows that do - // not exist, which is the failure class AGENTS.md singles out. 0.0 is an - // ordinary value for a real numeric column, so nothing about that output - // looked wrong. - // - // Numeric and boolean columns also suppress statistics above when rows - // exist but no values parsed. This guard covers zero rows for every type. + // A column with no non-null value has counts, but no statistics: every + // aggregate over zero values is undefined, and the repo's convention for + // undefined is absent, not zero. Without this guard, a column with no rows + // reported a numeric min, max, mean, std_dev and variance of 0.0 and a + // true_ratio of 0.0, and a text column of nulls reported lengths of 0 + // (#983): plausible numbers describing values that do not exist, which is + // the failure class AGENTS.md singles out. 0.0 and 0 are ordinary values + // for a real column, so nothing about that output looked wrong. // // The counts above are untouched. "0 of 0 values were invalid" is a fact // about a column that was analyzed, not a statistic over nothing. - let stats = if input.total_count == 0 { - ColumnStats::None - } else { - stats - }; + let stats = stats_over_values(stats, input.total_count, input.null_count); let patterns = if input.skip_patterns { None diff --git a/docs/python/README.md b/docs/python/README.md index 0516ec88..60f90ea6 100644 --- a/docs/python/README.md +++ b/docs/python/README.md @@ -473,7 +473,10 @@ boolean columns with no parsed booleans have no statistics block. Their statisti accessors return `None`, including boolean counts and `true_ratio`. The declared column type, row/null counts, and numeric `invalid_count` remain available. A measured numeric zero or an all-false column still reports `0.0`. CSV columns -containing only nulls infer as text because CSV carries no declared type. +containing only nulls infer as text because CSV carries no declared type. A +column with no non-null value has no statistics block whatever its type, so an +all-null text column's `min_length`, `max_length` and `avg_length` are `None`, +not `0`. This uses the existing absent-statistics representation (`ColumnStats::None` in Rust); numeric fields do not become optional within `NumericStats`, and the report diff --git a/python/tests/test_all_null_columns.py b/python/tests/test_all_null_columns.py new file mode 100644 index 00000000..864342ca --- /dev/null +++ b/python/tests/test_all_null_columns.py @@ -0,0 +1,64 @@ +"""A column with no non-null value carries no statistics (#983). + +An all-null text column used to report lengths of 0, the same as a column of +real zero-length values, while an all-null numeric column already reported no +statistics. `tests/all_null_columns.rs` is the Rust twin over the file engines; +this covers the in-memory routes. The oracle (`test_oracle.py`, `all_null`) +checks the same case on every route against an independent answer. +""" + +from __future__ import annotations + +import dataprof as dp +import pytest + +pd = pytest.importorskip("pandas") +pa = pytest.importorskip("pyarrow") +pl = pytest.importorskip("polars") + +EMPTY = [None, None, None] +BLANK = ["NA", "null", ""] +NOTE = [None, "x", None] + + +def _routes(): + frame = {"id": [1, 2, 3], "empty": EMPTY, "blank": BLANK, "note": NOTE} + records = [dict(zip(frame, row)) for row in zip(*frame.values())] + table = pa.table( + { + "id": [1, 2, 3], + "empty": pa.array(EMPTY, pa.string()), + "blank": BLANK, + "note": NOTE, + } + ) + return { + "dict": lambda: dp.profile(frame), + "records": lambda: dp.profile(records), + "pandas": lambda: dp.profile(pd.DataFrame(frame)), + "arrow": lambda: dp.profile(table), + "polars": lambda: dp.profile(pl.from_arrow(table)), + } + + +def test_a_column_of_nulls_reports_no_statistics_and_keeps_its_counts(): + wrong = [] + for route, run in _routes().items(): + report = run() + for name in ("empty", "blank"): + column = report[name] + exported = report.to_dict()["columns"][list(report.column_profiles).index(name)] + if exported.get("stats") is not None or column.min_length is not None: + wrong.append(f"{route}: {name} stats {exported.get('stats')}") + if (column.null_count, column.total_count) != (3, 3): + wrong.append(f"{route}: {name} counts {column.null_count}/{column.total_count}") + note = report["note"] + if (note.min_length, note.max_length) != (1, 1): + wrong.append(f"{route}: note lengths {note.min_length}/{note.max_length}") + assert not wrong, "\n".join(wrong) + + +def test_an_identifier_column_of_nulls_reports_no_statistics(): + report = dp.profile({"id": [1, 2], "code": [None, None]}, identifier_columns=["code"]) + assert report.to_dict()["columns"][1].get("stats") is None + assert report["code"].min_length is None diff --git a/python/tests/test_oracle.py b/python/tests/test_oracle.py index f1062f75..2f274adc 100644 --- a/python/tests/test_oracle.py +++ b/python/tests/test_oracle.py @@ -63,8 +63,6 @@ }, ("epoch_nanoseconds", "*", "quartiles.iqr"): "#877", ("uint64_edge", "*", "quartiles.iqr"): "#877", - # An all-null text column reports lengths of 0 rather than none. - ("all_null", "*", "stats"): "#983", # The pandas mixed-object fallback renders a container as the text a # string can spell, so the two sign one row. ("container_vs_its_text", "pandas", "duplicate_rows"): "#982", diff --git a/tests/all_null_columns.rs b/tests/all_null_columns.rs new file mode 100644 index 00000000..83e16fd1 --- /dev/null +++ b/tests/all_null_columns.rs @@ -0,0 +1,112 @@ +//! A column with no non-null value carries no statistics (#983). +//! +//! Statistics are aggregates over values, and over none they are undefined, +//! which the report states as absent. An all-null text column used to report +//! lengths of 0, the same as a column of real zero-length values, while an +//! all-null numeric column already reported no statistics. +//! +//! `python/tests/test_all_null_columns.py` is the Python twin, over the +//! in-memory routes. + +use std::io::Write; + +use dataprof::{ColumnStats, EngineType, ProfileReport, Profiler}; + +const ENGINES: [EngineType; 3] = [ + EngineType::Auto, + EngineType::Incremental, + EngineType::Columnar, +]; + +fn write(suffix: &str, contents: &str) -> tempfile::NamedTempFile { + let mut file = tempfile::Builder::new().suffix(suffix).tempfile().unwrap(); + write!(file, "{contents}").unwrap(); + file.flush().unwrap(); + file +} + +/// Every route this file can reach, by label. +fn reports( + csv: &str, + jsonl: &str, + profiler: impl Fn() -> Profiler, +) -> Vec<(String, ProfileReport)> { + let csv_file = write(".csv", csv); + let jsonl_file = write(".jsonl", jsonl); + let mut out: Vec<(String, ProfileReport)> = ENGINES + .iter() + .map(|engine| { + ( + format!("csv {engine:?}"), + profiler() + .engine(*engine) + .analyze_file(csv_file.path()) + .unwrap(), + ) + }) + .collect(); + out.push(( + "jsonl".to_string(), + profiler().analyze_file(jsonl_file.path()).unwrap(), + )); + out +} + +fn column<'r>(report: &'r ProfileReport, name: &str) -> &'r dataprof::ColumnProfile { + report + .column_profiles + .iter() + .find(|column| column.name == name) + .unwrap() +} + +#[test] +fn a_column_of_nulls_reports_no_statistics_and_keeps_its_counts() { + // `blank` holds only null tokens; `note` holds one real value. + let csv = "id,empty,blank,note\n1,,NA,\n2,,null,x\n3,,,\n"; + let jsonl = concat!( + "{\"id\":1,\"empty\":null,\"blank\":\"NA\",\"note\":null}\n", + "{\"id\":2,\"empty\":null,\"blank\":\"null\",\"note\":\"x\"}\n", + "{\"id\":3,\"empty\":null,\"blank\":\"\",\"note\":null}\n", + ); + let mut wrong = Vec::new(); + for (route, report) in reports(csv, jsonl, Profiler::new) { + for name in ["empty", "blank"] { + let profile = column(&report, name); + if !matches!(profile.stats, ColumnStats::None) { + wrong.push(format!("{route}: {name} stats {:?}", profile.stats)); + } + if (profile.total_count, profile.null_count) != (3, 3) { + wrong.push(format!( + "{route}: {name} counts {}/{}", + profile.null_count, profile.total_count + )); + } + } + // One value is enough for lengths. + match &column(&report, "note").stats { + ColumnStats::Text(text) if (text.min_length, text.max_length) == (1, 1) => {} + other => wrong.push(format!("{route}: note stats {other:?}")), + } + } + assert!(wrong.is_empty(), "{}", wrong.join("\n")); +} + +#[test] +fn an_identifier_column_of_nulls_reports_no_statistics() { + let csv = "id,code\n1,\n2,\n"; + let jsonl = "{\"id\":1,\"code\":null}\n{\"id\":2,\"code\":null}\n"; + let mut wrong = Vec::new(); + for (route, report) in reports(csv, jsonl, || { + Profiler::new().identifier_columns(vec!["code".to_string()]) + }) { + let profile = column(&report, "code"); + if !matches!(profile.stats, ColumnStats::None) { + wrong.push(format!( + "{route}: {:?} {:?}", + profile.data_type, profile.stats + )); + } + } + assert!(wrong.is_empty(), "{}", wrong.join("\n")); +} diff --git a/tests/cross_engine_consistency.rs b/tests/cross_engine_consistency.rs index 9b4b0969..badbe3bc 100644 --- a/tests/cross_engine_consistency.rs +++ b/tests/cross_engine_consistency.rs @@ -34,9 +34,18 @@ fn serialized_text_stats_match_column_analysis_and_csv_engines() { let standard = analyze_csv_file(csv.path(), &CsvParserConfig::default()).unwrap(); let expected = serde_json::to_value(&standard.column_profiles[0].stats).unwrap(); - assert!(expected["Text"].is_object(), "{values:?}"); - assert!(expected["Text"].get("most_frequent").is_none()); - assert!(expected["Text"].get("least_frequent").is_none()); + // A column of nulls only has no values, so no statistics (#983); the + // others are text, with frequencies left unassessed. + if values + .iter() + .all(|value| dataprof::is_null_like_token(value.trim())) + { + assert_eq!(expected, json!("None"), "{values:?}"); + } else { + assert!(expected["Text"].is_object(), "{values:?}"); + assert!(expected["Text"].get("most_frequent").is_none()); + assert!(expected["Text"].get("least_frequent").is_none()); + } for column in [ dataprof::analyze_column("text", &data), diff --git a/tests/oracle.rs b/tests/oracle.rs index 92762689..edfcedca 100644 --- a/tests/oracle.rs +++ b/tests/oracle.rs @@ -42,8 +42,6 @@ const KNOWN: &[(&str, &str, &str, &str)] = &[ ("uint64_edge", "*", "max", "#877"), ("uint64_edge", "*", "mode", "#877"), ("uint64_edge", "*", "quartiles.iqr", "#877"), - // An all-null text column reports lengths of 0 rather than none. - ("all_null", "*", "stats", "#983"), ]; #[derive(Debug, Clone, PartialEq)] From d9f30bbcba3397d8a7f08820833da0bac41de3a7 Mon Sep 17 00:00:00 2001 From: 1932819331hy-debug <1932819331hy@gmail.com> Date: Mon, 19 Oct 2026 00:52:31 +0900 Subject: [PATCH 10/16] fix(sqlite): retain SQLite URI options and access modes (#912) --- crates/dataprof-db/src/connectors/sqlite.rs | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/crates/dataprof-db/src/connectors/sqlite.rs b/crates/dataprof-db/src/connectors/sqlite.rs index 6741a3b3..8c847562 100644 --- a/crates/dataprof-db/src/connectors/sqlite.rs +++ b/crates/dataprof-db/src/connectors/sqlite.rs @@ -69,11 +69,20 @@ impl DatabaseConnector for SqliteConnector { async fn connect(&mut self) -> Result<(), DataProfilerError> { #[cfg(feature = "sqlite")] { - let db_path = self.get_db_path()?; - let connection_string = if db_path == ":memory:" { - "sqlite::memory:".to_string() + // sqlx parses SQLite-specific URL options (mode, cache, immutable, + // vfs). Passing the original sqlite: URI through unchanged also + // preserves its absolute/relative path semantics (#912 / #839). + // Previously rebuilding the URL from get_db_path() silently + // dropped every query parameter, including mode=ro and mode=rwc. + let connection_string = if self.config.connection_string.starts_with("sqlite:") { + self.config.connection_string.clone() } else { - format!("sqlite://{}", db_path) + let db_path = self.get_db_path()?; + if db_path == ":memory:" { + "sqlite::memory:".to_string() + } else { + format!("sqlite://{}", db_path) + } }; let pool = SqlitePoolOptions::new() From 76e6ca99be3afad0d18aa965ec188370d58f54d2 Mon Sep 17 00:00:00 2001 From: 1932819331hy-debug <1932819331hy@gmail.com> Date: Mon, 19 Oct 2026 00:53:11 +0900 Subject: [PATCH 11/16] test(sqlite): prove mode=rwc/rw/ro survive URL parsing (#912) --- .../dataprof-db/tests/sqlite_url_options.rs | 81 +++++++++++++++++++ 1 file changed, 81 insertions(+) create mode 100644 crates/dataprof-db/tests/sqlite_url_options.rs diff --git a/crates/dataprof-db/tests/sqlite_url_options.rs b/crates/dataprof-db/tests/sqlite_url_options.rs new file mode 100644 index 00000000..6cf8e18d --- /dev/null +++ b/crates/dataprof-db/tests/sqlite_url_options.rs @@ -0,0 +1,81 @@ +//! #912: SQLite URI query parameters must not be stripped before sqlx sees +//! them. These are behavioral tests: they exercise actual read-only and +//! create-if-missing modes, not only string reconstruction. +#![cfg(feature = "sqlite")] + +use dataprof_db::{DatabaseConfig, create_connector}; +use sqlx::sqlite::SqlitePoolOptions; + +fn config(url: String) -> DatabaseConfig { + DatabaseConfig { + connection_string: url, + load_credentials_from_env: false, + ..Default::default() + } +} + +fn sqlite_url(path: &std::path::Path, mode: &str) -> String { + // sqlx accepts sqlite:///abs/path on Unix and sqlite:///C:/path on + // Windows. A slash is preserved rather than trimming URL path segments. + let path = path.to_string_lossy().replace('\\', "/"); + let prefix = if cfg!(windows) { "sqlite:///" } else { "sqlite://" }; + format!("{prefix}{path}?mode={mode}") +} + +#[tokio::test] +async fn mode_rwc_creates_a_missing_database() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("created-by-mode-rwc.db"); + assert!(!path.exists()); + + let mut connector = create_connector(config(sqlite_url(&path, "rwc"))).unwrap(); + connector.connect().await.unwrap(); + assert!(connector.test_connection().await.unwrap()); + assert!(path.exists(), "SQLite must honor mode=rwc, not drop it"); + connector.disconnect().await.unwrap(); +} + +#[tokio::test] +async fn mode_rw_does_not_create_a_missing_database() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("must-not-be-created.db"); + assert!(!path.exists()); + + let mut connector = create_connector(config(sqlite_url(&path, "rw"))).unwrap(); + assert!( + connector.connect().await.is_err(), + "mode=rw must reject a missing DB rather than silently create it" + ); + assert!(!path.exists()); +} + +#[tokio::test] +async fn mode_ro_refuses_writes_to_existing_database() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("read-only.db"); + + let pool = SqlitePoolOptions::new() + .max_connections(1) + .connect(&sqlite_url(&path, "rwc")) + .await + .unwrap(); + sqlx::query("CREATE TABLE rows (id INTEGER)") + .execute(&pool) + .await + .unwrap(); + pool.close().await; + + let mut connector = create_connector(config(sqlite_url(&path, "ro"))).unwrap(); + connector.connect().await.unwrap(); + assert!(connector.test_connection().await.unwrap()); + let error = connector + .profile_query("INSERT INTO rows (id) VALUES (1)") + .await + .expect_err("mode=ro must reject writes to the database"); + assert!( + error.to_string().to_ascii_lowercase().contains("readonly") + || error.to_string().to_ascii_lowercase().contains("read-only"), + "unexpected SQLite error: {error}" + ); + connector.disconnect().await.unwrap(); +} From 8c791b959a4564502e88d793d460ac60dd35870f Mon Sep 17 00:00:00 2001 From: Hysann <1932819331hy@gmail.com> Date: Mon, 19 Oct 2026 09:52:40 +0900 Subject: [PATCH 12/16] style(sqlite): apply rustfmt to URI-mode test helper --- crates/dataprof-db/tests/sqlite_url_options.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/crates/dataprof-db/tests/sqlite_url_options.rs b/crates/dataprof-db/tests/sqlite_url_options.rs index 6cf8e18d..a5436d74 100644 --- a/crates/dataprof-db/tests/sqlite_url_options.rs +++ b/crates/dataprof-db/tests/sqlite_url_options.rs @@ -18,7 +18,11 @@ fn sqlite_url(path: &std::path::Path, mode: &str) -> String { // sqlx accepts sqlite:///abs/path on Unix and sqlite:///C:/path on // Windows. A slash is preserved rather than trimming URL path segments. let path = path.to_string_lossy().replace('\\', "/"); - let prefix = if cfg!(windows) { "sqlite:///" } else { "sqlite://" }; + let prefix = if cfg!(windows) { + "sqlite:///" + } else { + "sqlite://" + }; format!("{prefix}{path}?mode={mode}") } From 3a3475559029aa72d97b56d39b2a229bd5a0624e Mon Sep 17 00:00:00 2001 From: Hysann <1932819331hy@gmail.com> Date: Mon, 19 Oct 2026 11:59:51 +0900 Subject: [PATCH 13/16] fix(sqlite): exclude foreign environment credentials from explicit SQLite URIs --- crates/dataprof-db/src/lib.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/crates/dataprof-db/src/lib.rs b/crates/dataprof-db/src/lib.rs index 70985a52..a315f651 100644 --- a/crates/dataprof-db/src/lib.rs +++ b/crates/dataprof-db/src/lib.rs @@ -144,7 +144,11 @@ fn apply_environment_configuration( .apply_to_connection_string(config.connection_string.clone(), database_type); } - if config.load_credentials_from_env { + if config.load_credentials_from_env && database_type != "sqlite" { + // An explicit SQLite URI is a local file target, not a consumer of + // generic DATABASE_URL / DB_* credentials from other engines. + // Rebuilding it with foreign settings may replace its path or + // introduce unsupported user-info/host components (#912). let credentials = DatabaseCredentials::from_environment(database_type); config.connection_string = credentials.apply_to_connection_string(&config.connection_string); From b39fbace9ec0e5c1a50a1ec58b6e6badb4df64b3 Mon Sep 17 00:00:00 2001 From: Hysann <1932819331hy@gmail.com> Date: Mon, 19 Oct 2026 12:00:12 +0900 Subject: [PATCH 14/16] test(sqlite): reproduce DATABASE_URL contamination under default config in isolated process --- .../dataprof-db/tests/sqlite_url_options.rs | 45 +++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/crates/dataprof-db/tests/sqlite_url_options.rs b/crates/dataprof-db/tests/sqlite_url_options.rs index a5436d74..bf718388 100644 --- a/crates/dataprof-db/tests/sqlite_url_options.rs +++ b/crates/dataprof-db/tests/sqlite_url_options.rs @@ -83,3 +83,48 @@ async fn mode_ro_refuses_writes_to_existing_database() { ); connector.disconnect().await.unwrap(); } + +/// Run in a separate test process so hostile DATABASE_URL values never leak +/// into other Rust tests (the process environment is global). +#[tokio::test] +async fn sqlite_explicit_url_default_env_child() { + if std::env::var_os("DATAPROF_SQLITE_ENV_CHILD").is_none() { + return; + } + + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("explicit-default-rwc.db"); + let url = sqlite_url(&path, "rwc"); + // Unlike the other tests, use the public default setting: + // load_credentials_from_env = true. A PostgreSQL DATABASE_URL must not + // replace the SQLite path or its explicitly supplied mode. + let mut connector = create_connector(DatabaseConfig { + connection_string: url, + ..Default::default() + }) + .unwrap(); + connector.connect().await.unwrap(); + assert!(path.exists(), "the requested SQLite path must be created"); + assert!(connector.test_connection().await.unwrap()); + connector.disconnect().await.unwrap(); +} + +#[test] +fn explicit_sqlite_mode_survives_unrelated_database_url() { + let executable = std::env::current_exe().expect("locate Rust test executable"); + let output = std::process::Command::new(executable) + .args(["--exact", "sqlite_explicit_url_default_env_child", "--nocapture"]) + .env("DATAPROF_SQLITE_ENV_CHILD", "1") + .env( + "DATABASE_URL", + "postgresql://unrelated:unrelated@127.0.0.1:1/unrelated_database", + ) + .output() + .expect("execute isolated default-configuration regression"); + assert!( + output.status.success(), + "an unrelated DATABASE_URL must not hijack an explicit SQLite target:\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); +} From 2169383916812507e8bb71b19583c7247d646844 Mon Sep 17 00:00:00 2001 From: Hysann <1932819331hy@gmail.com> Date: Mon, 19 Oct 2026 12:02:37 +0900 Subject: [PATCH 15/16] test(sqlite): ensure isolated child test actually executes --- crates/dataprof-db/tests/sqlite_url_options.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/dataprof-db/tests/sqlite_url_options.rs b/crates/dataprof-db/tests/sqlite_url_options.rs index bf718388..ed195ae1 100644 --- a/crates/dataprof-db/tests/sqlite_url_options.rs +++ b/crates/dataprof-db/tests/sqlite_url_options.rs @@ -122,7 +122,7 @@ fn explicit_sqlite_mode_survives_unrelated_database_url() { .output() .expect("execute isolated default-configuration regression"); assert!( - output.status.success(), + output.status.success() && String::from_utf8_lossy(&output.stdout).contains("1 passed"), "an unrelated DATABASE_URL must not hijack an explicit SQLite target:\nstdout:\n{}\nstderr:\n{}", String::from_utf8_lossy(&output.stdout), String::from_utf8_lossy(&output.stderr) From 4dee70674bb1e10b645db6ff6e4324885ae0ae9b Mon Sep 17 00:00:00 2001 From: Hysann <1932819331hy@gmail.com> Date: Mon, 19 Oct 2026 12:45:28 +0900 Subject: [PATCH 16/16] style(sqlite): apply rustfmt to env-isolation regression --- crates/dataprof-db/tests/sqlite_url_options.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/crates/dataprof-db/tests/sqlite_url_options.rs b/crates/dataprof-db/tests/sqlite_url_options.rs index ed195ae1..2121dde3 100644 --- a/crates/dataprof-db/tests/sqlite_url_options.rs +++ b/crates/dataprof-db/tests/sqlite_url_options.rs @@ -113,7 +113,11 @@ async fn sqlite_explicit_url_default_env_child() { fn explicit_sqlite_mode_survives_unrelated_database_url() { let executable = std::env::current_exe().expect("locate Rust test executable"); let output = std::process::Command::new(executable) - .args(["--exact", "sqlite_explicit_url_default_env_child", "--nocapture"]) + .args([ + "--exact", + "sqlite_explicit_url_default_env_child", + "--nocapture", + ]) .env("DATAPROF_SQLITE_ENV_CHILD", "1") .env( "DATABASE_URL",