Repository: genjidb/genji (go)

Issue:
Panic when running GROUP BY on array value
```
Opened an in-memory database.
Enter ".help" for usage hints.
genji> create table foo;
genji> insert INTO foo (a) VALUES ([1, 2, 3]);
genji> select max(a) from foo group by a;
panic: runtime error: hash of unhashable type msgpack.EncodedArray

goroutine 1 [running]:
github.com/genjidb/genji/document.Stream.Aggregate.func1.1(0xa9b600, 0xc00032a870, 0xa9b600, 0xc00032a870)
        /home/asdine/code/github.com/genjidb/genji/document/iterator.go:297 +0xcd
github.com/genjidb/genji/document.Stream.Iterate.func1(0xa9b540, 0xc0001c6fc0, 0xc0001c6ec0, 0x0)
        /home/asdine/code/github.com/genjidb/genji/document/iterator.go:141 +0x78
github.com/genjidb/genji/database.(*Table).Iterate(0xc00015c6c0, 0xc00032f140, 0x0, 0x0)
        /home/asdine/code/github.com/genjidb/genji/database/table.go:343 +0x1b5
github.com/genjidb/genji/document.Stream.Iterate(0xa97680, 0xc00015c6c0, 0x0, 0xc00032f140, 0xc00011a301, 0xc00032f140)
        /home/asdine/code/github.com/genjidb/genji/document/iterator.go:126 +0x151
github.com/genjidb/genji/document.Stream.Iterate(0xa98200, 0xc000156f00, 0xc00011a400, 0xc000180aa0, 0xc00011a420, 0x0)
        /home/asdine/code/github.com/genjidb/genji/document/iterator.go:131 +0xa2
github.com/genjidb/genji/document.Stream.Aggregate.func1(0xc000156f60, 0xa98200, 0x18)
        /home/asdine/code/github.com/genjidb/genji/document/iterator.go:290 +0xff
github.com/genjidb/genji/document.IteratorFunc.Iterate(0xc000166540, 0xc000156f60, 0xc00005a800, 0x7fd1fab327d0)
        /home/asdine/code/github.com/genjidb/genji/document/iterator.go:45 +0x30
github.com/genjidb/genji/document.Stream.Iterate(0xa981e0, 0xc000166540, 0x0, 0xc000156f60, 0x1, 0xc000156f60)
        /home/asdine/code/github.com/genjidb/genji/document/iterator.go:126 +0x151
github.com/genjidb/genji/document.Stream.Iterate(0xa98200, 0xc000156f40, 0xc00011a420, 0xc00011a430, 0x0, 0x0)
        /home/asdine/code/github.com/genjidb/genji/document/iterator.go:131 +0xa2
github.com/genjidb/genji/cmd/genji/shell.(*Shell).runQuery(0xc000243500, 0xc0002e8120, 0x22, 0x0, 0x0)
        /home/asdine/code/github.com/genjidb/genji/cmd/genji/shell/shell.go:287 +0x19c
github.com/genjidb/genji/cmd/genji/shell.(*Shell).executeInput(0xc000243500, 0xc0002e8120, 0x22, 0x2, 0x3)
        /home/asdine/code/github.com/genjidb/genji/cmd/genji/shell/shell.go:222 +0x1fe
github.com/genjidb/genji/cmd/genji/shell.(*Shell).execute(0xc000243500, 0xc0002e8120, 0x22)
        /home/asdine/code/github.com/genjidb/genji/cmd/genji/shell/shell.go:204 +0x85
github.com/c-bata/go-prompt.(*Prompt).Run(0xc0001f01b0)
        /home/asdine/go/pkg/mod/github.com/c-bata/go-prompt@v0.2.5/prompt.go:84 +0x85f
github.com/genjidb/genji/cmd/genji/shell.Run(0xc0001a3640, 0xc0001a3640, 0x0)
        /home/asdine/code/github.com/genjidb/genji/cmd/genji/shell/shell.go:128 +0x385
main.main.func2(0xc0001c6840, 0xc0001a3600, 0x4)
        /home/asdine/code/github.com/genjidb/genji/cmd/genji/main.go:102 +0x1a5
github.com/urfave/cli/v2.(*App).RunContext(0xc000186480, 0xaa0600, 0xc0001ae030, 0xc0001922a0, 0x1, 0x1, 0x0, 0x0)
        /home/asdine/go/pkg/mod/github.com/urfave/cli/v2@v2.2.0/app.go:315 +0x70d
github.com/urfave/cli/v2.(*App).Run(...)
        /home/asdine/go/pkg/mod/github.com/urfave/cli/v2@v2.2.0/app.go:215
main.main()
        /home/asdine/code/github.com/genjidb/genji/cmd/genji/main.go:108 +0x4dd
```

Pull request description:
Fix grouping of lazily decoded values
This PR fixes #208 by encoding values before using them as keys of the `groups` map

Patch:
diff --git a/document/iterator.go b/document/iterator.go
index 98a7153e..cd1f3107 100644
--- a/document/iterator.go
+++ b/document/iterator.go
@@ -2,6 +2,7 @@ package document
 
 import (
 	"bufio"
+	"bytes"
 	"errors"
 	"io"
 )
@@ -282,11 +283,14 @@ func (s Stream) GroupBy(groupFn func(d Document) (Value, error)) Stream {
 // Aggregate builds a list of aggregators for each group of documents and passes each document of the stream to them.
 func (s Stream) Aggregate(aggregatorBuilders ...AggregatorBuilder) Stream {
 	return NewStream(IteratorFunc(func(fn func(d Document) error) error {
-		aggregates := make(map[Value][]Aggregator)
-		var groups []Value
+		aggregates := make(map[string][]Aggregator)
+		var groupKeys []string
 
 		nullValue := NewNullValue()
 
+		var b bytes.Buffer
+
+		enc := NewValueEncoder(&b)
 		err := s.Iterate(func(d Document) error {
 			group := nullValue
 
@@ -294,17 +298,26 @@ func (s Stream) Aggregate(aggregatorBuilders ...AggregatorBuilder) Stream {
 				group = gd.group
 			}
 
-			aggs, ok := aggregates[group]
+			b.Reset()
+
+			err := enc.Encode(group)
+			if err != nil {
+				return err
+			}
+
+			// using b.Bytes() and cast as a string is much faster than using b.String()
+			// because of compiler optimizations.
+			aggs, ok := aggregates[string(b.Bytes())]
 			if !ok {
-				groups = append(groups, group)
+				groupKey := b.String()
+				groupKeys = append(groupKeys, groupKey)
 				aggs = make([]Aggregator, len(aggregatorBuilders))
 				for i, builder := range aggregatorBuilders {
 					aggs[i] = builder.NewAggregator(group)
 				}
-				aggregates[group] = aggs
+				aggregates[groupKey] = aggs
 			}
 
-			var err error
 			for _, agg := range aggs {
 				err = agg.Add(d)
 				if err != nil {
@@ -318,9 +331,9 @@ func (s Stream) Aggregate(aggregatorBuilders ...AggregatorBuilder) Stream {
 			return err
 		}
 
-		for _, group := range groups {
+		for _, groupKey := range groupKeys {
 			fb := NewFieldBuffer()
-			aggs := aggregates[group]
+			aggs := aggregates[groupKey]
 			for _, agg := range aggs {
 				err = agg.Aggregate(fb)
 				if err != nil {
diff --git a/sql/planner/tree.go b/sql/planner/tree.go
index dbe1eb11..d45c9d83 100644
--- a/sql/planner/tree.go
+++ b/sql/planner/tree.go
@@ -42,7 +42,7 @@ const (
 	// Unset is an operation that removes a value at a given path from every document of a stream
 	Unset
 	// Group is an operation that groups documents based on a given path.
-	_
+	Group
 	// Dedup is an operation that removes duplicate documents from a stream
 	Dedup
 )
@@ -444,9 +444,9 @@ func (n *unsetNode) String() string {
 type GroupingNode struct {
 	node
 
-	Expr   expr.Expr
 	Tx     *database.Transaction
 	Params []expr.Param
+	Expr   expr.Expr
 }
 
 var _ operationNode = (*GroupingNode)(nil)
@@ -455,7 +455,7 @@ var _ operationNode = (*GroupingNode)(nil)
 func NewGroupingNode(n Node, e expr.Expr) Node {
 	return &GroupingNode{
 		node: node{
-			op:   Projection,
+			op:   Group,
 			left: n,
 		},
 		Expr: e,
@@ -469,6 +469,8 @@ func (n *GroupingNode) Bind(tx *database.Transaction, params []expr.Param) (err
 	return
 }
 
+// toStream uses the GroupBy stream operation. It evaluates Expr for every field and returns
+// the result.
 func (n *GroupingNode) toStream(st document.Stream) (document.Stream, error) {
 	return st.GroupBy(func(d document.Document) (document.Value, error) {
 		return n.Expr.Eval(expr.EvalStack{


Test patch:
diff --git a/sql/query/select_test.go b/sql/query/select_test.go
index 160817a6..5cfb7f35 100644
--- a/sql/query/select_test.go
+++ b/sql/query/select_test.go
@@ -3,6 +3,7 @@ package query_test
 import (
 	"bytes"
 	"database/sql"
+	"encoding/json"
 	"strconv"
 	"testing"
 
@@ -221,6 +222,24 @@ func TestSelectStmt(t *testing.T) {
 		require.NoError(t, err)
 		require.JSONEq(t, `[{"foo": true},{"foo": 1}, {"foo": 2},{"foo": "hello"}]`, buf.String())
 	})
+
+	// https://github.com/genjidb/genji/issues/208
+	t.Run("group by with arrays", func(t *testing.T) {
+		db, err := genji.Open(":memory:")
+		require.NoError(t, err)
+		defer db.Close()
+
+		err = db.Exec("CREATE TABLE test; INSERT INTO test (a) VALUES ([1, 2, 3]);")
+		require.NoError(t, err)
+
+		d, err := db.QueryDocument("SELECT MAX(a) from test GROUP BY a")
+		require.NoError(t, err)
+
+		enc, err := json.Marshal(d)
+		require.NoError(t, err)
+
+		require.JSONEq(t, `{"MAX(a)": [1, 2, 3]}`, string(enc))
+	})
 }
 
 func TestDistinct(t *testing.T) {
@@ -297,63 +316,3 @@ func TestDistinct(t *testing.T) {
 		})
 	}
 }
-
-// func TestSelectIndexedDoubleAndIntegers(t *testing.T) {
-// 	tests := []struct {
-// 		a      interface{}
-// 		query  string
-// 		isTrue bool
-// 		param  interface{}
-// 	}{
-// 		{math.MaxInt64, "SELECT true FROM test WHERE a = ?", true, math.MaxInt64},
-// 		{math.MaxInt64, "SELECT true FROM test WHERE a > ?", false, math.MaxInt64},
-// 		{math.MaxInt64, "SELECT true FROM test WHERE a > ?", true, math.MaxInt64 - 1},
-// 		{math.MaxInt64, "SELECT true FROM test WHERE a >= ?", true, math.MaxInt64},
-// 		{math.MaxInt64, "SELECT true FROM test WHERE a >= 9223372036854775807.0", true, nil},
-// 		{math.MaxInt64, "SELECT true FROM test WHERE a > 9223372036854775800.0", false, nil},
-// 		{math.MaxInt64, "SELECT true FROM test WHERE a < 9223372036854775800.0", false, nil},
-// 		{math.MaxInt64, "SELECT true FROM test WHERE a = 9223372036854775800.0", true, nil},
-// 	}
-
-// 	for _, indexed := range []bool{false, true} {
-// 		for _, test := range tests {
-// 			t.Run(fmt.Sprintf("indexed: %v / %q", indexed, test.query), func(t *testing.T) {
-// 				fmt.Println("---")
-// 				db, err := genji.Open(":memory:")
-// 				require.NoError(t, err)
-// 				defer db.Close()
-
-// 				tx, err := db.Begin(true)
-// 				require.NoError(t, err)
-// 				defer tx.Rollback()
-
-// 				err = tx.Exec("CREATE TABLE test")
-// 				require.NoError(t, err)
-
-// 				if indexed {
-// 					err = tx.Exec("CREATE INDEX idx_foo_a ON test(a)")
-// 					require.NoError(t, err)
-// 				}
-
-// 				// a = math.MaxInt64
-// 				err = tx.Exec("INSERT INTO test (a) VALUES (?)", test.a)
-// 				require.NoError(t, err)
-
-// 				var params []interface{}
-// 				if test.param != nil {
-// 					params = append(params, test.param)
-// 				}
-// 				doc, err := tx.QueryDocument(test.query, params...)
-// 				if test.isTrue {
-// 					require.NoError(t, err)
-// 					var got bool
-// 					err = document.Scan(doc, &got)
-// 					require.NoError(t, err)
-// 					require.True(t, got)
-// 				} else {
-// 					require.Equal(t, database.ErrDocumentNotFound, err)
-// 				}
-// 			})
-// 		}
-// 	}
-// }