Pin ReadStat_jll to 1.1.9, raise Julia floor to 1.10, fix wrapper defects - #95
Merged
Merged
Conversation
The old bounds did not actually deliver the latest binary. ReadStat_jll 1.1.8 and newer require julia 1.6, but our julia compat floor was 1.3 and our ReadStat_jll bound was "1.1.1" (i.e. [1.1.1, 2.0)). On julia 1.3-1.5 the resolver therefore fell back to ReadStat_jll 1.1.5, shipping the December 2020 readstat build rather than 1.1.9. Pin ReadStat_jll to "1.1.9" and raise the julia floor to 1.10, matching Query.jl and ReadStatTables.jl. Raising the floor is breaking for old-julia users, so bump the version to 1.2.0-DEV. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
get_alignment called readstat_variable_get_alignment's neighbour, readstat_variable_get_measure, so ReadStatDataFrame.alignments was silently a copy of .measures. Every fixture reports measure UNKNOWN, so the .dta and .xpt files were reporting all-zero alignments where readstat actually says RIGHT. The correct accessor was already wrapped in C_interface.jl but never called. The two get_type methods also disagreed about READSTAT_TYPE_STRING_REF (6): get_type(::Value) mapped it to String, while get_type(::Cint) had no branch for it and fell through to Nothing. That disagreement is not reachable today -- the dta reader rewrites STRING_REF to STRING both when it initialises the variable and when it emits each value, and no other reader mentions the type at all -- but it is worth removing rather than leaving as a trap. Both methods now share one lookup table, which is also where the enum constants pick up their modern spelling: upstream renamed CHAR to INT8 and LONG_STRING to STRING_REF, leaving the values unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The four readstat_set_*_handler calls declared a return type of Int, which is 64 bits, where C returns readstat_error_t, a 4-byte enum. The values are discarded, so this was harmless in practice, but the declaration was wrong. readstat_get_modified_time returns time_t, which is signed. Declaring it UInt turned any pre-1970 timestamp into a huge positive number that unix2datetime then choked on. handle_info! was compiled into a cfunction on every parse and then never registered: readstat dropped readstat_set_info_handler long ago, and the metadata handler already sets rows and columns. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Pins the readstat_alignment_t values each fixture actually carries. The .dta and .xpt files report RIGHT (and LEFT for the xpt string column) while every fixture reports measure UNKNOWN, so reading the wrong accessor yields all zeros and fails these assertions. Verified by reverting the fix: the test item fails, and passes again once restored. Also assert no column type comes back as Nothing, which is what a gap in the readstat_type_t mapping would look like. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Aug 31, 2026
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #95 +/- ##
==========================================
+ Coverage 79.66% 83.33% +3.67%
==========================================
Files 2 2
Lines 177 156 -21
==========================================
- Hits 141 130 -11
+ Misses 36 26 -10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Audit of what binary we actually ship, plus the wrapper defects it turned up.
We were not shipping the latest binary
ReadStat_jll1.1.8 and newer require julia 1.6, but our julia floor was 1.3 and our bound wasReadStat_jll = "1.1.1"(i.e.[1.1.1, 2.0)). On julia 1.3-1.5 the resolver therefore fell back to ReadStat_jll 1.1.5, shipping the December 2020 readstat build instead of 1.1.9.Pinned to
"1.1.9"withjulia = "1.10", matching Query.jl and ReadStatTables.jl. Raising the floor is breaking for old-julia users, so the version goes to1.2.0-DEV.For the record on the binary itself: 1.1.9 (2023-02-20) is still upstream's newest release, so we are now on the latest tag that exists. Upstream
devis 58 commits ahead and most other bindings (pyreadstat, haven, readstat-rs) have moved to pinneddevcommits, but this PR deliberately stays on the released tag. Asking upstream to tag is being handled separately on WizardMac/ReadStat#343.Wrapper defects
get_alignmentcalledreadstat_variable_get_measure, soReadStatDataFrame.alignmentswas silently a copy of.measures. Every fixture reports measure UNKNOWN, so.dtaand.xptwere reporting all-zero alignments where readstat actually says RIGHT. The correct accessor was already wrapped inC_interface.jlbut never called.get_typemethods disagreed aboutREADSTAT_TYPE_STRING_REF: one mapped it toString, the other fell through toNothing. Not reachable today - the dta reader rewrites STRING_REF to STRING before the handlers ever see it, and no other reader mentions the type - so this removes a trap rather than fixing a live bug. Both now share one lookup table, which also picks up the modern enum spelling: upstream renamed CHAR to INT8 and LONG_STRING to STRING_REF, values unchanged.ccallsignatures. The fourreadstat_set_*_handlercalls declaredInt(64-bit) where C returns a 4-byte enum; harmless only because the values are discarded.readstat_get_modified_timewas declaredUIntwhere C returns signedtime_t, so a pre-1970 timestamp became a huge positive number thatunix2datetimechoked on.handle_info!was compiled into a cfunction on every parse and then never registered - readstat droppedreadstat_set_info_handlerlong ago, and the metadata handler already sets rows and columns.Testing
The test item now pins the
readstat_alignment_tvalues each fixture carries. Verified the assertion is real by reverting the alignment fix: the item fails, and passes again once restored. Full suite green, and StatFiles' own suite (including its exact-stringShowitem) passes against this branch.Known gaps left as follow-up issues:
read_porhas no test or fixture, andread_xportis exported but StatFiles never wires.xptup.🤖 Generated with Claude Code