From 367cab1815c6e976b7233b5400d9c3781af13b6e Mon Sep 17 00:00:00 2001 From: Steven McCanne Date: Thu, 10 Sep 2026 06:57:23 -0700 Subject: [PATCH 1/2] fix DotExpr for fusions and improve missing field semantics This commit makes DotExpr's work with fusion and nones. There is currently a slow-path defuse whenever a none value or missing-field error is encountered. We can improve this later. In making these changes, we realized it would be better semantics for a reference to a non-existent field always return an error even for the "?." operator. Instead, ok() with a none operator should be used when missing fields are expected. Ok-pushdown will be added in a subsequent PR where the defuse step required to create the structured error can be avoided when we know the dot operation is wrapped in an ok() or is_ok(). We added the flag to the DotExpr struct but still need to wire it up. --- compiler/ztests/sql/groupby.yaml | 4 +- runtime/vam/expr/dot.go | 123 +++++++++++++++-------- runtime/ztests/expr/dot-fusion-none.yaml | 13 +++ runtime/ztests/expr/dot.yaml | 2 +- runtime/ztests/op/distinct.yaml | 2 +- runtime/ztests/op/join-empty-inner.yaml | 4 +- 6 files changed, 100 insertions(+), 48 deletions(-) create mode 100644 runtime/ztests/expr/dot-fusion-none.yaml diff --git a/compiler/ztests/sql/groupby.yaml b/compiler/ztests/sql/groupby.yaml index 540f08bc3c..789fa7c52c 100644 --- a/compiler/ztests/sql/groupby.yaml +++ b/compiler/ztests/sql/groupby.yaml @@ -1,7 +1,7 @@ script: | - super -s -c "select val.radius ?? error('missing') as radius,count() from shapes.json group by val.radius ?? error('missing') | sort this" + super -s -c "select val.radius.ok() ?? error('missing') as radius,count() from shapes.json group by val.radius.ok() ?? error('missing') | sort this" echo === - super -s -c 'select type,sum(val.radius) from shapes.json group by type | sort this' + super -s -c 'select type,sum(val.radius.ok()) from shapes.json group by type | sort this' inputs: - name: shapes.json diff --git a/runtime/vam/expr/dot.go b/runtime/vam/expr/dot.go index 0c53fdb8de..73354d85a6 100644 --- a/runtime/vam/expr/dot.go +++ b/runtime/vam/expr/dot.go @@ -2,6 +2,7 @@ package expr import ( "fmt" + "slices" "github.com/brimdata/super" "github.com/brimdata/super/pkg/field" @@ -17,16 +18,19 @@ func (*This) Eval(val vector.Any) vector.Any { type DotExpr struct { sctx *super.Context - record Evaluator - field string + defuse *Defuse + entity Evaluator + key string noneish bool + okPush bool } func NewDotExpr(sctx *super.Context, record Evaluator, field string, noneish bool) *DotExpr { return &DotExpr{ sctx: sctx, - record: record, - field: field, + defuse: NewDefuse(sctx), + entity: record, + key: field, noneish: noneish, } } @@ -40,49 +44,84 @@ func NewDottedExpr(sctx *super.Context, f field.Chain) Evaluator { } func (d *DotExpr) Eval(vec vector.Any) vector.Any { - return vector.Apply(vector.ApplyRipFusions|vector.ApplyRipUnions, d.eval, d.record.Eval(vec)) + return vector.Apply(vector.ApplyNone, d.eval, d.entity.Eval(vec)) } -func (d *DotExpr) eval(vecs ...vector.Any) vector.Any { - switch val := vector.Under(vector.Super(vecs[0])).(type) { - case *vector.None: - return val - case *vector.Record: - i, ok := val.Typ.IndexOfField(d.field) - if !ok { - if d.noneish { - return vector.NewNone(val.Len()) +func (d *DotExpr) eval(outerVecs ...vector.Any) vector.Any { + vec := outerVecs[0] + var missing bool + eval := func(innerVecs ...vector.Any) vector.Any { + switch val := vector.Under(innerVecs[0]).(type) { + case *vector.None: + return val + case *vector.Record: + i, ok := val.Typ.IndexOfField(d.key) + if !ok { + 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.field)), val) - } - return val.Fields[i] - case *vector.TypeValue: - var errs []uint32 - typvals := vector.NewTypeValueEmpty() - for i := range val.Len() { - typ := val.Value(i) - if typ, ok := super.TypeUnder(typ).(*super.TypeRecord); ok { - if typ, ok := typ.TypeOfField(d.field); ok { - typvals.Append(typ) - continue + out := val.Fields[i] + if hasNone(out) { + missing = true + } + return out + case *vector.TypeValue: + var errs []uint32 + typvals := vector.NewTypeValueEmpty() + for i := range val.Len() { + typ := val.Value(i) + if typ, ok := super.TypeUnder(typ).(*super.TypeRecord); ok { + if typ, ok := typ.TypeOfField(d.key); ok { + typvals.Append(typ) + continue + } } + errs = append(errs, i) } - errs = append(errs, i) - } - if len(errs) > 0 { - return vector.NewCombinedError(d.sctx, fmt.Sprintf("no such field %s", sup.QuotedName(d.field)), typvals, val, errs) - } - return typvals - case *vector.Map: - keyVec := vector.NewConstString(d.field, val.Len()) - return indexMap(d.sctx, val, keyVec) - case *vector.View: - return vector.Pick(d.eval(val.Any), val.Index) - default: - dot := "." - if d.noneish { - dot = "?." + if len(errs) > 0 { + return vector.NewCombinedError(d.sctx, fmt.Sprintf("no such field %s", sup.QuotedName(d.key)), typvals, val, errs) + } + return typvals + case *vector.Map: + keyVec := vector.NewConstString(d.key, val.Len()) + return indexMap(d.sctx, val, keyVec) + case *vector.View: + return vector.Pick(d.eval(val.Any), val.Index) + default: + dot := "." + if d.noneish { + dot = "?." + } + return vector.NewWrappedError(d.sctx, fmt.Sprintf("'%s': applied to non-record", dot), innerVecs[0]) } - return vector.NewWrappedError(d.sctx, fmt.Sprintf("'%s': applied to non-record", dot), vecs[0]) } + 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 && vec.Kind() == vector.KindFusion { + return vector.Apply(vector.ApplyRipFusions|vector.ApplyRipUnions, d.eval, d.defuse.Eval(vec)) + } + return out +} + +func hasNone(vec vector.Any) bool { + if _, ok := vec.(*vector.None); ok { + return true + } + if vec, ok := vec.(*vector.Fusion); ok { + return hasNone(vec.Values) + } + if vec, ok := vec.(*vector.Dynamic); ok { + return slices.IndexFunc(vec.Values, hasNone) >= 0 + } + if super.IsOptionType(vec.Type()) { + return hasNone(vec.(*vector.Union).Dynamic()) + } + return false } diff --git a/runtime/ztests/expr/dot-fusion-none.yaml b/runtime/ztests/expr/dot-fusion-none.yaml new file mode 100644 index 0000000000..5beab28c51 --- /dev/null +++ b/runtime/ztests/expr/dot-fusion-none.yaml @@ -0,0 +1,13 @@ +spq: fuse | values x + +input: | + {x:"foo"} + {x:1} + {y:1} + {} + +output: | + "foo" + 1 + error({message:"no such field x",on:{y:1}}) + error({message:"no such field x",on:{}}) diff --git a/runtime/ztests/expr/dot.yaml b/runtime/ztests/expr/dot.yaml index 120b6f65a6..c3e6440ac5 100644 --- a/runtime/ztests/expr/dot.yaml +++ b/runtime/ztests/expr/dot.yaml @@ -19,7 +19,7 @@ output: | 1 1 error({message:"'.': applied to non-record",on:null}) - none + error({message:"no such field b",on:{}}) 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/op/distinct.yaml b/runtime/ztests/op/distinct.yaml index 65bef8d382..fcf73c8da7 100644 --- a/runtime/ztests/op/distinct.yaml +++ b/runtime/ztests/op/distinct.yaml @@ -1,5 +1,5 @@ script: | - super -s -c 'from "1.sup" | distinct x' + super -s -c 'from "1.sup" | distinct x.ok()' echo === super -s -c 'from "2.sup" | distinct abs(this)' echo === diff --git a/runtime/ztests/op/join-empty-inner.yaml b/runtime/ztests/op/join-empty-inner.yaml index bc0e947205..d2471249f6 100644 --- a/runtime/ztests/op/join-empty-inner.yaml +++ b/runtime/ztests/op/join-empty-inner.yaml @@ -1,8 +1,8 @@ script: | echo === hash join - super -dynamic -s -c 'left join (from C.sup) on left.a=right.a | values {...left,hit:this?.right?.sc ?? error("missing")} | sort' A.sup + super -dynamic -s -c 'left join (from C.sup) on left.a=right.a | values {...left,hit:right.sc.ok() ?? error("missing")} | sort' A.sup echo === nested loop join - super -dynamic -s -c 'left join (from C.sup) on left.a Date: Fri, 11 Sep 2026 07:20:28 -0700 Subject: [PATCH 2/2] address PR feedback --- runtime/vam/expr/dot.go | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/runtime/vam/expr/dot.go b/runtime/vam/expr/dot.go index 73354d85a6..784d95bd52 100644 --- a/runtime/vam/expr/dot.go +++ b/runtime/vam/expr/dot.go @@ -111,17 +111,15 @@ func (d *DotExpr) eval(outerVecs ...vector.Any) vector.Any { } func hasNone(vec vector.Any) bool { - if _, ok := vec.(*vector.None); ok { + switch vec := vec.(type) { + case *vector.None: return true - } - if vec, ok := vec.(*vector.Fusion); ok { + case *vector.Union: + return super.IsOptionType(vec.Type()) && hasNone(vec.Dynamic()) + case *vector.Fusion: return hasNone(vec.Values) - } - if vec, ok := vec.(*vector.Dynamic); ok { + case *vector.Dynamic: return slices.IndexFunc(vec.Values, hasNone) >= 0 } - if super.IsOptionType(vec.Type()) { - return hasNone(vec.(*vector.Union).Dynamic()) - } return false }