Json struct alignment

[petrelharp]

Edit: This doesn't really fix #3423 at all yet, since the important work is actually in the python codec. Working on that next.

Okay; here's the C side of things to do #3423. Missing yet is:

• the python side
• C documentation
• python documentation

However, I've got confused. Examining #3306 I see that the python code does not use the C function (tsk_json_struct_metadata_get_blob) at all, so we've got no good current way to test these against each other. Options that I can think of are:

1. remove the C code entirely
2. write an entirely new class of tests that reads in python-produced tree sequence files and then parses them in C and compares to what is expected
3. write a python method that uses the C function
4. not test that the C getter and the python getter do the same thing

However, (2) and (3) are ugly. (2) would break the nice clean one-way C->python arrangment we have. And (3) would leave us with two ways of doing the same thing. This leaves us with (1) or (4).

The motivation for putting the C code in here was that it's very annoying to write and it would help client code a lot to have it available. However, the setter is arguably more useful than the getter (since one does not actually decode the metadata in C), so for (4) to make sense we'd still want to additionally write the setter. So, I'm leaning towards (1), out of laziness. We could perhaps document somewhere the example setter/getter code (copied from SLiM and here respectively), to make this easy for other client code, which was the goal.

Edit: doing (3) would I think just mean replacing these two lines:

            json_bytes = encoded[start : start + jlen]
            blob_bytes = encoded[start + jlen : start + jlen + blen]

with a call to the C function. (Those lines need to be adjusted to include padding, tho, btw.) That would be fine but also seems very awkward.

[codecov]

Codecov Report

❌ Patch coverage is 95.45455% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 91.91%. Comparing base (17bacda) to head (c88027e).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3437      +/-   ##
==========================================
- Coverage   91.92%   91.91%   -0.01%     
==========================================
  Files          37       37              
  Lines       32153    32170      +17     
  Branches     5143     5146       +3     
==========================================
+ Hits        29556    29570      +14     
- Misses       2264     2266       +2     
- Partials      333      334       +1     

Flag	Coverage Δ
C	82.70% <95.45%> (-0.01%)	⬇️
c-python	77.29% <0.00%> (-0.06%)	⬇️
python-tests	96.40% <ø> (ø)
python-tests-no-jit	33.22% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
Python API	98.69% <ø> (ø)
Python C interface	91.23% <ø> (ø)
C library	88.85% <95.45%> (-0.01%)	⬇️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[petrelharp]

Update: @bhaller tells me that

The problem with having a setter function is that it would require the client to prepare the JSON is a buffer, prepare the binary in a buffer, and then pass pointers to those buffers in to the setter function, which would set up the final aggregate buffer and copy the JSON and binary in.  That is a bad design for performance reasons – it copies all the metadata (which might be quite large), wasting time but even more importantly causing the high-water memory usage for the process to double.  I suppose tskit could provide a setter anyway, for simplicity for clients that don’t care about those performance issues; but SLiM wouldn’t use it, probably.  If having the setter would make your life easier for the testing, I’d have no objection to adding it as long as there is no assertion that clients have to use the setter; the format used by the codec should remain public, and clients should remain free to write their data themselves.

[petrelharp]

After some more discussion with @bhaller:

Why do we want support for JSON+struct? We want to have support for JSON+struct metadata because it is extremely useful. The reason it is extremely useful is that sometimes client code will want to store large amounts of information that doesn't fit in the standard table format for some reason, and this is a very clever (thanks @benjeffery) way to do that, that is way more seamless than other alternatives.

Why have support for it at all in C? Other metadata parsing stuff is not in C at all, for good reason (we don't want to write or import a JSON parser or a struct parser, etcetera). However, the json and struct codecs are pretty straightforward for client C code to do themselves; this codec is annoying enough to set up (special header with magic bytes, etc) that it does make sense to provide a bit of help.

Well then, why don't we have a setter in C, then? I think that #3306 intended to provide a setter, but this fell through the cracks. So, I think it would be a good thing to have for the same reason as in the previous paragraph. However, I can't currently justify the time to actually impmlement and test it. (If we did provide a setter, then (with the note by @bhaller about memory requirements above in mind) we might want to have an API return a pointer to the binary metadata portion, so that the client code could fill it in, rather than passing in the pre-assembled binary portion, thus requiring an extra copy.)

So, I'm leaning towards option (4), "leave things the way they are". This is not perfect because (a) no testing that the C and python do the same thing, and (b) no setter in C; however, what we have is useful and well-tested, and furthermore, if the C and python were in disagreement we'd find out in obvious ways immediately.

[bhaller]

Sounds good, with the caveat that "leave things the way they are" still needs the alignment padding bytes to be added in the python code as we talked about, yes?