-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ntuple] Multiple column representations: foundation #16054
[ntuple] Multiple column representations: foundation #16054
Conversation
Test Results 13 files 13 suites 2d 20h 43m 6s ⏱️ Results for commit 6205bac. ♻️ This comment has been updated with latest results. |
c305ec2
to
ea017ad
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall design LGTM, some comments on details of the implementation
if (fDescriptor.FindLogicalColumnId(fieldId, 0, representationIndex - 1) == kInvalidDescriptorId) | ||
return R__FAIL("out of bounds representation index"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we maybe want to check here that the previous column representations has all of its columns added? ie loop over column = 0 .. GetColumnCardinality()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This and the next comment: I slightly patched the sanity checks. The result should be the same: when the descriptor is built, we ensure that all column representations have the same number of columns, and that the column index and the representation index are consecutive, starting at 0.
for (auto firstColumnIdx = 0u; firstColumnIdx < nColumns; firstColumnIdx += columnCardinality) { | ||
const auto &columnDesc = fDescriptor.GetColumnDescriptor(logicalColumnIds[firstColumnIdx]); | ||
if (columnDesc.GetRepresentationIndex() != firstColumnIdx / columnCardinality) { | ||
return R__FAIL("field with id '" + std::to_string(fieldId) + "' has irregular column representations"); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could probably just check this for all columns, not only the first of each representation.
std::uint64_t GetFirstElementIndex() const { return fFirstElementIndex; } | ||
std::uint64_t GetFirstElementIndex() const { return std::abs(fFirstElementIndex); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we expect to call this function for suppressed columns? Maybe add an assertion of not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is called also for suppressed columns. E.g. in the RNTupleSerializer.
8a7f155
to
6205bac
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good from my point of view; one minor comment, but I may be missing why that case is important...
if (nColumns <= columnCardinality) | ||
continue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this an error as well because it means the field doesn't have all columns? On the other hand, the previous condition allowed nColumns == 0
, but shouldn't this already be taken care of by columnCardinality == 0
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condition triggers if we have only a single column representation. I this case, by construction we have nColumns + 1 == columnCardinality
. Perhaps not the clearest way to express that in the current if
6205bac
to
29cab72
Compare
Provides the foundations for multiple, alternative column representations of a single field. Extends the binary format to store multiple sets of columns per field and handles (de-)serialization and the descriptor representation (backwards compatible).
A follow-up PR will use this capability from the RField/RColumn classes.