Skip to content

avro: Extract DecoderResolver to provide cached ResolvingDecoder for resolving avro decoder - #1234

Merged
rdblue merged 3 commits into
apache:masterfrom
JingsongLi:DecoderResolver
Jul 26, 2020
Merged

avro: Extract DecoderResolver to provide cached ResolvingDecoder for resolving avro decoder#1234
rdblue merged 3 commits into
apache:masterfrom
JingsongLi:DecoderResolver

Conversation

@JingsongLi

Copy link
Copy Markdown
Contributor

No description provided.

Comment thread core/src/main/java/org/apache/iceberg/avro/GenericAvroReader.java Outdated
Comment on lines +48 to +52
ResolvingDecoder resolver = fileSchemaToResolver.get(fileSchema);
if (resolver == null) {
resolver = newResolver(readSchema, fileSchema);
fileSchemaToResolver.put(fileSchema, resolver);
}

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.

Could simplify those lines as fileSchemaToResolver.computeIfAbsent ?


private DecoderResolver() {}

private static final ThreadLocal<Map<Schema, Map<Schema, ResolvingDecoder>>> DECODER_CACHES =

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.

Q: Will it have problem when the GenericAvroReader , DataReader, SparkAvroReader share the same cache ?

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.

ResolvingDecoder is used by per record:

resolver.configure(decoder);
reader.read(resolver, reuse);
resolver.drain();

I think this pattern should be thread safe and can be shared.

@rdblue

rdblue commented Jul 26, 2020

Copy link
Copy Markdown
Contributor

Looks great! I'll merge this. Thanks @JingsongLi! And thanks to @openinx for reviewing as well.

@rdblue
rdblue merged commit cd283d8 into apache:master Jul 26, 2020
rdblue pushed a commit to rdblue/iceberg that referenced this pull request Jul 29, 2020
cmathiesen pushed a commit to ExpediaGroup/iceberg that referenced this pull request Aug 19, 2020
@JingsongLi
JingsongLi deleted the DecoderResolver branch November 5, 2020 09:42
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.

3 participants