Skip to content

[rntuple] Give page descriptions their column, field and global cluster ID - #131

Open
sathabbott wants to merge 2 commits into
nsmith-:mainfrom
sathabbott:fix/49-page-descriptions
Open

sathabbott wants to merge 2 commits into
nsmith-:mainfrom
sathabbott:fix/49-page-descriptions

Conversation

@sathabbott

Copy link
Copy Markdown
Collaborator

🤖 AI generated content

Fixes #49 and #116. Depends on #127 (field names are plain bytes there); until #127 merges, this PR also shows its commit.

RNTuple.get_extended_page_descriptions()

  • One entry per physical column in every cluster, so a position is the column ID, as the spec guarantees (Page Locations: "the order of the outer items must match the order of columns"). With the old default includeSuppressed=False, suppressed columns were dropped and positions shifted. In test_multiple_representations_rntuple_v1-0-0-0.root, cluster 1's only entry was column 1, at position 0. The parameter is removed: a suppressed column is simply an entry with no pages. The column zip is strict=True, as you suggested on [rntuple] Add Field information to InterpretablePage class #49.
  • Global cluster IDs ([rntuple] Page locations give no global cluster ID across cluster groups #116). Each page carries its RNTuple-wide clusterID. The new RNTuple.firstClusterIDs is the running sum of the footer's ClusterGroup.fNClusters (Page List Envelope), and each page list's cluster count is checked against its group's.
  • Field information ([rntuple] Add Field information to InterpretablePage class #49). Each InterpretablePage also carries columnID, fieldID (from fFieldID), the resolved fieldDescription and the qualified fieldPath.

SchemaDescription.field_path(field_id) -> bytes walks fParentFieldID up to the top-level field, which names itself as parent, and joins the names with .. . is forbidden in names, so the result is unambiguous. It returns bytes, like the names (#127), and raises on an out-of-range ID or a cycle.

The schema is built once per call. The schemaDescription property itself is unchanged.

Tests (tests/test_rntuple_page_descriptions.py):

  • multiple representations: each cluster keeps both columns, and the suppressed one is empty;
  • multiple cluster groups: 5 + 4 + 3 clusters come out as IDs 0–11, with firstClusterIDs == [0, 5, 9] and first entries 0…900;
  • field paths in rntuple/user-class.root, checked against the names root-io-spec's case.toml uses (fHit.:_0, fHits._0.:_0, fFlavour._0, …);
  • field IDs continue into the schema extension: test_extension_columns gives int_field, float_field, intvec_field, intvec_field._0;
  • on every RNTuple fixture, every page's column and field agree with the schema;
  • out-of-range and cyclic parent chains raise.

The hardcoded tests gain the new members. Full suite: 370 passed, 53 skipped, 46 xfailed (on top of #127).

Breaking changes:

  • get_extended_page_descriptions() no longer takes includeSuppressed, and always includes suppressed columns (as empty entries).
  • InterpretablePage has five new required members.

Assisted-by: claude-code:claude-opus-5-5

docs/design.md promises that where a builtin Python type captures a ROOT
type, the deserialized member is that builtin: bytes for strings. RNTuple
strings were RString wrappers instead.

Add CountedString, a MemberSerDe for a string stored as its length (in a
given struct format) followed by that many bytes, read as plain bytes. An
RNTuple string is Annotated[bytes, CountedString("<I")], exported from
rntuple.schema as RNTupleString, and the eight RNTuple string members (the
header's name, description and library; a field's name, type name, type
alias and description; extra type information's type name) use it.
RString is removed.

This is the RNTuple row of nsmith-#68. The TString, std::string and char* rows
stay open there; char* (nsmith-#20) can use CountedString(">i").

Part of nsmith-#68

Assisted-by: claude-code:claude-opus-5-5
RNTuple.get_extended_page_descriptions():
- returns one entry per physical column in every cluster, so a position is
  the column ID, as the spec guarantees (Page Locations: the outer items
  follow column order). With the old default includeSuppressed=False,
  suppressed columns were dropped and positions shifted. The parameter is
  removed; a suppressed column is an entry with no pages. The zip over
  columns is strict, as suggested on nsmith-#49.
- gives each page its RNTuple-wide cluster ID. Cluster IDs continue across
  cluster groups (Page List Envelope); the new RNTuple.firstClusterIDs is
  the running sum of the footer's fNClusters. Each page list's cluster
  count is checked against its group.
- gives each page its column ID, field ID, FieldDescription and qualified
  field path.

SchemaDescription.field_path(field_id) walks fParentFieldID up to the
top-level field (which names itself as parent) and joins the names with
"." (forbidden in names, so unambiguous). It returns bytes, like the names,
and raises on an out-of-range ID or a cycle.

The schema is built once per call; the schemaDescription property itself
is unchanged.

Fixes nsmith-#49
Fixes nsmith-#116

Assisted-by: claude-code:claude-opus-5-5
@codecov-commenter

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 95.12195% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.91%. Comparing base (3f43287) to head (f869278).

Files with missing lines Patch % Lines
src/rootfilespec/rntuple/RNTuple.py 92.98% 4 Missing ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #131      +/-   ##
==========================================
+ Coverage   91.82%   91.91%   +0.08%     
==========================================
  Files          37       37              
  Lines        2962     3019      +57     
==========================================
+ Hits         2720     2775      +55     
- Misses        242      244       +2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

This branch has not been deployed

No deployments
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.

[rntuple] Add Field information to InterpretablePage class

2 participants