Repository navigation
feat(writing): size a string attribute to its own value - #179
Conversation
7041a7f to
e56748f
Compare
|
The build failure is by the way exactly the bug fixed in the other PR :) |
ab7a4c8 to
259a424
Compare
|
Thanks for this PR. Another nice improvement :-) I have applied minor changes, please review commit 0334b9e and tell me if you have doubts. Please see the commit message for the reasoning for the changes. Apart from that I have trouble reviewing the changes because the changed files contain many PR-unrelated changes like white space removals, var -> named type changes or the change from named parameter to positional parameter. Here is an example where none of the changes are related to this PRs goal:
Please remove unrelated changes if possible :-) Thank you! |
|
One thing that also comes into my mind is, now with |
Encoding runs through reflection, so an exception raised while encoding reached the caller wrapped in TargetInvocationException, reporting "Exception has been thrown by the target of an invocation" rather than what was wrong with the value. It is now rethrown unwrapped, with its original stack preserved. BREAKING CHANGE: an encoding failure surfaces as the exception the encoder raised rather than a TargetInvocationException wrapping it. Co-Authored-By: Claude <noreply@anthropic.com>
A fixed-length string attribute's width came from DefaultStringLength, which is file-global: it has to be as wide as the widest attribute anywhere in the file, and every narrower attribute is then padded out to it. It was also the only defence against data loss, since anything wider is silently truncated - by byte, so a multi-byte character can be cut in half. A string attribute is now measured from its own value. AttributeStringLength decides that independently of DefaultStringLength, so the two can disagree: Measured (the default), Inherit or VariableLength. An attribute holding a null element stays variable-length, that being the one value a fixed-length field cannot represent, so measuring never loses anything. A measured width declares NullPad. NullTerminate reserves the field's final byte and the C library honours that when converting a value in, so a tool rewriting the attribute through a wider datatype would drop the last character of the longest element - the very value the width was measured from. Reads are unaffected either way: there is no conversion path between fixed- and variable-length strings in the C library, so a consumer reads the field into a fixed-width buffer and finds the same bytes under both declarations. Datasets and compound members keep caller-declared widths, including the string members of a compound attribute. For those, StringOverflow opts into failing the write rather than truncating, and a null is refused instead of crashing inside the UTF-8 encoder. BREAKING CHANGE: a string attribute is fixed-length and sized to its own value whatever DefaultStringLength says, where a zero DefaultStringLength previously left it variable-length. Set AttributeStringLength to Inherit for the old behaviour. Co-Authored-By: Claude <noreply@anthropic.com>
… vlen padding Three changes to the attribute string-width feature introduced in 259a424: 1. Move AttributeStringLength and StringOverflow from init-only body properties to primary constructor parameters on H5WriteOptions. The original commit placed them in the body to avoid a binary-breaking signature change, but the two are options like the rest and belong with them; the consolidated signature is simpler to discover and document. The body properties and their "deliberately in the body" comment are removed; their remarks merge into the param doc tags. 2. Rewrite GetStringLengthForAttribute (AttributeMessage.Writing.cs) as a single switch expression over the mode, replacing three if/else-if chains. The Measured arm delegates to a new static local function MeasureStringWidth, which scans the attribute elements and returns (maxWidth, NullPad) or, if any element is null, (0, default) to fall back to variable-length. An unknown mode now throws with the value. 3. Fix variable-length string padding: DatatypeMessage.Writing.cs wrote NullPad where the HDF5 C library uses NullTerminate (H5T_STR_NULLTERM, H5T.c:380). For vlen strings padding is purely semantic metadata on disk (bits 4-7 of the class bit field) and never affects the stored data bytes, so only the on-disk type declaration changes. The eight affected dump files are updated (H5T_STR_NULLPAD -> H5T_STR_NULLTERM where STRSIZE is H5T_VARIABLE). Documentation (data-types.md) clarifies that a null into a fixed-length string and a StringOverflow.Throw both raise InvalidOperationException (not a vague "refused"/"fail"), and links the Measured null-element fallback to avoiding that exception. Tests: AttributeTests@datatypes.cs uses named constructor args after the signature change. BREAKING CHANGE: H5WriteOptions gains two constructor parameters (AttributeStringLength, StringOverflow). Code using positional arguments beyond DefaultStringLength must switch to named arguments.
…ze limit An object header message declares its own size in two bytes, so 65535 is the most an attribute message can encode to. The guard checked the data alone against 64 KB, which is neither the right budget nor the right ceiling: the name, the datatype and the dataspace are counted in the same two bytes, and a data size of exactly 65536 passed the > comparison and then became 0 as a ushort. Sizes inside that gap wrapped, and ObjectHeader2.Encode allocates the object header from them before writing the messages at their real length - so the header overran its own allocation and the object stopped being readable altogether, taking the group's datasets and other attributes with it. h5dump rejected the file, and one width sent PureHDF's own reader into unbounded recursion. The size is now computed once, exactly, in a width that cannot wrap, and the limit is tested against the whole message. Reachable without strings at all: an int[16378] attribute is 65512 bytes of data and was silently unreadable. Co-Authored-By: Claude <noreply@anthropic.com>
… not fit A measured width is stored inline, so it competes with the attribute's name, datatype and dataspace for the 65535 bytes an attribute message can declare. That made a string attribute larger than about 64 KB fail, where under a zero DefaultStringLength it used to be variable-length and write at any size - the value living in the global heap for a fixed 16 bytes per element. Measuring is only taken where it costs nothing, which is why a null element already hands the attribute back to variable-length. A width that does not fit is the same case, so it falls back the same way, and the size a value used to write at keeps working. A width from DefaultStringLength is left alone: it was asked for explicitly, so it reports the limit rather than silently writing a different datatype than the caller declared. Datasets and compound members are untouched. Co-Authored-By: Claude <noreply@anthropic.com>
0334b9e to
10923b2
Compare
|
Thanks — both points addressed, and the object header question turned out to be worth asking. Rebased onto current The unrelated changes are goneThe reformatting came from auto-formatting tooling and is removed. The feature commit is rebuilt rather than patched, so none of it is in the history any more. Across the PR, deleted lines are down from 557 to 43, and every one of the 43 is a line the feature actually replaces. Your commit is applied unchanged apart from two conflict resolutions, both keeping the file's existing style over the reformatting: You were right about the size limit, and there was a bug underneath itAn attribute message declares its own size in a two-byte field, so 65535 is the ceiling for the whole message.
Sizes in the resulting gap wrapped, and This predates measured widths entirely —
Worth checking against the C library, which enforces the same rule: at default The one thing that was a genuine regressionMeasuring stores the value inline, so it competes for those 65535 bytes. A string attribute over roughly 64 KB therefore failed — where under a zero
Two boundaries on that, both deliberate and both worth your eye:
|
|
I found one scalar-null edge case that is not covered by the current tests. The PR says measured string attributes fall back to variable-length when they contain Reproduction: using PureHDF;
var file = new H5File();
file.Attributes["text"] = new H5Attribute<string?>((string?)null);
using var stream = new MemoryStream();
file.Write(stream);Current result: The reason appears to be that Can you suggest how you would prefer to solve this? A narrow fix would be to preserve scalar null string attributes as one null element before measuring, so the existing null-element fallback chooses variable-length. But this may also point to a broader question around how scalar null reference-type attributes should be represented generally. |
|
Thanks — and the broader question you raise is the right one to ask, because the answer turned out to settle the narrow case too. Two things before the fix itself. This is not caused by this PR: the same exception, at the same call site, reproduces on new H5Attribute<string?>((string?)null) // IndexOutOfRangeException
new H5Attribute<string>() // IndexOutOfRangeException
new H5Attribute<int>() // IndexOutOfRangeException
new H5Attribute<int[]>() // fine
A null dataspace should carry no data. The size was derived from the dataspace dimensions, which a null dataspace has none of, and the empty product is one — so one element's worth was reserved and then asked for. The C library writes nothing here whatever the datatype: its attribute messages are 34 bytes for a null-dataspace A scalar null is one element that is null. This is your broader question, and the C library answers it: Both are fixed in #186, off One incidental thing, since it cost me a test: |

A fixed-length string attribute's width came from
H5WriteOptions.DefaultStringLength, which is file-global: it has to be as wide as the widest attribute anywhere in the file, and every other attribute is then padded out to it. On a real 611 MB report that padding was over half of the file's 42 MB of metadata.It is also the only defence against data loss, because the encoder silently truncates anything wider — by byte, so a multi-byte character can be cut in half.
A width measured from the value cannot truncate it, and pads only out to the attribute's own longest value rather than to the file's widest — nothing at all for a scalar. So that is what a string attribute now gets.
The width is the attribute's own business
DefaultStringLengthwas doing two unrelated jobs for attributes: switching them to fixed-length, and then being ignored as the width. That left combinations unreachable — variable-length datasets alongside measured attributes, or a declared compound width alongside variable-length attributes.H5WriteOptions.AttributeStringLengthnow answers for attributes on its own:DefaultStringLength = 0DefaultStringLength = 32Measured(default)InheritVariableLengthDatasets are deliberately unchanged
A compound member's width has to cover every row and stay uniform across the tables sharing that type, so it stays caller-declared — and that includes the string members of a compound attribute, whatever
AttributeStringLengthsays. For those, the newH5WriteOptions.StringOverflowopts into failing the write instead of truncating:Truncateremains the default there, so nothing changes for declared widths unless asked.Encoding also now rethrows unwrapped rather than as
TargetInvocationException, so a rejected value reports why instead of "Exception has been thrown by the target of an invocation".Nulls
A fixed-length field has no way to hold the difference between
nulland"", so measuring steps back rather than losing it: an attribute holding a null element stays variable-length. That is what keeps the argument for measuring intact — it never loses a value.At a declared width there is nothing to step back to, and a null is already a crash on
dev:Encoding.UTF8.GetBytes((string)source)throwsArgumentNullException, for a dataset, a compound member and an attribute alike. Nothing caught it because the fixed-length test data has""but nonull. It is now refused with a message that says what to do instead.That is the one place this PR reaches datasets — a crash becoming a clear error rather than a change of behaviour, but worth calling out against the section above.
Worth flagging
This changes default behaviour for attributes.
DefaultStringLengthno longer governs their width at all, so files written with default options carry fixed-length attribute datatypes where they previously carried variable-length ones.AttributeStringLength.Inheritrestores the old behaviour exactly.ThePaddingWrittenToTheFileIsZerofrom fix(writing): zero the padding of a fixed-length string #177 now asserts through a compound member rather than a string attribute. A measured attribute has no padding to inspect, and the compound path is where a width is still declared — so the byte-level assertion still covers the pooled-buffer branch that test was written for.A measured width declares
H5T_STR_NULLPAD, notNULLTERM.NULLTERMreserves the field's final byte, and the C library honours that when converting a value in, so a tool rewriting the attribute through a wider datatype landswidth - 1bytes. A measured width is filled to its last byte by its longest element by construction, soNULLTERMwould put exactly that element at risk. Measured against the C library on a PureHDF-written attribute:NULLTERM7b'Wafer\x00\x00',b'Reticle'b'Wafer\x00\x00',b'Reticl\x00'NULLPAD7b'Wafer\x00\x00',b'Reticle'b'Wafer\x00\x00',b'Reticle'The declaration does not affect reads. There is no conversion path between fixed- and variable-length strings in the C library at all, so every consumer reads the field into a fixed-width buffer, and those bytes are identical under both declarations —
H5Areadthrough the file's own type givesb'Wafer\x00\x00'withstrlen5 either way, and h5py and PureHDF both returnWafer.The cost is that
h5dumphonoursNULLPADliterally and prints a shorter value with its padding attached, so the attribute dump fixtures show"ABCDEF\000\000\000\000"where they showed"ABCDEF". That is a rendering difference in one tool, not something a reader sees — and the alternative was silently truncating the longest value of every mixed-length attribute.Compound members are unaffected: they pass no measured width and keep
NULLTERM.The datatype cache key gains the padding. It is keyed by everything the message depends on (fix(writing): key the datatype cache by everything the message depends on #173), and an attribute measured at six bytes now differs from a compound member declared at six bytes. Without the extra key component the member silently inherits whichever padding was encoded first; there is a test that fails if it is removed.
The two global-heap tests in
MiscTestsnow ask for variable-length attributes explicitly. They filled a collection by writing string attributes under default options, which measuring empties out —AttributeStringLength.VariableLengthis what those tests are actually about.AttributeStringLengthTestscovers the three modes, the compound-member carve-out and the null fallback;AttributeStringWidthTestscovers the measured widths, the padding and overflow.CanWrite_Commonprefers anattr_dump fixture for the cases where an attribute genuinely differs from the dataset, and shares the dataset fixtures otherwise.doc/writing/data-types.mdgains aStringssection —DefaultStringLengthandStringOverflowwere not documented at all.Overlaps feat(writing): keep file structure together, opt-in per placement #180 only in
H5WriteOptions, where both add a property to the record body; whichever lands second takes a one-line conflict.