From d30db95e8fcf702a23f2a69169a69a40ed3fc6d7 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Tue, 3 Nov 2026 11:03:35 +0000 Subject: [PATCH] xlsx: resolve cell pictures through a per-structure layout and a relationship index Every rich value rebuilt its structure's whole key list before checking its value count, and scanned the image relationship table for a duplicate id. Structures are now laid out once and relationships are indexed by id (a repeated id stays invalid), so reading cell-picture metadata is linear in its records. --- crates/xlsx/src/richdata.rs | 68 ++++++++++++++++--------- crates/xlsx/src/tests/richdata_tests.rs | 24 +++++++++ 2 files changed, 69 insertions(+), 23 deletions(-) diff --git a/crates/xlsx/src/richdata.rs b/crates/xlsx/src/richdata.rs index 8a4f6ec0a..3a448d60e 100644 --- a/crates/xlsx/src/richdata.rs +++ b/crates/xlsx/src/richdata.rs @@ -142,13 +142,22 @@ pub fn read(cx: &mut Ctx<'_>, workbook_rels: &[Rel]) -> Result> = structures.iter().map(|s| Layout::of(s)).collect(); let image_rel_list = entries(cx, &image_rel_list, "rel")?; + // Relationships by id, a repeated id recorded as invalid (`None`), so a value's lookup is + // one probe rather than a scan of the whole table for duplicates. + let mut rels_by_id: HashMap<&str, Option<&Rel>> = HashMap::new(); + for rel in &image_rels { + rels_by_id.entry(rel.id.as_str()).and_modify(|r| *r = None).or_insert(Some(rel)); + } let values = entries(cx, &data, "rv")?; let mut pictures = Vec::with_capacity(values.len()); let mut cache = HashMap::new(); let mut total = 0usize; for value in values { - let picture = read_value(cx, value, &structures, &image_rel_list, &image_rels, &mut cache, &mut total)?; + let picture = read_value(cx, value, &layouts, &image_rel_list, &rels_by_id, &mut cache, &mut total)?; if picture.is_none() { cx.warn("some rich values are unsupported or malformed; their scalar fallbacks were retained (cell pictures may be missing)"); } @@ -188,43 +197,56 @@ pub fn read(cx: &mut Ctx<'_>, workbook_rels: &[Rel]) -> Result, + /// Index of the one `Text` key of type `s`, if exactly one. + text: Option, +} + +impl Layout { + fn of(structure: &El) -> Option { + if structure.attr("t") != Some("_localImage") { + return None; + } + let keys: Vec<&El> = structure.kids("k").collect(); + let one = |name: &str, ty: &str| { + let mut matches = keys.iter().enumerate().filter(|(_, k)| k.attr("n") == Some(name)); + let (i, key) = matches.next()?; + (matches.next().is_none() && key.attr("t") == Some(ty)).then_some(i) + }; + Some(Layout { keys: keys.len(), image: one("_rvRel:LocalImageIdentifier", "i"), text: one("Text", "s") }) + } +} + fn read_value( cx: &mut Ctx<'_>, value: &El, - structures: &[&El], + layouts: &[Option], image_rel_list: &[&El], - image_rels: &[Rel], + rels_by_id: &HashMap<&str, Option<&Rel>>, cache: &mut HashMap<(String, String), Arc>, total: &mut usize, ) -> Result>, IoError> { - let Some(structure) = value.attr_u32("s").and_then(|n| structures.get(n as usize)) else { return Ok(None) }; - if structure.attr("t") != Some("_localImage") { - return Ok(None); - } - let keys: Vec<_> = structure.kids("k").collect(); + let Some(Some(layout)) = value.attr_u32("s").and_then(|n| layouts.get(n as usize)) else { return Ok(None) }; let values: Vec<_> = value.kids("v").collect(); - if keys.len() != values.len() { + if layout.keys != values.len() { return Ok(None); } - let lookup = |name: &str, ty: &str| { - let mut matches = keys.iter().zip(&values).filter(|(k, _)| k.attr("n") == Some(name)); - let (key, value) = matches.next()?; - (matches.next().is_none() && key.attr("t") == Some(ty)).then_some(value.text.as_str()) - }; - let Some(rel_id) = lookup("_rvRel:LocalImageIdentifier", "i") - .and_then(|v| v.trim().parse::().ok()) - .and_then(|n| image_rel_list.get(n)) - .and_then(|rel| rel.attr("id")) + let lookup = |slot: Option| slot.and_then(|i| values.get(i)).map(|v| v.text.as_str()); + let Some(rel_id) = + lookup(layout.image).and_then(|v| v.trim().parse::().ok()).and_then(|n| image_rel_list.get(n)).and_then(|rel| rel.attr("id")) else { return Ok(None); }; - let mut matches = image_rels.iter().filter(|rel| rel.id == rel_id); - let Some(rel) = matches.next() else { return Ok(None) }; - if matches.next().is_some() || rel.kind != "image" || rel.external { + let Some(found) = rels_by_id.get(rel_id) else { return Ok(None) }; + let Some(rel) = found.filter(|rel| rel.kind == "image" && !rel.external) else { cx.warn("a cell picture has an external or invalid image relationship; kept its scalar fallback"); return Ok(None); - } - let alt = lookup("Text", "s").unwrap_or("").to_string(); + }; + let alt = lookup(layout.text).unwrap_or("").to_string(); let key = (normalize(&rel.target), alt.clone()); if let Some(picture) = cache.get(&key) { return Ok(Some(picture.clone())); diff --git a/crates/xlsx/src/tests/richdata_tests.rs b/crates/xlsx/src/tests/richdata_tests.rs index c7bd488f7..bed0cf130 100644 --- a/crates/xlsx/src/tests/richdata_tests.rs +++ b/crates/xlsx/src/tests/richdata_tests.rs @@ -270,3 +270,27 @@ fn picture_metadata_limits_and_relationship_read_errors_do_not_block_workbooks() assert!(report.warnings.iter().any(|w| w.contains("cell-picture data could not be read")), "{:?}", report.warnings); } } + +/// Many values over one wide structure, and a repeated relationship id: the structure's layout +/// is worked out once and relationships are indexed, so the import is linear in the records; +/// the repeated id is invalid and keeps the scalar fallback. +#[test] +fn wide_structures_and_repeated_relationship_ids_are_resolved_through_indexes() { + let mut parts = independent_parts(false); + // 200,000 keys in one structure and 50,000 short values that name it: rebuilding the key + // list per value would be ten billion steps. + let padding = r#""#.repeat(200_000); + replace(&mut parts, "rich/structures.xml", r#""#, &format!(r#"{padding}"#)); + let values = r#""#.repeat(50_000); + replace(&mut parts, "rich/values.xml", r#"green & <image>15"#, &values); + let (wb, report) = read_xlsx(&zip(&parts)).unwrap(); + assert!(!wb.sheet(0).unwrap().cell_pictures.contains_key(&at("A1"))); + assert!(report.warnings.iter().any(|w| w.contains("picture")), "{:?}", report.warnings); + // The same id twice makes every reference to it invalid. + let mut parts = independent_parts(false); + let dup = r#""#; + replace(&mut parts, "rich/_rels/images.xml.rels", dup, &dup.repeat(2)); + let (wb, report) = read_xlsx(&zip(&parts)).unwrap(); + assert!(!wb.sheet(0).unwrap().cell_pictures.contains_key(&at("A1"))); + assert!(report.warnings.iter().any(|w| w.contains("invalid image relationship")), "{:?}", report.warnings); +}