Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Draft catalog flatbuffers schema #82
Draft catalog flatbuffers schema #82
Changes from 6 commits
38c9602
5a41824
a45e13c
334a139
f5f6ea9
bacadc5
86f3584
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Flatbuffers allows you to explicitly assign integer IDs to fields ala protobufs, with the
id
attribute. Can that work for our field ordinal value?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't think of that. The ordinal was just mean to keep the order. I'm not sure if anyone planned to assign ids for access - we could use these ordinals for that as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Flatbuffers has a
deprecated
attribute which we should use.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is exactly what this would track. Whether this field should be marked as deprecated in the schema or not. Or were you suggesting that we call it 'deprecated' as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, just wanted to confirm that this tracks the flatbuffers deprecated attribute, although I would like to know why you diverged from their terminology.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't remember - something made me want to invert the name, but I don't remember now. Might have been to avoid negated logic, because 'enabled = true' reads better than 'deprecated = false'. Let me think a bit about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I renamed it to deprecated. I also renamed a couple of other fields (type->category, count->repeated_count) for better clarity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any field in a flatbuffers table is nullable by default (absence is denoted by an offset of 0 for that field in the object's vtbl).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but this was meant to track whether the field should accept null or not. In tables, you can treat null value as missing, but in structs we'd need some other approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, in that case I think we'd need to track a user-defined "missing" sentinel value in our schema representation. Or just say that all fields in a struct are required as flatbuffers does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, missing may not work for tables either. We've had an early discussion where we've said that we'd need to avoid missing values in production, to allow in-place updates. flatc has an option to allow default values to be written in the serialization. The null discussion may need to wait until after Q2, to be fully addressed.