Skip to content

fix(ebcdic): require record names to be unique - #4126

Open
aniketwaghh wants to merge 1 commit into
docling-project:mainfrom
aniketwaghh:fix-ebcdic-duplicate-record-names
Open

fix(ebcdic): require record names to be unique#4126
aniketwaghh wants to merge 1 commit into
docling-project:mainfrom
aniketwaghh:fix-ebcdic-duplicate-record-names

Conversation

@aniketwaghh

Copy link
Copy Markdown
Contributor

EbcdicLayout requires record selectors to be unique but says nothing about record names, and
EbcdicRecordLayout.name defaults to "record". A copybook with two schemas, unique selectors and
no names is therefore accepted, and _RecordParser.parse buckets the decoded rows by name:

rows = {record.name: [] for record in self._layout.records}

Records are selected by selector but stored by name, so both schemas share one bucket. convert()
then hands each schema that combined list while _build_table writes the schema's own header over
it, and every table ends up holding every record. The conversion reports ConversionStatus.SUCCESS.

This adds the missing check to _validate_records, next to the selector one it mirrors. It also
resolves the two identical ## record headings such a layout produced, since name is what
add_heading prints.

Before, on main at 97bdf30, converting an 84-byte cp037 file holding two customer records tagged
C and two transaction records tagged T:

## record

| customer_name    |   account_no |
|------------------|--------------|
| ACME SUPPLIES    |         0041 |
| BOREALIS TRADING |         0042 |
| INVOICE 88213    |      1250.50 |
| REFUND  88240    |       -49.99 |

## record

| memo             |   amount |
|------------------|----------|
| ACME SUPPLIES    |     0041 |
| BOREALIS TRADING |     0042 |
| INVOICE 88213    |  1250.50 |
| REFUND  88240    |   -49.99 |

A transaction amount of 1250.50 is presented as a customer's account_no, and the account number
0041 as a monetary amount. After, the same layout raises at construction:

pydantic_core._pydantic_core.ValidationError: 1 validation error for EbcdicLayout
  Value error, record names must be unique

Adding name="customer" and name="transaction", changing nothing else, converts to the two
correct tables both before and after this change.

I considered keying rows by the schema's index instead, which would fix the data without rejecting
any layout. I went with the validator because name is also the table heading, so two schemas
sharing one is ambiguous output even when the rows are right — but say the word if you would rather
not reject layouts that are currently accepted, and I will switch it.

Three tests. test_record_names_must_be_unique and test_record_names_must_be_unique_from_a_layout_file
cover the two ways a copybook arrives, inline and as JSON; both fail on main and pass here.
test_a_single_record_keeps_the_default_name guards the case the check must not break — one schema
keeping the "record" default — and passes either way, so it is a regression guard rather than a
bug test.

Suite on this branch and on main at 97bdf30, same machine: 12 failed / 1777 passed here against
12 failed / 1774 passed there, the three extra passes being the tests above. The twelve failures are
the same twelve on both sides — asr_pipeline, webp, the two e2e conversions and the two extraction
template tests, all model- or network-dependent. make validate is clean.

Issue resolved by this Pull Request:
Resolves #4125

Checklist:

  • Documentation has been updated, if necessary.
  • Examples have been added, if necessary.
  • Tests have been added, if necessary.

EbcdicLayout checks that record selectors are unique but not that names are,
and EbcdicRecordLayout.name defaults to "record". A copybook with two schemas,
unique selectors and no names was therefore accepted, and _RecordParser.parse
buckets decoded rows by name, so both schemas shared one bucket. Every table
then held every record, with each schema's own header written over the combined
list, and the conversion still reported success.

Add the missing check next to the selector one it mirrors. This also resolves
the two identical "## record" headings such a layout produced, since name is
what add_heading prints.

Signed-off-by: aniketwaghh <aniketwaghh@users.noreply.github.com>
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

DCO Check Passed

Thanks @aniketwaghh, all your commits are properly signed off. 🎉

@mergify

mergify Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Merge Protections

🟢 Merge protection satisfied — ready to merge.

Show 1 satisfied protection

🟢 Enforce conventional commit

Make sure that we follow https://www.conventionalcommits.org/en/v1.0.0/

  • title ~= ^(fix|feat|docs|style|refactor|perf|test|build|ci|chore|revert)(?:\(.+\))?(!)?:

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.

EBCDIC: record schemas that share a name (the default) get each other's rows

1 participant