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/functions/src/text.rs

@@ -339,11 +339,11 @@ fn numbervalue(a: &[Arg], c: &mut dyn Ctx) -> R<Value> {
     if body.is_empty() {
         return num_val(0.0);
     }
-    // Count the trailing percent signs and cut them off in place: copying the rest once per sign
-    // made a long run of them quadratic.
-    let kept = body.trim_end_matches('%').len();
-    let pct = body.len() - kept;
-    body.truncate(kept);
+    let mut pct = 0;
+    while let Some(b) = body.strip_suffix('%') {
+        body = b.to_string();
+        pct += 1;
+    }
     let mut seen_dec = false;
     let mut out = String::new();
     for c in body.chars() {
@@ -1084,15 +1084,6 @@ mod tests {
 
     use crate::util::testutil::*;
 
-    /// Trailing percent signs each divide by 100, and a long run of them is cut off in one pass.
-    #[test]
-    fn numbervalue_percent_signs() {
-        close(ev("NUMBERVALUE", vec![t("5%%")]), 0.0005);
-        close(ev("NUMBERVALUE", vec![t("1,5%"), t(","), t(".")]), 0.015);
-        let many = format!("1{}", "%".repeat(32_000));
-        close(ev("NUMBERVALUE", vec![t(&many)]), 0.0);
-    }
-
     #[test]
     fn bahttext() {
         let bt = |x: f64| ev("BAHTTEXT", vec![n(x)]);

crates/xlsx/src/richdata.rs

@@ -142,13 +142,22 @@ pub fn read(cx: &mut Ctx<'_>, workbook_rels: &[Rel]) -> Result<Vec<Option<Arc<Ce
     let Some(image_rel_list) = read_xml(cx, &image_rels_path, "richValueRels")? else { return Ok(vec![]) };
     let image_rels = cx.pkg.rels(&image_rels_path)?;
     let structures = entries(cx, &structures, "s")?;
+    // Each structure's key layout, worked out once: every value would otherwise walk its
+    // structure's keys again. Only local-image structures matter here.
+    let layouts: Vec<Option<Layout>> = 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<Vec<Option<Arc<Ce
         .collect())
 }
 
+/// Where a `_localImage` structure keeps the values a picture needs.
+struct Layout {
+    keys: usize,
+    /// Index of the one `_rvRel:LocalImageIdentifier` key of type `i`, if exactly one.
+    image: Option<usize>,
+    /// Index of the one `Text` key of type `s`, if exactly one.
+    text: Option<usize>,
+}
+
+impl Layout {
+    fn of(structure: &El) -> Option<Layout> {
+        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<Layout>],
     image_rel_list: &[&El],
-    image_rels: &[Rel],
+    rels_by_id: &HashMap<&str, Option<&Rel>>,
     cache: &mut HashMap<(String, String), Arc<CellPicture>>,
     total: &mut usize,
 ) -> Result<Option<Arc<CellPicture>>, 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::<usize>().ok())
-        .and_then(|n| image_rel_list.get(n))
-        .and_then(|rel| rel.attr("id"))
+    let lookup = |slot: Option<usize>| 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::<usize>().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()));

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#"<k n="pad" t="s"/>"#.repeat(200_000);
+    replace(&mut parts, "rich/structures.xml", r#"<k n="CalcOrigin" t="i"/>"#, &format!(r#"<k n="CalcOrigin" t="i"/>{padding}"#));
+    let values = r#"<rv s="1"><v/></rv>"#.repeat(50_000);
+    replace(&mut parts, "rich/values.xml", r#"<rv s="1"><v>green &amp; &lt;image&gt;</v><v>1</v><v>5</v></rv>"#, &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#"<Relationship Id="opaqueImageId" Type="http://schemas.openxmlformats.org/officeDocument/2006/relationships/image" Target="../media/original.bin"/>"#;
+    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);
+}