[ntuple] Fix serialization when a header extension is already present #17597
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
silverweed
force-pushed
the
ntuple_desc_schema
branch
from
b8908f5 to
d0ecd0a
Compare
This was referenced Feb 3, 2025
silverweed
force-pushed
the
ntuple_desc_schema
branch
from
d0ecd0a to
3cc778c
Compare
Test Results 18 files 18 suites 4d 4h 11m 36s ⏱️ For more details on these failures, see this check. Results for commit 95b11a3. ♻️ This comment has been updated with latest results. |
2 tasks
hahnjo
requested changes
Feb 4, 2025
silverweed
force-pushed
the
ntuple_desc_schema
branch
3 times, most recently
from
c641310 to
95b11a3
Compare
jblomer
requested changes
Feb 6, 2025
silverweed
force-pushed
the
ntuple_desc_schema
branch
from
95b11a3 to
6a6b791
Compare
jblomer
approved these changes
Feb 6, 2025
CloneSchema is used to create a new descriptor that contains the same schema information as the given one but no information about clustering etc.
It currently does the wrong thing by merging the header extension with the regular header, which results in a descriptor that cannot be written back to disk (as it contains column ranges with PageZero locators). Also, the model it creates must be kept alive in order for it to be used correctly (e.g. when late model extending)
When serializing the header and header extension we're currently assuming that the first time we serialize it we don't have a header extension. While this is true in most cases, it's not true when we are incrementally merging an existing RNTuple. With this fix we correctly handle this case by skipping all columns belonging to the extension header when serializing the regular header.
- verify that we can correctly serialize/deserialize a header that already contains a header extension before the first serialization - verify that we can correctly serialize/deserialize a header that contains deferred columns in the non-extension header
silverweed
force-pushed
the
ntuple_desc_schema
branch
from
6a6b791 to
03e6aed
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Based on #17596, derived from #17563.
Introduces a new method in RNTupleDescriptor/Builder to allow cloning only the "schema" part of a descriptor, then uses this new method to fix
RPagePersistentSink::InitFromDescriptorin the general case where the given descriptor already contains a header extension.The RNTupleSerializer also needs a couple of changes to cope with this case, because we don't want to move the fields and columns from the header extension to the regular header (which would hinder the Merger's capability of doing incremental merging).
Checklist: