Improve wording about error cases in VariantShredding
#523
Conversation
alamb
changed the title
VariantShredding
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thank you @ostronaut and @emkornfield
I think this is an improvement. I updated the title of this PR to reflect what I think this PR now represents
cc @scovich in case you would also like to review
VariantShredding.md
Outdated
| | INVALID | `02 00` (object with 0 fields) | null | | | | | INVALID: `typed_value` is null for object | | ||
| | INVALID CASE: `{"event_type": "login", "event_ts": 1729795057774}` | `{"event_type": "login"}` | non-null | null | `login` | null | 1729795057774 | INVALID: Shredded field is present in `value` | | ||
| | INVALID CASE: `"a"` | `"a"` | non-null | null | null | null | null | INVALID: `typed_value` is present for non-object | | ||
| | INVALID CASE: `{}` | `02 00` (object with 0 fields) | null | | | | | INVALID: `typed_value` is null for object | |
There was a problem hiding this comment.
What about this case?
| `{"event_type": "login"}` | `02 00` (object with 0 fields) | non-null | null | `login`
Is it legal for an object that shredded perfectly to still have an empty object in the value column? It's sub-optimal but is it incorrect?
Conversely, what about this case?
| `{"event_type": "login"}` | null | non-null | null | `login`
Is it allowed for an object that shredded perfectly to have NULL in the value column? Or is it required to have an empty object there? (guessing it's allowed to have NULL, and maybe even required to do so)
There was a problem hiding this comment.
These are good points! I could add them but first need to have confirmation on expected behaviour from other collaborators.
For the first point, can we say that that the following is correct:
| Event object | `value` | `typed_value` | `typed_value.event_type.value` | `typed_value.event_type.typed_value` | `typed_value.event_ts.value` | `typed_value.event_ts.typed_value` | Notes |
|-----------------------------------------|--------------------------------|---------------|--------------------------------|--------------------------------------|------------------------------|------------------------------------|-------------------------------------------------------------------------------------------|
| INVALID CASE: `{"event_type": "login"}` | `02 00` (object with 0 fields) | non-null | null | `login` | null | null | INVALID CASE: an empty object (object with 0 fields) is present for fully shredded object |
cc: @alamb @emkornfield
For the second point, i think it is described in the case one (fully shredded object), where value is null.
There was a problem hiding this comment.
For the first point, can we say that that the following is correct:
[valueof a perfectly shredded object is a variant object with no fields instead of NULL]
That's exactly my question... it's sub-optimal, but well-defined, and and I'm not immediately seeing why it would confuse or break readers?
There was a problem hiding this comment.
True! Lets here from the other collaborators maybe? I can add this case to the table once we agree on it.
cc: @alamb @emkornfield
scovich
left a comment
scovich
left a comment
There was a problem hiding this comment.
LGTM (FWIW), modulo some wording nits
VariantShredding.md
Outdated
| | INVALID CASE: `{"event_type": "login", "event_ts": 1729795057774}` | `{"event_type": "login"}` | non-null | null | `login` | null | 1729795057774 | INVALID CASE: Shredded field is present in `value` | | ||
| | INVALID CASE: `{"event_type": "login"}` | `{"event_type": "login"}` | null | | | | | INVALID CASE: Shredded field is present in `value`, while `typed_value` is null | | ||
| | INVALID CASE: `"a"` | `"a"` | non-null | null | null | null | null | INVALID CASE: `typed_value` is present for non-object while being null, even though `value` is not an object | | ||
| | INVALID CASE: `{}` | `02 00` (object with 0 fields) | null | | | | | INVALID CASE: `typed_value` is null for object | |
There was a problem hiding this comment.
The table is already super wide; I'm not sure CASE adds enough information to be worth the space it consumes?
There was a problem hiding this comment.
I added it for more clarity, but agree, it should be understandable just with INVALID as well
VariantShredding.md
Outdated
| | missing | null | null | | | | | Object/value is missing | | ||
| | INVALID CASE: `{"event_type": "login", "event_ts": 1729795057774}` | `{"event_type": "login"}` | non-null | null | `login` | null | 1729795057774 | INVALID CASE: Shredded field is present in `value` | | ||
| | INVALID CASE: `{"event_type": "login"}` | `{"event_type": "login"}` | null | | | | | INVALID CASE: Shredded field is present in `value`, while `typed_value` is null | | ||
| | INVALID CASE: `"a"` | `"a"` | non-null | null | null | null | null | INVALID CASE: `typed_value` is present for non-object while being null, even though `value` is not an object | |
There was a problem hiding this comment.
Not understanding the "while being null" part? What about just:
INVALID CASE:
typed_valueis present andvalueis not an object
There was a problem hiding this comment.
Rephrased as per suggestion!
|
LGTM, seems like others are happy with it, will merge once CI passes. Thank you @ostronaut |
|
@emkornfield do you think we can merge this PR now? |
|
Yes, apologies for the delay. |
|
Thank you @ostronaut @emkornfield and @scovich |
4 participants
Rationale for this change
Fix small typos in VariantShredding documentation
What changes are included in this PR?
Changes to MD file
Do these changes have PoC implementations?
No