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
ffi: Add class for key-value pair log events. #507
ffi: Add class for key-value pair log events. #507
Changes from 21 commits
0da3f44
141f954
f3525aa
bbd3a56
b1fa61c
e148af7
f41e5d6
d29a17a
12679f7
534abf0
b7bed71
88aa36a
92c3388
698b406
bc565f5
2393038
285aab3
7623d76
ef0dd80
54ec36a
941fcbc
6623b5a
3ae389a
a066c67
353dccd
09dd01a
83ae4e0
7cd1ea7
6dd95a6
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.
A non-leaf node paired with a value actually returns
protocol_not_supported
. That said, I think it should returnoperation_not_permitted
since if it was permitted,node_id_value_pairs
wouldn't be a tree.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.
Disclaimer: I only have a rough understanding on how schema tree works.
This function is bit confusing for me.
"A node is considered a leaf if none of its descendants appear in the
node_id_value_pairs
." <- This doesn't seem to be the normal definition for a leaf node. Is this expected?In addition, "the sub schema tree defined by
node_id_value_pairs
." Is there any guarantee that nodes in thenode_id_value_pairs
will form a subtree? Just looking at the argument without thinking about how it is generated, it is possible node_id_value_pairs contains a list of unconnected nodes.And lastly a highlevel comment is that I feel this function should belong to the Schema tree class.
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 a validation method: if an object type node appears in the key value pairs, it must be a leaf node. The way we store key value pairs is to store the schema (all as leaf nodes), so the path from the root to the leaves are implicitly indicated. In the merged schema tree, usually an object type node is a non leaf node. But in a key value pairs' schema, this node can be a leaf node as long as its value is
{}
ornull
. Our goal is to validate that if such a node appears, all its descendants in the merged schema tree can't appear in the schema of the given key value pair log event.it is possible node_id_value_pairs contains a list of unconnected nodes
isn't true since everything will be connected to the root if they are valid leaf nodes. This function is used to validate pairs using schema tree's information. I think we should split it from a schema tree's method.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.
Let's move this into the try to reduce its scope.
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 assume this comment tries to explain why the line doesn't use
value.is_null();
in a way similar to line 68?I don't think I follow the comment though. Can you clarify a bit?
And we can combine the two if statements?
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.
{}
andnull
are two different types of values.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 still don't get when there will be {} and when there will be null (and why we only check for {} but not null in this function)
Is it because null is already handled in the validation? If so, I think you should leave this information in the comment.
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.
{}
is considered as a special case, wherenull
is a valid primitive value (like an int or a string). These are user-input values.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.
Have you considered using a
deque
?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.
In the next PR, you will see that I'm actually using
std::stack
(which is implemented usingdeque
) for dfs traversal, which is because the stack element is more complicated and using a vector means I need to provide move/copy semantics.However, in this scenario, the stack is simply an int. I think the guideline of using
std
container is you should always try to usevector
unless you have a strong motivation to use other ones. In this particular function, usingvector
have better locality, fewer indirections, and less memory allocation. Therefore, I would prefer to stick tovector
unless you have other concern 😄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.
Sure, was just asking.