Skip to content

Trim comma-padded blank rows in CSV table conversion - #2687

Open
Chen Yufeiyang (feiiiiii5) wants to merge 2 commits into
microsoft:mainfrom
feiiiiii5:fix/csv-comma-padded-blank-rows
Open

Chen Yufeiyang (feiiiiii5) wants to merge 2 commits into
microsoft:mainfrom
feiiiiii5:fix/csv-comma-padded-blank-rows

Conversation

@feiiiiii5

@feiiiiii5 Chen Yufeiyang (feiiiiii5) commented Oct 10, 2026 •

Copy link
Copy Markdown

A writer that pads every record to the column width emits a blank row as , instead of an empty line. _trim_outer_blank_rows only counted a row with no fields at all as blank, so on main this input keeps two empty data rows:

name,age
Alice,30
,
,

pandas.DataFrame.to_csv writes exactly that shape for all-NaN rows: to_csv(index=False) on a two-column frame whose rows are all-NaN ends with ,\n,\n (checked on pandas 3.0.6). A padded row wider than the header also widens the whole table -- name,age\nAlice,30\n,,\n comes out as three columns whose third is always empty. #2176 reported the unpadded form and #2303 handled rows that arrive empty; the padded form still reproduces on main.

The trim now counts a row whose cells are all empty after stripping as blank, which is how the PDF table builder already reads the same shape (_pdf_converter._to_markdown_table filters any(cell.strip() for cell in row)). Only rows after the header are judged that way: in the first row, empty fields cannot be told apart from a header whose column names are empty, so the leading run keeps the old "no fields at all" test. Blank rows between data rows are still kept, matching test_csv_blank_lines_between_rows_are_kept.

The header caveat is Bicheng (Kenneth) (@BichengWang)'s finding in review; the new cases in tests/test_csv.py pin it.

Test: pytest tests/test_csv.py -q gives 47 passed here. Over the modules that reach CSV conversion (tests/test_csv.py tests/test_module_misc.py tests/test_cu.py) this branch gives 325 passed, 11 skipped; the same command with only _csv_converter.py put back at 4cc9fa1 gives 322 passed and 3 failed -- the three new trimming cases. Also ran black --check on both changed files (clean).

Details

Shapes the tests pin. , and "","" and ,, as the first row all stay a header with empty column names, at their original column count. A padded row directly after the header and a trailing padded run are trimmed; an internal padded row keeps its place, as an internal empty line does today.

Why not an opt-in for the leading run. A reviewer suggested keeping padded headers and making trimming of leading padded records opt-in. That needs a new keyword argument with no caller to set it, and the leading run is not what pandas.to_csv emits -- its blank rows land after the named header -- so this drops the leading trim rather than adding surface. Happy to add the flag if a maintainer wants leading padding handled.

Scope. Two files: the converter and tests/test_csv.py. No new dependency, no API change.

Not verified. The environment is .[all] installed on Python 3.11; CI runs the suite on 3.10-3.14 over ubuntu and windows. A whole-suite run did not finish inside my command budget here, so beyond the three modules above the remaining suites are CI's to cover.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found a header-handling regression; details inline.

"""Remove empty rows from the beginning and end, and immediately after the header. This operation is performed in-place."""
start = 0
while start < len(rows) and not rows[start]:
while start < len(rows) and _is_blank_row(rows[start]):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also removes legitimate headers whose column names are empty. For example, converting b",\nAlice,30\nBob,40\n" through MarkItDown.convert_stream previously produced:

|  |  |
| --- | --- |
| Alice | 30 |
| Bob | 40 |

On this branch it produces:

| Alice | 30 |
| --- | --- |
| Bob | 40 |

Alice's record is now treated as the header. Quoted empty names have the same problem; a three-column unnamed header also loses its extra column.

Could we preserve field-bearing headers by default and make trimming leading padded records opt-in? Parsed empty fields alone can't distinguish padding from an unnamed header. I reproduced these three regressions on c037894 and checked that they pass on the base.

Tested with Hermes AI assistance.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, and reproduced: all three cases changed on this branch and matched main again once I narrowed the rule. You're right that parsed empty fields alone can't separate padding from an unnamed header — pandas.read_csv reads a first row of , / ,, into Unnamed: 0 / Unnamed: 1, so it resolves the ambiguity as a header. This branch now agrees: the all-empty-cells rule applies only to rows after the header (the run right after it, and the trailing run), and the leading run keeps the old "no fields at all" test. _is_blank_row is now _is_padding_row, with that caveat in its docstring. Done in 93cbcc6.

I dropped the leading trim rather than gating it behind an opt-in, because nothing would set the flag and pandas.to_csv puts its blank rows after the named header — the trailing and after-header cases are the ones that reproduce in practice. If you or a maintainer would still rather trim leading padding behind an option, say so and I'll add it.

The three shapes you listed are now pinned as tests (,, "","", and ,, as the first row, including the column count). pytest tests/test_csv.py -q → 47 passed; over tests/test_csv.py tests/test_module_misc.py tests/test_cu.py → 325 passed here, against 322 passed plus the three trimming failures with only _csv_converter.py put back at 4cc9fa1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants