Skip to content

Conversation

@panbingkun
Copy link
Contributor

@panbingkun panbingkun commented Aug 27, 2024

What changes were proposed in this pull request?

The pr aims to add 3 test cases for from_avro/to_avro from the perspective of GenericRecord.

Why are the changes needed?

Just to add test cases and help better understand these two functions: from_avro & to_avro.

Does this PR introduce any user-facing change?

No, only just supplementary test cases.

How was this patch tested?

Update existed UT.

Was this patch authored or co-authored using generative AI tooling?

No.

@panbingkun panbingkun marked this pull request as ready for review August 27, 2024 10:55
@panbingkun
Copy link
Contributor Author

cc @MaxGekk

Copy link
Member

@MaxGekk MaxGekk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

record
}

test("GenericRecord serialize/deserialize") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this test check Spark's code? If not, let's remove it.

}
}

test("use `to_avro` to read GenericRecord(stored in `struct` datatype)") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to read? it should write, shouldn't it?

Copy link
Contributor Author

@panbingkun panbingkun Aug 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have refactored this code and it seems clearer now. Please help review it again in your free time, thanks!

@panbingkun panbingkun changed the title [SPARK-49406][SQL][TESTS] Add two test cases for from_avro/to_avro from the perspective of GenericRecord [SPARK-49406][SQL][TESTS] Add three test cases for from_avro/to_avro from the perspective of GenericRecord Aug 29, 2024
@panbingkun panbingkun requested a review from MaxGekk August 29, 2024 02:11
@MaxGekk
Copy link
Member

MaxGekk commented Aug 29, 2024

+1, LGTM. Merging to master.
Thank you, @panbingkun.

@MaxGekk MaxGekk closed this in d78199a Aug 29, 2024
@panbingkun
Copy link
Contributor Author

+1, LGTM. Merging to master. Thank you, @panbingkun.

Thanks @MaxGekk.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants