From f59edb1783909a560b8c24bb15378d86785e44f2 Mon Sep 17 00:00:00 2001 From: PawelKra <15887575+PawelKra@users.noreply.github.com> Date: Fri, 30 Oct 2026 16:16:28 +0100 Subject: [PATCH 1/3] fix(analysis): Join Field matches a text key with an integer key written out in full An all-digit id is often a number in one table and text in the other: the CSV reader types the Census Gazetteer's GEOID 47157000100 as an integer, while the TIGER/Line tract shapefile stores it as text. Join Field refused the pair with "key field type mismatch", so the most common Census join needed an extra Add Field and Calculate Field. A Text key against an Integer key now compares each whole number written out in full with the text, exactly: no trimming or parsing, so leading zeros stay meaningful ("01001" does not match 1001). Other incompatible pairs keep the refusal, and its message now names the way out (Add Field and Calculate Field). validate_key_compatibility keeps its signature. Unit tests cover every key type pair, i64 bounds, look-alike texts and values that disagree with their field's type; tests/join_field.rs runs the tool on tables read by the CSV and Shapefile readers, in both directions, with selections, duplicate strategies and codes with leading zeros. Closes #1178 --- crates/analysis/src/attribute_join.rs | 266 +++++++++++++++++++++++++- crates/analysis/src/tools.rs | 16 +- crates/analysis/tests/join_field.rs | 161 ++++++++++++++++ docs/user-guide/analysis.md | 7 +- 4 files changed, 435 insertions(+), 15 deletions(-) create mode 100644 crates/analysis/tests/join_field.rs diff --git a/crates/analysis/src/attribute_join.rs b/crates/analysis/src/attribute_join.rs index 7b75c7e0b..3f7ba9f79 100644 --- a/crates/analysis/src/attribute_join.rs +++ b/crates/analysis/src/attribute_join.rs @@ -4,7 +4,7 @@ //! Conforms to RustGIS zero-panic architecture: bounded allocations, typed error handling, //! preservation of target ordering, schema preservation, and deterministic collision handling. -use rustgis_data::{Dataset, Field, Value}; +use rustgis_data::{Dataset, Field, FieldType, Value}; use std::collections::HashMap; use std::fmt::Write; @@ -68,6 +68,15 @@ impl JoinKey { } } + /// Extracts a [`JoinKey`] for the given comparison: under [`KeyComparison::NumberAsText`] a + /// whole number is written out in full (`47157000100`, `-5`), so it equals the same digits as text. + pub(crate) fn from_value_compared(v: &Value, comparison: KeyComparison) -> Option { + match (comparison, Self::from_value(v)?) { + (KeyComparison::NumberAsText, JoinKey::Int(i)) => Some(JoinKey::Text(i.to_string())), + (_, key) => Some(key), + } + } + /// User-friendly display representation of the key for error messages. pub fn display(&self) -> String { match self { @@ -79,15 +88,36 @@ impl JoinKey { } } +/// How the values of two compatible key fields are compared. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(crate) enum KeyComparison { + /// Same type, or both numeric: values compared as they are. + Exact, + /// A Text key against an Integer key (an all-digit id typed as a number in one file and as + /// text in the other): the numbers are written out in full and compared with the text exactly, + /// so leading zeros stay meaningful (`"01001"` does not match `1001`). This follows the values, + /// not the side they come from: a whole number on either side (also a whole `Float` that a + /// SQLite-backed Integer column can hold) is compared as its text. + NumberAsText, +} + /// Validates that target key and source key fields have compatible data types. pub fn validate_key_compatibility(target_field: &Field, source_field: &Field) -> Result<(), String> { + key_comparison(target_field, source_field).map(|_| ()) +} + +/// How the values of the target and source key fields are compared, or why they can't be. +pub(crate) fn key_comparison(target_field: &Field, source_field: &Field) -> Result { let t_ty = target_field.ty; let s_ty = source_field.ty; if t_ty == s_ty || (t_ty.is_numeric() && s_ty.is_numeric()) { - Ok(()) + Ok(KeyComparison::Exact) + } else if matches!((t_ty, s_ty), (FieldType::Text, FieldType::Integer) | (FieldType::Integer, FieldType::Text)) { + Ok(KeyComparison::NumberAsText) } else { Err(format!( - "key field type mismatch: target field '{}' is {}, but source field '{}' is {}", + "key field type mismatch: target field '{}' is {}, but source field '{}' is {}; \ + convert one key to the other's type with Add Field and Calculate Field", target_field.name, t_ty.label(), source_field.name, @@ -156,7 +186,7 @@ pub fn join_field(target: &Dataset, source: &Dataset, opts: &JoinFieldOptions<'_ let source_key_field = source.fields.get(source_key_idx).ok_or_else(|| "source key field missing".to_string())?; // 2. Validate key type compatibility - validate_key_compatibility(target_key_field, source_key_field)?; + let comparison = key_comparison(target_key_field, source_key_field)?; // 3. Resolve which source fields to transfer let transfer_indices: Vec = match opts.fields_to_transfer { @@ -214,7 +244,7 @@ pub fn join_field(target: &Dataset, source: &Dataset, opts: &JoinFieldOptions<'_ for (s_idx, s_feat) in source.features.iter().enumerate() { let val = s_feat.attrs.get(source_key_idx).unwrap_or(&Value::Null); - let Some(key) = JoinKey::from_value(val) else { + let Some(key) = JoinKey::from_value_compared(val, comparison) else { // Null or non-finite keys in source cannot match continue; }; @@ -260,7 +290,7 @@ pub fn join_field(target: &Dataset, source: &Dataset, opts: &JoinFieldOptions<'_ new_attrs.reserve(transfer_indices.len()); let t_val = t_feat.attrs.get(target_key_idx).unwrap_or(&Value::Null); - let matching_s_idx = JoinKey::from_value(t_val).and_then(|k| source_index.get(&k).copied()); + let matching_s_idx = JoinKey::from_value_compared(t_val, comparison).and_then(|k| source_index.get(&k).copied()); if let Some(s_idx) = matching_s_idx { matched_records += 1; @@ -538,7 +568,7 @@ mod tests { #[test] fn test_incompatible_key_types_error() { let target = make_table("target", vec![("key", FieldType::Text)], vec![]); - let source = make_table("source", vec![("key", FieldType::Integer)], vec![]); + let source = make_table("source", vec![("key", FieldType::Boolean)], vec![]); let opts = JoinFieldOptions { target_field: "key", join_field: "key", @@ -549,6 +579,228 @@ mod tests { }; let err = join_field(&target, &source, &opts).unwrap_err(); assert!(err.contains("key field type mismatch")); + assert!(err.contains("Add Field and Calculate Field"), "{err}"); + } + + fn key_opts(duplicate_strategy: DuplicateStrategy) -> JoinFieldOptions<'static> { + JoinFieldOptions { + target_field: "GEOID", + join_field: "GEOID", + fields_to_transfer: None, + prefix: "", + duplicate_strategy, + output_name: "out".into(), + } + } + + #[test] + fn test_text_key_matches_integer_key_both_ways() { + // TIGER tracts store GEOID as text; the Census Gazetteer CSV reads it as a number. + let tracts = make_table( + "tracts", + vec![("GEOID", FieldType::Text)], + vec![vec![Value::Text("47157000100".into())], vec![Value::Text("47157000200".into())]], + ); + let gazetteer = make_table( + "gazetteer", + vec![("GEOID", FieldType::Integer), ("ALAND", FieldType::Integer)], + vec![vec![Value::Int(47157000200), Value::Int(2)], vec![Value::Int(47157000100), Value::Int(1)]], + ); + let (out, rep) = join_field(&tracts, &gazetteer, &key_opts(DuplicateStrategy::First)).unwrap(); + assert_eq!(rep.matched_records, 2); + assert_eq!(out.features[0].attrs[1], Value::Int(1)); + assert_eq!(out.features[1].attrs[1], Value::Int(2)); + // The key fields keep their own types. + assert_eq!(out.fields[0].ty, FieldType::Text); + + let (out, rep) = join_field(&gazetteer, &tracts, &key_opts(DuplicateStrategy::First)).unwrap(); + assert_eq!(rep.matched_records, 2); + assert_eq!(out.fields[0].ty, FieldType::Integer); + assert_eq!(out.features[0].attrs[0], Value::Int(47157000200)); + } + + #[test] + fn test_number_as_text_is_exact() { + let target = make_table( + "target", + vec![("GEOID", FieldType::Text)], + vec![ + vec![Value::Text("01001".into())], + vec![Value::Text("1001".into())], + vec![Value::Text("-5".into())], + vec![Value::Text(" 7".into())], + vec![Value::Text("7.0".into())], + vec![Value::Null], + ], + ); + let source = make_table( + "source", + vec![("GEOID", FieldType::Integer), ("tag", FieldType::Text)], + vec![ + vec![Value::Int(1001), Value::Text("county".into())], + vec![Value::Int(-5), Value::Text("negative".into())], + vec![Value::Int(7), Value::Text("seven".into())], + vec![Value::Null, Value::Text("null".into())], + ], + ); + let (out, rep) = join_field(&target, &source, &key_opts(DuplicateStrategy::First)).unwrap(); + let tags: Vec<&Value> = out.features.iter().map(|f| &f.attrs[1]).collect(); + // Leading zeros stay meaningful, text isn't trimmed or parsed, and nulls never match. + assert_eq!( + tags, + [&Value::Null, &Value::Text("county".into()), &Value::Text("negative".into()), &Value::Null, &Value::Null, &Value::Null] + ); + assert_eq!(rep.matched_records, 2); + } + + #[test] + fn test_number_as_text_duplicate_keys() { + let target = make_table("target", vec![("GEOID", FieldType::Text)], vec![vec![Value::Text("42".into())]]); + let source = make_table( + "source", + vec![("GEOID", FieldType::Integer), ("tag", FieldType::Text)], + vec![vec![Value::Int(42), Value::Text("first".into())], vec![Value::Int(42), Value::Text("last".into())]], + ); + let (out, rep) = join_field(&target, &source, &key_opts(DuplicateStrategy::Last)).unwrap(); + assert_eq!(out.features[0].attrs[1], Value::Text("last".into())); + assert_eq!(rep.duplicate_keys, 1); + let err = join_field(&target, &source, &key_opts(DuplicateStrategy::Error)).unwrap_err(); + assert!(err.contains("duplicate key \"42\""), "{err}"); + } + + #[test] + fn test_text_key_against_double_key_is_refused() { + let target = make_table("target", vec![("GEOID", FieldType::Text)], vec![]); + let source = make_table("source", vec![("GEOID", FieldType::Double)], vec![]); + let err = join_field(&target, &source, &key_opts(DuplicateStrategy::First)).unwrap_err(); + assert!(err.contains("key field type mismatch"), "{err}"); + } + + #[test] + fn test_key_type_compatibility_matrix() { + use FieldType::*; + use KeyComparison::*; + let types = [Integer, Double, Text, Boolean]; + for t in types { + for s in types { + let expected = match (t, s) { + _ if t == s => Some(Exact), + (Integer, Double) | (Double, Integer) => Some(Exact), + (Text, Integer) | (Integer, Text) => Some(NumberAsText), + _ => None, + }; + let (a, b) = (Field::new("a", t), Field::new("b", s)); + let got = key_comparison(&a, &b); + assert_eq!(got.as_ref().ok().copied(), expected, "{t:?} against {s:?}: {got:?}"); + assert_eq!(validate_key_compatibility(&a, &b), got.clone().map(|_| ())); + if let Err(e) = got { + assert!(e.contains(t.label()) && e.contains(s.label()) && e.contains("Add Field and Calculate Field"), "{e}"); + } + } + } + } + + /// Joins one text key against one integer key, both ways round, and says whether they matched. + fn text_matches_int(text: &str, int: i64) -> bool { + let text_table = make_table( + "t", + vec![("GEOID", FieldType::Text), ("tag", FieldType::Text)], + vec![vec![Value::Text(text.into()), Value::Text("text side".into())]], + ); + let int_table = make_table( + "i", + vec![("GEOID", FieldType::Integer), ("tag", FieldType::Text)], + vec![vec![Value::Int(int), Value::Text("int side".into())]], + ); + let (_, a) = join_field(&text_table, &int_table, &key_opts(DuplicateStrategy::First)).unwrap(); + let (_, b) = join_field(&int_table, &text_table, &key_opts(DuplicateStrategy::First)).unwrap(); + assert_eq!(a.matched_records, b.matched_records, "{text:?} against {int} differs by direction"); + a.matched_records == 1 + } + + #[test] + fn test_number_as_text_round_trips_every_magnitude() { + let mut values = vec![0, 1, -1, 9, 10, 99, 100, 1001, 47157000100, i64::MAX, i64::MIN, i64::MAX - 1, i64::MIN + 1]; + let mut p = 1i64; + while let Some(next) = p.checked_mul(10) { + values.extend([p - 1, p, p + 1, -p]); + p = next; + } + for n in values { + assert!(text_matches_int(&n.to_string(), n), "{n}"); + // Spellings of the same number that are not the same text never match. + for other in [format!("0{n}"), format!("+{n}"), format!(" {n}"), format!("{n} "), format!("{n}.0")] { + assert!(!text_matches_int(&other, n), "{other:?} matched {n}"); + } + } + } + + #[test] + fn test_number_as_text_rejects_look_alikes() { + for (text, n) in [ + ("-0", 0), + ("00", 0), + ("4_2", 42), + ("42e0", 42), + ("0x2A", 42), + ("4,2", 42), + ("４２", 42), // full-width digits + ("٤٢", 42), // Arabic-Indic digits + ("42\u{200b}", 42), + ("", 0), + ("9223372036854775808", i64::MIN), // past i64::MAX, not a wrap-around + ] { + assert!(!text_matches_int(text, n), "{text:?} matched {n}"); + } + } + + #[test] + fn test_number_as_text_compares_values_not_declared_types() { + // Rows can hold values that disagree with their field's type (a hostile or hand-built + // file): the comparison follows the value, and nothing panics. + let target = make_table( + "target", + vec![("GEOID", FieldType::Integer)], + vec![ + vec![Value::Float(42.0)], + vec![Value::Float(42.5)], + vec![Value::Float(f64::NAN)], + vec![Value::Text("7".into())], + vec![Value::Bool(true)], + ], + ); + let source = make_table( + "source", + vec![("GEOID", FieldType::Text), ("tag", FieldType::Text)], + vec![ + vec![Value::Text("42".into()), Value::Text("forty-two".into())], + vec![Value::Text("42.5".into()), Value::Text("fraction".into())], + vec![Value::Text("NaN".into()), Value::Text("nan".into())], + vec![Value::Int(7), Value::Text("seven".into())], + vec![Value::Text("true".into()), Value::Text("bool".into())], + ], + ); + let (out, rep) = join_field(&target, &source, &key_opts(DuplicateStrategy::Error)).unwrap(); + let tags: Vec<&Value> = out.features.iter().map(|f| &f.attrs[1]).collect(); + assert_eq!(tags, [&Value::Text("forty-two".into()), &Value::Null, &Value::Null, &Value::Text("seven".into()), &Value::Null]); + assert_eq!((rep.matched_records, rep.duplicate_keys), (2, 0)); + } + + #[test] + fn test_exact_comparison_is_unchanged_for_same_type_keys() { + // Text against text stays exact: no number parsing sneaks in. + let target = + make_table("target", vec![("GEOID", FieldType::Text)], vec![vec![Value::Text("042".into())], vec![Value::Text("42".into())]]); + let source = make_table( + "source", + vec![("GEOID", FieldType::Text), ("tag", FieldType::Text)], + vec![vec![Value::Text("42".into()), Value::Text("hit".into())]], + ); + let (out, _) = join_field(&target, &source, &key_opts(DuplicateStrategy::First)).unwrap(); + assert_eq!(out.features[0].attrs[1], Value::Null); + assert_eq!(out.features[1].attrs[1], Value::Text("hit".into())); + assert_eq!(JoinKey::from_value_compared(&Value::Int(5), KeyComparison::Exact), Some(JoinKey::Int(5))); + assert_eq!(JoinKey::from_value_compared(&Value::Null, KeyComparison::NumberAsText), None); } #[test] diff --git a/crates/analysis/src/tools.rs b/crates/analysis/src/tools.rs index 70ca3ff70..dffa70255 100644 --- a/crates/analysis/src/tools.rs +++ b/crates/analysis/src/tools.rs @@ -8131,19 +8131,21 @@ mod tests { .unwrap_err(); assert!(err_dup.contains("duplicate key")); - // 4. Incompatible key type error - let mut int_source = Dataset::new("int_source", GeometryType::None, vec![Field::new("num_key", FieldType::Integer)]); - int_source.push(None, vec![Value::Int(123)]); - let m4 = - Map(HashMap::from([("inventory".to_string(), m2.0.get("inventory").unwrap().clone()), ("int_source".to_string(), int_source)])); + // 4. Incompatible key type error (a text key matches an integer key, not a boolean one) + let mut bool_source = Dataset::new("bool_source", GeometryType::None, vec![Field::new("flag_key", FieldType::Boolean)]); + bool_source.push(None, vec![Value::Bool(true)]); + let m4 = Map(HashMap::from([ + ("inventory".to_string(), m2.0.get("inventory").unwrap().clone()), + ("bool_source".to_string(), bool_source), + ])); let err_type = crate::run( "joinField", &m4, &serde_json::json!({ "target": "inventory", "targetField": "sku", - "join": "int_source", - "joinField": "num_key" + "join": "bool_source", + "joinField": "flag_key" }), ) .unwrap_err(); diff --git a/crates/analysis/tests/join_field.rs b/crates/analysis/tests/join_field.rs new file mode 100644 index 000000000..7a7ddff50 --- /dev/null +++ b/crates/analysis/tests/join_field.rs @@ -0,0 +1,161 @@ +//! Join Field through the tool, on tables read by the real CSV and Shapefile readers: an all-digit +//! id that one file types as a number and the other as text still joins (#1178). +use rustgis_analysis::{Inputs, Output}; +use rustgis_data::io::{read_csv, read_path, write_path}; +use rustgis_data::{Coord, Dataset, Field, FieldType, Geometry, GeometryType, Value}; +use serde_json::json; +use std::collections::HashMap; +use std::sync::atomic::{AtomicUsize, Ordering}; + +#[derive(Default)] +struct Layers { + data: HashMap, + selected: HashMap>, +} +impl Inputs for Layers { + fn dataset(&self, name: &str) -> Option<&Dataset> { + self.data.get(name) + } + fn selection(&self, name: &str) -> &[usize] { + self.selected.get(name).map(Vec::as_slice).unwrap_or(&[]) + } +} +impl Layers { + fn with(mut self, ds: Dataset) -> Self { + self.data.insert(ds.name.clone(), ds); + self + } +} + +fn field_ty(ds: &Dataset, name: &str) -> FieldType { + ds.fields.iter().find(|f| f.name == name).map(|f| f.ty).unwrap() +} +fn column<'a>(ds: &'a Dataset, name: &str) -> Vec<&'a Value> { + let i = ds.fields.iter().position(|f| f.name == name).unwrap(); + ds.features.iter().map(|f| &f.attrs[i]).collect() +} + +/// Census tracts as a TIGER/Line shapefile holds them (`GEOID` a character field), written and +/// read back through the Shapefile reader. +fn tracts() -> Dataset { + static CALLS: AtomicUsize = AtomicUsize::new(0); + let mut ds = + Dataset::new("tracts", GeometryType::Point, vec![Field::new("GEOID", FieldType::Text), Field::new("NAMELSAD", FieldType::Text)]); + for (geoid, x) in [("47157000100", -90.05), ("47157000200", -90.04), ("47157000300", -90.03), ("47157980100", -90.02)] { + ds.push_next(Some(Geometry::Point(Coord::new(x, 35.1))), vec![Value::Text(geoid.into()), Value::Text(format!("Tract {geoid}"))]); + } + let dir = std::env::temp_dir().join(format!("rustgis-join-field-{}-{}", std::process::id(), CALLS.fetch_add(1, Ordering::Relaxed))); + std::fs::create_dir_all(&dir).unwrap(); + write_path(&ds, &dir.join("tracts.shp")).unwrap(); + let (read, _) = read_path(&dir.join("tracts.shp")).unwrap(); + std::fs::remove_dir_all(&dir).unwrap(); + assert_eq!(read.name, "tracts"); + assert_eq!(field_ty(&read, "GEOID"), FieldType::Text, "premise: the Shapefile reader keeps an all-digit character GEOID as text"); + read +} + +/// The Census Gazetteer's tract table as a CSV: the reader types an all-digit `GEOID` as a number. +fn gazetteer() -> Dataset { + let csv = "GEOID,ALAND,AWATER\n47157000300,3000,30\n47157000100,1000,10\n47157000200,2000,20\n47157999999,9,0\n"; + let (ds, _) = read_csv(csv, "gazetteer").unwrap(); + assert_eq!(field_ty(&ds, "GEOID"), FieldType::Integer, "premise: the CSV reader types GEOID as Integer"); + ds +} + +#[test] +fn census_gazetteer_joins_tiger_tracts_by_geoid() { + let layers = Layers::default().with(tracts()).with(gazetteer()); + let res = + rustgis_analysis::run("joinField", &layers, &json!({"target":"tracts","targetField":"GEOID","joinTable":"gazetteer"})).unwrap(); + let Output::Layer(out) = res.output else { panic!("a layer target gives a layer") }; + assert_eq!(column(&out, "ALAND"), [&Value::Int(1000), &Value::Int(2000), &Value::Int(3000), &Value::Null]); + assert_eq!(column(&out, "AWATER"), [&Value::Int(10), &Value::Int(20), &Value::Int(30), &Value::Null]); + // The target's key, rows, ids and geometry are kept as they were. + let before = tracts(); + assert_eq!(field_ty(&out, "GEOID"), FieldType::Text); + assert_eq!(column(&out, "GEOID"), column(&before, "GEOID")); + assert_eq!(out.features.iter().map(|f| f.id).collect::>(), before.features.iter().map(|f| f.id).collect::>()); + assert!(out.features.iter().zip(before.features.iter()).all(|(a, b)| a.geometry == b.geometry)); + assert!(res.messages.iter().any(|m| m.contains("3/4 records matched")), "{:?}", res.messages); +} + +#[test] +fn a_number_key_target_joins_a_text_key_source() { + let layers = Layers::default().with(tracts()).with(gazetteer()); + let res = rustgis_analysis::run( + "joinField", + &layers, + &json!({"target":"gazetteer","targetField":"GEOID","joinTable":"tracts","fields":["NAMELSAD"]}), + ) + .unwrap(); + let Output::Table(out) = res.output else { panic!("a table target gives a table") }; + assert_eq!(field_ty(&out, "GEOID"), FieldType::Integer, "the target key keeps its type"); + assert_eq!(field_ty(&out, "NAMELSAD"), FieldType::Text); + assert_eq!( + column(&out, "NAMELSAD"), + [ + &Value::Text("Tract 47157000300".into()), + &Value::Text("Tract 47157000100".into()), + &Value::Text("Tract 47157000200".into()), + &Value::Null + ] + ); +} + +#[test] +fn codes_with_leading_zeros_only_match_as_written() { + // The CSV reader keeps `01001` as text; a layer stores the same county as the number 1001. + let (codes, _) = read_csv("FIPS,county\n01001,Autauga\n01003,Baldwin\n1005,Barbour\n", "codes").unwrap(); + assert_eq!(field_ty(&codes, "FIPS"), FieldType::Text, "premise: a leading zero keeps the column text"); + let (counties, _) = read_csv("FIPS,pop\n1001,58805\n1003,231767\n1005,25223\n", "counties").unwrap(); + assert_eq!(field_ty(&counties, "FIPS"), FieldType::Integer); + let layers = Layers::default().with(codes).with(counties); + let res = rustgis_analysis::run("joinField", &layers, &json!({"target":"counties","targetField":"FIPS","joinTable":"codes"})).unwrap(); + let Output::Table(out) = res.output else { panic!("a table target gives a table") }; + // No leading zeros are made up: only the code written without one matches. + assert_eq!(column(&out, "county"), [&Value::Null, &Value::Null, &Value::Text("Barbour".into())]); + assert!(res.messages.iter().any(|m| m.contains("1/3 records matched")), "{:?}", res.messages); +} + +#[test] +fn selections_are_honoured_with_mixed_key_types() { + let mut layers = Layers::default().with(tracts()).with(gazetteer()); + layers.selected.insert("tracts".into(), vec![1, 3]); + layers.selected.insert("gazetteer".into(), vec![0, 1]); + let res = + rustgis_analysis::run("joinField", &layers, &json!({"target":"tracts","targetField":"GEOID","joinTable":"gazetteer"})).unwrap(); + let Output::Layer(out) = res.output else { panic!("a layer target gives a layer") }; + assert_eq!(column(&out, "GEOID"), [&Value::Text("47157000200".into()), &Value::Text("47157980100".into())]); + // 47157000200 is in the gazetteer but not among its selected records. + assert_eq!(column(&out, "ALAND"), [&Value::Null, &Value::Null]); +} + +#[test] +fn duplicate_strategies_apply_to_mixed_key_types() { + let (dups, _) = read_csv("GEOID,ALAND\n47157000100,1\n47157000100,2\n", "dups").unwrap(); + let layers = Layers::default().with(tracts()).with(dups); + let run = |duplicates: &str| { + rustgis_analysis::run( + "joinField", + &layers, + &json!({"target":"tracts","targetField":"GEOID","joinTable":"dups","duplicates":duplicates}), + ) + }; + let Output::Layer(first) = run("First").unwrap().output else { panic!() }; + assert_eq!(column(&first, "ALAND")[0], &Value::Int(1)); + let Output::Layer(last) = run("Last").unwrap().output else { panic!() }; + assert_eq!(column(&last, "ALAND")[0], &Value::Int(2)); + let err = run("Error").err().unwrap(); + assert!(err.contains("duplicate key \"47157000100\""), "{err}"); +} + +#[test] +fn other_key_type_pairs_are_refused_with_the_way_out() { + let (decimal, _) = read_csv("GEOID,v\n47157000100.5,1\n", "decimal").unwrap(); + assert_eq!(field_ty(&decimal, "GEOID"), FieldType::Double); + let layers = Layers::default().with(tracts()).with(decimal); + let err = + rustgis_analysis::run("joinField", &layers, &json!({"target":"tracts","targetField":"GEOID","joinTable":"decimal"})).err().unwrap(); + assert!(err.contains("target field 'GEOID' is Text, but source field 'GEOID' is Double"), "{err}"); + assert!(err.contains("Add Field and Calculate Field"), "{err}"); +} diff --git a/docs/user-guide/analysis.md b/docs/user-guide/analysis.md index ead5f7783..7ae231c16 100644 --- a/docs/user-guide/analysis.md +++ b/docs/user-guide/analysis.md @@ -411,7 +411,12 @@ into a `target` table or layer based on matching key values: `joinField` (aliases: `join_field`) names the matching key in the source table; when omitted, empty, or whitespace-only, it defaults to `targetField`. Numeric keys match across integer and floating-point types when whole numbers (e.g. `10` matches `10.0`), while non-finite floats (`NaN`, infinity) and - nulls never match. Incompatible key data types (e.g. text vs integer) are rejected. + nulls never match. A text key matches an integer key when the text is the number written out in + full (`"47157000100"` matches `47157000100`), so an all-digit ID typed as a number in one table and + as text in the other (Census GEOIDs, FIPS codes, ZIP codes) still joins; the text is compared + exactly, so leading zeros stay meaningful (`"01001"` does not match `1001`). Other incompatible key + data types (e.g. text vs floating point or boolean) are rejected, and the message suggests converting + one key with Add Field and Calculate Field. - **Transferred fields:** `fields` selects specific source fields to transfer (a comma-separated list or a JSON array of strings, e.g. `["tax_rate", "zone_code"]`; non-string and non-array types are rejected). Repeated field names are deduplicated in order. When omitted or passed as a blank string (`""`), every source field except From 167dda04243b74242625880769f8d93363a6d601 Mon Sep 17 00:00:00 2001 From: PawelKra <15887575+PawelKra@users.noreply.github.com> Date: Sat, 31 Oct 2026 16:47:10 +0100 Subject: [PATCH 2/3] fix(analysis): Join Field warns when records miss only by leading zeros A Text key against an Integer key is compared as written, so a county FIPS stored as the number 1001 doesn't match the text "01001". Those records were only counted as unmatched, with nothing saying why. Join Field now counts unmatched records whose key equals a key in the other table once leading zeros are dropped, either way round, and adds one warning with the count and the first pair (target key 1001, source key "01001"). The user guide no longer says FIPS and ZIP codes always join: they join when the codes match as written, and a lost zero leaves the record unmatched with that warning. validate_key_compatibility is removed: Join Field calls key_comparison directly and nothing else used it. The selection test now selects a target record whose match is among the selected source records, so it fails if selected mixed-key matches are dropped. --- crates/analysis/src/attribute_join.rs | 144 ++++++++++++++++++++++++-- crates/analysis/src/tools.rs | 1 + crates/analysis/tests/join_field.rs | 17 ++- docs/user-guide/analysis.md | 9 +- 4 files changed, 156 insertions(+), 15 deletions(-) diff --git a/crates/analysis/src/attribute_join.rs b/crates/analysis/src/attribute_join.rs index 3f7ba9f79..8f4b4b9c7 100644 --- a/crates/analysis/src/attribute_join.rs +++ b/crates/analysis/src/attribute_join.rs @@ -101,11 +101,6 @@ pub(crate) enum KeyComparison { NumberAsText, } -/// Validates that target key and source key fields have compatible data types. -pub fn validate_key_compatibility(target_field: &Field, source_field: &Field) -> Result<(), String> { - key_comparison(target_field, source_field).map(|_| ()) -} - /// How the values of the target and source key fields are compared, or why they can't be. pub(crate) fn key_comparison(target_field: &Field, source_field: &Field) -> Result { let t_ty = target_field.ty; @@ -150,6 +145,11 @@ pub struct JoinFieldReport { pub total_records: usize, pub transferred_fields: usize, pub duplicate_keys: usize, + /// Unmatched target records whose key equals a source key once leading zeros are dropped + /// (`"01001"` against `1001`): compared as written, they don't match. + pub leading_zero_mismatches: usize, + /// The first such pair as written, target key then source key (`"01001"`, `1001`). + pub leading_zero_example: Option<(String, String)>, } impl JoinFieldReport { @@ -163,6 +163,31 @@ impl JoinFieldReport { } msg } + + /// A warning when records were left unmatched only by leading zeros, or `None`. + pub fn leading_zero_notice(&self) -> Option { + let (target_key, source_key) = self.leading_zero_example.as_ref().filter(|_| self.leading_zero_mismatches > 0)?; + Some(format!( + "Warning: {} record(s) left unmatched because one key has leading zeros the other lacks \ + (first: target key {target_key}, source key {source_key}); keys are compared as written, \ + so give both the same digits, e.g. as text with the zeros kept", + self.leading_zero_mismatches + )) + } +} + +/// An all-digit text key with leading zeros, without them (`"01001"` → `"1001"`, `"00"` → `"0"`). +fn without_leading_zeros(s: &str) -> Option<&str> { + if s.len() < 2 || !s.starts_with('0') || !s.bytes().all(|b| b.is_ascii_digit()) { + return None; + } + let rest = s.trim_start_matches('0'); + Some(if rest.is_empty() { "0" } else { rest }) +} + +/// A key value as the user wrote it: text quoted, numbers bare. +fn written(v: &Value) -> String { + JoinKey::from_value(v).map(|k| k.display()).unwrap_or_default() } /// Executes a non-spatial attribute join from `source` onto `target`. @@ -268,6 +293,22 @@ pub fn join_field(target: &Dataset, source: &Dataset, opts: &JoinFieldOptions<'_ } } + // Source text keys with leading zeros, by their digits without them: a target number that + // lost its zeros (`1001` for `"01001"`) doesn't match, and the report says so. + let mut zero_padded_sources: HashMap = HashMap::new(); + if comparison == KeyComparison::NumberAsText { + for (s_idx, s_feat) in source.features.iter().enumerate() { + if let Some(Value::Text(text)) = s_feat.attrs.get(source_key_idx) + && let Some(digits) = without_leading_zeros(text) + { + zero_padded_sources.entry(digits.to_string()).or_insert(s_idx); + } + } + } + let source_key_value = |s_idx: usize| source.features.get(s_idx).and_then(|f| f.attrs.get(source_key_idx)).unwrap_or(&Value::Null); + let mut leading_zero_mismatches = 0usize; + let mut leading_zero_example = None; + // 6. Join records into target let target_len = target.features.len(); let mut matched_records = 0usize; @@ -290,7 +331,22 @@ pub fn join_field(target: &Dataset, source: &Dataset, opts: &JoinFieldOptions<'_ new_attrs.reserve(transfer_indices.len()); let t_val = t_feat.attrs.get(target_key_idx).unwrap_or(&Value::Null); - let matching_s_idx = JoinKey::from_value_compared(t_val, comparison).and_then(|k| source_index.get(&k).copied()); + let t_key = JoinKey::from_value_compared(t_val, comparison); + let matching_s_idx = t_key.as_ref().and_then(|k| source_index.get(k).copied()); + if matching_s_idx.is_none() + && comparison == KeyComparison::NumberAsText + && let Some(JoinKey::Text(text)) = &t_key + { + // A target text key with leading zeros against a source key without them, or a target + // key without them against a source text key with them. + let near = without_leading_zeros(text) + .and_then(|digits| source_index.get(&JoinKey::Text(digits.to_string())).copied()) + .or_else(|| zero_padded_sources.get(text).copied()); + if let Some(s_idx) = near { + leading_zero_mismatches += 1; + leading_zero_example.get_or_insert_with(|| (written(t_val), written(source_key_value(s_idx)))); + } + } if let Some(s_idx) = matching_s_idx { matched_records += 1; @@ -317,6 +373,8 @@ pub fn join_field(target: &Dataset, source: &Dataset, opts: &JoinFieldOptions<'_ total_records: target_len, transferred_fields: transfer_indices.len(), duplicate_keys: duplicate_count, + leading_zero_mismatches, + leading_zero_example, }; Ok((out, report)) @@ -651,6 +709,79 @@ mod tests { [&Value::Null, &Value::Text("county".into()), &Value::Text("negative".into()), &Value::Null, &Value::Null, &Value::Null] ); assert_eq!(rep.matched_records, 2); + // Only "01001" is a leading-zero miss; " 7" and "7.0" aren't digits, and nulls aren't keys. + assert_eq!(rep.leading_zero_mismatches, 1); + assert_eq!(rep.leading_zero_example, Some(("\"01001\"".into(), "1001".into()))); + let notice = rep.leading_zero_notice().unwrap(); + assert!(notice.contains("1 record(s) left unmatched"), "{notice}"); + assert!(notice.contains("target key \"01001\", source key 1001"), "{notice}"); + } + + #[test] + fn test_leading_zero_misses_are_reported_both_ways() { + // Target numbers that lost their zeros against source text that kept them. + let target = make_table( + "target", + vec![("GEOID", FieldType::Integer)], + vec![vec![Value::Int(1001)], vec![Value::Int(7)], vec![Value::Int(0)], vec![Value::Int(42)], vec![Value::Int(5)]], + ); + let source = make_table( + "source", + vec![("GEOID", FieldType::Text), ("tag", FieldType::Text)], + vec![ + vec![Value::Text("01001".into()), Value::Text("county".into())], + vec![Value::Text("007".into()), Value::Text("seven".into())], + vec![Value::Text("00".into()), Value::Text("zero".into())], + vec![Value::Text("42".into()), Value::Text("answer".into())], + vec![Value::Text("0x5".into()), Value::Text("hex".into())], + ], + ); + let (out, rep) = join_field(&target, &source, &key_opts(DuplicateStrategy::First)).unwrap(); + assert_eq!(out.features.iter().filter(|f| f.attrs[1] != Value::Null).count(), 1, "only 42 matches"); + assert_eq!((rep.matched_records, rep.leading_zero_mismatches), (1, 3)); + assert_eq!(rep.leading_zero_example, Some(("1001".into(), "\"01001\"".into()))); + assert!(rep.leading_zero_notice().unwrap().contains("3 record(s) left unmatched")); + + // The same tables the other way round: target text with zeros against source numbers. + let (_, rep) = join_field(&source, &target, &key_opts(DuplicateStrategy::First)).unwrap(); + assert_eq!((rep.matched_records, rep.leading_zero_mismatches), (1, 3)); + assert_eq!(rep.leading_zero_example, Some(("\"01001\"".into(), "1001".into()))); + } + + #[test] + fn test_no_leading_zero_notice_without_a_miss() { + // Every key matches, or misses with no near match: no notice. + let target = make_table( + "target", + vec![("GEOID", FieldType::Text)], + vec![vec![Value::Text("0100".into())], vec![Value::Text("0200".into())]], + ); + let source = make_table( + "source", + vec![("GEOID", FieldType::Integer), ("tag", FieldType::Text)], + vec![vec![Value::Int(100), Value::Text("hundred".into())], vec![Value::Int(300), Value::Text("other".into())]], + ); + let (_, rep) = join_field(&target, &source, &key_opts(DuplicateStrategy::First)).unwrap(); + assert_eq!((rep.matched_records, rep.leading_zero_mismatches), (0, 1), "premise: \"0100\" is a miss, \"0200\" isn't near"); + let clean = + make_table("clean", vec![("GEOID", FieldType::Text)], vec![vec![Value::Text("100".into())], vec![Value::Text("200".into())]]); + let (_, rep) = join_field(&clean, &source, &key_opts(DuplicateStrategy::First)).unwrap(); + assert_eq!((rep.matched_records, rep.leading_zero_mismatches, &rep.leading_zero_example), (1, 0, &None)); + assert_eq!(rep.leading_zero_notice(), None); + // Same-type text keys are compared exactly, as before, and aren't second-guessed. + let text_source = make_table("ts", vec![("GEOID", FieldType::Text)], vec![vec![Value::Text("100".into())]]); + let (_, rep) = join_field(&target, &text_source, &key_opts(DuplicateStrategy::First)).unwrap(); + assert_eq!((rep.matched_records, rep.leading_zero_mismatches), (0, 0)); + } + + #[test] + fn test_without_leading_zeros() { + assert_eq!(without_leading_zeros("01001"), Some("1001")); + assert_eq!(without_leading_zeros("00"), Some("0")); + assert_eq!(without_leading_zeros("000120"), Some("120")); + for s in ["", "0", "1001", "10", "0x5", "0 1", "-01", "01.0", "０1"] { + assert_eq!(without_leading_zeros(s), None, "{s:?}"); + } } #[test] @@ -692,7 +823,6 @@ mod tests { let (a, b) = (Field::new("a", t), Field::new("b", s)); let got = key_comparison(&a, &b); assert_eq!(got.as_ref().ok().copied(), expected, "{t:?} against {s:?}: {got:?}"); - assert_eq!(validate_key_compatibility(&a, &b), got.clone().map(|_| ())); if let Err(e) = got { assert!(e.contains(t.label()) && e.contains(s.label()) && e.contains("Add Field and Calculate Field"), "{e}"); } diff --git a/crates/analysis/src/tools.rs b/crates/analysis/src/tools.rs index dffa70255..e8cc2bd8c 100644 --- a/crates/analysis/src/tools.rs +++ b/crates/analysis/src/tools.rs @@ -4102,6 +4102,7 @@ fn join_field_tool(inp: &dyn Inputs, p: &Json) -> Result { let (out, report) = crate::attribute_join::join_field(&target_ref, &source_ref, &opts)?; messages.push(report.message(&target.name)); + messages.extend(report.leading_zero_notice()); let output = if out.geometry_type == rustgis_data::GeometryType::None { Output::Table(out) } else { Output::Layer(out) }; diff --git a/crates/analysis/tests/join_field.rs b/crates/analysis/tests/join_field.rs index 7a7ddff50..6a798b2be 100644 --- a/crates/analysis/tests/join_field.rs +++ b/crates/analysis/tests/join_field.rs @@ -77,6 +77,7 @@ fn census_gazetteer_joins_tiger_tracts_by_geoid() { assert_eq!(out.features.iter().map(|f| f.id).collect::>(), before.features.iter().map(|f| f.id).collect::>()); assert!(out.features.iter().zip(before.features.iter()).all(|(a, b)| a.geometry == b.geometry)); assert!(res.messages.iter().any(|m| m.contains("3/4 records matched")), "{:?}", res.messages); + assert!(!res.messages.iter().any(|m| m.contains("leading zeros")), "no zeros were lost: {:?}", res.messages); } #[test] @@ -115,19 +116,27 @@ fn codes_with_leading_zeros_only_match_as_written() { // No leading zeros are made up: only the code written without one matches. assert_eq!(column(&out, "county"), [&Value::Null, &Value::Null, &Value::Text("Barbour".into())]); assert!(res.messages.iter().any(|m| m.contains("1/3 records matched")), "{:?}", res.messages); + // The two codes that lost their zero are named, not only counted. + let notice = res.messages.iter().find(|m| m.contains("leading zeros")).unwrap_or_else(|| panic!("{:?}", res.messages)); + assert!(notice.contains("2 record(s) left unmatched"), "{notice}"); + assert!(notice.contains("target key 1001, source key \"01001\""), "{notice}"); } #[test] fn selections_are_honoured_with_mixed_key_types() { let mut layers = Layers::default().with(tracts()).with(gazetteer()); - layers.selected.insert("tracts".into(), vec![1, 3]); + layers.selected.insert("tracts".into(), vec![0, 1, 3]); layers.selected.insert("gazetteer".into(), vec![0, 1]); let res = rustgis_analysis::run("joinField", &layers, &json!({"target":"tracts","targetField":"GEOID","joinTable":"gazetteer"})).unwrap(); let Output::Layer(out) = res.output else { panic!("a layer target gives a layer") }; - assert_eq!(column(&out, "GEOID"), [&Value::Text("47157000200".into()), &Value::Text("47157980100".into())]); - // 47157000200 is in the gazetteer but not among its selected records. - assert_eq!(column(&out, "ALAND"), [&Value::Null, &Value::Null]); + assert_eq!( + column(&out, "GEOID"), + [&Value::Text("47157000100".into()), &Value::Text("47157000200".into()), &Value::Text("47157980100".into())] + ); + // 47157000100 matches a selected record; 47157000200 is in the gazetteer but not among its + // selected records. + assert_eq!(column(&out, "ALAND"), [&Value::Int(1000), &Value::Null, &Value::Null]); } #[test] diff --git a/docs/user-guide/analysis.md b/docs/user-guide/analysis.md index 7ae231c16..1ec528b00 100644 --- a/docs/user-guide/analysis.md +++ b/docs/user-guide/analysis.md @@ -413,10 +413,11 @@ into a `target` table or layer based on matching key values: types when whole numbers (e.g. `10` matches `10.0`), while non-finite floats (`NaN`, infinity) and nulls never match. A text key matches an integer key when the text is the number written out in full (`"47157000100"` matches `47157000100`), so an all-digit ID typed as a number in one table and - as text in the other (Census GEOIDs, FIPS codes, ZIP codes) still joins; the text is compared - exactly, so leading zeros stay meaningful (`"01001"` does not match `1001`). Other incompatible key - data types (e.g. text vs floating point or boolean) are rejected, and the message suggests converting - one key with Add Field and Calculate Field. + as text in the other (Census GEOIDs, FIPS codes, ZIP codes) joins when the codes match exactly as + written. Leading zeros stay meaningful: a number that lost its zero (`1001` for `"01001"`) leaves its + record unmatched, and a warning says how many records missed this way, with the first pair. Other + incompatible key data types (e.g. text vs floating point or boolean) are rejected, and the message + suggests converting one key with Add Field and Calculate Field. - **Transferred fields:** `fields` selects specific source fields to transfer (a comma-separated list or a JSON array of strings, e.g. `["tax_rate", "zone_code"]`; non-string and non-array types are rejected). Repeated field names are deduplicated in order. When omitted or passed as a blank string (`""`), every source field except From 60d928f9a3a27f6e9f6de5d16cc2c9fb13cfe7e0 Mon Sep 17 00:00:00 2001 From: PawelKra <15887575+PawelKra@users.noreply.github.com> Date: Sat, 31 Oct 2026 21:34:43 +0100 Subject: [PATCH 3/3] fix(analysis): mark JoinFieldReport non_exhaustive Join Field's report gained two leading-zero counts. Only join_field builds a JoinFieldReport, so marking it non_exhaustive lets new counts be added later without breaking struct literals in other crates. --- crates/analysis/src/attribute_join.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/crates/analysis/src/attribute_join.rs b/crates/analysis/src/attribute_join.rs index 8f4b4b9c7..fa9505fb8 100644 --- a/crates/analysis/src/attribute_join.rs +++ b/crates/analysis/src/attribute_join.rs @@ -137,8 +137,10 @@ pub struct JoinFieldOptions<'a> { pub output_name: String, } -/// Result metadata from a successful Join Field execution. +/// Result metadata from a successful Join Field execution. Only `join_field` builds one, so +/// it is `non_exhaustive`: a new count can be added without breaking callers. #[derive(Clone, Debug, PartialEq, Eq)] +#[non_exhaustive] pub struct JoinFieldReport { pub matched_records: usize, pub unmatched_records: usize,