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/
