[SPARK-39575][AVRO] add ByteBuffer#rewind after ByteBuffer#get in Avr…

[wzx140]

…oDeserializer

What changes were proposed in this pull request?

Add ByteBuffer#rewind after ByteBuffer#get in AvroDeserializer.

Why are the changes needed?

• HeapBuffer.get(bytes) puts the data from POS to the end into bytes, and sets POS as the end. The next call will return empty bytes.
• The second call of AvroDeserializer will return an InternalRow with empty binary column when avro record has binary column.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Add ut in AvroCatalystDataConversionSuite.

[wzx140]

@gengliangwang Would you like to review this pr?

[mridulm]

+CC @xkrogen

[srowen]

I think you're right but can you explain why this is needed?

[wzx140]

HeapBuffer.get(bytes) puts the data from POS to the end into bytes, and sets POS as the end. The next call will return empty bytes. You can take a look at added unit test. The second call of deserializer will return an InternalRow with empty binary column.

[srowen]

Sounds good. I wonder why this never surfaced before? seems like it would mean any binary cols in Avro don't work?

[wzx140]

Maybe this is not common to call this twice to deserialize one avro data object

[xkrogen]

Code changes LGTM. I checked the other place the deserializer uses a ByteBuffer, in the (BYTES, _: DecimalType) case, and it doesn't have the same problem because decimalConversions.fromBytes() duplicates the ByteBuffer before extracting from it.

Can you update the PR description to have more details on why this is needed, basically what you described in your comment? It might also be helpful to update the summary to be more descriptive about the impact rather than the mechanics of the change, something like "Fix repeated deserialization of BYTES type in AvroDeserializer" (and then the body can describe the mechanical/technical aspects of the change).

[xkrogen]

Why is there a nested type here instead of just "type": "bytes" at the top-level?

[wzx140]

This is not necessary and it has been removed.

[wzx140]

Code changes LGTM. I checked the other place the deserializer uses a ByteBuffer, in the (BYTES, _: DecimalType) case, and it doesn't have the same problem because decimalConversions.fromBytes() duplicates the ByteBuffer before extracting from it.

Can you update the PR description to have more details on why this is needed, basically what you described in your comment? It might also be helpful to update the summary to be more descriptive about the impact rather than the mechanics of the change, something like "Fix repeated deserialization of BYTES type in AvroDeserializer" (and then the body can describe the mechanical/technical aspects of the change).

@xkrogen Already updated the PR description.

[AmplabJenkins]

Can one of the admins verify this patch?

[srowen]

Can you try rebasing and pushing? some doc build steps failed, which is unrelated to the change, but might resolve it

[wzx140]

@srowen Thanks for your help. I have rebased and pushed. Now github Action is all successful.

[srowen]

Merged to master/3.3/3.2