From f94438adccd8785fed24b3e91004243144b6fc15 Mon Sep 17 00:00:00 2001 From: Jack Date: Tue, 18 Aug 2026 09:45:46 +0800 Subject: [PATCH] fix(parser): check every grid row, not just the first, before cross-page rebuild (#18378) --- .../deepdoc/parser/pdf/table/table_merge.go | 47 ++++++-- .../parser/pdf/table/table_merge_test.go | 114 +++++++++++------- 2 files changed, 106 insertions(+), 55 deletions(-) diff --git a/internal/deepdoc/parser/pdf/table/table_merge.go b/internal/deepdoc/parser/pdf/table/table_merge.go index 0dd3196c66..233ba91688 100644 --- a/internal/deepdoc/parser/pdf/table/table_merge.go +++ b/internal/deepdoc/parser/pdf/table/table_merge.go @@ -132,18 +132,19 @@ func MergeTablesAcrossPages(tables []pdf.TableItem, medianHeights map[int]float6 // len(anchor.Grid)==0 path (continuation rows dropped, but // structurally valid HTML). if len(anchor.Grid) > 0 && len(contGrids) > 0 { - anchorCols := len(anchor.Grid[0]) - uniform := true - for _, cg := range contGrids { - if len(cg) == 0 || len(cg[0]) != anchorCols { - uniform = false - break - } - } - if uniform { - allGrids := make([][][]pdf.TSRCell, 0, 1+len(contGrids)) - allGrids = append(allGrids, anchor.Grid) - allGrids = append(allGrids, contGrids...) + // Stack the per-page grids the same way stackGrids expects them. + allGrids := make([][][]pdf.TSRCell, 0, 1+len(contGrids)) + allGrids = append(allGrids, anchor.Grid) + allGrids = append(allGrids, contGrids...) + // Every row of every merged grid (anchor included) must share one + // column count, otherwise stacking yields a non-uniform grid that + // CalSpans / CleanupOrphanColumns / RowsToHTML would misalign or + // silently drop. uniformColCount checks each row of each grid, not + // just the first row of each continuation grid, so a continuation + // whose interior rows are narrower than its first row is caught. + // It returns 0 (skip rebuild, keep anchor-only Grid) on any + // mismatch — the same safe degrade as len(anchor.Grid)==0. + if uniformColCount(allGrids...) > 0 { if rebuilt := stackGrids(allGrids...); len(rebuilt) > 0 { anchor.Grid = rebuilt } @@ -207,6 +208,28 @@ func gridYExtent(g [][]pdf.TSRCell) (minY, maxY float64) { return minY, maxY } +// uniformColCount returns the shared column count if every non-empty row of +// every given grid has the same number of columns, or 0 otherwise. Unlike a +// first-row-only check, it inspects each row of each grid, so a grid whose +// interior rows are narrower than its first row is reported non-uniform. Empty +// rows are skipped so legitimate empty rows do not cause a false mismatch. +func uniformColCount(grids ...[][]pdf.TSRCell) int { + cols := -1 + for _, g := range grids { + for _, row := range g { + if len(row) == 0 { + continue + } + if cols == -1 { + cols = len(row) + } else if len(row) != cols { + return 0 + } + } + } + return cols +} + // shiftGridY returns a copy of g with every cell's Y0/Y1 shifted by dy. func shiftGridY(g [][]pdf.TSRCell, dy float64) [][]pdf.TSRCell { out := make([][]pdf.TSRCell, len(g)) diff --git a/internal/deepdoc/parser/pdf/table/table_merge_test.go b/internal/deepdoc/parser/pdf/table/table_merge_test.go index 1072af0f81..29ae8fc2d4 100644 --- a/internal/deepdoc/parser/pdf/table/table_merge_test.go +++ b/internal/deepdoc/parser/pdf/table/table_merge_test.go @@ -232,13 +232,19 @@ func TestMergeTablesAcrossPages_RebuildsGridAcrossPages(t *testing.T) { } // TestMergeTablesAcrossPages_JaggedContinuationFallsBackToAnchorGrid verifies -// that when a continuation page's grid has a different number of columns than -// the anchor (a jagged cross-page stack), MergeTablesAcrossPages does NOT -// rebuild a non-uniform Grid. Instead it keeps the anchor-only Grid, so -// ConstructTable emits a structurally valid (if continuation-dropping) table -// rather than malformed HTML. This is the same safe degrade as the -// len(anchor.Grid)==0 path, and keeps the merge decision (and the appended -// continuation Cells) unchanged. +// that when a continuation page's grid is not column-uniform with the anchor +// (a jagged cross-page stack), MergeTablesAcrossPages does NOT rebuild a +// non-uniform Grid. Instead it keeps the anchor-only Grid, so ConstructTable +// emits a structurally valid (if continuation-dropping) table rather than +// malformed HTML. This is the same safe degrade as the len(anchor.Grid)==0 +// path, and keeps the merge decision (and the appended continuation Cells) +// unchanged. +// +// Each case below is a jagged stack the OLD guard (which only compared the +// first row of each continuation grid) would have wrongly allowed to rebuild. +// The first case differs on the first row; the second matches on the first +// row but narrows on an interior row — exactly the gap the per-row +// uniformColCount check closes. func TestMergeTablesAcrossPages_JaggedContinuationFallsBackToAnchorGrid(t *testing.T) { pageGrid := func(rows [][]string) [][]pdf.TSRCell { g := make([][]pdf.TSRCell, len(rows)) @@ -267,44 +273,66 @@ func TestMergeTablesAcrossPages_JaggedContinuationFallsBackToAnchorGrid(t *testi } return cs } - // Anchor: 3 columns. Continuation: 2 columns (jagged). - pg0 := pdf.TableItem{ - Positions: []pdf.Position{{PageNumbers: []int{0}, Left: 0, Right: 300, Top: 0, Bottom: 60}}, - Scale: 1.0, - Grid: pageGrid([][]string{{"a", "b", "c"}, {"d", "e", "f"}}), - Cells: cells([][]string{{"a", "b", "c"}, {"d", "e", "f"}}), - } - pg1 := pdf.TableItem{ - Positions: []pdf.Position{{PageNumbers: []int{1}, Left: 0, Right: 200, Top: 0, Bottom: 60}}, - Scale: 1.0, - Grid: pageGrid([][]string{{"g", "h"}, {"i", "j"}}), - Cells: cells([][]string{{"g", "h"}, {"i", "j"}}), + cases := []struct { + name string + anchorRows [][]string + contRows [][]string + contCellText string + }{ + { + name: "first row column count differs", + anchorRows: [][]string{{"a", "b", "c"}, {"d", "e", "f"}}, + contRows: [][]string{{"g", "h"}, {"i", "j"}}, + contCellText: "g", + }, + { + name: "interior row column count differs", + anchorRows: [][]string{{"a", "b", "c"}, {"d", "e", "f"}}, + contRows: [][]string{{"g", "h", "i"}, {"j", "k"}}, + contCellText: "g", + }, } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + pg0 := pdf.TableItem{ + Positions: []pdf.Position{{PageNumbers: []int{0}, Left: 0, Right: 300, Top: 0, Bottom: 60}}, + Scale: 1.0, + Grid: pageGrid(tc.anchorRows), + Cells: cells(tc.anchorRows), + } + pg1 := pdf.TableItem{ + Positions: []pdf.Position{{PageNumbers: []int{1}, Left: 0, Right: 300, Top: 0, Bottom: 60}}, + Scale: 1.0, + Grid: pageGrid(tc.contRows), + Cells: cells(tc.contRows), + } - merged := MergeTablesAcrossPages([]pdf.TableItem{pg0, pg1}, nil) - if len(merged) != 1 { - t.Fatalf("expected 1 merged table, got %d", len(merged)) - } - // Columns differ (3 vs 2) → rebuild must be skipped → Grid stays - // anchor-only (2 rows), NOT a 4-row jagged grid. - if len(merged[0].Grid) != 2 { - t.Fatalf("jagged continuation must fall back to anchor-only Grid (want 2 rows), got %d", len(merged[0].Grid)) - } - // Anchor rows preserved; continuation NOT stacked into the Grid. - if merged[0].Grid[0][0].Text != "a" || merged[0].Grid[1][0].Text != "d" { - t.Errorf("anchor rows corrupted after jagged fallback: %s / %s", merged[0].Grid[0][0].Text, merged[0].Grid[1][0].Text) - } - // Continuation Cells are still appended (pre-fix behaviour) — the merge - // decision is unchanged; only the Grid stays uniform so HTML is valid. - hasCont := false - for _, c := range merged[0].Cells { - if c.Text == "g" { - hasCont = true - break - } - } - if !hasCont { - t.Errorf("continuation Cells should still be appended even when Grid rebuild is skipped") + merged := MergeTablesAcrossPages([]pdf.TableItem{pg0, pg1}, nil) + if len(merged) != 1 { + t.Fatalf("expected 1 merged table, got %d", len(merged)) + } + // Columns differ (anchor 3 vs continuation jagged) → rebuild must + // be skipped → Grid stays anchor-only, NOT a 4-row jagged grid. + if len(merged[0].Grid) != len(tc.anchorRows) { + t.Fatalf("jagged continuation must fall back to anchor-only Grid (want %d rows), got %d", len(tc.anchorRows), len(merged[0].Grid)) + } + // Anchor rows preserved; continuation NOT stacked into the Grid. + if merged[0].Grid[0][0].Text != tc.anchorRows[0][0] || merged[0].Grid[1][0].Text != tc.anchorRows[1][0] { + t.Errorf("anchor rows corrupted after jagged fallback: %s / %s", merged[0].Grid[0][0].Text, merged[0].Grid[1][0].Text) + } + // Continuation Cells are still appended (pre-fix behaviour) — the + // merge decision is unchanged; only the Grid stays uniform. + hasCont := false + for _, c := range merged[0].Cells { + if c.Text == tc.contCellText { + hasCont = true + break + } + } + if !hasCont { + t.Errorf("continuation Cells should still be appended even when Grid rebuild is skipped") + } + }) } }