[CALCITE-4787] Replace ImmutableBeans with Immutables #2531

jacques-n · 2021-09-20T19:58:04Z

Replace the use of reflection/dynamic proxies with the AnnotationProcessor provided by Immutables

NOTE: This is an initial patch that only changes one ImmutableBean to Immutables to show what the changes look like

jcamachor · 2021-09-20T23:10:36Z

This seems like a good idea (rather than relying on our own code). I assume we should also deprecate ImmutableBeans if we want to move in this direction.

jacques-n · 2021-09-21T00:00:16Z

I assume we should also deprecate ImmutableBeans if we want to move in this direction.

Do we consider this an external api? I figured we would just delete...

jcamachor · 2021-09-21T00:13:32Z

I mentioned that because ImmutableBeans is part of the core, publicly accessible and was introduced a few releases ago in 1.22, ~~so I am not sure anyone is relying on it~~ and it seems to be relied upon by other projects. My understanding is that in most (all?) of these cases, we mark as deprecated and delete the classes / APIs after a couple of releases.

jacques-n · 2021-09-21T00:54:44Z

I mentioned that because ImmutableBeans is part of the core, publicly accessible and was introduced a few releases ago in 1.22, so I am not sure anyone is relying on it (not sure they should). My understanding is that in most (all?) of these cases, we mark as deprecated and delete the classes / APIs after a couple of releases. If that is not the case, it should be fine to simply delete it.

It isn't marked as internal/experimental so I guess deprecated is the right approach. Sadness. I was so hopeful this would be a net negative patch. I love those!

core/build.gradle.kts

laurentgo · 2021-09-21T05:44:38Z

core/src/main/java/org/apache/calcite/sql/pretty/SqlPrettyWriter.java

@@ -360,7 +353,7 @@ public SqlPrettyWriter() {

  /** Creates a {@link SqlWriterConfig} with Calcite's SQL dialect. */
  public static SqlWriterConfig config() {
-    return CONFIG;
+    return SqlWriterConfig.of().withDialect(CalciteSqlDialect.DEFAULT);


I guess this is a slight change of behavior as this is not a constant but a new instance each time?

jamesstarr · 2021-09-21T20:04:49Z

core/src/main/java/org/apache/calcite/sql/SqlWriterConfig.java

@@ -433,4 +424,6 @@ SqlWriterConfig withWhereListItemsOnSeparateLines(
    /** Wrap always. Items are on separate lines. */
    TALL
  }
+
+  static SqlWriterConfig DEFAULT = ImmutableSqlWriterConfig.DEFAULT;


Do we need this? Couldn't we simply reference ImmutableSqlWriterConfig.DEFAULT directly in SqlPrettyWriter.config? Or alternatively deprecate the config function SqlPrettyWriter?

Modified. This isn't included anymore

julianhyde · 2021-09-22T17:32:06Z

As @jacques-n said, this is a minimal change - in fact it's just a proof-of-concept. The real change is going to affect hundreds of files, and dozens of lines in each, because there are 142 sub-classes of RelRule.Config. And these configs are public APIs.

Removing the as method is a net positive.

Removing the with methods is a massive negative because of the disruption to dependent projects.

Does Immutables have a mode where it generates copy-on-write beans (with with methods) as opposed to builders? That's the crux of the problem here.

Is there anything in Immutables that converts a bean into a builder (with the same property values)? That will be necessary.

vlsi · 2021-09-22T17:48:36Z

An interesting side effect is that build fails at Travis: https://app.travis-ci.com/github/apache/calcite/jobs/538524980#L633

It might be related to a relatively "old" Java 1.8 (Travis uses 1.8u252 vs GitHub 1.8u302)

javac 1.8.0_252
...
java.lang.OutOfMemoryError: GC overhead limit exceeded
	at com.sun.tools.javac.code.Types.subst(Types.java:2984)
	at com.sun.tools.javac.comp.Infer$InferenceContext.asUndetVar(Infer.java:2064)
	at com.sun.tools.javac.comp.Resolve$MethodCheckContext.compatible(Resolve.java:967)
	at com.sun.tools.javac.comp.Check.checkType(Check.java:546)
	at com.sun.tools.javac.comp.Attr$ResultInfo.check(Attr.java:482)

jacques-n · 2021-09-22T18:10:45Z

Just a small correction @julianhyde

Removing the with methods is a massive negative because of the disruption to dependent projects.

That's incorrect interpretation of the proposed changes. The with methods remain without any disruption. Only the RelRule.Config.as method and the RelRule.Config.EMPTY singleton would be removed. The latter is removed because it doesn't make sense without the as method.

julianhyde · 2021-09-22T18:17:01Z

Ah, so is https://www.diffchecker.com/zqTmEKdz not minimal? Could you use withers rather than a builder when initializing DEFAULT?

jacques-n · 2021-09-22T18:19:51Z

Ah, so is https://www.diffchecker.com/zqTmEKdz not minimal? Could you use withers rather than a builder when initializing DEFAULT?

Good point. Yes, it could be less impactful. The key difference is that you must use a builder if a property is marked as non-nullable and has no default (since you can't use a 'withFoo' operation until you have a fully valid pojo). When I did the change I was thinking that one or more of those properties were required and not yet default defined. I'll do an update to clarify.

julianhyde · 2021-09-22T18:22:59Z

(I removed this comment. Let's have the conversation in JIRA.)

jacques-n · 2021-09-22T18:25:24Z

Updated simpler example: https://www.diffchecker.com/Ba5a0agP

This is possible because the current definition of RelRule.Config declares a default for the OperandTransformer. It's a little icky because the default throws an exception and I'd prefer to delegate the error on missing value to the generated code just like any other missing value.

asereda-gs · 2021-09-22T18:35:50Z

Does Immutables have a mode where it generates copy-on-write beans (with with methods) as opposed to builders? That's the crux of the problem here.

Yes. ImmutableBean will have with method which acts as copy-on-write.

Is there anything in Immutables that converts a bean into a builder (with the same property values)? That will be necessary.

One can use from() method of a builder:

ImmutableBean.builder()
    .from(other) // merges attribute value into builder
    .value("foo")
   .build();

jacques-n · 2021-09-22T22:57:30Z

Converting to draft until I have a newer patch ready to review.

core/build.gradle.kts

...rc/main/java/org/apache/calcite/rel/rules/materialize/MaterializedViewOnlyAggregateRule.java

asereda-gs · 2021-09-23T03:01:25Z

The key difference is that you must use a builder if a property is marked as non-nullable and has no default (since you can't use a 'withFoo' operation until you have a fully valid pojo).

You can use @Value.Parameter (which generates static method of(param1, param2)) to avoid using builder when a class can be constructed from few parameters.

jacques-n · 2021-09-26T17:40:00Z

@vlsi, I can't figure out what is the right way to express the annotationProcessor dependency constraint in the bom. Any chance you could tell me how to do that?

vlsi · 2021-09-27T07:05:00Z

@jacques-n , please try the following in

calcite/build.gradle.kts

Lines 302 to 306 in e533f4b

    
           plugins.withId("java-library") { 
        
               dependencies { 
        
                   "implementation"(platform(project(":bom"))) 
        
               } 
        
           }

     plugins.withId("java-library") {
         dependencies {
+            "annotationProcessor"(platform(project(":bom")))
             "implementation"(platform(project(":bom")))
         }
     }

I am not sure if testAnnotationProcessor is needed there or not.

jacques-n · 2021-09-27T16:16:19Z

Thanks @vlsi, that resolved it. Just needed annotationProcessor, ~~not testAnnotationProcessor~~.

Scratch that, testAnnotationProcessor is also required.

bom/build.gradle.kts

core/src/main/java/org/apache/calcite/sql/parser/SqlParser.java

core/build.gradle.kts

jacques-n · 2021-09-30T16:31:46Z

Hey @vlsi, I've pulled out use of immutables in test for this first patch. It's already pretty large. I also cleaned up the gradle stuff. LMK if you think it looks good to merge. Can do a test patch as follow-up.

core/build.gradle.kts

jacques-n · 2021-09-30T17:14:28Z

@vlsi , updated to change condition of task. It seems to work the same in both cases (original and new condition) but I'm happy changing it.

vlsi · 2021-09-30T18:15:04Z

gradle.properties

@@ -14,7 +14,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 #
-org.gradle.jvmargs=-Xmx512m -XX:MaxMetaspaceSize=512m
+org.gradle.jvmargs=-XX:+UseG1GC -Xmx512m -XX:MaxMetaspaceSize=512m


Why G1GC?

If you profile the gradle daemon build using old and g1, g1 performs better. It more effectively clears things away. The patch builds with either though.

In general, the gradle settings seem low given the amount of continuous memory builds take. In many cases I see non trivial amounts of time against gc while profiling.

vlsi · 2021-09-30T18:15:56Z

core/build.gradle.kts

@@ -16,6 +16,9 @@
 */
 import com.github.vlsi.gradle.crlf.CrLfSpec
 import com.github.vlsi.gradle.crlf.LineEndings
+import com.github.vlsi.gradle.ide.dsl.settings
+import com.github.vlsi.gradle.ide.dsl.taskTriggers
+import org.jetbrains.kotlin.backend.common.onlyIf


this onlyIf looks weird

Not clear if you're asking for a change. It works and was the way I found others doing this. Keep in mind that this is my first effort in gradle and kotlin.

Please drop onlyIf import. The package name org.jetbrains.kotlin.backend.common looks as if it has something to do with kotlin backend which is not something you want to use here. I belive the import has been added by mistake. You should call Task#onlyIf method which should not require an import.

You were right. IDE (not)helpfulness. Fixed.

vlsi · 2021-09-30T18:47:42Z

core/build.gradle.kts

+    // only if we aren't running compileJava, since doing twice fails.
+    onlyIf { project.gradle.taskGraph.hasTask("compileJava") }


by the way, compileJava is available from sourceSet.getCompileTaskName("java"), so onlyIf could be moved to configureAnnotationSet

Nice, updated.

vlsi · 2021-10-01T11:51:58Z

core/build.gradle.kts

+    options.compilerArgs.add("-proc:only")
+    org.gradle.api.plugins.internal.JvmPluginsHelper.configureAnnotationProcessorPath(sourceSet, sourceSet.java, options, project)
+    // only if we aren't running compileJava, since doing twice fails.
+    onlyIf { project.gradle.taskGraph.hasTask(sourceSet.getCompileTaskName("java")) }


I belive hasTask condition should be reversed.

Nice catch.

The jvm bug is apparently only triggered in the test kotlin/java combination (which is why the build still worked here).

Fixed now.

- Move core rules to use Immutables instead of ImmutableBeans - Update ImmutableBeans to ignore default property return values used by Immutables - Adjust EnumerableProjectToCalcRule so it doesn't try to use proxy behavior with non-empty config - Update MaterializedViewProjectAggregateRule.DEFAULT so it sets fastBailout config property - Update SqlValidator, SqlWriter, SqlParser, SqlToRelConverter, RelDecorrelator, Hoist and RelBuilder to use Immutables for configuration - Switch gradle to use G1 Garbage collector to improve GC performance - Update ConverterRule to have default properties since properties were declared required but had no default values. - Update gradle build to automatically generate annotation source on Intellij and Eclipse sync operations - Add travis_wait to jdk11 checkerframework build to avoid premature termination due to core compile time. - Make ExchangeRemoveConstantKeysRule and ValuesReduceRule consistent with SubqueryRemoveRule by making MatchHandler type parameter concrete. Co-authored-by: Vladimir Sitnikov <sitnikov.vladimir@gmail.com>

laurentgo reviewed Sep 21, 2021

View reviewed changes

jamesstarr reviewed Sep 21, 2021

View reviewed changes

jacques-n marked this pull request as draft September 22, 2021 22:57

asereda-gs reviewed Sep 22, 2021

View reviewed changes

core/build.gradle.kts Outdated Show resolved Hide resolved

asereda-gs reviewed Sep 22, 2021

View reviewed changes

...rc/main/java/org/apache/calcite/rel/rules/materialize/MaterializedViewOnlyAggregateRule.java Show resolved Hide resolved

jacques-n force-pushed the immutables branch from e498f3a to 6fbe1c2 Compare September 23, 2021 00:57

jacques-n force-pushed the immutables branch 3 times, most recently from e319c38 to 11262c9 Compare September 26, 2021 17:38

jacques-n marked this pull request as ready for review September 27, 2021 16:16

vlsi reviewed Sep 27, 2021

View reviewed changes

bom/build.gradle.kts Outdated Show resolved Hide resolved

vlsi reviewed Sep 27, 2021

View reviewed changes

core/src/main/java/org/apache/calcite/sql/parser/SqlParser.java Outdated Show resolved Hide resolved

jacques-n force-pushed the immutables branch from baffb4f to 4a27ff6 Compare September 29, 2021 04:07

jacques-n force-pushed the immutables branch 2 times, most recently from 47861a4 to 47448dd Compare September 29, 2021 05:20

vlsi reviewed Sep 29, 2021

View reviewed changes

core/build.gradle.kts Show resolved Hide resolved

vlsi reviewed Sep 29, 2021

View reviewed changes

core/build.gradle.kts Outdated Show resolved Hide resolved

vlsi reviewed Sep 29, 2021

View reviewed changes

core/build.gradle.kts Outdated Show resolved Hide resolved

vlsi reviewed Sep 29, 2021

View reviewed changes

core/build.gradle.kts Outdated Show resolved Hide resolved

jacques-n force-pushed the immutables branch 2 times, most recently from bd7fb01 to 2fd74ec Compare September 30, 2021 16:29

vlsi reviewed Sep 30, 2021

View reviewed changes

core/build.gradle.kts Outdated Show resolved Hide resolved

jacques-n force-pushed the immutables branch from 2fd74ec to eac8e0b Compare September 30, 2021 17:12

vlsi reviewed Sep 30, 2021

View reviewed changes

jacques-n force-pushed the immutables branch 2 times, most recently from 82d1983 to c1e5c33 Compare September 30, 2021 18:58

vlsi reviewed Oct 1, 2021

View reviewed changes

jacques-n force-pushed the immutables branch from c1e5c33 to fb7b1b5 Compare October 2, 2021 21:42

jacques-n force-pushed the immutables branch from fb7b1b5 to d70583c Compare October 2, 2021 21:45

jacques-n merged commit d70583c into apache:master Oct 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CALCITE-4787] Replace ImmutableBeans with Immutables #2531

[CALCITE-4787] Replace ImmutableBeans with Immutables #2531

jacques-n commented Sep 20, 2021

jcamachor commented Sep 20, 2021 •

edited

Loading

jacques-n commented Sep 21, 2021

jcamachor commented Sep 21, 2021 •

edited

Loading

jacques-n commented Sep 21, 2021

laurentgo Sep 21, 2021

jacques-n Sep 28, 2021

jamesstarr Sep 21, 2021 •

edited

Loading

jacques-n Sep 28, 2021

julianhyde commented Sep 22, 2021

vlsi commented Sep 22, 2021

jacques-n commented Sep 22, 2021 •

edited

Loading

julianhyde commented Sep 22, 2021

jacques-n commented Sep 22, 2021 •

edited

Loading

julianhyde commented Sep 22, 2021 •

edited

Loading

jacques-n commented Sep 22, 2021

asereda-gs commented Sep 22, 2021

jacques-n commented Sep 22, 2021

asereda-gs commented Sep 23, 2021

jacques-n commented Sep 26, 2021

vlsi commented Sep 27, 2021 •

edited

Loading

jacques-n commented Sep 27, 2021 •

edited

Loading

jacques-n commented Sep 30, 2021

jacques-n commented Sep 30, 2021

vlsi Sep 30, 2021

jacques-n Sep 30, 2021

vlsi Sep 30, 2021

jacques-n Sep 30, 2021

vlsi Sep 30, 2021

jacques-n Sep 30, 2021

vlsi Sep 30, 2021

jacques-n Sep 30, 2021

vlsi Oct 1, 2021

jacques-n Oct 2, 2021

		// only if we aren't running compileJava, since doing twice fails.
		onlyIf { project.gradle.taskGraph.hasTask("compileJava") }

[CALCITE-4787] Replace ImmutableBeans with Immutables #2531

[CALCITE-4787] Replace ImmutableBeans with Immutables #2531

Conversation

jacques-n commented Sep 20, 2021

jcamachor commented Sep 20, 2021 • edited Loading

jacques-n commented Sep 21, 2021

jcamachor commented Sep 21, 2021 • edited Loading

jacques-n commented Sep 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesstarr Sep 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julianhyde commented Sep 22, 2021

vlsi commented Sep 22, 2021

jacques-n commented Sep 22, 2021 • edited Loading

julianhyde commented Sep 22, 2021

jacques-n commented Sep 22, 2021 • edited Loading

julianhyde commented Sep 22, 2021 • edited Loading

jacques-n commented Sep 22, 2021

asereda-gs commented Sep 22, 2021

jacques-n commented Sep 22, 2021

asereda-gs commented Sep 23, 2021

jacques-n commented Sep 26, 2021

vlsi commented Sep 27, 2021 • edited Loading

jacques-n commented Sep 27, 2021 • edited Loading

jacques-n commented Sep 30, 2021

jacques-n commented Sep 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcamachor commented Sep 20, 2021 •

edited

Loading

jcamachor commented Sep 21, 2021 •

edited

Loading

jamesstarr Sep 21, 2021 •

edited

Loading

jacques-n commented Sep 22, 2021 •

edited

Loading

jacques-n commented Sep 22, 2021 •

edited

Loading

julianhyde commented Sep 22, 2021 •

edited

Loading

vlsi commented Sep 27, 2021 •

edited

Loading

jacques-n commented Sep 27, 2021 •

edited

Loading