-
Notifications
You must be signed in to change notification settings - Fork 18
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
Handle null values when deserializing series and structs for command expansion #1347
Handle null values when deserializing series and structs for command expansion #1347
Conversation
@cartermak thanks for looking into this! I'm curious about implications for generated code - let's say that an Activity takes an Does this mean that the expansion logic needs to handle the case where I think I'm a bit concerned about the implications of "any value can be null", since to me that means "downstream tools must handle any value being null" 🤔 This leads to a question of "is All that said, I'm in favor of pursuing your suggested fix in the short term, since I think the broader question of "what's the best way to represent Optionals in ValueSchemas" can be answered independently of making the sequencing server more robust |
Ah, I realize that your specific examples have to do with |
@mattdailis Great questions that I've pondered a bit already. My current philosophical stance is that any field in a serialized value ought to be nullable (at any "level" of the value schema, if that makes sense, including "parent" structs/series). A few justifications I have for that position:
With that said, some specific answers based on that philosophy:
If it's meaningful that the value is null, then the expansion logic should handle that case. If it's not expected, then I think it's OK for the expansion logic to assume the value is not
I haven't developed this thought as much, but I think there ought to be a distinction between a
I don't think we'd run into the same issue with |
I've opened #1348 for philosophical discussion - I don't want it to get in the way of this PR
That's a fair point - I guess all the other cases are pass-through, yes? So they will simply return |
It seems so, with the exception of |
Oh, and it's maybe noteworthy that the |
That's a vestige of Aerie's previous encoding of the "empty" type. This is the type used when an activity type does not return anything from its effect model - it still produces a "computed attributes" value, but that value contains no information. "A type that contains no information" is called There are a couple different ways to encode a type that has only one possible value. One of them is to define an enum with only one variant - this is how Aerie first implemented the unitary type. After some confusion around this special enum with VOID as its only variant, Aerie switched over to using the empty struct to represent a unitary type (if you declare an empty struct value schema, there is exactly one value that satisfies that schema*). This was the PR that made the switch: #311 I think that makes the code you're reading obsolete... there is of course the chance that some existing Aerie deployment still has vestigial
*Side note - if every value can be null, then that means there are exactly two values that satisfy this schema, making it no longer particularly "unitary" |
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.
Hi Carter, Thank you for looking into this. I have a quick question about using empty arrays or objects instead of null
. I am not sure how that would work with the ValueMappers within the Clipper Mission Model. I think this might be a better approach for the expansion logic, but let me know if it causes more headaches on your end.
@@ -246,9 +246,10 @@ function convertType(value: any, schema: Schema): any { | |||
case SchemaTypes.String: | |||
return value; | |||
case SchemaTypes.Series: | |||
return value.map((value: any) => convertType(value, schema.items)); | |||
return value == null ? null : value.map((value: any) => convertType(value, schema.items)); |
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.
Would returning an empty array []
be better than returning null
? This would show an absent collection while preserving the series structure.
case SchemaTypes.Struct: | ||
const struct: { [attributeName: string]: any } = {}; | ||
if (value == null) {return null;} |
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.
Similar to the series but returning an empty {}
@goetzrrGit I haven't tested so I'm not sure how it would behave, but I expect this would be equivalent for our use case (we don't actually expand any of the activities with this issue right now; Aerie just deserializes all activities regardless of whether there's an expansion rule). That said, is there a reason to change the value from |
@cartermak Early in my career, I was heavily influenced by senior developers who strongly opposed the use of In situations where we're deserializing data into an array or objects, I believe an empty form serves as a better alternative to |
@goetzrrGit That's fair; in this case, I'd argue that avoiding usage of For now, this is still a high-priority blocking issue and I'm happy with any solution that doesn't break when running command expansion on a simulation dataset with these |
In general, I agree with @goetzrrGit that an empty object is a cleaner "no data" value for a list or map. But in this case we are blocked on using our mission model, I would vote for an expedient fix (and maybe move to something more elegant in future). |
@parkerabercrombie and @cartermak Dan, Matt, and I will have an internal meeting today to discuss this further. For the most part, we don't want to block you so we are leaning toward using |
@goetzrrGit thanks for the update. To be clear, the design solution won't impact us either way -- we don't expand any of these problem activities; the problem is just that we can't expand other activities if these problem activities are in the plan. We'll take whatever is fastest and most palatable to implement right now. |
Discussed this with @goetzrrGit and @mattdailis today and I think we agree this is a good change for the sake of consistency until we figure out long-term null handling. A few things still outstanding:
@goetzrrGit will open a new PR with these changes. |
@dandelany Thank you for the update! That all sounds good to me. |
Description
Pass
null
values for series and structs to TS simulated activity instances for command expansion when deserializing simulation datasets.Verification
Tested locally on one test case, but not tested extensively.
Documentation
None
Future work
Add the expected/allowed behavior of
null
values to the "Value Schema" documentation.