Repository navigation
Conversation
nwt
reviewed
Oct 19, 2026
| func (u *union) Consume(vec vector.Any) { | ||
| if k := vec.Kind(); k == vector.KindNone { | ||
| // XXX TBD - missing should be error | ||
| if k := vec.Kind(); k == vector.KindMissing { |
Collaborator
There was a problem hiding this comment.
Nit:
Suggested change
| if k := vec.Kind(); k == vector.KindMissing { | |
| if vec.Kind() == vector.KindMissing { |
| return v | ||
| } | ||
|
|
||
| // to its underlying value. It does not Deunion union values that are named types. |
Collaborator
There was a problem hiding this comment.
Nit: The sentence fragment "to its underlying value." does not make sense.
| tag := DecodeUint(it.Next()) | ||
| if tag == OptionNoneTag { | ||
| return TypeNone, nil | ||
| return nil, nil |
Collaborator
There was a problem hiding this comment.
Nit: Adding a Boolean "ok" return paramter would make for a nicer interface.
| return resultTags, results | ||
| } | ||
|
|
||
| func optionTags(index []uint32, d *Dynamic) ([]uint32, []uint32, uint32) { |
Collaborator
There was a problem hiding this comment.
Nits: We usually put a vector parameter before an index parameter. And simplify both this interface and the implementation by letting the caller compute noneLen since it's just len(newTags)-len(someIndex).
| // Dynamic with the first value a some and the second value a none. | ||
| // This design relies upon super.OptionSomeTag=0 and super.OptionNoneTag=1. | ||
| // Note that the Missing vector use here is arbitrary: we just need a singleton | ||
| // type that is cannot otherwise be an option type because we need to |
Collaborator
There was a problem hiding this comment.
Suggested change
| // type that is cannot otherwise be an option type because we need to | |
| // type that cannot otherwise be an option type because we need to |
Comment on lines
+27
to
+31
| func NewSome(typ *super.TypeOption, vec Any) *Option { | ||
| return &Option{Typ: typ, Any: vec} | ||
| } | ||
|
|
||
| func NewNone(typ *super.TypeOption, noneLen uint32) *Option { |
Collaborator
There was a problem hiding this comment.
Nit: I think we should call these NewOptionSome and NewOptionNone.
mattnibs
reviewed
Oct 19, 2026
Comment on lines
+72
to
+73
| atyp := sctx.LookupTypeArray(super.TypeMissing) | ||
| return vector.NewArray(atyp, []uint32{0, 0}, vector.NewMissing(0)) |
Collaborator
There was a problem hiding this comment.
Not trying to hold up the review but I'm not sure about giving an empty array a type of missing- feels like its in an error state when there is nothing wrong the array.
Collaborator
Author
There was a problem hiding this comment.
It's not an error. typeof([]) has to have a type for the inner type. As we discussed, I'm going to put back TypeNone in a subsequent PR and this will change back to [none] then.
nwt
approved these changes
Oct 19, 2026
nwt
left a comment
nwt
left a comment
Collaborator
There was a problem hiding this comment.
All my unresolved comments are just nits.
This commit addresses some confusion that has been arising regarding the difference between option types and a union (none|T). To do so, we renamed the none type as missing and introduce the concept that there are two types of nones: a missing none (i.e., none::missing), and an option none (i.e., none::option(T) aka none::T). none::missing values can be cast to option(T) by casting the none to type T. While none::missing exists in the runtime as a transient value, they are not expected to be serialized anywhere (except for inside fusion values). That said, we allow their serialization for test and debug with the goal of making sure the runtime has sufficient type safety so that they can never be materialized, e.g., "values none" (which implies none::missing) should produce an error though we haven't implemented this yet. Option nones are always serializable. Fusion now creates fused records with missing fields with the missing type. This seems to be less confusing. The option type now includes vector.Missing to represent none case but we change the conventions so that the these none::missing values are never expressed out of an option value. Instead, option.Coalesce is called with default values or errors are expressed as nones. We changed Apply so RipFusions does not rip options and RipOption continues to mean to separate the nones (as missings) and the somes. It's up to the caller of Apply to handle the option values appropriately. This moves that logic to the call sites where type-safe semantics can be implemented requiring users to specify queries that handle the none cases when they arise. (There are more cases to cover in the runtime, and the next step with the type checker is to catch at compile time as many as the option-handling type errors runtime as possiblea)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
This commit addresses some confusion that has been arising regarding the difference between option types and a union (none|T). To do so, we renamed the none type as missing and introduce the concept that there are two types of nones: a missing none (i.e., none::missing), and an option none (i.e., none::option(T) aka none::T).
none::missing values can be cast to option(T) by casting the none to type T. While none::missing exists in the runtime as a transient value, they are not expected to be serialized anywhere (except for inside fusion values). That said, we allow their serialization for test and debug with the goal of making sure the runtime has sufficient type safety so that they can never be materialized, e.g., "values none" (which implies none::missing) should produce an error though we haven't implemented this yet. Option nones are always serializable.
Fusion now creates fused records with missing fields with the missing type. This seems to be less confusing.
The option type now includes vector.Missing to represent none case but we change the conventions so that the these none::missing values are never expressed out of an option value. Instead, option.Coalesce is called with default values or errors are expressed as nones.
We changed Apply so RipFusions does not rip options and RipOption continues to mean to separate the nones (as missings) and the somes. It's up to the caller of Apply to handle the option values appropriately. This moves that logic to the call sites where type-safe semantics can be implemented requiring users to specify queries that handle the none cases when they arise. (There are more cases to cover in the runtime, and the next step with the type checker is to catch at compile time as many as the option-handling type errors runtime as possible)
Fixes #7431