[DISCUSS] Changing from ImmutableBeans to Immutables

[Jacques Nadeau <ja...@apache.org>]

In the original creation of ImmutableBeans [1], there was a discussion of
using Immutables instead of something custom to do the configuration pojos.
Because no one did the work of integrating one of those off the shelf
packages at the time, we just went forward with the custom ImmutableBeans.
When I started working on supporting AOT compilation (specifically GraalVM,
CALCITE-4786) it became clear that the ways that the proxies are used in
Immutable beans are fairly difficult to make work correctly. (Especially
the evil ImmutableBeans.copy operation.)

Because of this, I opened up CALCITE-4787 [2] to replace ImmutableBeans
with the great Immutables package [6]. It should ultimately reduce the code
we have to maintain while providing an extended set of well maintained
capabilities. It uses an AnnotationProcessor to generate real code so you
can actually look at any immutable/builder classes and is all done at
compile time so performance is exceptional. The change feels like a win/win
(better maturity, less api surface area, ahead of time compilation) but I
wanted to make sure people were onboard with it. I've opened an initial PR
which shows the conversion for one class [3]. As you should be able to see,
the changes are fairly minimal to the code and configuration APIs don't
change.

There are ~40 classes that directly reference ImmutableBeans so this change
will do small amounts of changes to each but should not impact
functionality/behavior. However, the use of RelRule.Config.EMPTY actually
means that all rule files need a small change in them (even if they don't
directly reference ImmutableBeans). More details below on why this change
is necessary. Needless to say I believe it is minor and given the benefits
it is worth it. That being said, it will be a breaking change. As such, I
wanted to hear people's feedback before doing all the boring minor code
changes.

Are people okay with the breaking change? To give people an example of how
a rule would change, here is an example in a diff [4].

Thanks,
Jacques

---
More details on the differences between the two systems and why small rule
changes are necessary:

   - Item 1: ImmutableBeans is actually quite lenient on the construction
   of a proxy object (due to no separation of build vs copy). If a user
   doesn't populate a non-nullable field, ImmutableBeans doesn't actually
   declare any problem until the specific property is retrieved. On the other
   hand, Immutables is much stricter and has a clear distinction between build
   and copy (e.g. withFooProperty() type methods). If a property is abstract
   (no default method) and is not marked nullable, it is impossible in
   Immutables to construct a concrete POJO of that class. An example of this
   weird "broken class availability" is here [5] in the invocation chain.
   - Item 2: ImmutableBeans.copy (and the RelRule.Config.as() operation is
   best described as an unsafe cast. It works when used in the constrained
   example of interface subclassing by copying the invocation handlers to a
   new dynamic proxy. This is somewhat anti-pattern since most problems
   wouldn't occur until runtime. This pattern seems to exist in ImmutableBeans
   because it doesn't seem to distinguish between construction and copying. In
   Immutables you have the option to handle this two different ways: default
   values are always declared on the properties, possibly overridden OR
   through the use of a partial builder.


[1] https://issues.apache.org/jira/browse/CALCITE-3328
[2] https://issues.apache.org/jira/browse/CALCITE-4787
[3] https://github.com/apache/calcite/pull/2531
[4] https://www.diffchecker.com/zqTmEKdz
[5]
https://github.com/apache/calcite/blob/master/core/src/main/java/org/apache/calcite/rel/rules/ProjectFilterTransposeRule.java#L234
[6] https://immutables.github.io/

Re: [DISCUSS] Changing from ImmutableBeans to Immutables

[Jacques Nadeau <ja...@apache.org>]

Just to clarify my message slightly. Rule behaviors and interfaces wouldn't
change.

The only breaking change would be RelRule would no longer expose the 'as'
function nor the EMPTY constant. So new RelRules written outside of the
Calcite project would need to change when they used either of those items.

Also, Julian pointed out that my rule example was slightly misleading and a
larger change than necessary. I made a new diff to show smaller example
rule change:
https://www.diffchecker.com/Ba5a0agP




On Tue, Sep 21, 2021 at 5:47 PM Jacques Nadeau <ja...@apache.org> wrote:

> In the original creation of ImmutableBeans [1], there was a discussion of
> using Immutables instead of something custom to do the configuration pojos.
> Because no one did the work of integrating one of those off the shelf
> packages at the time, we just went forward with the custom ImmutableBeans.
> When I started working on supporting AOT compilation (specifically GraalVM,
> CALCITE-4786) it became clear that the ways that the proxies are used in
> Immutable beans are fairly difficult to make work correctly. (Especially
> the evil ImmutableBeans.copy operation.)
>
> Because of this, I opened up CALCITE-4787 [2] to replace ImmutableBeans
> with the great Immutables package [6]. It should ultimately reduce the code
> we have to maintain while providing an extended set of well maintained
> capabilities. It uses an AnnotationProcessor to generate real code so you
> can actually look at any immutable/builder classes and is all done at
> compile time so performance is exceptional. The change feels like a win/win
> (better maturity, less api surface area, ahead of time compilation) but I
> wanted to make sure people were onboard with it. I've opened an initial PR
> which shows the conversion for one class [3]. As you should be able to see,
> the changes are fairly minimal to the code and configuration APIs don't
> change.
>
> There are ~40 classes that directly reference ImmutableBeans so this
> change will do small amounts of changes to each but should not impact
> functionality/behavior. However, the use of RelRule.Config.EMPTY actually
> means that all rule files need a small change in them (even if they don't
> directly reference ImmutableBeans). More details below on why this change
> is necessary. Needless to say I believe it is minor and given the benefits
> it is worth it. That being said, it will be a breaking change. As such, I
> wanted to hear people's feedback before doing all the boring minor code
> changes.
>
> Are people okay with the breaking change? To give people an example of how
> a rule would change, here is an example in a diff [4].
>
> Thanks,
> Jacques
>
> ---
> More details on the differences between the two systems and why small rule
> changes are necessary:
>
>    - Item 1: ImmutableBeans is actually quite lenient on the construction
>    of a proxy object (due to no separation of build vs copy). If a user
>    doesn't populate a non-nullable field, ImmutableBeans doesn't actually
>    declare any problem until the specific property is retrieved. On the other
>    hand, Immutables is much stricter and has a clear distinction between build
>    and copy (e.g. withFooProperty() type methods). If a property is abstract
>    (no default method) and is not marked nullable, it is impossible in
>    Immutables to construct a concrete POJO of that class. An example of this
>    weird "broken class availability" is here [5] in the invocation chain.
>    - Item 2: ImmutableBeans.copy (and the RelRule.Config.as() operation
>    is best described as an unsafe cast. It works when used in the constrained
>    example of interface subclassing by copying the invocation handlers to a
>    new dynamic proxy. This is somewhat anti-pattern since most problems
>    wouldn't occur until runtime. This pattern seems to exist in ImmutableBeans
>    because it doesn't seem to distinguish between construction and copying. In
>    Immutables you have the option to handle this two different ways: default
>    values are always declared on the properties, possibly overridden OR
>    through the use of a partial builder.
>
>
> [1] https://issues.apache.org/jira/browse/CALCITE-3328
> [2] https://issues.apache.org/jira/browse/CALCITE-4787
> [3] https://github.com/apache/calcite/pull/2531
> [4] https://www.diffchecker.com/zqTmEKdz
> [5]
> https://github.com/apache/calcite/blob/master/core/src/main/java/org/apache/calcite/rel/rules/ProjectFilterTransposeRule.java#L234
> [6] https://immutables.github.io/
>
>