Decoding of map[string]any behavior changed

[thorfour]

I noticed that the behavior of decoding a complex type of map[string]any changed between v2.19.0 and v2.20.0 and was curious if that was intended to change how avro parses this type of struct?

The test below encodes and decodes a struct that has a map[sting]any element.
If I decode with v2.19.0 I get an output of {map[a:1 b:2]} which is what I was expectingas that's the map type I encoded. But when upgrading to v2.20.0 or higher I get {map[a:map[int:1] b:map[int:2]]}

Am I doing something wrong or is this working as intended?

  func Test_Avro(t *testing.T) {
      schema := `{
          "type": "record",
          "name": "data",
          "fields": [
              { "name": "partition", "type": {"type": "record", "name": "test1", "fields": [
                  { "name": "a", "type": ["null", "int"]},
                  { "name": "b", "type": ["null", "int"]}
              ]}}
          ]
      }`

      avro.MustParse(schema)

      type Data struct {
          Partition map[string]any `avro:"partition"`
      }

      w := &bytes.Buffer{}
      enc, err := ocf.NewEncoder(
          schema,
          w,
          ocf.WithMetadata(map[string][]byte{
              "avro.codec": []byte("deflate"),
          }),
          ocf.WithCodec(ocf.Deflate),
      )
      require.NoError(t, err)

      require.NoError(t, enc.Encode(Data{
          Partition: map[string]any{
              "a": 1,
              "b": 2,
          },
      }))

      require.NoError(t, enc.Close())

      // Read back the file
      d, err := ocf.NewDecoder(bytes.NewReader(w.Bytes()))
      require.NoError(t, err)

      for d.HasNext() {
          var data Data
          require.NoError(t, d.Decode(&data))

          fmt.Println(data)
      }

  }

[nrwiersma]

Yes, this was intended.

Map mode record decode map[string]any is generic decoding, and should be inline with reader.ReadNext. In generic decoding Unions should be in the form map[string]any, which they now are.

In 1.18, generic decoding and struct decoding were split, generic decoding being handled by reader.ReadNext. In 1.19 they were merged to allow for composable schemas, so all generic decoding happens outside of the reader and also supports composable schemas. In 1.20, this issue was discovered and fixed along with performance improvements to the new generic decoding.

Side Note: It is a little odd that you mix generic and struct en/decoding. As this is schemaed, I would expect it all to be structs, as this is has higher performance. Generic decoding is more difficult for the decoder, and has a heavy performance penalty.

[thorfour]

Thanks for the quick info! I appreciate it.

What do you mean by mixing genreric and struct? The partition struct above gets populated at runtime with potentially arbitrary types, so I needed a way to encode those types. I template the avro spec based off of the types that are found in the map.

Edit: I tried using reader.ReadNext and there's no way for it to read the encoding into a struct like decode does, so that's unfortunate.

[nrwiersma]

What do you mean by mixing generic and struct?

Data here is a strict that maps to the data record. Partition is a map[string]any that maps to the partition. The map->record decoding is what I refer to as generic as it is not a specific type, where the struct is a fixed runtime type. The struct decoding is fast, the map decoding is slow due to number of allocations it must make. If you check the avro benchmarks you see the difference:

BenchmarkSuperheroDecode-8          	 5282898	       224.4 ns/op	      47 B/op	       0 allocs/op
BenchmarkSuperheroGenericDecode-8   	  546651	      2170 ns/op	    2040 B/op	      60 allocs/op

The partition struct above gets populated at runtime with potentially arbitrary types

That makes sense then. Just through I would point it out.

[TheGreatAbyss]

Hey @thorfour

Did you find a work around for this? Is there anything better then iterating through the decoded results and converting every value from map[string]any to the underlying interface?

I also like not having to hardcode structs into my application. The schema is already laid out in Schema Registry so I'd rather control it dynamically from there, then having to coordinate a redeploy on every minor schema change.

[thorfour]

@TheGreatAbyss unfortunately no I didn't. My solution was to pin my version of this library at v2.19.0 and not upgrade.

[TheGreatAbyss]

I'll probably do the same for now. Thanks

[TheGreatAbyss]

@nrwiersma

Thank you for this library, it's amazing. I'm not even going to pretend I understand the bowels of everything that it's doing, but following along the decoder it looks like the actual interface value is extracted all the way up until this line:

avro/codec_union.go

Line 115 in 4aff30f

d.mapType.UnsafeSetIndex(ptr, keyPtr, elemPtr)

UnsafeSetIndex in the reflect package is what is wrapping them in a map. Again without completely understanding everything, the line above it actually sets the pointer to the underlying interface. Is there any reason to call UnsafeSetIndex? If so can something else in Reflect be called? Like what was happening in <= 19?

Thanks again, and apologies if my question is not informed.

[nrwiersma]

UnsafeSetIndex sets the map key to a value. Because it is Unsafe everything is a pointer. If you have a string, it is actually using a *string in the form of an unsafe.Pointer.

[nrwiersma]

Wait. It is possible I got something wrong in my explanation. I need to check this.

[TheGreatAbyss]

Thank You!!!

FWIW, In the old version this was the code that worked:

	default:
		newPtr = typ.UnsafeNew()
	}

	d.decoders[i].Decode(newPtr, r)
	*pObj = typ.UnsafeIndirect(newPtr)
}

[TheGreatAbyss]

The current version still calls genericDecode which does the same thing as the old version:

func genericDecode(typ reflect2.Type, dec ValDecoder, r *Reader) any {
	ptr := typ.UnsafeNew()
	dec.Decode(ptr, r)
	if r.Error != nil {
		return nil
	}

	obj := typ.UnsafeIndirect(ptr)
	if reflect2.IsNil(obj) {
		return nil
	}
	return obj
}

But once that object is returned, instead of it being set, you now call d.mapType.UnsafeSetIndex(ptr, keyPtr, elemPtr) which is what causes the map of maps for unions.

Again sorry if I'm over my head but just trying to help make this more seamless for users.

[nrwiersma]

The actual thing we are looking at is this: https://github.com/hamba/avro/blob/v2.24.0/codec_record.go#L278

It is possible it is actually a bug. I apologise if I missed this. Just need to double check everything before I actually call it a bug.

[TheGreatAbyss]

I never get to that code block in my tests which have a simple schema of something like:

  "type": "record",
  "name": "example",
  "namespace": "example",
  "fields": [
    ....
    {
      "name": "subscriber_id",
      "type": [
        "null",
        "string"
      ],
      "logicalType": "UUID"
    },

]

I simple encode a test map[string]any and then decode it back into another map[string]any

Regardless, thank you for investigating!!

[nrwiersma]

Here is the comparison I am making, based on the original schema and example:
In 2.18 the decode results are as follows:

// Decoding into Data:
{map[a:1 b:2]}
// Decoding into map[string]any:
map[partition:map[a:map[int:1] b:map[int:2]]]
// reader.ReadNext():
map[partition:map[a:map[int:1] b:map[int:2]]]

in 2.24 the same result is:

// Decoding into Data:
{map[a:map[int:1] b:map[int:2]]}
// Decoding into map[string]any:
map[partition:map[a:map[int:1] b:map[int:2]]]
// reader.ReadNext():
map[partition:map[a:map[int:1] b:map[int:2]]]

So in effect, the change was to aline all the decodings to be the same. So I would not class this as a bug.
I hope that while it is not the result you were looking for, it is a satisfactory result.

[nrwiersma]

The flip side of the argument is that it is an interface and should be handled by the Resolver, which would not be embedded without the map key.

[TheGreatAbyss]

I think the original example is adding some confusion here being a record with a nested record. Now I understand why you were looking at the block of code.

If you just have a simple schema without nested records the old versions simply decoded into a map[string]any. Now it's decoding into map[sting]map[string]any

Example schema:

  "type": "record",
  "name": "example",
  "namespace": "example",
  "fields": [
    {
      "name": "subscriber_id",
      "type": [
        "null",
        "string"
      ],
      "logicalType": "UUID"
    },

Since I stick to flat simple schemas, as far as I'm concerned there is no consistency issues. It went from flat to flat to flat to map for seemingly no reason for a very simple schema.

I can work on posting a complete example. My tests have a bunch of underlying mocks and types so it's not easy to quickly copy and paste

[nrwiersma]

@redaLaanait You want to weigh in here? I don't really know which handling is "technically" correct here. Handling this like reader.ReadNext is what is now done, but that might not be correct if you see this as decoding into an interface{}. However this requires complex types to be regesterd.

[TheGreatAbyss]

Here's a simple example. This test passes in <= 19 but fails now:

func TestLeaveMeExampleOfConvertToAvroWithMap(t *testing.T) {
	schema, err := avro.Parse(`{
    "type": "record",
    "name": "simple",
    "namespace": "org.hamba.avro",
    "fields" : [
        {"name": "a", "type": "int"},
        {"name": "b", "type": "string"},
		{"name": "c", "type": "string"},
		{"name": "d", "type": "string"},
        {"name": "e","type": ["null","string"],"default": null}]
}`)
	require.NoError(t, err)

	// encode
	// Need to add and remove magic bytes before and after the wire
	require.NoError(t, err)
	buf := bytes.NewBuffer([]byte{})
	enc := avro.NewEncoderForSchema(schema, buf)

	obj2 := map[string]any{
		"a": 3,
		"b": "John",
		"c": "Jill",
		"d": "Jack",
		"e": "George",
	}

	err = enc.Encode(obj2)
	data2 := buf.Bytes()
	fmt.Println(data2)

	// Decode
	decoder := avro.NewDecoderForSchema(schema, buf)
	stringToInterfaceMap := map[string]interface{}{}

	err = decoder.Decode(&stringToInterfaceMap)
	require.NoError(t, err)
	fmt.Println(stringToInterfaceMap)

	assert.EqualValues(t, obj2, stringToInterfaceMap)
}

IMHO - Seems inconsistent to not be able to decode into exactly the same object you encoded in the first place.

[nrwiersma]

@TheGreatAbyss So the discussion is about how Unions are handled in a Record represented by a map[string]any. The nesting does not add anything or hinder anything. It can be seen in 1 of 2 ways:

1. interface{}: This looks like 2.18, but is not exactly the same.
2. map[string]any: This is the same as reader.ReadNext() and the method selected. This can decode anything.

We need to first check that nested schemas do not break in Option 1, and figure out the "correct" way forward.
I will reopen this issue for the mean time.

[nrwiersma]

Seeing as this changed as part of an optimisation commit, I am tending towards this being a bug.

[nrwiersma]

@TheGreatAbyss Could you test the above PR to see that it sorts your issue, for confirmation?

[TheGreatAbyss]

Yes it passed!!

	github.com/hamba/avro/v2 v2.24.1-0.20240806190614-b616a1aa82a2

Thank You!

[thorfour]

Oh wow this is great news!

[nrwiersma]

@thorfour Apologies for not digging deeper the first time. I will see if @redaLaanait has time to look and double check, as he did the original work, so could take a short while, but the more I look at it, the more it seems that I made a mistake here.

[TheGreatAbyss]

Thank You for such a quick turnaround!