Repository: segmentio/parquet-go (go)

Issue:
Optional slice causes panic
Using `optional` on a slice causes the following panic:
```
panic: reflect: call of reflect.Value.Field on slice Value [recovered]
        panic: reflect: call of reflect.Value.Field on slice Value

goroutine 4 [running]:
testing.tRunner.func1.2({0x104afcc40, 0x1400000eb10})
        /usr/local/go/src/testing/testing.go:1396 +0x1c8
testing.tRunner.func1()
        /usr/local/go/src/testing/testing.go:1399 +0x378
panic({0x104afcc40, 0x1400000eb10})
        /usr/local/go/src/runtime/panic.go:884 +0x204
reflect.Value.Field({0x104aca6c0?, 0x140001f8e70?, 0x104afb6e0?}, 0x70?)
        /usr/local/go/src/reflect/value.go:1266 +0xec
github.com/segmentio/parquet-go.(*structField).Value(0x140000cf600, {0x104aca6c0?, 0x140001f8e70?, 0x10455b72c?})
        /Users/julienfabre/segment/code/github.com/segmentio/parquet-go/schema.go:440 +0x2a8
github.com/segmentio/parquet-go.reconstructFuncOfGroup.func1({0x104aca6c0?, 0x140001f8e70?, 0x140000c1ab8?}, {0x3c?, 0xc0?, 0x87?}, {0x1400000eaf8, 0x1, 0x1})
        /Users/julienfabre/segment/code/github.com/segmentio/parquet-go/row.go:729 +0x340
github.com/segmentio/parquet-go.reconstructFuncOfOptional.func1({0x104aca6c0?, 0x140001f8e70?, 0x140001f8e70?}, {0x18?, 0x0?, 0x0?}, {0x1400000eaf8?, 0x1, 0x1})
        /Users/julienfabre/segment/code/github.com/segmentio/parquet-go/row.go:571 +0x208
github.com/segmentio/parquet-go.reconstructFuncOfGroup.func1({0x104b25120?, 0x140001f8e70?, 0x0?}, {0x70?, 0xfa?, 0xbc?}, {0x1400000eaf8, 0x1, 0x1})
        /Users/julienfabre/segment/code/github.com/segmentio/parquet-go/row.go:729 +0x374
github.com/segmentio/parquet-go.(*Schema).Reconstruct(0x140000942a0, {0x104ac0ae0?, 0x140001f8e70?}, {0x1400000eaf8, 0x1, 0x1})
        /Users/julienfabre/segment/code/github.com/segmentio/parquet-go/schema.go:264 +0x22c
github.com/segmentio/parquet-go.(*GenericReader[...]).readRows(0x140000ad180, {0x140001f8e70, 0x2, 0x0})
        /Users/julienfabre/segment/code/github.com/segmentio/parquet-go/reader_go18.go:142 +0x148
github.com/segmentio/parquet-go.(*GenericReader[...]).Read(0x104bbe120?, {0x140001f8e70?, 0x0?, 0x51b?})
        /Users/julienfabre/segment/code/github.com/segmentio/parquet-go/reader_go18.go:107 +0x38
github.com/segmentio/parquet-go_test.TestIssueXXX(0x140000fe340)
        /Users/julienfabre/segment/code/github.com/segmentio/parquet-go/reader_go18_test.go:184 +0x22c
testing.tRunner(0x140000fe340, 0x104bb5af0)
        /usr/local/go/src/testing/testing.go:1446 +0x10c
created by testing.(*T).Run
        /usr/local/go/src/testing/testing.go:1493 +0x300
```

Reproduction:
```
	type B struct {
		Name string
	}
	type A struct {
		B []B `parquet:",optional"`
	}

	b := new(bytes.Buffer)
	w := parquet.NewGenericWriter[A](b)
	expect := []A{
		{
			B: []B{
				{
					Name: "foo",
				},
			},
		},
	}
	_, err := w.Write(expect)
	if err != nil {
		t.Fatal(err)
	}
	if err = w.Close(); err != nil {
		t.Fatal(err)
	}

	bufReader := bytes.NewReader(b.Bytes())
	r := parquet.NewGenericReader[A](bufReader)
	values := make([]A, 2)
	_, err = r.Read(values)
	if err != nil {
		t.Fatal(err)
	}
```

Pull request:
schema: fix handling of optional slice value
Previously we'd encode an optional slice as
Optional(Repeated(Struct)), which treated the entire slice as optional, but Parquet needs us to treat this as Repeated(Optional(struct)) where each element of the struct is optional.

The handling for this is a little bit messy, but it's also messy to pass the context down further, so we end up with a special case inside of the struct handling.

Fixes #400.

Patch:
diff --git a/Makefile b/Makefile
index 38a0097..b58b15c 100644
--- a/Makefile
+++ b/Makefile
@@ -7,3 +7,6 @@ AUTHORS.txt: .mailmap
 format:
 	go install github.com/kevinburke/differ@latest
 	differ gofmt -w .
+
+test:
+	go test -trimpath -race ./...
diff --git a/schema.go b/schema.go
index 744db90..a62e43f 100644
--- a/schema.go
+++ b/schema.go
@@ -844,6 +844,22 @@ func makeNodeOf(t reflect.Type, name string, tag []string) Node {
 		}
 	})
 
+	// Special case: an "optional" struct tag on a slice applies to the
+	// individual items, not the overall list. The least messy way to
+	// deal with this is at this level, instead of passing down optional
+	// information into the nodeOf function, and then passing back whether an
+	// optional tag was applied.
+	if node == nil && t.Kind() == reflect.Slice {
+		isUint8 := t.Elem().Kind() == reflect.Uint8
+		// Note for strings "optional" applies only to the entire BYTE_ARRAY and
+		// not each individual byte.
+		if optional && !isUint8 {
+			node = Repeated(Optional(nodeOf(t.Elem(), tag)))
+			// Don't also apply "optional" to the whole list.
+			optional = false
+		}
+	}
+
 	if node == nil {
 		node = nodeOf(t, tag)
 	}


Test patch:
diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
index 166b8ec..52bce27 100644
--- a/.github/workflows/test.yml
+++ b/.github/workflows/test.yml
@@ -39,10 +39,10 @@ jobs:
       run: go mod download
 
     - name: Run Tests
-      run: go test -race -tags=${{ matrix.tags }} ./...
+      run: go test -trimpath -race -tags=${{ matrix.tags }} ./...
 
     - name: Run Benchmarks
-      run: go test -short -tags=${{ matrix.tags }} -run '^$' -bench . -benchtime 1x ./...
+      run: go test -trimpath -short -tags=${{ matrix.tags }} -run '^$' -bench . -benchtime 1x ./...
 
   format:
     runs-on: ubuntu-latest
diff --git a/reader_go18_test.go b/reader_go18_test.go
index 191efc6..d44b4aa 100644
--- a/reader_go18_test.go
+++ b/reader_go18_test.go
@@ -88,6 +88,46 @@ func testGenericReaderRows[Row any](rows []Row) error {
 	return nil
 }
 
+func TestIssue400(t *testing.T) {
+	type B struct {
+		Name string
+	}
+	type A struct {
+		B []B `parquet:",optional"`
+	}
+
+	b := new(bytes.Buffer)
+	w := parquet.NewGenericWriter[A](b)
+	expect := []A{
+		{
+			B: []B{
+				{
+					// 32 bytes random so we can see in the binary parquet if we
+					// actually wrote the value
+					Name: "9e7eb1f0-bbcc-43ec-bfad-a9fac1bb0feb",
+				},
+			},
+		},
+	}
+	_, err := w.Write(expect)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if err = w.Close(); err != nil {
+		t.Fatal(err)
+	}
+
+	r := parquet.NewGenericReader[A](bytes.NewReader(b.Bytes()))
+	values := make([]A, 2)
+	_, err = r.Read(values)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if !reflect.DeepEqual(expect[0], values[0]) {
+		t.Errorf("want %q got %q", values[0], expect[0])
+	}
+}
+
 func BenchmarkGenericReader(b *testing.B) {
 	benchmarkGenericReader[benchmarkRowType](b)
 	benchmarkGenericReader[booleanColumn](b)