Avro: Change union read schema from hive to trino

[rzhang10]

This rb changes the avro spark reader code path to read a union data type of [int, string] to a spark schema/data of [tag, field0, field1] instead of the previous [tag_0, tag_1].
it also fixed an issue that when the avro union is a null, the reader should return a plain null instead of [0, null, null] or [null, null] (as the previous schema did).

[funcheetah]

Thanks @rzhang10 for the PR! Could you please integrate test to make sure the new schema works?

[autumnust]

is this considered invalid at all? or what's the reason to get rid of this case ?

[rzhang10]

Yes, this schema itself shouldn't appear in the first place, user shouldn't define this kind of schema.

[autumnust]

is this modifier change necessary ? this might make it harder for us to contributing back in the future.

[autumnust]

do you also need to do the same for the vectorized orc reader?

[rzhang10]

This mimics the same access control modifier for the similar method in this class SparkValueReaders, the relevant change has been made in SparkOrcValueReaders in pr #85 .

[autumnust]

What about the modifier change ? Is that really needed?

[rzhang10]

Yeah if you look at other nested class in this file you will see:
private static class MapReader
private static class EnumReader
private static class ArrayReader

so I think it's best to make the modifiers consistent.

[autumnust]

is there an official definition of what is complex and what is simple ? If we target to contribute it back, maybe better to clarify that thru javadoc -- If this is merely internally usage I am fine with what we have now.

[rzhang10]

simple union is [sometype, null] while complex union is [sometype1, sometype2, ...], where there are at least 2 non-null types in the union, I think we can probably add a java doc in AvroSchemaUtil.isOptionSchema, which is an existing method in upstream iceberg.

[autumnust]

Not sure if I follow this, what does the relative position between value index and null index have anything to do with how we assign the value in the struct ?

[rzhang10]

Because in avro the nullability of a union type is presented by the existence of a NULL type inside the union alternatives, and the NULL type can occur in any order inside the union, but in our converted struct, we don't have a field that corresponds to that NULL type, thus we have -1 field count.

[autumnust]

I guess what I didn't know earlier was, if NULL type appears in the union it seems it has to be the first position in
the alternative list ? But the avro spec said "Thus, for unions containing "null", the "null" is usually listed first,"

Will this lead to a silent failure if you don't check this first?

[rzhang10]

I think that's just a recommendation from the Avro spec, not mandated, in fact, in our ecosystem there are many avro schemas which failed to put null as the first type element in the union.

And that's why I'm specifically computing the null index here and branching on it.

[autumnust]

why is this needed if you are still returning struct which is a InternalRow ?

[rzhang10]

See line 323, if it's null data, we will return null directly, so this method returns either null or an InternalRow, so the return type should only be Object. Please see the same method signature in ValueReaders.

[autumnust]

So why not returning null directly at line 323 if nullIndex is hit ?

[rzhang10]

readers[index].read(decoder, reuse) will return null, I just feel writing this way will make the code more consistent with later part.

[autumnust]

Hmm not sure that's worthwhile with the cost of changing the method signature ... But I don't have strong opinion on this either.

[rzhang10]

I think using InternalRow is better, let me make the change accordingly. Thanks for spotting this!

[autumnust]

A couple of more comments ... @rzhang10

[autumnust]

LGTM. Most of conservations above are just questions. I still believe we shouldn't change the signature of union read method from InternalRow to Object since a return type as Object doesn't seem to right if we can make it more specific. But I am no strong opinion on that and don't take that as blocker.