Skip to content

Extend avro reader to support more primitive types#16280

Merged
beinan merged 1 commit intoprestodb:masterfrom
yangy0000:avro
Jan 26, 2022
Merged

Extend avro reader to support more primitive types#16280
beinan merged 1 commit intoprestodb:masterfrom
yangy0000:avro

Conversation

@yangy0000
Copy link
Copy Markdown
Contributor

@yangy0000 yangy0000 commented Jun 16, 2021

cherry-pick of trinodb/trino@bf91d13

Co-Authored-By: Elon Azoulay elon.azoulay@gmail.com

Test plan - (Please fill in how you tested your changes)

Please make sure your submission complies with our Development, Formatting, and Commit Message guidelines. Don't forget to follow our attribution guidelines for any code copied from other projects.

Fill in the release notes towards the bottom of the PR description.
See Release Notes Guidelines for details.

== RELEASE NOTES ==

Avro Decoder Changes
* Extend avro reader to support more primitive types 

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@beinan can you help to review the PR, thanks!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@beinan gentle bump :)

Copy link
Copy Markdown
Member

@beinan beinan left a comment

Choose a reason for hiding this comment

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

Thank you for this contribution! Looks good to me except a couple clarification questions. Could you also update the release note section in this PR? Thanks!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just wanna confirm, could the value be an Integer or something else here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes, the value could be Integer, which will fall into

type.writeLong(blockBuilder, ((Number) value).longValue());
I'm not an expert on this, but my test indicate this line it is compatible with Integer

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

do you have any context about why we get rid of Utf8 here? or is there any test covered the VARCHAR type to make sure we're not breaking anything?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Utf8 implements CharSequence so it should be more compatible in situations where the Avro schema used a different class for string types.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I see, thank you for the explanation!

cherry-pick of trinodb/trino@bf91d13

Co-Authored-By: Elon Azoulay <elon.azoulay@gmail.com>
@yangy0000
Copy link
Copy Markdown
Contributor Author

Thank you for this contribution! Looks good to me except a couple clarification questions. Could you also update the release note section in this PR? Thanks!

updated

Copy link
Copy Markdown
Member

@beinan beinan left a comment

Choose a reason for hiding this comment

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

lgtm

@beinan beinan merged commit 8c2757a into prestodb:master Jan 26, 2022
@neeradsomanchi neeradsomanchi mentioned this pull request Feb 8, 2022
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants