Skip to content

Commit

Permalink
remove mapstructure (#2703)
Browse files Browse the repository at this point in the history
I have tripped over mapstructure one too many times so this commit
removes it.  When/if the extra round-trip through JSON adds
measurable overhead to Zed query times, we can revisit this and
hopefully mapstructure will be more mature.  For now, I wanted to
add order.Which to proc.Sort and similar but mapstructure cannot
handle custom JSON unmarshalers so this caused unpack to fail
for apparently no reason.

See mitchellh/mapstructure#115
  • Loading branch information
mccanne committed May 11, 2021
1 parent 3dcb327 commit 730d19c
Show file tree
Hide file tree
Showing 7 changed files with 34 additions and 42 deletions.
6 changes: 3 additions & 3 deletions compiler/ast/dag/unpack.go
Expand Up @@ -80,7 +80,7 @@ func UnpackJSON(buf []byte) (interface{}, error) {
if len(buf) == 0 {
return nil, nil
}
return unpacker.UnpackBytes(buf)
return unpacker.Unmarshal(buf)
}

// UnpackJSONAsOp transforms a JSON representation of an operator into an dag.Op.
Expand All @@ -97,7 +97,7 @@ func UnpackJSONAsOp(buf []byte) (Op, error) {
}

func UnpackMapAsOp(m interface{}) (Op, error) {
object, err := unpacker.UnpackMap(m)
object, err := unpacker.UnmarshalObject(m)
if object == nil || err != nil {
return nil, err
}
Expand All @@ -109,7 +109,7 @@ func UnpackMapAsOp(m interface{}) (Op, error) {
}

func UnpackMapAsExpr(m interface{}) (Expr, error) {
object, err := unpacker.UnpackMap(m)
object, err := unpacker.UnmarshalObject(m)
if object == nil || err != nil {
return nil, err
}
Expand Down
6 changes: 3 additions & 3 deletions compiler/ast/unpack.go
Expand Up @@ -86,7 +86,7 @@ func UnpackJSON(buf []byte) (interface{}, error) {
if len(buf) == 0 {
return nil, nil
}
return unpacker.UnpackBytes(buf)
return unpacker.Unmarshal(buf)
}

// UnpackJSONAsProc transforms a JSON representation of a proc into an ast.Proc.
Expand All @@ -103,7 +103,7 @@ func UnpackJSONAsProc(buf []byte) (Proc, error) {
}

func UnpackMapAsProc(m interface{}) (Proc, error) {
object, err := unpacker.UnpackMap(m)
object, err := unpacker.UnmarshalObject(m)
if object == nil || err != nil {
return nil, err
}
Expand All @@ -115,7 +115,7 @@ func UnpackMapAsProc(m interface{}) (Proc, error) {
}

func UnpackMapAsExpr(m interface{}) (Expr, error) {
object, err := unpacker.UnpackMap(m)
object, err := unpacker.UnmarshalObject(m)
if object == nil || err != nil {
return nil, err
}
Expand Down
1 change: 0 additions & 1 deletion go.mod
Expand Up @@ -19,7 +19,6 @@ require (
github.com/hashicorp/golang-lru v0.5.4
github.com/kr/text v0.2.0
github.com/mattn/go-isatty v0.0.12 // indirect
github.com/mitchellh/mapstructure v1.3.3
github.com/niemeyer/pretty v0.0.0-20200227124842-a10e7caefd8e // indirect
github.com/pbnjay/memory v0.0.0-20190104145345-974d429e7ae4
github.com/peterh/liner v1.1.0
Expand Down
2 changes: 0 additions & 2 deletions go.sum
Expand Up @@ -112,8 +112,6 @@ github.com/matttproud/golang_protobuf_extensions v1.0.1 h1:4hp9jkHxhMHkqkrB3Ix0j
github.com/matttproud/golang_protobuf_extensions v1.0.1/go.mod h1:D8He9yQNgCq6Z5Ld7szi9bcBfOoFv/3dc6xSMkL2PC0=
github.com/mitchellh/go-homedir v1.1.0/go.mod h1:SfyaCUpYCn1Vlf4IUYiD9fPX4A5wJrkLzIz1N1q0pr0=
github.com/mitchellh/mapstructure v1.1.2/go.mod h1:FVVH3fgwuzCH5S8UJGiWEs2h04kUh9fWfEaFds41c1Y=
github.com/mitchellh/mapstructure v1.3.3 h1:SzB1nHZ2Xi+17FP0zVQBHIZqvwRN9408fJO8h+eeNA8=
github.com/mitchellh/mapstructure v1.3.3/go.mod h1:bFUtVrKA4DC2yAKiSyO/QUcy7e+RRV2QTWOzhPopBRo=
github.com/modern-go/concurrent v0.0.0-20180228061459-e0a39a4cb421/go.mod h1:6dJC0mAP4ikYIbvyc7fijjWJddQyLn8Ig3JB5CqoB9Q=
github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd/go.mod h1:6dJC0mAP4ikYIbvyc7fijjWJddQyLn8Ig3JB5CqoB9Q=
github.com/modern-go/reflect2 v0.0.0-20180701023420-4b7aa43c6742/go.mod h1:bx2lNnkwVCuqBIxFjflWJWanXIb3RllmbCylyMrvgv0=
Expand Down
38 changes: 14 additions & 24 deletions pkg/unpack/reflector.go
Expand Up @@ -4,8 +4,6 @@ import (
"encoding/json"
"fmt"
"reflect"

"github.com/mitchellh/mapstructure"
)

var zero reflect.Value
Expand Down Expand Up @@ -69,19 +67,15 @@ func (r Reflector) get(unpackKey string, create bool) map[string]reflect.Type {
return types
}

func (r Reflector) Unpack(s string) (interface{}, error) {
return r.UnpackBytes([]byte(s))
func (r Reflector) UnmarshalString(s string) (interface{}, error) {
return r.Unmarshal([]byte(s))
}

func (r Reflector) UnpackBytes(b []byte) (interface{}, error) {
var jsonMap interface{}
if err := json.Unmarshal(b, &jsonMap); err != nil {
func (r Reflector) Unmarshal(b []byte) (interface{}, error) {
var m interface{}
if err := json.Unmarshal(b, &m); err != nil {
return nil, fmt.Errorf("unpacker error parsing JSON: %w", err)
}
return r.UnpackMap(jsonMap)
}

func (r Reflector) UnpackMap(m interface{}) (interface{}, error) {
object, ok := m.(map[string]interface{})
if !ok {
return nil, fmt.Errorf("cannot unpack non-object JSON value")
Expand All @@ -93,22 +87,18 @@ func (r Reflector) UnpackMap(m interface{}) (interface{}, error) {
if rv, ok := skeleton.(reflect.Value); ok {
skeleton = rv.Interface()
}
v := skeleton
if _, ok := v.(map[string]interface{}); ok {
// If the root record wasn't decoded to a struct ptr,
// we pass a pointer to mapstructure as it requires
// a pointer val.
v = &skeleton
}
c := &mapstructure.DecoderConfig{
TagName: "json",
Result: v,
if err := json.Unmarshal(b, &skeleton); err != nil {
return nil, err
}
dec, err := mapstructure.NewDecoder(c)
return skeleton, err
}

func (r Reflector) UnmarshalObject(v interface{}) (interface{}, error) {
b, err := json.Marshal(v)
if err != nil {
return nil, fmt.Errorf("unpack (mapstructure): %w", err)
return nil, err
}
return skeleton, dec.Decode(m)
return r.Unmarshal(b)
}

func (r Reflector) lookup(object map[string]interface{}) (reflect.Value, error) {
Expand Down
16 changes: 8 additions & 8 deletions pkg/unpack/unpack_test.go
Expand Up @@ -51,7 +51,7 @@ func TestUnpackBinaryExpr(t *testing.T) {
Terminal{},
List{},
)
actual, err := reflector.Unpack(binaryExprJSON)
actual, err := reflector.UnmarshalString(binaryExprJSON)
require.NoError(t, err)
assert.Equal(t, binaryExprExpected, actual)
}
Expand Down Expand Up @@ -80,7 +80,7 @@ func TestUnpackTypeTag(t *testing.T) {
BinaryExpr2{},
Terminal{},
)
actual, err := reflector.Unpack(typeTagJSON)
actual, err := reflector.UnmarshalString(typeTagJSON)
require.NoError(t, err)
assert.Equal(t, typeTagExpected, actual)
}
Expand Down Expand Up @@ -119,7 +119,7 @@ func TestUnpackNested(t *testing.T) {
Terminal{},
List{},
)
actual, err := reflector.Unpack(nestedJSON)
actual, err := reflector.UnmarshalString(nestedJSON)
require.NoError(t, err)
assert.Equal(t, nestedExpected, actual)
}
Expand Down Expand Up @@ -172,7 +172,7 @@ func TestUnpackEmbedded(t *testing.T) {
List{},
Embedded{},
)
actual, err := reflector.Unpack(embeddedJSON)
actual, err := reflector.UnmarshalString(embeddedJSON)
require.NoError(t, err)
assert.Equal(t, embeddedExpected, actual)
}
Expand All @@ -197,7 +197,7 @@ func TestUnpackList(t *testing.T) {
Terminal{},
List{},
)
actual, err := reflector.Unpack(listJSON)
actual, err := reflector.UnmarshalString(listJSON)
require.NoError(t, err)
assert.Equal(t, listExpected, actual)
}
Expand Down Expand Up @@ -242,7 +242,7 @@ func TestUnpackPairList(t *testing.T) {
Terminal{},
PairList{},
)
actual, err := reflector.Unpack(pairListJSON)
actual, err := reflector.UnmarshalString(pairListJSON)
require.NoError(t, err)
assert.Equal(t, pairListExpected, actual)
}
Expand Down Expand Up @@ -321,7 +321,7 @@ func TestUnpackCut(t *testing.T) {
Identifier{},
Assignment{},
)
actual, err := reflector.Unpack(cutJSON)
actual, err := reflector.UnmarshalString(cutJSON)
require.NoError(t, err)
assert.Equal(t, cutExpected, actual)
}
Expand Down Expand Up @@ -352,7 +352,7 @@ func TestUnpackSkip(t *testing.T) {
BinaryExpr3{},
Terminal{},
)
actual, err := reflector.Unpack(skipJSON)
actual, err := reflector.UnmarshalString(skipJSON)
require.NoError(t, err)
assert.Equal(t, skipExpected, actual)
}
7 changes: 6 additions & 1 deletion zio/zjsonio/writer.go
Expand Up @@ -26,7 +26,12 @@ func unmarshal(b []byte) (*Object, error) {
}
var types []zed.Type
for _, t := range template.Types {
object, err := unpacker.UnpackMap(t)
// We should enhance the unpacker to take the template struct
// here so we don't have to call UnmarshalObject. But not
// a big deal because we only do it for inbound zjson (which is
// not performance critical and only for typedefs which are
// typically infrequent.) See issue #2702.
object, err := unpacker.UnmarshalObject(t)
if object == nil || err != nil {
return nil, err
}
Expand Down

0 comments on commit 730d19c

Please sign in to comment.