Fix check offsets

[benjeffery]

Fixes #1509

Turns out this was wrong in every call to check_offset_overflow.

Tested by twiddling the internal state of the table struct. We could probably make large columns, but this seemed simpler.

[codecov]

Codecov Report

Merging #1511 (73f1a67) into main (2c897a3) will increase coverage by 0.04%.
The diff coverage is 97.95%.

@@            Coverage Diff             @@
##             main    #1511      +/-   ##
==========================================
+ Coverage   93.70%   93.74%   +0.04%     
==========================================
  Files          27       27              
  Lines       22759    22620     -139     
  Branches     1076     1076              
==========================================
- Hits        21326    21205     -121     
+ Misses       1399     1381      -18     
  Partials       34       34              

Flag	Coverage Δ
c-tests	92.09% <97.95%> (+0.06%)	⬆️
lwt-tests	92.97% <ø> (ø)
python-c-tests	95.29% <ø> (ø)
python-tests	98.80% <ø> (ø)

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

Impacted Files	Coverage Δ
c/tskit/tables.c	90.23% <97.95%> (+0.07%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2c897a3...73f1a67. Read the comment docs.

[jeromekelleher]

I think it's probably worth abstracting out an expand_ragged_column here which tsk_individual_table_expand_parents etc could call, as there's a lot of duplicated code which we could get rid of by using a few pointers. Otherwise we'll be making the same changes a dozen or more times here.

I guess the call would look something like

static int
tsk_individual_table_expand_parents(
    tsk_individual_table_t *self, tsk_size_t additional_length)
{
    return expand_ragged_column(self->max_parents_length_increment, &self->max_parents_length,
        &self->parents);
}

[benjeffery]

Yep, will do that. Also reduces test duplication which was getting boring!

[jeromekelleher]

What's the status of this @benjeffery? I think it's worth adding the expand_ragged_column infrastructure, as it'll be handy in anycase for doing things like setting individual rows.

[benjeffery]

It's waiting for me to get back around to it - finishing up an tsconvert PR then will get to it today.

[benjeffery]

Ok, after some head-scratching this is done. A few queries:

• See the comment in expand_ragged_columns. Instead of erroring when max_length+increment overflow we could/should just allocate to the max size - however this can't practically be tested.
• All the tsk_TABLE_table_expand_RAGGEDCOLUMN functions are now one-liners that could be removed - I kept them in as expand_ragged_column has 6 arguments and the existing calls look cleaner in the higher level methods.
• Shouldn't tsk_TABLE_table_set_max_COLUMN_length_increment be tsk_TABLE_table_set_min_COLUMN_length_increment??!!

[jeromekelleher]

LGTM, just need to double-check we're getting all the ragged columns.

[jeromekelleher]

See the comment in expand_ragged_columns. Instead of erroring when max_length+increment overflow we could/should just allocate to the max size - however this can't practically be tested.

Yeah - let's not worry about it though. I guess it'll be become more pressing at some point when we implement the doubling behaviour (#5), when increment will become large quickly. I think it's fine for now, though.

All the tsk_TABLE_table_expand_RAGGEDCOLUMN functions are now one-liners that could be removed - I kept them in as expand_ragged_column has 6 arguments and the existing calls look cleaner in the higher level methods.

Yeah, let's keep then. More readable this way as you say.

Shouldn't tsk_TABLE_table_set_max_COLUMN_length_increment be tsk_TABLE_table_set_min_COLUMN_length_increment??!!

I guess tsk_TABLE_table_set_COLUMN_length_increment would be better again?

[benjeffery]

Shouldn't tsk_TABLE_table_set_max_COLUMN_length_increment be tsk_TABLE_table_set_min_COLUMN_length_increment??!!

I guess tsk_TABLE_table_set_COLUMN_length_increment would be better again?

Yes, it is a public method, but undocumented so we could change it. However if #5 is planned then not worth it as it will be obsolete then.

[jeromekelleher]

However if #5 is planned then not worth it as it will be obsolete then.

I think the idea for #5 is to support both the current behaviour, and also doubling. If size_increment == 0, then increment = 2 * len(table); otherwise, increment = size_increment. So, we can do it simply enough in the current framework, while keeping the flexibility.

Probably a single size_increment per table is sufficient, having different increments for the number of rows and the ragged data columns is overboard.

[benjeffery]

@jeromekelleher I think this is ready to go

[jeromekelleher]

This needs some attention @benjeffery - some compiler warnings popping up