[
https://issues.apache.org/jira/browse/CALCITE-4787?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17420413#comment-17420413
]
Jacques Nadeau edited comment on CALCITE-4787 at 9/27/21, 3:04 AM:
-------------------------------------------------------------------
I've updated my patch to convert all core rules
(org.apache.calcite.rel.rules.*) as well as non-rule classes that use the
config system (SqlParser, RelToSqlConverter, SqlValidator, etc). Notes follow:
h4. Incremental Approach: Success
As discussed, I went with the incremental strategy approach and it looks like
it is working well. I've converted a little over half of all rules in Calcite
and all test cases continue to pass. Incremental patch sets to change the rest
of the rules should be straightforward. As hoped, the fact that this is working
well also should mean that external Calcite users are not disrupted by this
change.
h4. Unwanted upcasts: Stay As Is
I left the behavior as is. When you use the Immutables generated objects
directly (builder or immutable concrete class), you don't have to upcast but
the moment you use the interface, you still need to use 'as'. Since I made this
patch keep all immutables related apis package protected (no new apis), this
reduced the amount of noise in the change. This means I also removed the
intermediate generic classes. ImmutableBeans just wasn't built to play with it
and if we want to introduce, I think it would be better to do so after
ImmutableBeans is gone.
h4. Using 'as' to convert from concrete configs to proxy configs: One Found,
Prefer Simple over Noisy
As mentioned previously, the approach I took allows ImmutableBeans patterns
using RelRule.EMPTY (to be deprecated before release) but disallows conversion
from Immutables to ImmutableBeans (using 'as'). There was [one
example|https://github.com/apache/calcite/blob/master/core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableProjectToCalcRule.java#L59]
of the unsupported pattern in the Calcite codebase which I had to change.
Spot-checking Flink and Hive, I don't see anyone else using this pattern. If
someone would be using this pattern, this change would be a breaking change.
Given the unlikeliness of the use of the pattern, I'm inclined to keep it
breaking. The only alternative I see for a soft entry is to introduce a new
"unwrap" method on RelRule.Config that will have the downcast-only version of
as (as opposed to supporting both proxy conversion and downcasts). We would
then mark as() as deprecated in this release as well. This is a softer landing
(deprecated before fail) but means that we're effectively renaming the function
for all the internal code. If someone can find an external use of the non-empty
proxy-as use, I'd be more up for using the noisier approach.
h4. Incomplete Objects
Because ImmutableBeans allowed one to have instances with required properties
unset, it was/is possible that users created objects without all required
properties set. There was also one place where a completed ImmutableBeans
object was actually incomplete (one of the required configuration properties
wasn't set). This was in MaterializedViewProjectAggregateRule where fastBailOut
wasn't set for the DEFAULT rule. I had to pick a default and picked false
(since that seems most conservative). I believe this wasn't an actual issue
previously because MaterializedViewProjectAggregateRule doesn't try to actually
read that configuration property. In the case of Immutables, one is unable to
construct an instance unless all required fields are populated, thus the need
for a change.
h4. Arbitrary cast on return
One thing that Immutables disallows is a property that has a generic parameter
that isn't expressed at the class level. This feels reasonable since it
basically is an unsafe dynamic binding. I found two places where this was being
used [1
|https://github.com/apache/calcite/blob/master/core/src/main/java/org/apache/calcite/rel/rules/ExchangeRemoveConstantKeysRule.java#L197]and
[2|https://github.com/apache/calcite/blob/master/core/src/main/java/org/apache/calcite/rel/rules/ValuesReduceRule.java#L284].
In both cases, a more concrete type made more sense, did not require other
changes in the Calcite codebase and was more consistent with the [third place
MatchHandler was
used|https://github.com/apache/calcite/blob/master/core/src/main/java/org/apache/calcite/rel/rules/SubQueryRemoveRule.java#L890]
where the type parameter was already bound. Note that Immutables does have
very [good support for
generics|https://immutables.github.io/immutable.html#generics-are-fully-supported]
but it only allows a generic like this if one were to actually bind the type
to the class instead of the method.
h4. Things still pending for this patch:
* I haven't figured out what is the right way to use a bom-based constraint
dependency for a annotationProcessor. Have requested help from @vlsi.
* I have seen the message "Expiring Daemon because JVM heap space is
exhausted". The build continues on but it seems somewhat concerning. Since I
haven't done any Calcite development since the build system was change, I don't
know if my change introduced this, it is a normal part of long-lived Gradle
daemons or something else. Would love some thoughts. I adjusted what I think is
the parameter controlling heap but am mostly in the dark on the new build
system. Would love other's thoughts on this.
was (Author: jnadeau):
I've updated my patch to convert all core rules
(org.apache.calcite.rel.rules.*) as well as non-rule classes that use the
config system (SqlParser, RelToSqlConverter, SqlValidator, etc). Notes follow:
h4. Incremental Approach: Success
As discussed, I went with the incremental strategy approach and it looks like
it is working well. I've converted a little over half of all rules in Calcite
and all test cases continue to pass. Incremental patch sets to change the rest
of the rules should be straightforward. As hoped, the fact that this is working
well also should mean that external Calcite users are not disrupted by this
change.
h4. Unwanted upcasts: Stay As Is
I left the behavior as is. When you use the Immutables generated objects
directly (builder or immutable concrete class), you don't have to upcast but
the moment you use the interface, you still need to use 'as'. Since I made this
patch keep all immutables related apis package protected (no new apis), this
reduced the amount of noise in the change. This means I also removed the
intermediate generic classes. ImmutableBeans just wasn't built to play with it
and if we want to introduce, I think it would be better to do so after
ImmutableBeans is gone.
h4. Using 'as' to convert from concrete configs to proxy configs: One Found,
Prefer Simple or Noisy
As mentioned previously, the approach I took allows ImmutableBeans patterns
using RelRule.EMPTY (to be deprecated before release) but disallows conversion
from Immutables to ImmutableBeans (using 'as'). There was [one
example|https://github.com/apache/calcite/blob/master/core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableProjectToCalcRule.java#L59]
of the unsupported pattern in the Calcite codebase which I had to change.
Spot-checking Flink and Hive, I don't see anyone else using this pattern. If
someone would be using this pattern, this change would be a breaking change.
Given the unlikeliness of the use of the pattern, I'm inclined to keep it
breaking. The only alternative I see for a soft entry is to introduce a new
"unwrap" method on RelRule.Config that will have the downcast-only version of
as (as opposed to supporting both proxy conversion and downcasts). We would
then mark as() as deprecated in this release as well. This is a softer landing
(deprecated before fail) but means that we're effectively renaming the function
for all the internal code. If someone can find an external use of the non-empty
proxy-as use, I'd be more up for using the noisier approach.
h4. Incomplete Objects
Because ImmutableBeans allowed one to have instances with required properties
unset, it was/is possible that users created objects without all required
properties set. There was also one place where a completed ImmutableBeans
object was actually incomplete (one of the required configuration properties
wasn't set). This was in MaterializedViewProjectAggregateRule where fastBailOut
wasn't set for the DEFAULT rule. I had to pick a default and picked false
(since that seems most conservative). I believe this wasn't an actual issue
previously because MaterializedViewProjectAggregateRule doesn't try to actually
read that configuration property. In the case of Immutables, one is unable to
construct an instance unless all required fields are populated, thus the need
for a change.
h4. Arbitrary cast on return
One thing that Immutables disallows is a property that has a generic parameter
that isn't expressed at the class level. This feels reasonable since it
basically is an unsafe dynamic binding. I found two places where this was being
used [1
|https://github.com/apache/calcite/blob/master/core/src/main/java/org/apache/calcite/rel/rules/ExchangeRemoveConstantKeysRule.java#L197]and
[2|https://github.com/apache/calcite/blob/master/core/src/main/java/org/apache/calcite/rel/rules/ValuesReduceRule.java#L284].
In both cases, a more concrete type made more sense, did not require other
changes in the Calcite codebase and was more consistent with the [third place
MatchHandler was
used|https://github.com/apache/calcite/blob/master/core/src/main/java/org/apache/calcite/rel/rules/SubQueryRemoveRule.java#L890]
where the type parameter was already bound. Note that Immutables does have
very [good support for
generics|https://immutables.github.io/immutable.html#generics-are-fully-supported]
but it only allows a generic like this if one were to actually bind the type
to the class instead of the method.
h4. Things still pending for this patch:
* I haven't figured out what is the right way to use a bom-based constraint
dependency for a annotationProcessor. Have requested help from @vlsi.
* I have seen the message "Expiring Daemon because JVM heap space is
exhausted". The build continues on but it seems somewhat concerning. Since I
haven't done any Calcite development since the build system was change, I don't
know if my change introduced this, it is a normal part of long-lived Gradle
daemons or something else. Would love some thoughts. I adjusted what I think is
the parameter controlling heap but am mostly in the dark on the new build
system. Would love other's thoughts on this.
> Evaluate use of Immutables instead of ImmutableBeans
> ----------------------------------------------------
>
> Key: CALCITE-4787
> URL: https://issues.apache.org/jira/browse/CALCITE-4787
> Project: Calcite
> Issue Type: Improvement
> Reporter: Jacques Nadeau
> Assignee: Jacques Nadeau
> Priority: Major
> Labels: pull-request-available
> Time Spent: 4h 40m
> Remaining Estimate: 0h
>
> In the creation of CALCITE-3328, [Immutables|https://immutables.github.io/]
> was discussed as an alternative to a custom implementation. This ticket is to
> evaluate the impact to the codebase of changing. Ideally, introduction of
> immutables would both add flexibility and reduce the amount of code
> associated with these classes.
> Immutables works via annotation processor which means that it is should be
> relatively seamless to build systems and IDEs.
> The switch would also make it easier to work with these objects types in the
> context of aot compilation tools like GraalVM.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
