[BEAM-7802] Expose a method to make an Schema coder from an Avro coder #9130

iemejia · 2019-07-23T19:34:17Z

This is just my first PR around Schema/SQL. I found that it was not straight forward to transform an Avro-based PCollection into a Beam schema one so did this. It also does some minor clean ups in AvroUtils and the SQL example.

R: @reuvenlax @kanterov

reuvenlax · 2019-07-23T20:21:36Z

Very cool!

…

On Tue, Jul 23, 2019 at 12:34 PM Ismaël Mejía ***@***.***> wrote: This is just my first PR while playing a bit with Schemas and SQL. I found that it was not straight forward to transform an Avro-based PCollection into a Beam schema one so did this. It also does some minor clean ups in AvroUtils and the SQL example. R: @reuvenlax <https://github.com/reuvenlax> ------------------------------ You can view, comment on, or merge this pull request online at: #9130 Commit Summary - [BEAM-7802] Make SQL example slightly simpler - [BEAM-7802] Inline AvroUtils methods to have only one public AvroUtils class in core SDK - [BEAM-7802] Expose a method to make an Avro-based PCollection into an Schema-based one - [BEAM-7802] Fix minor issues (access modifiers + static) in AvroUtils File Changes - *M* sdks/java/core/src/main/java/org/apache/beam/sdk/io/AvroIO.java <https://github.com/apache/beam/pull/9130/files#diff-0> (55) - *D* sdks/java/core/src/main/java/org/apache/beam/sdk/io/AvroUtils.java <https://github.com/apache/beam/pull/9130/files#diff-1> (40) - *M* sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/utils/AvroUtils.java <https://github.com/apache/beam/pull/9130/files#diff-2> (28) - *M* sdks/java/core/src/test/java/org/apache/beam/sdk/schemas/utils/AvroUtilsTest.java <https://github.com/apache/beam/pull/9130/files#diff-3> (62) - *M* sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/example/BeamSqlExample.java <https://github.com/apache/beam/pull/9130/files#diff-4> (15) - *M* sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/example/BeamSqlPojoExample.java <https://github.com/apache/beam/pull/9130/files#diff-5> (17) Patch Links: - https://github.com/apache/beam/pull/9130.patch - https://github.com/apache/beam/pull/9130.diff — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#9130?email_source=notifications&email_token=AFAYJVMG43LNYQ3KSZNZ6RLQA5MMXA5CNFSM4IGIIBM2YY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4HBAPOHA>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFAYJVJ67DOAFUJSA3GSK33QA5MMXANCNFSM4IGIIBMQ> .

iemejia · 2019-08-02T13:33:28Z

Rebased to fix a merge issue PTAL when you have some time. Notice that there are other Avro / Schema / SQL improvements as part of this but all are isolated in its own commits to ease the review.

kanterov

I've left a few comments. I see your point that we need a way to use Avro SchemaCoder without changing each Source like KafkaIO, PubSubIO, etc. What I'm not certain about is the approach of having a static method that takes PCollection in favor of using existing APIs such as PCollection#setCoder and SchemaRegistry.

kanterov · 2019-08-05T09:15:02Z

sdks/java/core/src/main/java/org/apache/beam/sdk/io/AvroUtils.java

-import org.apache.beam.vendor.guava.v26_0_jre.com.google.common.base.Suppliers;
-
-/** Helpers for working with Avro. */
-class AvroUtils {


There is one more similar class in AvroCoder, but I guess changing it will introduce incompatible change into AvroCoder and break streaming pipelines, so let's keep it for now.

Ok just for curiosity which class is it?

AvroCoder.SerializableSchemaSupplier

But we can't change it, or it will break Java serialization.

Actually, I don't know how it works now, but it uses classes from vendored guava, that changes namespace each time we change guava version. I'm wondering if we already broke it without noticing.

I asked on dev, but probably we already broke it a couple of times

sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/utils/AvroUtils.java

kanterov · 2019-08-05T09:35:24Z

sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/utils/AvroUtils.java

+    if (!pc.hasSchema()) {
+      Schema beamSchema = getSchema(clazz, schema);
+      if (beamSchema != null) {
+        pc.setSchema(beamSchema, getToRowFunction(clazz, schema), getFromRowFunction(clazz));


pc.setSchema is equivalent to pc.setCoder(SchemaCoder.of(...))

I was thinking, what if we just create a static method to create SchemaCoder instead, then user-facing API would be like:

KafkaIO.read(MyRecord.class) .setCoder(AvroUtils.schemaCoder(MyRecord.class))

or, it can be done implicitly by registering classes in advance:

p.getSchemaRegistry().registerSchemaProvider(MyRecord.class, new AvroRecordSchema());

or for every SpecificRecord (didn't try this, but it should work as well):

p.getSchemaRegistry().registerSchemaProvider(SpecificRecord.class, new AvroRecordSchema());

Yes good idea, that looks cleaner I will add the schemaCoder method to AvroUtils, it should cover all the mentioned cases. will ping you back when done. Thanks for the review ideas.

iemejia · 2019-08-06T13:41:12Z

Did suggested updates PTAL @kanterov I added schemaCoder methods to match those of AvroCoder.of and one additional one to do it from an existing AvroCoder that one is relatively simple from the others and I can remove it if you prefer, but I ended up rewriting these two lines multiple times.

iemejia · 2019-08-06T13:43:27Z

I was starting to think if we should somehow make every AvroCoder PCollection a SchemaCoder for user friendliness, but I have my doubts on taking this 'implicit' approach, I am a bit worried about breaking backwards compatibility, but somehow it makes sense too, mmm... hard to decide/know.

kanterov · 2019-08-13T19:40:41Z

Changing AvroCoder will definitely break compatibility, especially streaming pipelines reading from PubSub or Kafka. In addition, SchemaCoder for Avro isn't as good (yet) as AvroCoder. As an example, it would serialize enums as strings, that is very inefficient when shuffling data. Another source of problems is that it doesn't support all Avro features. I believe once it matures we it could be the default, but we aren't there. In any case, I think it's a good exercise to think where we want to put SchemaCoder and how we are going to evolve AvroCoder, so, probably we should start a threat on dev@.

The code looks good. I agree and support your motivation on making fewer things private, but I don't find it practical to break it now given that we know for sure that there are codebases relying on it being public to avoid limitations of existing APIs, so I propose to postpone this before things stabilize.

sdks/java/core/src/main/java/org/apache/beam/sdk/io/AvroIO.java

…s class in core SDK Even if this class is not public, it is barely used so better to make it part of AvroIO.

…Test

…rom Avro

iemejia · 2019-08-15T21:38:47Z

@kanterov I restored the access modifiers and let everything as suggested.

I think we should merge this as it is and open the discussion in dev@ since the classes are still experimental we can still adapt the changes we conclude from the discussion and ‘experimental’ users can benefit of having this available in the meantime. WDYT?

kanterov · 2019-08-16T09:04:42Z

@iemejia great, agree, please feel free to merge when PreCommit check passes

iemejia · 2019-08-16T20:30:45Z

Merging since it is green now. Thanks for the review @kanterov and @RyanSkraba

iemejia requested a review from reuvenlax July 23, 2019 19:34

iemejia force-pushed the BEAM-7802-avro-schema-pcollection branch from 1d12b70 to ae3c28c Compare August 2, 2019 13:31

iemejia requested a review from kanterov August 2, 2019 13:31

kanterov reviewed Aug 5, 2019

View reviewed changes

iemejia force-pushed the BEAM-7802-avro-schema-pcollection branch from ae3c28c to 050e296 Compare August 6, 2019 13:38

RyanSkraba approved these changes Aug 14, 2019

View reviewed changes

iemejia added 5 commits August 15, 2019 22:31

[BEAM-7802] Make SQL example slightly simpler

0015cae

[BEAM-7802] Inline AvroUtils methods to have only one public AvroUtil…

0c1e63d

…s class in core SDK Even if this class is not public, it is barely used so better to make it part of AvroIO.

[BEAM-7802] Fix minor issues (access modifiers + static) in AvroUtils…

3cf7e4b

…Test

[BEAM-7802] Make Schema.toString method multi OS friendly

be6e077

[BEAM-7802] Add AvroUtils.schemaCoder method to infer a Beam schema f…

6978655

…rom Avro

iemejia force-pushed the BEAM-7802-avro-schema-pcollection branch from 050e296 to 6978655 Compare August 15, 2019 21:37

iemejia merged commit c825d71 into apache:master Aug 16, 2019

iemejia changed the title ~~[BEAM-7802] Expose a method to make an Avro-based PCollection into an Schema-based one~~ [BEAM-7802] Expose a method to make an Schema coder from an Avro coder Aug 16, 2019

iemejia deleted the BEAM-7802-avro-schema-pcollection branch August 16, 2019 20:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BEAM-7802] Expose a method to make an Schema coder from an Avro coder #9130

[BEAM-7802] Expose a method to make an Schema coder from an Avro coder #9130

iemejia commented Jul 23, 2019 •

edited

Loading

reuvenlax commented Jul 23, 2019 via email

iemejia commented Aug 2, 2019

kanterov left a comment

kanterov Aug 5, 2019

iemejia Aug 5, 2019

kanterov Aug 13, 2019

kanterov Aug 13, 2019

kanterov Aug 13, 2019

kanterov Aug 13, 2019

kanterov Aug 5, 2019

iemejia Aug 5, 2019

iemejia commented Aug 6, 2019

iemejia commented Aug 6, 2019 •

edited

Loading

kanterov commented Aug 13, 2019

iemejia commented Aug 15, 2019

kanterov commented Aug 16, 2019

iemejia commented Aug 16, 2019

[BEAM-7802] Expose a method to make an Schema coder from an Avro coder #9130

[BEAM-7802] Expose a method to make an Schema coder from an Avro coder #9130

Conversation

iemejia commented Jul 23, 2019 • edited Loading

reuvenlax commented Jul 23, 2019 via email

iemejia commented Aug 2, 2019

kanterov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iemejia commented Aug 6, 2019

iemejia commented Aug 6, 2019 • edited Loading

kanterov commented Aug 13, 2019

iemejia commented Aug 15, 2019

kanterov commented Aug 16, 2019

iemejia commented Aug 16, 2019

iemejia commented Jul 23, 2019 •

edited

Loading

iemejia commented Aug 6, 2019 •

edited

Loading