Skip to content
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

Add force_offset64 option to TableCollection.asdict #1598

Closed
jeromekelleher opened this issue Jul 30, 2021 · 2 comments · Fixed by #1602
Closed

Add force_offset64 option to TableCollection.asdict #1598

jeromekelleher opened this issue Jul 30, 2021 · 2 comments · Fixed by #1602
Assignees
Labels
Python API Issue is about the Python API

Comments

@jeromekelleher
Copy link
Member

To keep compatibility with existing code, the TableCollection.asdict method will need to return 32 bit offset values, like the lightweight TableCollection. So, as another step on the 64 bit road, we should add the option to force returning 64 bit offset values. This is actually trickier then for the LightweightTableCollection, because we're getting the table columns via attribute accesses rather than method calls, and so will have to change some of the low-level infrastructure.

Another tricky question arises then: when someone does a direct access to (say) NodeTable.metadata_offset should they get a 64 bit value, or a 32/64 bit value depending on the size? I think probably 64 bit, but it may turn out that this breaks too much code. We'll see, I guess.

@jeromekelleher jeromekelleher added the Python API Issue is about the Python API label Jul 30, 2021
@jeromekelleher jeromekelleher self-assigned this Jul 30, 2021
@benjeffery
Copy link
Member

because we're getting the table columns via attribute accesses rather than method calls, and so will have to change some of the low-level infrastructure.

I'm not clear here on why we need to change the low level - don't we just convert to the desired width at the python level?

I agree on making the direct access 64bit only as I can't see how breakage can come from that, especially as we are going to a larger size.

@jeromekelleher
Copy link
Member Author

I'm not clear here on why we need to change the low level - don't we just convert to the desired width at the python level?

Right, you could do a post-hoc conversion up to 64 bit, I didn't think of that. I've figured out a better way in #1602 though, that reuses the C code for making the dict encoding so we only have one implementation of asdict.

@mergify mergify bot closed this as completed in #1602 Aug 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Python API Issue is about the Python API
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants