[hpoettker]

Problem

In sas7bdat files, a format such as DOLLAR12.2 is stored in two places:

• the name DOLLAR in the column text subheader
• the width 12 and digits 2 in the column subheader

The writer kept the split only if the rebuilt string matched the input exactly. Formats with a trailing dot, like YYMMDD10., never matched, so the whole string was written as the name with width 0. The reader left out the dot whenever digits were 0 (DOLLAR12 instead of DOLLAR12.), and it didn't check the stored data.

This is intended as a follow-up on #398, which unlocked the writing of SAS data files readable by SAS. The setup of the sas7bdat writer for formats and informats too easily produced SAS data files unreadable by SAS.

Changes

Writer (readstat_sas7bdat_write.c)

• Always splits the format with the existing XPT format parser.
• Invalid format strings now fail with READSTAT_ERROR_BAD_FORMAT_STRING. They used to be written as the name without any error, producing SAS data files unreadable by SAS.

Reader (readstat_sas7bdat_read.c)

• Rebuilds the format as NAME., NAMEw. or NAMEw.d. An empty name gives the plain w.d format (e.g. 10.3).
• Returns READSTAT_ERROR_PARSE if the stored name contains ., of if width is 0 and digit isn't.

Shared (readstat_xport.h)

• xport_format_t.name grows to 33 bytes so that names of 32 characters, the SAS maximum, fit with their terminator.

Behavior changes

• Format read from sas7bdat files now always contain a dot, e.g. $CHAR8. instead of $CHAR8.
• Writing a sas7bdat file with an invalid format or informat string now fails instead of silently storing it.

[hpoettker]

@evanmiller Do you have reservations about merging this PR?

I can see multiple possibilities:

• There is a technical no-go in it.
  • In this case, please post a Fable review like on previous PRs. I'll then take care of requested changes.
• It creates questions about how SAS formats in sas7bdat and XPT files interlock as one would assume that they work the same in both file formats
  • If this is the case, please make it explicit. My personal use case uses only sas7bdat and sas7bcat files but not XPT files. But I can research this question further if requested.
• The rational in the PR description is weak.
  • In this case, please find my motivation below.

Background

This PR is motivated by the discussion in the pyreadstat issue to enable the ReadStat write API for sas7bdat files: Roche/pyreadstat#205

Previously, it was more of a cosmetic issue that ReadStat didn't follow SAS conventions when reporting formats: DOLLAR and BEST32 were just as good as the actually expected DOLLAR. and BEST32.. The PR fixes this in a similar way to #415 but for sas7bdat instead of XPT.

The more relevant problem is that the SAS7BDAT writer currently accepts a non-SAS format name like YYMMDD10 and does the expected thing but with the correct SAS format name YYMMDD10. it produces data files that are unreadable by SAS.

With this PR, the writer accepts both YYMMDD10 and YYMMDD10., produces data files readable by SAS in both cases, and the reader consistently reports the correct SAS format name YYMMDD10..

[evanmiller]

Review of #419

The overall direction looks sound: always splitting the format on write, rejecting invalid format strings, and reporting NAMEw. / NAME. on read the way SAS does (and the way the XPT reader does since #415). There is one blocking issue.

Blocker: files written by earlier ReadStat versions become unreadable

Up to now, including v1.1.9 and current dev, the writer has stored a dotted format such as YYMMDD10. or BEST. verbatim as the format name, with width 0 and digits 0. The new check in sas7bdat_construct_format

if (strchr(dst, '.') || (width == 0 && digits != 0))
    return READSTAT_ERROR_PARSE;

rejects those names. Because the error propagates, the whole file fails to parse, not just that column's format.

Test: a one-column file written by the dev writer, read back with dev and with this PR (538635a):

Format written by dev	Read by dev	Read by this PR
YYMMDD10.	YYMMDD10.	READSTAT_ERROR_PARSE (whole file)
BEST.	BEST.	READSTAT_ERROR_PARSE (whole file)
YYMMDD10	YYMMDD10	YYMMDD10.
DOLLAR12.2	DOLLAR12.2	DOLLAR12.2

Any sas7bdat written through haven or pyreadstat with an ordinary SAS format string like DATE9. falls into the first group.

Suggested fix

Make the reader lenient for this legacy layout: if the stored name contains a . and width and digits are both 0, return the name unchanged instead of failing. That recovers exactly what the old writer was given. Keeping READSTAT_ERROR_PARSE for digits without a width seems reasonable, since no writer should ever have produced that.

A test covering this would also help, e.g. a small fixture file written by the current writer with a YYMMDD10. format, checked to read back as YYMMDD10..

Other notes

• The writer changes look good. Failing with READSTAT_ERROR_BAD_FORMAT_STRING is a clear improvement over silently writing a file SAS can't open.
• name[33] for 32-character names and the snprintf truncation check both look correct.
• Always returning a trailing dot on read is a visible behavior change for downstream packages that match format names exactly. It's consistent with XPT, but worth noting in the release notes.

[hpoettker]

Thanks for the detailled feedback! Much appreciated!

I've made the reader more lenient as requested, and rebased on dev.

I've not added the requested test fixture as the project previously has been quite judicious about adding static files to the test framework.

And I think the lean testing approach based on round-trips has been spectacularly validated by the very minor fixes that are now required to fully build out the sas7bdat write feature after the breakthrough of #398.

[evanmiller]

Thanks!