From b4c34ba4b798901e85ad4ce2d1b9d66a559bcbc3 Mon Sep 17 00:00:00 2001 From: Abimael Martell Date: Mon, 20 Apr 2026 14:04:50 -0700 Subject: [PATCH] refactor: classify Table as Data vs Toc once at construction (#52) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The TOC/data-table distinction was being recomputed at every consumer: - format.rs::table_to_markdown ran is_table_of_contents to decide between flat-list and markdown-table rendering. - compute_layout_complexity ran it again to filter TOCs out of pages_with_tables. - detect_heuristic validations used it to decide whether to relax val 1/9. Each caller had to remember tables can be either kind, which leaks the TOC concept across the codebase. Add `TableKind { Data, Toc }` and a `Table::new` constructor that classifies once from the cells. All five detectors (heuristic, rect, line, struct, columns) now go through `Table::new`. Consumers match on `kind` instead of re-running classification. Pure refactor — no behavior change. Verified: pdf-evals output is byte-for- byte identical (0 changed snapshots). Co-authored-by: Claude Opus 4.7 (1M context) --- src/lib.rs | 13 +++---- src/tables/detect_heuristic.rs | 7 +--- src/tables/detect_lines.rs | 8 ++--- src/tables/detect_rects.rs | 28 +++------------ src/tables/detect_struct.rs | 10 +++--- src/tables/format.rs | 42 +++++++++++------------ src/tables/grid.rs | 6 ++++ src/tables/mod.rs | 62 +++++++++++++++++++++++++++------- 8 files changed, 96 insertions(+), 80 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 086e636..2006f6f 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1786,25 +1786,22 @@ fn compute_layout_complexity( // lists. They aren't tables in any user-facing sense, so don't // count them toward LayoutComplexity (would also trip the // table-page guard in column detection below). - let has_real_table = |tables: &[tables::Table]| { - tables - .iter() - .any(|t| !tables::is_table_of_contents(&t.cells)) - }; + let has_data_table = + |tables: &[tables::Table]| tables.iter().any(|t| t.kind == tables::TableKind::Data); let (rect_tables, _) = tables::detect_tables_from_rects(&band_items, &band_rects, page); - if has_real_table(&rect_tables) { + if has_data_table(&rect_tables) { found_table = true; break; } let line_tables = tables::detect_tables_from_lines(&band_items, &band_lines, page); - if has_real_table(&line_tables) { + if has_data_table(&line_tables) { found_table = true; break; } // Heuristic fallback for borderless tables let heuristic_tables = tables::detect_tables(&band_items, base_size, false); - if has_real_table(&heuristic_tables) { + if has_data_table(&heuristic_tables) { found_table = true; break; } diff --git a/src/tables/detect_heuristic.rs b/src/tables/detect_heuristic.rs index 0bad7a5..78e064f 100644 --- a/src/tables/detect_heuristic.rs +++ b/src/tables/detect_heuristic.rs @@ -687,12 +687,7 @@ fn detect_table_in_region(items: &[(usize, &TextItem)], mode: TableDetectionMode item_indices.len() ); - Some(Table { - columns, - rows, - cells, - item_indices, - }) + Some(Table::new(columns, rows, cells, item_indices)) } /// Check if this looks like a key-value pair layout rather than a table diff --git a/src/tables/detect_lines.rs b/src/tables/detect_lines.rs index af51035..fd89d35 100644 --- a/src/tables/detect_lines.rs +++ b/src/tables/detect_lines.rs @@ -265,12 +265,12 @@ pub fn detect_tables_from_lines(items: &[TextItem], lines: &[PdfLine], page: u32 page, num_rows, num_cols, item_indices.len(), page_item_count, non_empty_rows, cols_with_content ); - vec![Table { - columns: col_edges, - rows: row_edges_desc[..num_rows].to_vec(), + vec![Table::new( + col_edges, + row_edges_desc[..num_rows].to_vec(), cells, item_indices, - }] + )] } #[cfg(test)] diff --git a/src/tables/detect_rects.rs b/src/tables/detect_rects.rs index ca70231..70f2d37 100644 --- a/src/tables/detect_rects.rs +++ b/src/tables/detect_rects.rs @@ -1037,12 +1037,7 @@ fn try_build_grid( (columns, cells) }; - GridResult::Ok(Table { - columns, - rows, - cells, - item_indices, - }) + GridResult::Ok(Table::new(columns, rows, cells, item_indices)) } /// Deduplicate nearby edge values within a tolerance, returning sorted unique edges. @@ -1442,12 +1437,7 @@ fn detect_row_stripe_table( content_ratio * 100.0 ); - Some(Table { - columns: column_centers, - rows: row_centers, - cells, - item_indices, - }) + Some(Table::new(column_centers, row_centers, cells, item_indices)) } /// Detect a table from cell-background rects that failed grid detection. @@ -1691,12 +1681,7 @@ fn detect_row_stripe_table_from_cell_rects( non_empty_cells as f32 / total_cells * 100.0 ); - Some(Table { - columns: column_centers, - rows: row_centers, - cells, - item_indices, - }) + Some(Table::new(column_centers, row_centers, cells, item_indices)) } /// Detect a table by merging all cluster rects into one group. @@ -1875,12 +1860,7 @@ fn detect_merged_cluster_table( content_ratio * 100.0 ); - Some(Table { - columns: column_centers, - rows: row_centers, - cells, - item_indices, - }) + Some(Table::new(column_centers, row_centers, cells, item_indices)) } /// Cluster text item X positions into column centers with a given minimum threshold. diff --git a/src/tables/detect_struct.rs b/src/tables/detect_struct.rs index 182c543..63a7a49 100644 --- a/src/tables/detect_struct.rs +++ b/src/tables/detect_struct.rs @@ -188,12 +188,12 @@ pub fn detect_tables_from_struct_tree( all_item_indices.sort_unstable(); all_item_indices.dedup(); - tables.push(Table { - columns: col_positions, - rows: row_positions, + tables.push(Table::new( + col_positions, + row_positions, cells, - item_indices: all_item_indices, - }); + all_item_indices, + )); } tables diff --git a/src/tables/format.rs b/src/tables/format.rs index d5b0fa6..12199b6 100644 --- a/src/tables/format.rs +++ b/src/tables/format.rs @@ -1,26 +1,19 @@ //! Table-to-markdown formatting and cell cleanup. -use super::detect_heuristic::is_table_of_contents; -use super::Table; +use super::{Table, TableKind}; pub fn table_to_markdown(table: &Table) -> String { if table.cells.is_empty() || table.cells[0].is_empty() { return String::new(); } - // Detect TOC on the raw cells: clean_table_cells merges rows in ways - // that can make genuine data tables superficially resemble a TOC - // (short numeric cells, few columns) — but the raw detection here - // preserves the original multi-column structure and only matches the - // true TOC pattern. - // - // Tables of contents render poorly as markdown tables — emit a flat - // per-row text list instead so the page numbers stay aligned with - // their section titles rather than drifting to a separate column. - // Format from raw cells: continuation-row merging collapses separate - // TOC entries (e.g. "6.2 Contamination" + "6.2.1 SWE-bench") into a - // single line because sub-entries leave column 0 empty. - if is_table_of_contents(&table.cells) { + // TOCs render poorly as markdown tables — emit a flat per-row text list + // instead so the page numbers stay aligned with their section titles + // rather than drifting to a separate column. Format from raw cells + // because continuation-row merging in clean_table_cells collapses + // separate TOC entries (e.g. "6.2 Contamination" + "6.2.1 SWE-bench") + // into one line where sub-entries leave column 0 empty. + if table.kind == TableKind::Toc { return format_toc_as_list(&table.cells, &[]); } @@ -470,6 +463,7 @@ mod tests { vec!["Bob".into(), "25".into()], ], item_indices: vec![], + kind: TableKind::Data, }; let md = table_to_markdown(&table); assert!(md.contains("|Name|")); @@ -485,6 +479,7 @@ mod tests { rows: vec![500.0], cells: vec![vec!["Only".into(), "Row".into()]], item_indices: vec![], + kind: TableKind::Data, }; let md = table_to_markdown(&table); assert!(md.contains("|Only|")); @@ -498,6 +493,7 @@ mod tests { rows: vec![], cells: vec![], item_indices: vec![], + kind: TableKind::Data, }; assert_eq!(table_to_markdown(&table), ""); } @@ -513,6 +509,7 @@ mod tests { vec!["(1)".into(), "Footnote text".into()], ], item_indices: vec![], + kind: TableKind::Data, }; let md = table_to_markdown(&table); assert!(md.contains("(1) Footnote text")); @@ -528,6 +525,7 @@ mod tests { vec!["太郎".into(), "25".into()], ], item_indices: vec![], + kind: TableKind::Data, }; let md = table_to_markdown(&table); assert!(md.contains("名前")); @@ -541,6 +539,7 @@ mod tests { rows: vec![500.0], cells: vec![vec![]], item_indices: vec![], + kind: TableKind::Data, }; assert_eq!(table_to_markdown(&table), ""); } @@ -550,10 +549,10 @@ mod tests { // A TOC-shaped table with section numbers in col 0 and page numbers // in the last column should render as a flat list, not a markdown // table, so the page numbers stay on the same line as their titles. - let table = Table { - columns: vec![50.0, 80.0, 300.0], - rows: vec![500.0; 5], - cells: vec![ + let table = Table::new( + vec![50.0, 80.0, 300.0], + vec![500.0; 5], + vec![ vec![ "4.3".into(), "Case studies and targeted evaluations".into(), @@ -572,8 +571,9 @@ mod tests { vec!["4.4".into(), "Capability evaluations".into(), "101".into()], vec!["4.5".into(), "White-box analyses".into(), "113".into()], ], - item_indices: vec![], - }; + vec![], + ); + assert_eq!(table.kind, TableKind::Toc); let md = table_to_markdown(&table); assert!( !md.contains("|---|"), diff --git a/src/tables/grid.rs b/src/tables/grid.rs index 880b7d4..7fda970 100644 --- a/src/tables/grid.rs +++ b/src/tables/grid.rs @@ -499,6 +499,7 @@ pub(crate) fn recover_header_row( #[cfg(test)] mod tests { use super::*; + use crate::tables::TableKind; use crate::types::ItemType; fn make_item(text: &str, x: f32, y: f32, font_size: f32) -> TextItem { @@ -756,6 +757,7 @@ mod tests { rows: vec![500.0, 480.0], cells: vec![vec!["A".into(), "B".into()], vec!["C".into(), "D".into()]], item_indices: vec![2, 3], + kind: TableKind::Data, }; recover_header_row(&mut table, &all_items, 9.0); @@ -774,6 +776,7 @@ mod tests { rows: vec![500.0], cells: vec![vec!["A".into(), "B".into()]], item_indices: vec![0, 1], + kind: TableKind::Data, }; let rows_before = table.rows.len(); @@ -794,6 +797,7 @@ mod tests { rows: vec![500.0, 480.0], cells: vec![vec!["A".into(), "B".into()], vec!["C".into(), "D".into()]], item_indices: vec![2, 3], + kind: TableKind::Data, }; let rows_before = table.rows.len(); @@ -814,6 +818,7 @@ mod tests { rows: vec![500.0], cells: vec![vec!["A".into(), "B".into()]], item_indices: vec![1, 2], + kind: TableKind::Data, }; let rows_before = table.rows.len(); @@ -829,6 +834,7 @@ mod tests { rows: vec![], cells: vec![], item_indices: vec![], + kind: TableKind::Data, }; recover_header_row(&mut table, &all_items, 9.0); diff --git a/src/tables/mod.rs b/src/tables/mod.rs index 3cc0419..c2e55eb 100644 --- a/src/tables/mod.rs +++ b/src/tables/mod.rs @@ -10,7 +10,8 @@ mod financial; mod format; mod grid; -pub use detect_heuristic::{detect_tables, is_table_of_contents}; +pub use detect_heuristic::detect_tables; +pub(crate) use detect_heuristic::is_table_of_contents; pub use detect_lines::detect_tables_from_lines; pub(crate) use detect_rects::cluster_rects; pub use detect_rects::{detect_tables_from_rects, RectHintRegion}; @@ -166,12 +167,12 @@ pub(crate) fn try_build_rect_guided_table( used_indices.sort_unstable(); used_indices.dedup(); - Some(Table { - columns: col_boundaries, - rows: row_boundaries, + Some(Table::new( + col_boundaries, + row_boundaries, cells, - item_indices: used_indices, - }) + used_indices, + )) } /// Split a TextItem whose text contains multiple whitespace-separated tokens @@ -541,12 +542,22 @@ pub(crate) fn try_build_table_from_columns(items: &[TextItem], page: u32) -> Opt multi_col_rows ); - Some(Table { - columns: col_xs, - rows: row_ys, - cells, - item_indices, - }) + Some(Table::new(col_xs, row_ys, cells, item_indices)) +} + +/// What kind of structure a detected `Table` represents. Classification is +/// computed once at construction so consumers don't have to re-analyze the +/// cells (and stay consistent across detection backends). +#[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] +pub enum TableKind { + /// A real data table — renders as markdown table syntax. + #[default] + Data, + /// A table of contents — renders as a flat list with tab-aligned page + /// numbers via `format_toc_as_list`. Detected through the table pipeline + /// because TOCs share row/column structure with tables, but they are not + /// data tables and shouldn't appear in `pages_with_tables` etc. + Toc, } /// A detected table. @@ -560,6 +571,31 @@ pub struct Table { pub cells: Vec>, /// Items that belong to this table pub item_indices: Vec, + /// Data table vs TOC. Set by `Table::new` from `cells`. + pub kind: TableKind, +} + +impl Table { + /// Build a table and classify it (data vs TOC) from its cells. + pub fn new( + columns: Vec, + rows: Vec, + cells: Vec>, + item_indices: Vec, + ) -> Self { + let kind = if is_table_of_contents(&cells) { + TableKind::Toc + } else { + TableKind::Data + }; + Self { + columns, + rows, + cells, + item_indices, + kind, + } + } } #[cfg(test)] @@ -642,6 +678,7 @@ mod tests { vec!["Cell 1".into(), "Cell 2".into()], ], item_indices: vec![], + kind: TableKind::Data, }; let md = table_to_markdown(&table); @@ -1002,6 +1039,7 @@ mod tests { vec!["3".into(), "5/2".into(), "Item C".into(), "300".into()], ], item_indices: vec![], + kind: TableKind::Data, }; let md = table_to_markdown(&table);