From 5353c0d60943afb6b101cc39660066d959619fa7 Mon Sep 17 00:00:00 2001 From: Steven McCanne Date: Tue, 13 Oct 2026 21:04:23 -0700 Subject: [PATCH 1/3] fix bugs in dot expressions with union-missings This commit fixes how dot expressions handle missing-field errors by recognizing the only place these come from are either from fields missing in a plain record or union-missings that are none::union fusion values. This code path is simple and elegant now and will be easier to maintain. There is still a problem with fusion projections but this change hides that bug away from all our tests. We will fix this in a subsequent PR. Fixes #7469 --- runtime/expr/dot.go | 71 +++++++++++++++--------- runtime/ztests/expr/dot.yaml | 4 +- runtime/ztests/expr/fuser.yaml | 2 +- runtime/ztests/expr/optional-fields.yaml | 2 +- runtime/ztests/op/fuse.yaml | 2 +- sio/jsonio/ztests/missing-none.yaml | 26 +++++++++ vector/apply.go | 46 +++++++++++++++ 7 files changed, 123 insertions(+), 30 deletions(-) diff --git a/runtime/expr/dot.go b/runtime/expr/dot.go index e13be66af8..07ec364d4e 100644 --- a/runtime/expr/dot.go +++ b/runtime/expr/dot.go @@ -52,16 +52,17 @@ func (d *DotExpr) Eval(vec vector.Any) vector.Any { } func (d *DotExpr) eval(outerVecs ...vector.Any) vector.Any { - vec := outerVecs[0] - var missing bool var eval func(...vector.Any) vector.Any eval = func(innerVecs ...vector.Any) vector.Any { - switch val := vector.Under(innerVecs[0]).(type) { + on := innerVecs[0] + ripped := innerVecs[1] + switch val := vector.Under(ripped).(type) { case *vector.Null: if d.nullish { return val } case *vector.None: + //XXX this should have noneish check return val case *vector.Record: i, ok := val.Typ.IndexOfField(d.key) @@ -69,17 +70,30 @@ func (d *DotExpr) eval(outerVecs ...vector.Any) vector.Any { if d.nullish { return vector.NewNull(val.Len()) } - missing = true - return vector.NewWrappedError(d.sctx, fmt.Sprintf("no such field %s", sup.QuotedName(d.key)), innerVecs[0]) + return vector.NewWrappedError(d.sctx, fmt.Sprintf("no such field %s", sup.QuotedName(d.key)), val) } out := val.Fields[i] if _, ok := out.(*vector.None); ok { - return vector.NewWrappedError(d.sctx, fmt.Sprintf("no such field %s", sup.QuotedName(d.key)), innerVecs[0]) + return vector.NewWrappedError(d.sctx, fmt.Sprintf("no such field %s", sup.QuotedName(d.key)), val) } - if hasNone(out) { - missing = true + if !hasMissingUnion(out) { + return out } - return out + return vector.Apply(vector.ApplyDefuseMissings, func(vecs ...vector.Any) vector.Any { + + on := vecs[0] + unripped := vecs[1] + ripped := vecs[2] + if _, ok := ripped.(*vector.Missing); ok { + if d.nullish { + return vector.NewNull(ripped.Len()) + } + //XXX will check noneish here like nullish above in + // a susequent PR + return vector.NewWrappedError(d.sctx, fmt.Sprintf("no such field %s", sup.QuotedName(d.key)), on) + } + return unripped + }, vector.AddNoRip(on), vector.AddNoRip(out), out) case *vector.TypeValue: var errs []uint32 typvals := vector.NewTypeValueEmpty() @@ -101,7 +115,13 @@ func (d *DotExpr) eval(outerVecs ...vector.Any) vector.Any { keyVec := vector.NewConstString(d.key, val.Len()) return indexMap(d.sctx, val, keyVec) case *vector.View: - return vector.Pick(eval(val.Any), val.Index) + if pushed := vector.PushView(val); pushed != vector.Any(val) { + return eval(on, pushed) + } + // PushView doesn't handle this kind (e.g., TypeValue), so use the + // underlying vector as its own "on" to keep lengths aligned. Error + // values on this path wrap the ripped value instead of the outer one. + return vector.Pick(eval(val.Any, val.Any), val.Index) } op := "." if d.noneish { @@ -109,21 +129,24 @@ func (d *DotExpr) eval(outerVecs ...vector.Any) vector.Any { } else if d.nullish { op = "??." } - return vector.NewWrappedError(d.sctx, fmt.Sprintf("'%s': applied to non-record", op), innerVecs[0]) + return vector.NewWrappedError(d.sctx, fmt.Sprintf("'%s': applied to non-record", op), on) } - out := vector.Apply(vector.ApplyRipFusions|vector.ApplyRipUnions, eval, vec) - // If there were any structured errors or none values (e.g., because we hit a none - // inside a fusion and thus should be an error), then we take the slow path - // by defusing and starting over. One simple optimization we can do is okPush - // to indicate that this reference is wrapped in an ok() or is_ok(), in which case, - // the on field of the structured error will be discarded and thus does not need - // to be correct. There are a number of other ways to avoid this slow path but let's - // get it working first before we make it fast. - // XXX we need to wire up okPush - if !d.okPush && missing && d.hasFusion.Check(vec.Type()) { - return vector.Apply(vector.ApplyRipFusions|vector.ApplyRipUnions, d.eval, d.defuse.Eval(vec)) + vec := outerVecs[0] + return vector.Apply(vector.ApplyRipFusions|vector.ApplyRipUnions, eval, vector.AddNoRip(vec), vec) +} + +func hasMissingUnion(vec vector.Any) bool { + switch vec := vec.(type) { + case *vector.Union: + return hasNone(vec.Dynamic()) + case *vector.Fusion: + return hasMissingUnion(vec.Values) + case *vector.Dynamic: + return slices.IndexFunc(vec.Values, hasMissingUnion) >= 0 + case *vector.View: + return hasMissingUnion(vec.Any) } - return out + return false } func hasNone(vec vector.Any) bool { @@ -132,8 +155,6 @@ func hasNone(vec vector.Any) bool { return vec.Len() > 0 case *vector.Union: return hasNone(vec.Dynamic()) - case *vector.Option: - return hasNone(vec.Any) case *vector.Fusion: return hasNone(vec.Values) case *vector.Dynamic: diff --git a/runtime/ztests/expr/dot.yaml b/runtime/ztests/expr/dot.yaml index 018404af00..06e954d732 100644 --- a/runtime/ztests/expr/dot.yaml +++ b/runtime/ztests/expr/dot.yaml @@ -7,7 +7,7 @@ input: | {a:{b:1}}::(int64|{a:{b:int64}}) {a:fusion({b:1}::(null|{b:int64}),<{b:int64}>)} {a:fusion(null::(null|{b:int64}),)} - {a:fusion({b:fusion(none::option(int64|string),)},<{}>)} + {a:{b?:none::option(int64|string)}} {a:1} {} null @@ -19,7 +19,7 @@ output: | 1 1 error({message:"'.': applied to non-record",on:null}) - error({message:"no such field b",on:{}}) + none::option(int64|string) error({message:"'.': applied to non-record",on:1}) error({message:"'.': applied to non-record",on:error({message:"no such field a",on:{}})}) error({message:"'.': applied to non-record",on:error({message:"'.': applied to non-record",on:null})}) diff --git a/runtime/ztests/expr/fuser.yaml b/runtime/ztests/expr/fuser.yaml index 0c1facbcc8..699adf117d 100644 --- a/runtime/ztests/expr/fuser.yaml +++ b/runtime/ztests/expr/fuser.yaml @@ -125,7 +125,7 @@ input: | output: | fusion(true::(int64|bool|none|option(int64)),) - error({message:"no such field x",on:{}}) + error({message:"no such field x",on:{x:fusion(none::(int64|bool|none|option(int64)),)}}) fusion(some(1)::(int64|bool|none|option(int64)),) fusion(some(2)::(int64|bool|none|option(int64)),) diff --git a/runtime/ztests/expr/optional-fields.yaml b/runtime/ztests/expr/optional-fields.yaml index 4f4908de0a..89b91445a3 100644 --- a/runtime/ztests/expr/optional-fields.yaml +++ b/runtime/ztests/expr/optional-fields.yaml @@ -195,6 +195,6 @@ output: | none none error({message:"'?.': applied to non-record",on:1}) - error({message:"'?.': applied to non-record",on:1}) + error({message:"'?.': applied to non-record",on?:1}) some(1) none::int64 diff --git a/runtime/ztests/op/fuse.yaml b/runtime/ztests/op/fuse.yaml index 37377f3dc2..7b81ac0b19 100644 --- a/runtime/ztests/op/fuse.yaml +++ b/runtime/ztests/op/fuse.yaml @@ -208,7 +208,7 @@ input: | output: | fusion({y:1}::(none|{y:int64}),<{y:int64}>) - error({message:"no such field x",on:{}}) + error({message:"no such field x",on:{x:fusion(none::(none|{y:int64}),)}}) --- diff --git a/sio/jsonio/ztests/missing-none.yaml b/sio/jsonio/ztests/missing-none.yaml index 6bfbb82f3e..bb433bb4b6 100644 --- a/sio/jsonio/ztests/missing-none.yaml +++ b/sio/jsonio/ztests/missing-none.yaml @@ -48,3 +48,29 @@ outputs: data: | {event:"a",count:1} {event:null,count:1} + +--- + +script: | + super -f bsup -o in.bsup in.json + super -s -I query.spq + +inputs: + - name: in.json + data: | + {"kind":"commit","commit":{"rev":"a","collection":"c"}} + {"kind":"identity","identity":{"did":"e"}} + - name: query.spq + data: | + SELECT + commit.collection AS event, + count() AS count + FROM 'in.bsup' + GROUP BY event + ORDER BY event + +outputs: + - name: stdout + data: | + {event:"c",count:1} + {event:null,count:1} diff --git a/vector/apply.go b/vector/apply.go index c3ebc18ac8..1b7bfe32a2 100644 --- a/vector/apply.go +++ b/vector/apply.go @@ -14,6 +14,7 @@ const ( ApplyRipUnions ApplyOpt = 1 << iota ApplyRipFusions ApplyRipOptions + ApplyDefuseMissings ) type NoRip struct { @@ -58,6 +59,16 @@ func Apply(opt ApplyOpt, eval func(...Any) Any, vecs ...Any) Any { } } } + if opt&ApplyDefuseMissings != 0 { + for k, vec := range vecs { + if _, ok := vec.(*NoRip); !ok && vec.Kind() == KindFusion { + if union, ok := Super(vec).(*Union); ok { + vecs[k] = defuseMissing(vec, union) + } + } + } + } + var needApply bool for k, vec := range vecs { if d, ok := vec.(*Dynamic); ok && len(d.Tags) > 0 && homogenous(d.Tags) { @@ -207,3 +218,38 @@ func ripOption(o *Option) Any { } return o } + +type Missing struct { + Any +} + +func defuseMissing(vec Any, union *Union) Any { + noneTag := findNone(union.Values()) + if noneTag < 0 { + return vec + } + var pick, tags []uint32 + for k, tag := range union.Tags() { + if int(tag) != noneTag { + pick = append(pick, uint32(k)) + tags = append(tags, 0) + } else { + tags = append(tags, 1) + } + } + vals := Pick(vec, pick) + missings := &Missing{NewNone(vec.Len() - uint32(len(pick)))} + return NewDynamic(tags, []Any{vals, missings}) +} + +func findNone(vecs []Any) int { + for k, vec := range vecs { + if vec.Type() == super.TypeNone { + if vec.Len() != 0 { + return k + } + break + } + } + return -1 +} From caa79963ef4cdf40a1009c03a490d4a1b26e6c50 Mon Sep 17 00:00:00 2001 From: Steven McCanne Date: Wed, 14 Oct 2026 13:05:00 -0700 Subject: [PATCH 2/3] remove bad comment --- runtime/expr/dot.go | 1 - 1 file changed, 1 deletion(-) diff --git a/runtime/expr/dot.go b/runtime/expr/dot.go index 07ec364d4e..2cbb384ec0 100644 --- a/runtime/expr/dot.go +++ b/runtime/expr/dot.go @@ -62,7 +62,6 @@ func (d *DotExpr) eval(outerVecs ...vector.Any) vector.Any { return val } case *vector.None: - //XXX this should have noneish check return val case *vector.Record: i, ok := val.Typ.IndexOfField(d.key) From aee3373044ca0c965eb182dff9d74e133c6048e0 Mon Sep 17 00:00:00 2001 From: Steven McCanne Date: Wed, 14 Oct 2026 15:34:54 -0700 Subject: [PATCH 3/3] address PR feedback --- runtime/expr/dot.go | 5 ++--- sio/jsonio/ztests/missing-none.yaml | 4 ++-- vector/apply.go | 8 ++++---- 3 files changed, 8 insertions(+), 9 deletions(-) diff --git a/runtime/expr/dot.go b/runtime/expr/dot.go index 2cbb384ec0..64e01f66ef 100644 --- a/runtime/expr/dot.go +++ b/runtime/expr/dot.go @@ -79,7 +79,6 @@ func (d *DotExpr) eval(outerVecs ...vector.Any) vector.Any { return out } return vector.Apply(vector.ApplyDefuseMissings, func(vecs ...vector.Any) vector.Any { - on := vecs[0] unripped := vecs[1] ripped := vecs[2] @@ -141,7 +140,7 @@ func hasMissingUnion(vec vector.Any) bool { case *vector.Fusion: return hasMissingUnion(vec.Values) case *vector.Dynamic: - return slices.IndexFunc(vec.Values, hasMissingUnion) >= 0 + return slices.ContainsFunc(vec.Values, hasMissingUnion) case *vector.View: return hasMissingUnion(vec.Any) } @@ -157,7 +156,7 @@ func hasNone(vec vector.Any) bool { case *vector.Fusion: return hasNone(vec.Values) case *vector.Dynamic: - return slices.IndexFunc(vec.Values, hasNone) >= 0 + return slices.ContainsFunc(vec.Values, hasNone) } return false } diff --git a/sio/jsonio/ztests/missing-none.yaml b/sio/jsonio/ztests/missing-none.yaml index bb433bb4b6..f059ea153a 100644 --- a/sio/jsonio/ztests/missing-none.yaml +++ b/sio/jsonio/ztests/missing-none.yaml @@ -26,7 +26,7 @@ outputs: --- script: | - super -f bsup -o in.bsup in.json + super -o in.bsup in.json super -s -I query.spq inputs: @@ -52,7 +52,7 @@ outputs: --- script: | - super -f bsup -o in.bsup in.json + super -o in.bsup in.json super -s -I query.spq inputs: diff --git a/vector/apply.go b/vector/apply.go index 1b7bfe32a2..d9705b2c03 100644 --- a/vector/apply.go +++ b/vector/apply.go @@ -228,17 +228,17 @@ func defuseMissing(vec Any, union *Union) Any { if noneTag < 0 { return vec } - var pick, tags []uint32 + var index, tags []uint32 for k, tag := range union.Tags() { if int(tag) != noneTag { - pick = append(pick, uint32(k)) + index = append(index, uint32(k)) tags = append(tags, 0) } else { tags = append(tags, 1) } } - vals := Pick(vec, pick) - missings := &Missing{NewNone(vec.Len() - uint32(len(pick)))} + vals := Pick(vec, index) + missings := &Missing{NewNone(vec.Len() - uint32(len(index)))} return NewDynamic(tags, []Any{vals, missings}) }