You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@beam.apache.org by "jmackay@godaddy.com" <jm...@godaddy.com> on 2020/01/28 02:18:51 UTC
Subclassing MapElements
Hi Beam Community,
Our team has a number of PTransforms that are basically wrappers around MapElements, which give us a concise syntax when specifying pipelines which leverage shared map stages. One example that we are looking at currently is a function which takes JSON and maps it into ProtoBuf Message object using protos declarative type annotation to specify metadata about how to perform the transform. Our proto logic is a bit custom so it might not be suitable for upstreaming. Our current implementations of these mapper PTransforms are pretty simple ‘expand’ methods which perform the mapping and reduce code clutter on our pipeline code. We recently found that we may want to leverage some of the bells and whistles of MapElements when we use these PTransforms, such as ‘ exceptionsInto’ etc. It seems like a clean way to do this would be to subclass MapElements directly and just override the expand function, but MapElements has a private constructor and builders etc… Im wondering if there is an existing way to do this, or maybe if it would be useful to make the MapElements constructor protected? Feels like this would be a little funny since MapElements uses a builder etc. Curious if anyone has a way to do what we are trying to do here, or hear any other thoughts on this.
-Jason
Re: Subclassing MapElements
Posted by "jmackay@godaddy.com" <jm...@godaddy.com>.
Jeff,
Cool, thanks for the thoughts, the point about naming is a very good one. I think we will probably look to do more with static builder methods as we discussed.
Cheers!
-J
From: Jeff Klukas <jk...@mozilla.com>
Reply-To: "dev@beam.apache.org" <de...@beam.apache.org>
Date: Tuesday, January 28, 2020 at 12:57 PM
To: dev <de...@beam.apache.org>
Subject: Re: Subclassing MapElements
Notice: This email is from an external sender.
Jason - I was going to suggest the same approach that you arrived at. I think using a static method to a MapElements instance is an elegant solution.
The one drawback I can think of with that approach is that you lose control over default naming. From your example usage:
myPCollection.apply(MyMapper.of(…).exceptionsInto(…))
if MyMapper returns a MapElements instance, then this line will appear in your pipeline graph as "MapWithFailures". If you do the more verbose thing and extend PTransform, then the default name would be "MyMapper" from the name of the class.
You can easily get around this problem by passing an explicit name to `apply` when using the transform (and indeed this style is encouraged in the Beam docs), but it could be a nuisance if you're used to relying on default names that match class names.
On Tue, Jan 28, 2020 at 6:41 PM jmackay@godaddy.com<ma...@godaddy.com> <jm...@godaddy.com>> wrote:
Yeah it just seems like a lot of boiler plate to do builders with lots of methods just to wrap a MapElements type for syntactic convenience. After thinking this over last night I’m wondering if it wouldn’t be better to use static factory methods for these, so rather than the following, which tends to be what we see in the PTransform style guide etc, but hides the MapElements instance and builder calls, closing off the callers opportunity to add additional tweaks to the transform:
public MyMapper extends PTransform<PCollection<U>, PCollection<V> {
@Override
public PCollection<T> expand(PCollection<U> input) {
return MapElements.into(…).via(…);
}
}
we might instead do something like this
public MyMapper {
public static MapElements<U, V> of(…) {
return MapElements.into(…).via(…);
}
}
Which might be used like so (shown with usage of MapElements additional calls):
myPCollection.apply(MyMapper.of(…).exceptionsInto(…))
For us, this saves us from having a lot of typedescriptor code and such ending up in high level pipeline setups, making it easier to understand.
-Jason
From: Kenneth Knowles <ke...@apache.org>>
Reply-To: "dev@beam.apache.org<ma...@beam.apache.org>" <de...@beam.apache.org>>
Date: Monday, January 27, 2020 at 9:10 PM
To: dev <de...@beam.apache.org>>
Subject: Re: Subclassing MapElements
Notice: This email is from an external sender.
You can, with care, create an abstract Builder class that can be extended. You have to be careful to never call a concrete constructor and only call its own builder methods, leaving the final construction abstract. Basically a bunch of setter methods in a builder style.
Kenn
On Mon, Jan 27, 2020 at 9:08 PM Kenneth Knowles <ke...@apache.org>> wrote:
It might be more trouble than it is worth, saving typing but adding complexity. Especially since you've got @AutoValue and @AutoValue.Builder to do all the heavy lifting anyhow (https://beam.apache.org/contribute/ptransform-style-guide/#api).
Kenn
Re: Subclassing MapElements
Posted by Jeff Klukas <jk...@mozilla.com>.
Jason - I was going to suggest the same approach that you arrived at. I
think using a static method to a MapElements instance is an elegant
solution.
The one drawback I can think of with that approach is that you lose control
over default naming. From your example usage:
myPCollection.apply(MyMapper.of(…).exceptionsInto(…))
if MyMapper returns a MapElements instance, then this line will appear in
your pipeline graph as "MapWithFailures". If you do the more verbose thing
and extend PTransform, then the default name would be "MyMapper" from the
name of the class.
You can easily get around this problem by passing an explicit name to
`apply` when using the transform (and indeed this style is encouraged in
the Beam docs), but it could be a nuisance if you're used to relying on
default names that match class names.
On Tue, Jan 28, 2020 at 6:41 PM jmackay@godaddy.com <jm...@godaddy.com>
wrote:
> Yeah it just seems like a lot of boiler plate to do builders with lots of
> methods just to wrap a MapElements type for syntactic convenience. After
> thinking this over last night I’m wondering if it wouldn’t be better to use
> static factory methods for these, so rather than the following, which tends
> to be what we see in the PTransform style guide etc, but hides the
> MapElements instance and builder calls, closing off the callers opportunity
> to add additional tweaks to the transform:
>
>
>
> public MyMapper extends PTransform<PCollection<U>, PCollection<V> {
>
> @Override
>
> public PCollection<T> expand(PCollection<U> input) {
>
> return MapElements.into(…).via(…);
>
> }
>
> }
>
> we might instead do something like this
>
>
>
>
> public MyMapper {
>
> public static MapElements<U, V> of(…) {
>
> return MapElements.into(…).via(…);
> }
> }
>
>
>
> Which might be used like so (shown with usage of MapElements additional
> calls):
>
>
> myPCollection.apply(MyMapper.of(…).exceptionsInto(…))
>
>
>
>
>
> For us, this saves us from having a lot of typedescriptor code and such
> ending up in high level pipeline setups, making it easier to understand.
>
>
>
> -Jason
>
>
>
> *From: *Kenneth Knowles <ke...@apache.org>
> *Reply-To: *"dev@beam.apache.org" <de...@beam.apache.org>
> *Date: *Monday, January 27, 2020 at 9:10 PM
> *To: *dev <de...@beam.apache.org>
> *Subject: *Re: Subclassing MapElements
>
>
>
> Notice: This email is from an external sender.
>
>
>
> You can, with care, create an abstract Builder class that can be extended.
> You have to be careful to never call a concrete constructor and only call
> its own builder methods, leaving the final construction abstract. Basically
> a bunch of setter methods in a builder style.
>
>
>
> Kenn
>
>
>
> On Mon, Jan 27, 2020 at 9:08 PM Kenneth Knowles <ke...@apache.org> wrote:
>
> It might be more trouble than it is worth, saving typing but adding
> complexity. Especially since you've got @AutoValue and @AutoValue.Builder
> to do all the heavy lifting anyhow (
> https://beam.apache.org/contribute/ptransform-style-guide/#api).
>
>
>
> Kenn
>
>
Re: Subclassing MapElements
Posted by "jmackay@godaddy.com" <jm...@godaddy.com>.
Yeah it just seems like a lot of boiler plate to do builders with lots of methods just to wrap a MapElements type for syntactic convenience. After thinking this over last night I’m wondering if it wouldn’t be better to use static factory methods for these, so rather than the following, which tends to be what we see in the PTransform style guide etc, but hides the MapElements instance and builder calls, closing off the callers opportunity to add additional tweaks to the transform:
public MyMapper extends PTransform<PCollection<U>, PCollection<V> {
@Override
public PCollection<T> expand(PCollection<U> input) {
return MapElements.into(…).via(…);
}
}
we might instead do something like this
public MyMapper {
public static MapElements<U, V> of(…) {
return MapElements.into(…).via(…);
}
}
Which might be used like so (shown with usage of MapElements additional calls):
myPCollection.apply(MyMapper.of(…).exceptionsInto(…))
For us, this saves us from having a lot of typedescriptor code and such ending up in high level pipeline setups, making it easier to understand.
-Jason
From: Kenneth Knowles <ke...@apache.org>
Reply-To: "dev@beam.apache.org" <de...@beam.apache.org>
Date: Monday, January 27, 2020 at 9:10 PM
To: dev <de...@beam.apache.org>
Subject: Re: Subclassing MapElements
Notice: This email is from an external sender.
You can, with care, create an abstract Builder class that can be extended. You have to be careful to never call a concrete constructor and only call its own builder methods, leaving the final construction abstract. Basically a bunch of setter methods in a builder style.
Kenn
On Mon, Jan 27, 2020 at 9:08 PM Kenneth Knowles <ke...@apache.org>> wrote:
It might be more trouble than it is worth, saving typing but adding complexity. Especially since you've got @AutoValue and @AutoValue.Builder to do all the heavy lifting anyhow (https://beam.apache.org/contribute/ptransform-style-guide/#api).
Kenn
Re: Subclassing MapElements
Posted by Kenneth Knowles <ke...@apache.org>.
You can, with care, create an abstract Builder class that can be extended.
You have to be careful to never call a concrete constructor and only call
its own builder methods, leaving the final construction abstract. Basically
a bunch of setter methods in a builder style.
Kenn
On Mon, Jan 27, 2020 at 9:08 PM Kenneth Knowles <ke...@apache.org> wrote:
> It might be more trouble than it is worth, saving typing but adding
> complexity. Especially since you've got @AutoValue and @AutoValue.Builder
> to do all the heavy lifting anyhow (
> https://beam.apache.org/contribute/ptransform-style-guide/#api).
>
> Kenn
>
Re: Subclassing MapElements
Posted by Kenneth Knowles <ke...@apache.org>.
It might be more trouble than it is worth, saving typing but adding
complexity. Especially since you've got @AutoValue and @AutoValue.Builder
to do all the heavy lifting anyhow (
https://beam.apache.org/contribute/ptransform-style-guide/#api).
Kenn