polarsignals/frostdb

Panic while writing structs with dynamic map column

gernest opened this issue · 2 comments

Calling (*Table).Write with struct that has map field defined as a dynamic column triggers a panic

Here is a test case to reproduce

func TestTable_write_struct_with_map_field(t *testing.T) {
	type SampleMap struct {
		Labels map[string]string
	}
	schema := &schemapb.Schema{
		Name: "test",
		Columns: []*schemapb.Column{
			{
				Name: "labels",
				StorageLayout: &schemapb.StorageLayout{
					Type:     schemapb.StorageLayout_TYPE_STRING,
					Nullable: true,
					Encoding: schemapb.StorageLayout_ENCODING_RLE_DICTIONARY,
				},
				Dynamic: true,
			},
		},
	}

	store, err := New()
	require.Nil(t, err)
	defer store.Close()
	db, err := store.DB(context.Background(), "tes")
	require.Nil(t, err)
	table, err := db.Table("test", NewTableConfig(schema))
	require.Nil(t, err)
	_, err = table.Write(context.Background(),
		SampleMap{
			Labels: map[string]string{
				"label1": "value1",
				"label2": "value2",
			},
		},
		SampleMap{
			Labels: map[string]string{
				"label1": "value1",
				"label2": "value2",
				"label3": "value3",
			},
		},
		SampleMap{
			Labels: map[string]string{
				"label1": "value1",
				"label2": "value2",
			},
		},
	)
	require.Nil(t, err)
}

Here is the stacktrace

--- FAIL: TestTable_write_struct_with_map_field (0.00s)
panic: runtime error: index out of range [3] with length 3 [recovered]
	panic: runtime error: index out of range [3] with length 3

goroutine 7 [running]:
testing.tRunner.func1.2({0x1a0a460, 0xc0000ca078})
	/usr/local/Cellar/go/1.21.4/libexec/src/testing/testing.go:1545 +0x238
testing.tRunner.func1()
	/usr/local/Cellar/go/1.21.4/libexec/src/testing/testing.go:1548 +0x397
panic({0x1a0a460?, 0xc0000ca078?})
	/usr/local/Cellar/go/1.21.4/libexec/src/runtime/panic.go:914 +0x21f
github.com/parquet-go/parquet-go.(*Buffer).WriteRows(0xc0002f43f0, {0xc00007f860?, 0x3, 0x0?})
	/Users/g.ernest/go/pkg/mod/github.com/parquet-go/parquet-go@v0.20.0/buffer.go:392 +0x418
github.com/polarsignals/frostdb/dynparquet.(*Buffer).WriteRows(...)
	/Volumes/code/frostdb/dynparquet/schema.go:960
github.com/polarsignals/frostdb/dynparquet.ValuesToBuffer(0xc0002f41b0, {0xc000151f28, 0x3, 0x120?})
	/Volumes/code/frostdb/dynparquet/schema.go:1141 +0x3d8
github.com/polarsignals/frostdb.(*Table).Write(0xc00047eaa0, {0x1bd33f0, 0x235d6c0}, {0xc000151f28?, 0x0?, 0x0?})
	/Volumes/code/frostdb/table.go:550 +0x7e
github.com/polarsignals/frostdb.TestTable_write_struct_with_map_field(0x0?)
	/Volumes/code/frostdb/db_test.go:2280 +0x505
testing.tRunner(0xc00017ed00, 0x1aca4e0)
	/usr/local/Cellar/go/1.21.4/libexec/src/testing/testing.go:1595 +0xff
created by testing.(*T).Run in goroutine 1
	/usr/local/Cellar/go/1.21.4/libexec/src/testing/testing.go:1648 +0x3ad
exit status 2
FAIL	github.com/polarsignals/frostdb	0.914s

@thorfour (*Table)Write creates parquet rows then converts them to arrow.Record since we have merged the new api that builds directly to arrow.Record I think we should deprecate this api.

I confirmed parca doesn't use (*Table)Write I think its better to invest more time into massaging (*Table)InsertRecord. Now we can document and iron out bits about tables and how to correctly use them.

Agreed, we should just delete that API entirely now.