Refactor NewSet internals into SetBuilder (#156)

* Refactor SetBuilder out of NewSet
* Clean up other new...set constructors
* Replace most NewSet usage with SetBuilder
* Simplify some SetBuilder helpers
* Factor out constants to keep golangci-lint happy

rel/constants.go

@@ -0,0 +1,6 @@
+package rel
+
+const (
+	// Keeping golangci-lint happy.
+	sTrue = "true"
+)

rel/expr_darrow.go

@@ -32,7 +32,7 @@ func (e *DArrowExpr) Eval(ctx context.Context, local Scope) (_ Value, err error)
 		return nil, WrapContextErr(err, e, local)
 	}
 	if set, ok := value.(Set); ok {
-		values := []Value{}
+		b := NewSetBuilder()
 		for i := set.Enumerator(); i.MoveNext(); {
 			var scope Scope
 			var err error
@@ -44,9 +44,9 @@ func (e *DArrowExpr) Eval(ctx context.Context, local Scope) (_ Value, err error)
 			if err != nil {
 				return nil, WrapContextErr(err, e, local)
 			}
-			values = append(values, v)
+			b.Add(v)
 		}
-		s, err := NewSet(values...)
+		s, err := b.Finish()
 		if err != nil {
 			return nil, WrapContextErr(err, e, local)
 		}

rel/expr_seqmap.go

@@ -121,7 +121,7 @@ func (e *SeqArrowExpr) Eval(ctx context.Context, local Scope) (_ Value, err erro
 		}
 		return d, nil
 	case Set:
-		values := []Value{}
+		b := NewSetBuilder()
 		for i := value.Enumerator(); i.MoveNext(); {
 			t, ok := i.Current().(Tuple)
 			if !ok {
@@ -139,9 +139,9 @@ func (e *SeqArrowExpr) Eval(ctx context.Context, local Scope) (_ Value, err erro
 			if err != nil {
 				return nil, WrapContextErr(err, e, local)
 			}
-			values = append(values, NewTuple(Attr{"@", at}, Attr{attr, newItem}))
+			b.Add(NewTuple(Attr{"@", at}, Attr{attr, newItem}))
 		}
-		s, err := NewSet(values...)
+		s, err := b.Finish()
 		if err != nil {
 			return nil, WrapContextErr(err, e, local)
 		}

rel/expr_set.go

@@ -17,15 +17,15 @@ type SetExpr struct {
 
 // NewSetExpr returns a new TupleExpr.
 func NewSetExpr(scanner parser.Scanner, elements ...Expr) (Expr, error) {
-	values := make([]Value, len(elements))
-	for i, expr := range elements {
+	b := NewSetBuilder()
+	for _, expr := range elements {
 		value, is := exprIsValue(expr)
 		if !is {
 			return &SetExpr{ExprScanner{scanner}, elements}, nil
 		}
-		values[i] = value
+		b.Add(value)
 	}
-	s, err := NewSet(values...)
+	s, err := b.Finish()
 	if err != nil {
 		return nil, err
 	}
@@ -48,15 +48,15 @@ func (e *SetExpr) String() string {
 
 // Eval returns the subject
 func (e *SetExpr) Eval(ctx context.Context, local Scope) (Value, error) {
-	values := make([]Value, 0, len(e.elements))
+	b := NewSetBuilder()
 	for _, expr := range e.elements {
 		value, err := expr.Eval(ctx, local)
 		if err != nil {
 			return nil, WrapContextErr(err, e, local)
 		}
-		values = append(values, value)
+		b.Add(value)
 	}
-	s, err := NewSet(values...)
+	s, err := b.Finish()
 	if err != nil {
 		return nil, WrapContextErr(err, e, local)
 	}

rel/ops_rel.go

@@ -157,17 +157,21 @@ func Joiner(combine func(common Names, a, b Tuple) Tuple) func(a, b Set) (Set, e
 				return value.(Tuple).Project(common)
 			},
 			func(key Value, a, b Set) Set {
-				values := []Value{}
+				sb := NewSetBuilder()
 				for i := a.Enumerator(); i.MoveNext(); {
 					for j := b.Enumerator(); j.MoveNext(); {
-						values = append(values, combine(
+						sb.Add(combine(
 							common,
 							i.Current().(Tuple),
 							j.Current().(Tuple),
 						))
 					}
 				}
-				return MustNewSet(values...)
+				result, err := sb.Finish()
+				if err != nil {
+					panic(err)
+				}
+				return result
 			},
 		), nil
 	}
@@ -266,24 +270,24 @@ func GenericJoin(
 // E.g., [1, 2] + [3] = [1, 2, 3]; "hell" + "o" = "hello"
 func Concatenate(a, b Set) (Set, error) {
 	offset := a.Count()
-	values := make([]Value, 0, a.Count()+b.Count())
+	sb := NewSetBuilder()
 	for e := a.Enumerator(); e.MoveNext(); {
-		values = append(values, e.Current())
+		sb.Add(e.Current())
 	}
 	for e := b.Enumerator(); e.MoveNext(); {
 		elt := e.Current()
 		if t, ok := elt.(Tuple); ok {
 			if pos, found := t.Get("@"); found {
 				if n, ok := pos.(Number); ok {
 					t = t.With("@", NewNumber(float64(offset)+n.Float64()))
-					values = append(values, t)
+					sb.Add(t)
 					continue
 				}
 			}
 		}
 		return nil, errors.Errorf("Mismatched elt in set + set: %v", elt)
 	}
-	return NewSet(values...)
+	return sb.Finish()
 }
 
 // NConcatenate applies concatenate to one or more sets.

rel/ops_set_rank.go

@@ -34,11 +34,11 @@ func Rank(s Set, rankerf func(v Tuple) (Tuple, error)) (Set, error) {
 		}
 	}
 
-	values := make([]Value, 0, len(entries))
+	b := NewSetBuilder()
 	for _, entry := range entries {
-		values = append(values, entry.input)
+		b.Add(entry.input)
 	}
-	return NewSet(values...)
+	return b.Finish()
 }
 
 type rankerEntry struct {

rel/value.go

@@ -128,7 +128,7 @@ type Set interface {
 	Without(Value) Set
 	Map(func(Value) (Value, error)) (Set, error)
 	Where(func(Value) (bool, error)) (Set, error)
-	CallAll(context.Context, Value) (Set, error)
+	CallAll(context.Context, Value, SetBuilder) error
 
 	ArrayEnumerator() (OffsetValueEnumerator, bool)
 }
@@ -146,7 +146,12 @@ func (n NoReturnError) Error() string {
 // SetCall is a convenience wrapper to call a set and return the result or an
 // error if there isn't exactly one result.
 func SetCall(ctx context.Context, s Set, arg Value) (Value, error) {
-	all, err := s.CallAll(ctx, arg)
+	b := NewSetBuilder()
+	err := s.CallAll(ctx, arg, b)
+	if err != nil {
+		return nil, err
+	}
+	all, err := b.Finish()
 	if err != nil {
 		return nil, err
 	}
@@ -162,7 +167,12 @@ func SetCall(ctx context.Context, s Set, arg Value) (Value, error) {
 }
 
 func mustCallAll(ctx context.Context, s Set, v Value) Value {
-	result, err := s.CallAll(ctx, v)
+	b := NewSetBuilder()
+	err := s.CallAll(ctx, v, b)
+	if err != nil {
+		panic(err)
+	}
+	result, err := b.Finish()
 	if err != nil {
 		panic(err)
 	}

rel/value_repr.go

@@ -105,7 +105,7 @@ func reprDict(d Dict, w io.Writer) {
 
 func reprSet(s GenericSet, w io.Writer) {
 	if s.Equal(True) {
-		fmt.Fprintf(w, "true")
+		fmt.Fprint(w, sTrue)
 		return
 	}
 	fmt.Fprint(w, "{")

rel/value_set_array.go

@@ -69,7 +69,7 @@ func AsArray(v Value) (Array, bool) {
 	return Array{}, false
 }
 
-func asArray(values ...Value) (Array, bool) {
+func asArray(values ...Value) Array {
 	minIndex := math.MaxInt32
 	maxIndex := math.MinInt32
 	for _, v := range values {
@@ -95,7 +95,7 @@ func asArray(values ...Value) (Array, bool) {
 		values: items,
 		offset: minIndex,
 		count:  n,
-	}, true
+	}
 }
 
 func (a Array) clone() Array {
@@ -285,7 +285,7 @@ func (a Array) With(value Value) Set {
 	if t, ok := value.(ArrayItemTuple); ok {
 		return a.withItem(t.at, t.item)
 	}
-	return newSetFromSet(a).With(value)
+	return newGenericSetFromSet(a).With(value)
 }
 
 // Without returns the original Array without the given value. Iff the value
@@ -325,15 +325,15 @@ func (a Array) Without(value Value) Set {
 
 // Map maps values per f.
 func (a Array) Map(f func(v Value) (Value, error)) (Set, error) {
-	var values []Value
+	b := NewSetBuilder()
 	for e := a.Enumerator(); e.MoveNext(); {
 		v, err := f(e.Current())
 		if err != nil {
 			return nil, err
 		}
-		values = append(values, v)
+		b.Add(v)
 	}
-	return NewSet(values...)
+	return b.Finish()
 }
 
 // Where returns a new Array with all the Values satisfying predicate p.
@@ -376,19 +376,18 @@ func (a Array) Where(p func(v Value) (bool, error)) (Set, error) {
 	return result, nil
 }
 
-func (a Array) CallAll(_ context.Context, arg Value) (Set, error) {
-	n, ok := arg.(Number)
-	if !ok {
-		return nil, fmt.Errorf("arg to CallAll must be a number, not %s", ValueTypeAsString(arg))
-	}
-	i := int(n.Float64()) - a.offset
-	if i < 0 || i >= len(a.values) {
-		return None, nil
-	}
-	if v := a.values[i]; v != nil {
-		return NewSet(v)
+func (a Array) CallAll(_ context.Context, arg Value, b SetBuilder) error {
+	if n, ok := arg.(Number); ok {
+		if i, is := n.Int(); is {
+			i -= a.offset
+			if 0 <= i && i < len(a.values) {
+				if v := a.values[i]; v != nil {
+					b.Add(v)
+				}
+			}
+		}
 	}
-	return None, nil
+	return nil
 }
 
 // Enumerator returns an enumerator over the Values in the Array.

rel/value_set_array_test.go

@@ -118,8 +118,13 @@ func TestArrayCallAll(t *testing.T) {
 	AssertEqualValues(t, None, mustCallAll(ctx, three, NewNumber(1)))
 	AssertEqualValues(t, None, mustCallAll(ctx, three, NewNumber(5)))
 
-	_, err := three.CallAll(ctx, NewString([]rune("0")))
-	assert.Error(t, err)
+	b := NewSetBuilder()
+	err := three.CallAll(ctx, NewString([]rune("0")), b)
+	if assert.NoError(t, err) {
+		set, err := b.Finish()
+		require.NoError(t, err)
+		assert.False(t, set.IsTrue())
+	}
 }
 
 func TestArrayWhere(t *testing.T) {

rel/value_set_builder.go

@@ -0,0 +1,86 @@
+package rel
+
+import (
+	"reflect"
+
+	"github.com/arr-ai/frozen"
+	"github.com/go-errors/errors"
+)
+
+// MustNewSet constructs a genericSet from a set of Values, or panics if construction fails.
+func MustNewSet(values ...Value) Set {
+	s, err := NewSet(values...)
+	if err != nil {
+		panic(err)
+	}
+	return s
+}
+
+// NewSet constructs a genericSet from a set of Values.
+func NewSet(values ...Value) (Set, error) {
+	b := NewSetBuilder()
+	for _, v := range values {
+		b.Add(v)
+	}
+	return b.Finish()
+}
+
+// NewSetFrom constructs a genericSet from interfaces.
+func NewSetFrom(intfs ...interface{}) (Set, error) {
+	b := NewSetBuilder()
+	for _, intf := range intfs {
+		value, err := NewValue(intf)
+		if err != nil {
+			return nil, err
+		}
+		b.Add(value)
+	}
+	return b.Finish()
+}
+
+type SetBuilder struct {
+	buckets map[interface{}][]Value
+}
+
+func NewSetBuilder() SetBuilder {
+	return SetBuilder{buckets: map[interface{}][]Value{}}
+}
+
+func (b *SetBuilder) Add(v Value) {
+	t := reflect.TypeOf(v)
+	b.buckets[t] = append(b.buckets[t], v)
+}
+
+func (b *SetBuilder) Finish() (Set, error) {
+	switch len(b.buckets) {
+	case 0:
+		return None, nil
+	case 1:
+		for typ, values := range b.buckets {
+			switch typ {
+			case stringCharTupleType:
+				return asString(values...), nil
+			case bytesByteTupleType:
+				if b, is := asBytes(values...); is {
+					return b, nil
+				}
+				return nil, errors.Errorf("unsupported byte array expr")
+			case arrayItemTupleType:
+				return asArray(values...), nil
+			case dictEntryTupleType:
+				tuples := make([]DictEntryTuple, 0, len(values))
+				for _, value := range values {
+					tuples = append(tuples, value.(DictEntryTuple))
+				}
+				return NewDict(true, tuples...)
+			}
+		}
+	}
+	sb := frozen.SetBuilder{}
+	for _, values := range b.buckets {
+		for _, value := range values {
+			sb.Add(value)
+		}
+	}
+	return GenericSet{sb.Finish()}, nil
+}