Skip to content

Add Field Descriptor Deserialization Logic #335

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

Merged
merged 5 commits into from
Jun 16, 2025

Conversation

Krmjn09
Copy link
Collaborator

@Krmjn09 Krmjn09 commented Jun 15, 2025

This PR adds the logic to read field descriptors from the RNTuple header. It includes:

  1. Reading envelope and metadata like name, description, and writer
  2. Parsing the list of field records (name, type, flags, etc.)
  3. Handling optional values like array size or checksum based on flags

However, there's an issue: the code throws an offset out of bounds error while reading. I've tried multiple approaches but haven’t been able to debug the reason of error yet.

@silverweed
Copy link
Contributor

silverweed commented Jun 16, 2025

Why have you created a second PR that overlaps with #334? Isn't this introducing the same changes?

@Krmjn09
Copy link
Collaborator Author

Krmjn09 commented Jun 16, 2025

Why have you created a second PR that overlaps with #334? Isn't this introducing the same changes?

I felt the previous pr failed , became really messy and confusing so created a new pr which only have right now field descriptor logic then in the future pr I will add rest of the logic taking the #334 pr as help

@silverweed
Copy link
Contributor

silverweed commented Jun 16, 2025

In general it's not great to "restart fresh" with PRs because you lose track of all comments and history of changes. In this case it may work because the amount of code changes is small, but still there are 4 or 5 comments of mine on the old PR which still need to be addressed.
What you should do if you think the history of changes has become too messy is squashing together some (or all) the commits using git rebase -i dev and then force-push over your development branch (git push -f <your_remote> feature/deserializing-header)

@Krmjn09
Copy link
Collaborator Author

Krmjn09 commented Jun 16, 2025

In general it's not great to "restart fresh" with PRs because you lose track of all comments and history of changes. In this case it may work because the amount of code changes is small, but still there are 4 or 5 comments of mine on the old PR which still need to be addressed. What you should do if you think the history of changes has become too messy is squashing together some (or all) the commits using git rebase -i dev and then force-push over your development branch (git push -f <your_remote> feature/deserializing-header)

Okay, Thankyou so much for telling me this .I will keep this in mind in the future

@silverweed silverweed requested review from silverweed and linev June 16, 2025 09:29
Copy link
Contributor

@silverweed silverweed left a comment

Choose a reason for hiding this comment

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

A couple of minor things before merging

@Krmjn09
Copy link
Collaborator Author

Krmjn09 commented Jun 16, 2025

@silverweed this >>> is givinng error , I think Javascript throws error when we use >>> on BigInt Numbers

Copy link
Contributor

@silverweed silverweed left a comment

Choose a reason for hiding this comment

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

Thanks!

@linev linev merged commit 9982c80 into root-project:dev Jun 16, 2025
24 checks passed
@Krmjn09 Krmjn09 deleted the feature-header-field-descriptor branch June 17, 2025 11:02
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.

3 participants