You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@calcite.apache.org by "Jacques Nadeau (Jira)" <ji...@apache.org> on 2021/09/27 03:05:00 UTC
[jira] [Comment Edited] (CALCITE-4787) Evaluate use of Immutables
instead of ImmutableBeans
[ 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)