Skip to content

Conversation

jeromekelleher
Copy link
Member

Stacked on #1552

This adds support for either 32 or 64 bit offsets in the file format and a new flag for dump that forces offsets to be 64 bit. This is an intermediate step towards having 64 bit offsets in memory. The TSK_DUMP_FORCE_OFFSET64 will be useful because it will allow us to test that we are loading correctly from 64 bit offset arrays in the kastore without needed to create huge metadata arrays.

@jeromekelleher jeromekelleher force-pushed the variable-size-offsets branch from 100e647 to ad02ac7 Compare July 19, 2021 13:02
@jeromekelleher
Copy link
Member Author

I think this is ready for review @benjeffery - it doesn't affect any existing behaviour, so should be OK to merge I think.

@jeromekelleher
Copy link
Member Author

Ah, crap, the 64 bit BIG_TABLE tests aren't going to work. I think it's probably simplest to disable these for a few days until the follow-ups can be done @benjeffery? Otherwise, we'll end up having to implement full change in this PR I think.

@jeromekelleher
Copy link
Member Author

I've added a commit to disable these tests - if you agree with this course of action @benjeffery we should open an issue so we don't forget to turn them back on again.

@jeromekelleher jeromekelleher force-pushed the variable-size-offsets branch from 7fb0653 to 7677b2b Compare July 19, 2021 14:25
@codecov
Copy link

codecov bot commented Jul 19, 2021

Codecov Report

Merging #1571 (c9b0a6a) into main (d9a9da4) will decrease coverage by 0.01%.
The diff coverage is 91.95%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1571      +/-   ##
==========================================
- Coverage   93.61%   93.60%   -0.02%     
==========================================
  Files          27       27              
  Lines       22982    23033      +51     
  Branches     1079     1079              
==========================================
+ Hits        21514    21559      +45     
- Misses       1434     1440       +6     
  Partials       34       34              
Flag Coverage Δ
c-tests 91.85% <91.95%> (-0.02%) ⬇️
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 89.90% <91.95%> (-0.02%) ⬇️

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 d9a9da4...c9b0a6a. Read the comment docs.

Copy link
Member

@benjeffery benjeffery left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, Seems a good idea to do this incrementally.

@jeromekelleher jeromekelleher force-pushed the variable-size-offsets branch from 7677b2b to c9b0a6a Compare July 21, 2021 14:27
@mergify mergify bot merged commit 3e427c2 into tskit-dev:main Jul 21, 2021
@jeromekelleher jeromekelleher deleted the variable-size-offsets branch August 16, 2021 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants