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

Support binding max 512 columns/fields instead of 256 #1038

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

xc
Copy link

@xc xc commented Dec 4, 2021

256 in a struct(not including embed struct) is bit small for us. We have 260 fields for now and may add more.

Also max columns better to be documented(in readme?).

Thanks.
XC

@xc xc changed the title Support max 512 columns/fields instead of 256 Support bind max 512 columns/fields instead of 256 Dec 4, 2021
@xc xc changed the title Support bind max 512 columns/fields instead of 256 Support binding max 512 columns/fields instead of 256 Dec 4, 2021
@aarondl
Copy link
Member

aarondl commented Dec 12, 2021

Unfortunately I think this is a backwards incompatible change. The way the pointer mappings work is that a single field is represented by a uint64 divided into 8 uint8.

Each byte represents a level of recursion into a struct, ie:

type User struct {
   OneLevel struct {
       TwoLevel struct {
       } `boil:"two_level"`
   } `boil:"one_level"`
}

This PR does two things: Increases the column max columns, but unfortunately it also trades for max recursive depth as we're using 9 bits per column now meaning we can only fit 7 levels of depth.

As is I don't think I can accept this.

@xc
Copy link
Author

xc commented Dec 12, 2021

Thanks for the answer.

Backward compatible wise it's true. For us and for most of users I think the more important is the field number instead of struct depth, but it's indeed about backward compatibility.

Maybe it's better to use 2 dimension slice eg. [10][512]int for grouping(one for struct depth, one for that depth's fields) instead of uint64 which has max limit? then the 'mapping' variable will not be uint64, but [2]int(eg.{0,20}). Not sure if it will affect performance. I'm not sure I'm able to write a PR but I can have a try.

@aarondl
Copy link
Member

aarondl commented Dec 12, 2021

It was definitely made this way for performance concerns so that's top of mind.

@xc
Copy link
Author

xc commented Dec 16, 2021

Give me some time to think if there is other possible to increase column number. Will update this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants