Fix issue #79: handle deleted observations and last-page type variants

[tk3369]

$(cat <<'EOF'

Summary

Fixes #79 — SAS datasets with logically deleted observations (or with a composite index / primary key) were returning wrong row counts or failing to read entirely.

Two root causes were identified and fixed:

• Wrong page types ignored: When a SAS dataset has deleted observations, SAS sets bit 7 (0x80) on the last page of each page-type group, producing page type 128 (0x080) for the last meta/compressed-data page and 384 (0x180) for the last uncompressed-data page. Neither was recognised by the reader, so those pages were silently skipped, causing missing rows.

• NOBS vs NLOBS confusion: The row-size subheader stores both the physical row count (NOBS, offset 6×int_len) and the deleted-observation count (DELOBS, offset 7×int_len). The reader was using NOBS for iteration, which caused over-allocation and included deleted rows in the output. The fix reads DELOBS and subtracts it to get the logical row count NLOBS = NOBS - DELOBS.

Changes

• src/constants.jl: add page_meta_type_last = 128, page_data_type_last = 384, update page_meta_data_mix_types to include both, add deleted_row_count_offset_multiplier = 7
• src/Types.jl: add deleted_row_count::Int64 field to Handler
• src/SASLib.jl:
  • _process_rowsize_subheader: read DELOBS; set row_count = NOBS - DELOBS when deletions exist
  • _process_page_meta: include page_meta_type_last in metadata scan
  • _read_next_page_content: call _process_page_metadata for page type 128
  • readline: dispatch page types 128 and 384 to their respective handlers
  • page_type_str: add "META_LAST" and "DATA_LAST" debug labels
• test/runtests.jl: new test block for issue Compressed dataset with index or primary key confuses dataset reader #79 (guarded by isdir("data_issue79") so CI passes without binary test data; files can be downloaded from the issue attachment)

Test plan

• Place the 6 .sas7bdat files from the issue Compressed dataset with index or primary key confuses dataset reader #79 attachment into test/data_issue79/
• Run julia --project=. test/runtests.jl — the new "deleted observations (issue #79)" test set should pass
• comp_ix.sas7bdat, comp_pk.sas7bdat → 2000 rows each
• nocomp_deletes.sas7bdat, nocomp_pk_deletes.sas7bdat, comp_deletes.sas7bdat, comp_pk_deletes.sas7bdat → 1990 rows each
• nocomp_deletes.sas7bdat[:rowno] sorted values run 1.0 … 1990.0 with no zeros

https://claude.ai/code/session_01ABxRa4ryWnCzjsNiNvPova
EOF
)

---

Generated by Claude Code

[tk3369]

@claude CI failed. Please fix.

[claude]

Claude finished @tk3369's task in 1m 30s —— View job

---

Fixing CI Failure

• Check CI failure details
• Identify root cause
• Fix the issue
• Push changes

Root cause: test/runtests.jl:487 used @test expr "message" — Julia's @test macro does not support trailing string messages; that syntax is invalid and raises a LoadError at parse time.

Fix: Replaced the invalid call with a @testset "$fname" begin @test ... end wrapper, which is the idiomatic way to surface the filename in test output on failure.

# Before (invalid)
@test size(rs, 1) == 1990  "$fname: expected 1990 rows, got $(size(rs,1))"

# After
@testset "$fname" begin
    @test size(rs, 1) == 1990
end

Commit: 1eff1e2 — pushed to claude/review-open-issues-vJIz4.
|

[tk3369]

@claude trigger claude code review

[claude]

Claude finished @tk3369's task in 2m 21s —— View job

---

Code Review — Fix issue #79: deleted observations and last-page type variants

• Gather context and understand the request
• Read CLAUDE.md for repo guidelines (none present)
• Review src/constants.jl changes
• Review src/Types.jl changes
• Review src/SASLib.jl changes
• Review test/runtests.jl changes
• Post comprehensive review feedback

---

The overall approach is correct and well-scoped. Both root causes (unknown last-page type bits and NOBS vs NLOBS confusion) are identified and fixed with minimal, targeted changes. A few points are worth discussing before merge.

---

Issues

1. Edge case: all rows deleted — row_count is not reduced to zero

src/SASLib.jl:613

if handler.deleted_row_count > 0 && handler.deleted_row_count < handler.row_count
    handler.row_count -= handler.deleted_row_count
end

The guard deleted_row_count < row_count is intended to prevent underflow, but it also means the condition is false when every row is deleted (deleted_row_count == row_count). In that edge case row_count is left at NOBS and all physically-deleted slots are returned to the caller. A safer guard is <= row_count:

if handler.deleted_row_count > 0 && handler.deleted_row_count <= handler.row_count
    handler.row_count -= handler.deleted_row_count
end

An all-deleted file is rare in practice, but the fix is a one-character change. Fix this →

---

2. mix_page_row_count is compared against the adjusted row_count

src/SASLib.jl:1162

mn = min(handler.row_count, handler.mix_page_row_count)

mix_page_row_count is a per-page count read from the subheader, while row_count (after this fix) is a global logical total. If a file uses MIX pages and has deletions, and the adjusted row_count happens to be smaller than mix_page_row_count, the page flip triggers early and rows are silently truncated. This comparison was arguably wrong before the fix too, but the adjustment makes the mismatch more likely. If the test suite doesn't include a MIX-pages-with-deletions file, this may remain a latent bug.

This does not block the fix for the common cases — just noting it for awareness.

---

3. Offset 7 for DELOBS is undocumented in the source

src/constants.jl:80

const deleted_row_count_offset_multiplier = 7

All other *_offset_multiplier constants lack references too, so this is consistent with the rest of the codebase. But a brief comment pointing to the offset derivation (e.g. the parso/ReadStat project or the issue itself) would help future maintainers verify this value. Not a blocker.

---

Minor Observations

4. The isdir guard is now redundant for CI

test/runtests.jl:473

if isdir("data_issue79")

Since all six .sas7bdat binary files were committed alongside the test, data_issue79/ is always present in the checkout and the guard is always true. The guard makes sense if the intent is that contributors without the binary files can still run the suite (the CI fix commit message mentions this). If the files are permanently committed, the guard can be removed; if they might be removed later, keep it — but then the skipped block should probably emit a @warn so developers know tests were skipped.

5. Nested @testset inside a loop reads oddly

test/runtests.jl:487-489

@testset "$fname" begin
    @test size(rs, 1) == 1990
end

This is correct Julia and shows the filename in failure output — the right fix. No action needed, just confirming the intent.

---

Positive Aspects

• The two-root-cause diagnosis is accurate and the fixes directly address both.
• Adding both new page types to page_meta_data_mix_types is the right place to prevent the skip-unknown-page recursion from triggering.
• The page_type_str debug helper is kept in sync.
• The Handler partial constructor is safe here: deleted_row_count is always written in _process_rowsize_subheader before it is read back at line 613.
• Test data committed to the repo means CI exercises the real binary format, which is the only reliable way to verify this kind of low-level fix.

---

Summary: The fix is sound. Issue 1 above (<= vs < guard) is the only correctness concern that could plausibly affect a real file, and it is a trivial one-character change. Issue 2 (MIX pages + deletions) is pre-existing and out of scope for this PR. Everything else is minor polish.

[tk3369]

@claude go ahead and fix the issues that you found from code review.

[claude]

Claude finished @tk3369's task in 1m 32s —— View job

---

Code Review Fixes Applied

• Gather context and read relevant files
• Fix Issue 1: <= guard in src/SASLib.jl for all-rows-deleted edge case
• Fix Issue 3: Add comment for deleted_row_count_offset_multiplier in src/constants.jl
• Fix Issue 4: Add @warn when data_issue79/ directory not found in test/runtests.jl
• Commit and push changes (317b000)

---

Three changes made in commit 317b000:

src/SASLib.jl:613 — Changed < to <= so an all-rows-deleted file correctly produces row_count == 0 instead of silently returning NOBS deleted rows:

# Before
if handler.deleted_row_count > 0 && handler.deleted_row_count < handler.row_count

# After
if handler.deleted_row_count > 0 && handler.deleted_row_count <= handler.row_count

src/constants.jl:80 — Added inline comment referencing the source of offset 7 for the DELOBS field:

const deleted_row_count_offset_multiplier = 7  # DELOBS field; see SAS internal format / ReadStat row-size subheader layout

test/runtests.jl — Added else @warn so developers know when issue #79 tests are being skipped rather than silently passing:

    else
        @warn "Skipping issue #79 tests: test/data_issue79/ not found (download files from issue #79 attachment)"
    end

Issue 2 (MIX pages + deletions) was pre-existing and out of scope for this PR — left as-is.
| Branch