I kinda feel like this can be moved into the value type validation method.

prefer to leave it outside for two reasons:

1. empty isn't a Value type
2. This simplifies the helper function so it doesn't have to check has_value() and we don't need to symbols inside the helper (one for the std::optional<Value> and the over for the reference of the underlying Value)

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 the node_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.

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 {} or null. 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.

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?

1. {} and null are two different types of values.
2. I'd prefer to handle them separately. Combining these two statements will make it very hard to read

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.

{} is considered as a special case, where null is a valid primitive value (like an int or a string). These are user-input values.

Techinically it is possible to modify the original schema tree using this pointer in the class, right?
Should we consider using std::shared_ptr<SchemaTree const> ?

Make sense

Let's move this into the try to reduce its scope.

A non-leaf node paired with a value actually returns protocol_not_supported. That said, I think it should return operation_not_permitted since if it was permitted, node_id_value_pairs wouldn't be a tree.

Hm, I missed this in the previous PRs but it seems like we should just have copy constructors for Value and EncodedTextAst. It feels like an overoptimization to force people to never use copy construction unless they write a method manually like this.

We can add a copy constructor for EncodedTextAst, however, I'd prefer to not have a copy constructor for Value to enforce move semantics. How about making this clone method as a member of Value, so that people can explicitly copy a value when they really need

What's bad about copying a Value? Is it not similar to copying a string?

Copying string and EncodedTextAst can be expensive in deserialization. With copy disabled, we are more confident that there's no copy overhead in deserialization.

Adding move constructor for EncodedTextAst and Value

Overzealous refactoring.

You can reduce the amount of code for this test by using SECTION and even nested SECTION macros. Recall that CMake runs every SECTION from the beginning of the test, so the schema tree, node_id_value_pairs would be reset for each outer SECTION.

E.g., instead of cloning valid_node_id_value_pairs, you could just create it outside a SECTION and use it inside different SECTIONs.

I actually thought about using SECTION but I didn't. Two reasons:

1. I don't want to rerun everything from the beginning
2. I want to test that the same schema tree is used across different log events (which have different life cycles)

1. But by using sections, you'd only run the things outside the section(s) from the beginning, right?
2. Testing the same instance of the schema tree across all of the tests seems a bit extra since it should be const in the class anyway (and if it wasn't const, you're still relying on finding a bug in the implementation with a set of test cases that isn't exhaustive). Imo, having less code in the test is more valuable than testing that corner case.

From my reading, you could do something like:

1. Create schema tree
2. Section (optional): Test empty id-value pairs
3. Section (optional): Test mismatched types
4. Section (optional): Test valid id-value pairs
   • Section: Test duplicate key conflicts
   • Section: Test implicit key conflicts
5. Section (optional): Test out of bound node ID

That would allow you to at least get rid of the clone_node_id_value_pairs function (and make the code a bit easier to read).

Since everything except the constructor is default, we don't need to say they're default explicitly, right?

Ditto about default.

I thought the idea of using nested sections (besides breaking the code up) is that you wouldn't need to duplicate valid_node_id_value_pairs since it would start with a fresh state in each section, no? (You'd obviously call it something other than valid_node_id_value_pairs, but still.