Extensible fragment metadata footer. #5686
Conversation
teo-tsirpanis
force-pushed
the
teo/core-415-implement-extensible-fragment-metadata-footer
branch
from
9d3c0ec to
17a1a24
Compare
| case DataDirectoryIdentifier::tile_global_order_offsets: | ||
| if (data_size != 2 * array_schema_->dim_num() * sizeof(uint64_t)) { | ||
| throw FragmentMetadataStatusException( | ||
| "Invalid size of tile global order bound offsets data directory"); |
There was a problem hiding this comment.
Let's add expected/found here so we can potentially cut a round trip out of the bug report.
There was a problem hiding this comment.
This error would happen only if the fragment metadata file was corrupted. If somebody got it, just the expected/found sizes would not help us with diagnosing it, and we would need the whole array and/or a repro either way.
| serializer.write(DataDirectoryIdentifier::tile_global_order_offsets); | ||
| const auto num_dims = array_schema_->dim_num(); | ||
| serializer.write<uint32_t>(2 * num_dims * sizeof(uint64_t)); | ||
|
|
There was a problem hiding this comment.
| iassert(gt_offsets_.tile_global_order_min_offsets_.size() == 2 * num_dims); | |
| iassert(gt_offsets_.tile_global_order_max_offsets_.size() == 2 * num_dims); |
and then pass the size() value to write instead
| } | ||
|
|
||
| /** has_tile_global_order_bounds accessor */ | ||
| bool& has_tile_global_order_bounds() { |
There was a problem hiding this comment.
This looks so odd... I see it's because the serializer requires friend access without being a friend. I've seen similarly odd non-const accessors in other places. I feel like that was the wrong paradigm to use.
I'm not suggesting anything actionable here, just musing
rroelke
left a comment
rroelke
left a comment
There was a problem hiding this comment.
These changes look good but I am still having a hard time with the terminology "data directory" and I do think we should pick something else. And yet this is why naming things is the second hard thing in computer science.
What comes to mind is "semi-structured data". This work turns the fragment metadata footer from a highly structured binary file to a semi-structured one.
Fragment metadata can contain semi-structured attributes.
Or "self-describing data" where the entries have a key that tells you what they are. This feels more apt.
Fragment metadata footers can contain self-describing attributes.
Or even just optional, since all fields are optional going forward with this design:
Fragment metadata footers can contain any number of optional attributes which provide additional metadata.
teo-tsirpanis
force-pushed
the
teo/core-415-implement-extensible-fragment-metadata-footer
branch
from
63e346f to
d26a8af
Compare
teo-tsirpanis
left a comment
teo-tsirpanis
left a comment
There was a problem hiding this comment.
Renamed "data directory" to "optional section".
| serializer.write(DataDirectoryIdentifier::tile_global_order_offsets); | ||
| const auto num_dims = array_schema_->dim_num(); | ||
| serializer.write<uint32_t>(2 * num_dims * sizeof(uint64_t)); | ||
|
|
|
Merging per offline discussion… |
teo-tsirpanis
merged commit eb415af
into
rr/core-321-fragment-metdata-global-order-bounds
teo-tsirpanis
deleted the
teo/core-415-implement-extensible-fragment-metadata-footer
branch
2 participants
This PR builds on top of #5646, and updates the fragment metadata format specification to an extensible design that will not require another versin bump for future additions.
TYPE: NO_HISTORY