Skip to content

Feature/page list envelope #342

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

Open
wants to merge 17 commits into
base: dev
Choose a base branch
from

Conversation

Krmjn09
Copy link
Collaborator

@Krmjn09 Krmjn09 commented Jun 19, 2025

Screenshot 2025-06-19 121459

This above image is the cluster detail since the count is 1 only one can be seen

Can you please help me in specifying entry point for page list envelope so that i can see its result.

@Krmjn09
Copy link
Collaborator Author

Krmjn09 commented Jun 19, 2025

image
this is the error due to which i am getting offset out of bounds i think , the value of envelope type id and length are not justified

@Krmjn09 Krmjn09 requested review from silverweed and removed request for silverweed June 20, 2025 06:55
const recordSize = reader.readS64(),
firstEntry = reader.readU64(),
combined = reader.readU64(),
flags = Number(combined & 0xFFn), // lower 8 bits
Copy link
Contributor

Choose a reason for hiding this comment

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

actually flags are the highest 8 bits, so you need to do flags = combined >> 56 (see DeserializeClusterSummary in RNTupleSerialize.cxx); likewise numEntries needs to mask out the most significant bits rather than being shifted by 8.

Copy link
Contributor

Choose a reason for hiding this comment

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

(Add tests as you go, it helps finding these mistakes early!)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK !! I will correct this code

@Krmjn09 Krmjn09 force-pushed the feature/page-list-Envelope branch from 5f08832 to 41649cb Compare June 20, 2025 12:31
_readLocator(reader) {
const sizeAndType = reader.readU32(), // 4 bytes: size + T bit
type = sizeAndType & 1, // last bit
size = sizeAndType >>> 1, // top 31 bits
Copy link
Contributor

Choose a reason for hiding this comment

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

This is wrong: the spec says:

Size: If T is zero, the number of bytes to read, i.e. the compressed size of the referenced block. Otherwise, the 16 least-significant bits, i.e. bits 0:15, specify the size of the locator itself.

So size is actually equal to sizeAndType if type === 0 (if it's not you throw the error so you don't have to handle it right now)

const pageListLocator = this._readLocator(reader);

// Seek to the page list offset
reader.seek(pageListLocator.fOffset);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
reader.seek(pageListLocator.fOffset);
reader.seek(pageListLocator.offset);

@Krmjn09
Copy link
Collaborator Author

Krmjn09 commented Jun 21, 2025

image

@Krmjn09 Krmjn09 requested a review from silverweed June 25, 2025 07:23
@Krmjn09 Krmjn09 requested a review from silverweed June 25, 2025 10: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