You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by GitBox <gi...@apache.org> on 2021/09/20 19:58:13 UTC
[GitHub] [calcite] jacques-n opened a new pull request #2531: [CALCITE-4787] Replace ImmutableBeans with Immutables
jacques-n opened a new pull request #2531:
URL: https://github.com/apache/calcite/pull/2531
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] jacques-n commented on a change in pull request #2531: [CALCITE-4787] Replace ImmutableBeans with Immutables
Posted by GitBox <gi...@apache.org>.
jacques-n commented on a change in pull request #2531:
URL: https://github.com/apache/calcite/pull/2531#discussion_r719651497
##########
File path: 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
Review comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2531: [CALCITE-4787] Replace ImmutableBeans with Immutables
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2531:
URL: https://github.com/apache/calcite/pull/2531#discussion_r718201340
##########
File path: core/build.gradle.kts
##########
@@ -187,6 +194,65 @@ ide {
generatedSource(javaCCTest, "test")
}
+fun configureAnnotationSet(task: JavaCompile, sourceSet: SourceSet) {
+ with(task) {
+ source = sourceSet.java
+ classpath = sourceSet.compileClasspath
+ destinationDirectory.set(options.generatedSourceOutputDirectory)
+ options.compilerArgs.add("-proc:only")
+ org.gradle.api.plugins.internal.JvmPluginsHelper.configureAnnotationProcessorPath(sourceSet, sourceSet.java, options, project)
+ }
+}
+
+val annotationProcessorMain by tasks.registering(JavaCompile::class) {
+ dependsOn(javaCCMain)
+ configureAnnotationSet(this, sourceSets.main.get())
Review comment:
If you use an extension function, then `this` would be passed automatically.
```suggestion
configureAnnotationSet(sourceSets.main.get())
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] jcamachor commented on pull request #2531: [CALCITE-4787] Replace ImmutableBeans with Immutables
Posted by GitBox <gi...@apache.org>.
jcamachor commented on pull request #2531:
URL: https://github.com/apache/calcite/pull/2531#issuecomment-923453477
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2531: [CALCITE-4787] Replace ImmutableBeans with Immutables
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2531:
URL: https://github.com/apache/calcite/pull/2531#discussion_r718198212
##########
File path: core/build.gradle.kts
##########
@@ -187,6 +194,65 @@ ide {
generatedSource(javaCCTest, "test")
}
+fun configureAnnotationSet(task: JavaCompile, sourceSet: SourceSet) {
+ with(task) {
+ source = sourceSet.java
+ classpath = sourceSet.compileClasspath
+ destinationDirectory.set(options.generatedSourceOutputDirectory)
+ options.compilerArgs.add("-proc:only")
+ org.gradle.api.plugins.internal.JvmPluginsHelper.configureAnnotationProcessorPath(sourceSet, sourceSet.java, options, project)
+ }
+}
+
+val annotationProcessorMain by tasks.registering(JavaCompile::class) {
+ dependsOn(javaCCMain)
+ configureAnnotationSet(this, sourceSets.main.get())
+ // only if we aren't running compileJava, since doing twice fails.
+ onlyIf { tasks.findByPath("compileJava")?.enabled != true }
+}
+
+val annotationProcessorTest by tasks.registering(JavaCompile::class) {
+ val kotlinTestCompile = tasks.withType<org.jetbrains.kotlin.gradle.tasks.KotlinCompile>()
+ .getByName("compileTestKotlin")
+
+ dependsOn(javaCCTest, kotlinTestCompile)
+
+ configureAnnotationSet(this, sourceSets.test.get())
+ classpath += files(kotlinTestCompile.destinationDirectory.get())
+
+ // only if we aren't running compileJavaTest, since doing twice fails.
+ onlyIf { tasks.findByPath("compileTestJava")?.enabled != true }
+}
+
+ide {
+ // generate annotation processed files on project import/sync.
+ // adds to idea path but skip don't add to SourceSet since that triggers checkstyle
Review comment:
Is adding to "idea path" really needed? In my experiments with your branch, IDEA was picking up the sources automatically
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] jacques-n commented on a change in pull request #2531: [CALCITE-4787] Replace ImmutableBeans with Immutables
Posted by GitBox <gi...@apache.org>.
jacques-n commented on a change in pull request #2531:
URL: https://github.com/apache/calcite/pull/2531#discussion_r719574722
##########
File path: core/build.gradle.kts
##########
@@ -187,6 +194,65 @@ ide {
generatedSource(javaCCTest, "test")
}
+fun configureAnnotationSet(task: JavaCompile, sourceSet: SourceSet) {
+ with(task) {
+ source = sourceSet.java
+ classpath = sourceSet.compileClasspath
+ destinationDirectory.set(options.generatedSourceOutputDirectory)
+ options.compilerArgs.add("-proc:only")
+ org.gradle.api.plugins.internal.JvmPluginsHelper.configureAnnotationProcessorPath(sourceSet, sourceSet.java, options, project)
+ }
+}
+
+val annotationProcessorMain by tasks.registering(JavaCompile::class) {
+ dependsOn(javaCCMain)
+ configureAnnotationSet(this, sourceSets.main.get())
+ // only if we aren't running compileJava, since doing twice fails.
+ onlyIf { tasks.findByPath("compileJava")?.enabled != true }
+}
+
+val annotationProcessorTest by tasks.registering(JavaCompile::class) {
+ val kotlinTestCompile = tasks.withType<org.jetbrains.kotlin.gradle.tasks.KotlinCompile>()
+ .getByName("compileTestKotlin")
+
+ dependsOn(javaCCTest, kotlinTestCompile)
+
+ configureAnnotationSet(this, sourceSets.test.get())
+ classpath += files(kotlinTestCompile.destinationDirectory.get())
+
+ // only if we aren't running compileJavaTest, since doing twice fails.
+ onlyIf { tasks.findByPath("compileTestJava")?.enabled != true }
+}
+
+ide {
+ // generate annotation processed files on project import/sync.
+ // adds to idea path but skip don't add to SourceSet since that triggers checkstyle
Review comment:
I think you're right. Removed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] jacques-n commented on a change in pull request #2531: [CALCITE-4787] Replace ImmutableBeans with Immutables
Posted by GitBox <gi...@apache.org>.
jacques-n commented on a change in pull request #2531:
URL: https://github.com/apache/calcite/pull/2531#discussion_r719003238
##########
File path: core/build.gradle.kts
##########
@@ -187,6 +194,65 @@ ide {
generatedSource(javaCCTest, "test")
}
+fun configureAnnotationSet(task: JavaCompile, sourceSet: SourceSet) {
+ with(task) {
+ source = sourceSet.java
+ classpath = sourceSet.compileClasspath
+ destinationDirectory.set(options.generatedSourceOutputDirectory)
+ options.compilerArgs.add("-proc:only")
+ org.gradle.api.plugins.internal.JvmPluginsHelper.configureAnnotationProcessorPath(sourceSet, sourceSet.java, options, project)
+ }
+}
+
+val annotationProcessorMain by tasks.registering(JavaCompile::class) {
+ dependsOn(javaCCMain)
+ configureAnnotationSet(this, sourceSets.main.get())
+ // only if we aren't running compileJava, since doing twice fails.
+ onlyIf { tasks.findByPath("compileJava")?.enabled != true }
+}
+
+val annotationProcessorTest by tasks.registering(JavaCompile::class) {
+ val kotlinTestCompile = tasks.withType<org.jetbrains.kotlin.gradle.tasks.KotlinCompile>()
+ .getByName("compileTestKotlin")
+
+ dependsOn(javaCCTest, kotlinTestCompile)
Review comment:
The annotation processor fails because there are references to kotlin based annotations in the test. It's possible that a change to classpath would resolve since I don't think the Kotlin annotations are Calcite specific (correct me if I'm wrong). If they are general Kotlin, how do I get the right classpath to add?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] julianhyde commented on pull request #2531: [CALCITE-4787] Replace ImmutableBeans with Immutables
Posted by GitBox <gi...@apache.org>.
julianhyde commented on pull request #2531:
URL: https://github.com/apache/calcite/pull/2531#issuecomment-925176021
I think this change is a net positive.
But if (1) we have to change a huge number of Calcite files, or (2) we require dependent projects to change their code, those are significant negatives.
My last concern is compatibility. If a dependent project is using a different version of Immutables, what will be the impact?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] jacques-n commented on pull request #2531: [CALCITE-4787] Replace ImmutableBeans with Immutables
Posted by GitBox <gi...@apache.org>.
jacques-n commented on pull request #2531:
URL: https://github.com/apache/calcite/pull/2531#issuecomment-928039556
Thanks @vlsi, that resolved it. Just needed annotationProcessor, not testAnnotationProcessor.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] jamesstarr commented on a change in pull request #2531: [CALCITE-4787] Replace ImmutableBeans with Immutables
Posted by GitBox <gi...@apache.org>.
jamesstarr commented on a change in pull request #2531:
URL: https://github.com/apache/calcite/pull/2531#discussion_r713378811
##########
File path: 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;
Review comment:
Do we need this? Couldn't we simply reference ImmutableSqlWriterConfig.DEFAULT directly in SqlPrettyWriter.config? Or alternatively deprecate the config function SqlPrettyWriter?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] jacques-n commented on pull request #2531: [CALCITE-4787] Replace ImmutableBeans with Immutables
Posted by GitBox <gi...@apache.org>.
jacques-n commented on pull request #2531:
URL: https://github.com/apache/calcite/pull/2531#issuecomment-925386830
Converting to draft until I have a newer patch ready to review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] jacques-n edited a comment on pull request #2531: [CALCITE-4787] Replace ImmutableBeans with Immutables
Posted by GitBox <gi...@apache.org>.
jacques-n edited a comment on pull request #2531:
URL: https://github.com/apache/calcite/pull/2531#issuecomment-925165013
Just a small correction @julianhyde
> Removing the `with` methods is a massive negative because of the disruption to dependent projects.
That's incorrect. 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2531: [CALCITE-4787] Replace ImmutableBeans with Immutables
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2531:
URL: https://github.com/apache/calcite/pull/2531#discussion_r719121268
##########
File path: core/build.gradle.kts
##########
@@ -187,6 +194,65 @@ ide {
generatedSource(javaCCTest, "test")
}
+fun configureAnnotationSet(task: JavaCompile, sourceSet: SourceSet) {
+ with(task) {
+ source = sourceSet.java
+ classpath = sourceSet.compileClasspath
+ destinationDirectory.set(options.generatedSourceOutputDirectory)
+ options.compilerArgs.add("-proc:only")
+ org.gradle.api.plugins.internal.JvmPluginsHelper.configureAnnotationProcessorPath(sourceSet, sourceSet.java, options, project)
+ }
+}
+
+val annotationProcessorMain by tasks.registering(JavaCompile::class) {
+ dependsOn(javaCCMain)
+ configureAnnotationSet(this, sourceSets.main.get())
+ // only if we aren't running compileJava, since doing twice fails.
+ onlyIf { tasks.findByPath("compileJava")?.enabled != true }
+}
+
+val annotationProcessorTest by tasks.registering(JavaCompile::class) {
+ val kotlinTestCompile = tasks.withType<org.jetbrains.kotlin.gradle.tasks.KotlinCompile>()
+ .getByName("compileTestKotlin")
+
+ dependsOn(javaCCTest, kotlinTestCompile)
Review comment:
Ah, it comes from https://github.com/apache/calcite/tree/master/core/src/test/kotlin/org/apache/calcite/testlib
The way out is probably to call annotation processing via [kapt](https://kotlinlang.org/docs/kapt.html#top). It looks like aptMode=stubs or aptMode=apt might be helpful.
----
Just in case, `:core:compileTestKotlin` requires `:core:classes`, so if you are going to trigger `compileTestKotlin` on IDE sync, then the dance with `-proc:only` is not really needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] julianhyde commented on pull request #2531: [CALCITE-4787] Replace ImmutableBeans with Immutables
Posted by GitBox <gi...@apache.org>.
julianhyde commented on pull request #2531:
URL: https://github.com/apache/calcite/pull/2531#issuecomment-925170056
Ah, so is https://www.diffchecker.com/zqTmEKdz not minimal? Could you use withers rather than a builder when initializing DEFAULT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] jacques-n commented on pull request #2531: [CALCITE-4787] Replace ImmutableBeans with Immutables
Posted by GitBox <gi...@apache.org>.
jacques-n commented on pull request #2531:
URL: https://github.com/apache/calcite/pull/2531#issuecomment-931511515
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] jacques-n commented on pull request #2531: [CALCITE-4787] Replace ImmutableBeans with Immutables
Posted by GitBox <gi...@apache.org>.
jacques-n commented on pull request #2531:
URL: https://github.com/apache/calcite/pull/2531#issuecomment-927342867
@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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2531: [CALCITE-4787] Replace ImmutableBeans with Immutables
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2531:
URL: https://github.com/apache/calcite/pull/2531#discussion_r718200620
##########
File path: core/build.gradle.kts
##########
@@ -187,6 +194,65 @@ ide {
generatedSource(javaCCTest, "test")
}
+fun configureAnnotationSet(task: JavaCompile, sourceSet: SourceSet) {
+ with(task) {
+ source = sourceSet.java
+ classpath = sourceSet.compileClasspath
+ destinationDirectory.set(options.generatedSourceOutputDirectory)
+ options.compilerArgs.add("-proc:only")
+ org.gradle.api.plugins.internal.JvmPluginsHelper.configureAnnotationProcessorPath(sourceSet, sourceSet.java, options, project)
+ }
+}
+
+val annotationProcessorMain by tasks.registering(JavaCompile::class) {
+ dependsOn(javaCCMain)
+ configureAnnotationSet(this, sourceSets.main.get())
+ // only if we aren't running compileJava, since doing twice fails.
+ onlyIf { tasks.findByPath("compileJava")?.enabled != true }
+}
+
+val annotationProcessorTest by tasks.registering(JavaCompile::class) {
+ val kotlinTestCompile = tasks.withType<org.jetbrains.kotlin.gradle.tasks.KotlinCompile>()
+ .getByName("compileTestKotlin")
+
+ dependsOn(javaCCTest, kotlinTestCompile)
Review comment:
Could you please clarify why compileTestKotlin is required?
It is sad because it would require "compile linq4j" and so on, so it would probably take noticeable time
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2531: [CALCITE-4787] Replace ImmutableBeans with Immutables
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2531:
URL: https://github.com/apache/calcite/pull/2531#discussion_r718198212
##########
File path: core/build.gradle.kts
##########
@@ -187,6 +194,65 @@ ide {
generatedSource(javaCCTest, "test")
}
+fun configureAnnotationSet(task: JavaCompile, sourceSet: SourceSet) {
+ with(task) {
+ source = sourceSet.java
+ classpath = sourceSet.compileClasspath
+ destinationDirectory.set(options.generatedSourceOutputDirectory)
+ options.compilerArgs.add("-proc:only")
+ org.gradle.api.plugins.internal.JvmPluginsHelper.configureAnnotationProcessorPath(sourceSet, sourceSet.java, options, project)
+ }
+}
+
+val annotationProcessorMain by tasks.registering(JavaCompile::class) {
+ dependsOn(javaCCMain)
+ configureAnnotationSet(this, sourceSets.main.get())
+ // only if we aren't running compileJava, since doing twice fails.
+ onlyIf { tasks.findByPath("compileJava")?.enabled != true }
+}
+
+val annotationProcessorTest by tasks.registering(JavaCompile::class) {
+ val kotlinTestCompile = tasks.withType<org.jetbrains.kotlin.gradle.tasks.KotlinCompile>()
+ .getByName("compileTestKotlin")
+
+ dependsOn(javaCCTest, kotlinTestCompile)
+
+ configureAnnotationSet(this, sourceSets.test.get())
+ classpath += files(kotlinTestCompile.destinationDirectory.get())
+
+ // only if we aren't running compileJavaTest, since doing twice fails.
+ onlyIf { tasks.findByPath("compileTestJava")?.enabled != true }
+}
+
+ide {
+ // generate annotation processed files on project import/sync.
+ // adds to idea path but skip don't add to SourceSet since that triggers checkstyle
Review comment:
Is adding to "idea path" really needed? In my experiments with your branch, IDEA was picking up the sources automatically
##########
File path: core/build.gradle.kts
##########
@@ -187,6 +194,65 @@ ide {
generatedSource(javaCCTest, "test")
}
+fun configureAnnotationSet(task: JavaCompile, sourceSet: SourceSet) {
+ with(task) {
+ source = sourceSet.java
+ classpath = sourceSet.compileClasspath
+ destinationDirectory.set(options.generatedSourceOutputDirectory)
+ options.compilerArgs.add("-proc:only")
+ org.gradle.api.plugins.internal.JvmPluginsHelper.configureAnnotationProcessorPath(sourceSet, sourceSet.java, options, project)
+ }
+}
Review comment:
```suggestion
fun JavaCompile.configureAnnotationSet(sourceSet: SourceSet) {
source = sourceSet.java
classpath = sourceSet.compileClasspath
destinationDirectory.set(options.generatedSourceOutputDirectory)
options.compilerArgs.add("-proc:only")
org.gradle.api.plugins.internal.JvmPluginsHelper.configureAnnotationProcessorPath(sourceSet, sourceSet.java, options, project)
}
```
`destinationDirectory` should be "directory with the generated class files", so `options.generatedSourceOutputDirectory` looks wrong here.
##########
File path: core/build.gradle.kts
##########
@@ -187,6 +194,65 @@ ide {
generatedSource(javaCCTest, "test")
}
+fun configureAnnotationSet(task: JavaCompile, sourceSet: SourceSet) {
+ with(task) {
+ source = sourceSet.java
+ classpath = sourceSet.compileClasspath
+ destinationDirectory.set(options.generatedSourceOutputDirectory)
+ options.compilerArgs.add("-proc:only")
+ org.gradle.api.plugins.internal.JvmPluginsHelper.configureAnnotationProcessorPath(sourceSet, sourceSet.java, options, project)
+ }
+}
+
+val annotationProcessorMain by tasks.registering(JavaCompile::class) {
+ dependsOn(javaCCMain)
+ configureAnnotationSet(this, sourceSets.main.get())
+ // only if we aren't running compileJava, since doing twice fails.
+ onlyIf { tasks.findByPath("compileJava")?.enabled != true }
+}
+
+val annotationProcessorTest by tasks.registering(JavaCompile::class) {
+ val kotlinTestCompile = tasks.withType<org.jetbrains.kotlin.gradle.tasks.KotlinCompile>()
+ .getByName("compileTestKotlin")
+
+ dependsOn(javaCCTest, kotlinTestCompile)
Review comment:
Could you please clarify why compileTestKotlin is required?
It is sad because it would require "compile linq4j" and so on, so it would probably take noticeable time
##########
File path: core/build.gradle.kts
##########
@@ -187,6 +194,65 @@ ide {
generatedSource(javaCCTest, "test")
}
+fun configureAnnotationSet(task: JavaCompile, sourceSet: SourceSet) {
+ with(task) {
+ source = sourceSet.java
+ classpath = sourceSet.compileClasspath
+ destinationDirectory.set(options.generatedSourceOutputDirectory)
+ options.compilerArgs.add("-proc:only")
+ org.gradle.api.plugins.internal.JvmPluginsHelper.configureAnnotationProcessorPath(sourceSet, sourceSet.java, options, project)
+ }
+}
+
+val annotationProcessorMain by tasks.registering(JavaCompile::class) {
+ dependsOn(javaCCMain)
+ configureAnnotationSet(this, sourceSets.main.get())
Review comment:
If you use an extension function, then `this` would be passed automatically.
```suggestion
configureAnnotationSet(sourceSets.main.get())
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] jamesstarr commented on a change in pull request #2531: [CALCITE-4787] Replace ImmutableBeans with Immutables
Posted by GitBox <gi...@apache.org>.
jamesstarr commented on a change in pull request #2531:
URL: https://github.com/apache/calcite/pull/2531#discussion_r713378811
##########
File path: 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;
Review comment:
Do we need this? Couldn't we simply reference ImmutableSqlWriterConfig.DEFAULT directly in SqlPrettyWriter.config?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] jacques-n commented on pull request #2531: [CALCITE-4787] Replace ImmutableBeans with Immutables
Posted by GitBox <gi...@apache.org>.
jacques-n commented on pull request #2531:
URL: https://github.com/apache/calcite/pull/2531#issuecomment-925172716
> 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 wither 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] jacques-n commented on a change in pull request #2531: [CALCITE-4787] Replace ImmutableBeans with Immutables
Posted by GitBox <gi...@apache.org>.
jacques-n commented on a change in pull request #2531:
URL: https://github.com/apache/calcite/pull/2531#discussion_r717127426
##########
File path: 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);
Review comment:
fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] jacques-n commented on pull request #2531: [CALCITE-4787] Replace ImmutableBeans with Immutables
Posted by GitBox <gi...@apache.org>.
jacques-n commented on pull request #2531:
URL: https://github.com/apache/calcite/pull/2531#issuecomment-925178166
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2531: [CALCITE-4787] Replace ImmutableBeans with Immutables
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2531:
URL: https://github.com/apache/calcite/pull/2531#discussion_r719649101
##########
File path: 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
Review comment:
Why `G1GC`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] jacques-n merged pull request #2531: [CALCITE-4787] Replace ImmutableBeans with Immutables
Posted by GitBox <gi...@apache.org>.
jacques-n merged pull request #2531:
URL: https://github.com/apache/calcite/pull/2531
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] julianhyde commented on pull request #2531: [CALCITE-4787] Replace ImmutableBeans with Immutables
Posted by GitBox <gi...@apache.org>.
julianhyde commented on pull request #2531:
URL: https://github.com/apache/calcite/pull/2531#issuecomment-925135979
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] jacques-n commented on pull request #2531: [CALCITE-4787] Replace ImmutableBeans with Immutables
Posted by GitBox <gi...@apache.org>.
jacques-n commented on pull request #2531:
URL: https://github.com/apache/calcite/pull/2531#issuecomment-931480612
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] jacques-n commented on pull request #2531: [CALCITE-4787] Replace ImmutableBeans with Immutables
Posted by GitBox <gi...@apache.org>.
jacques-n commented on pull request #2531:
URL: https://github.com/apache/calcite/pull/2531#issuecomment-923445662
> 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...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] jcamachor commented on pull request #2531: [CALCITE-4787] Replace ImmutableBeans with Immutables
Posted by GitBox <gi...@apache.org>.
jcamachor commented on pull request #2531:
URL: https://github.com/apache/calcite/pull/2531#issuecomment-923421571
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2531: [CALCITE-4787] Replace ImmutableBeans with Immutables
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2531:
URL: https://github.com/apache/calcite/pull/2531#discussion_r719666692
##########
File path: 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
Review comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2531: [CALCITE-4787] Replace ImmutableBeans with Immutables
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2531:
URL: https://github.com/apache/calcite/pull/2531#discussion_r716960147
##########
File path: bom/build.gradle.kts
##########
@@ -124,6 +124,8 @@ dependencies {
apiv("org.elasticsearch.client:elasticsearch-rest-client", "elasticsearch")
apiv("org.elasticsearch.plugin:transport-netty4-client", "elasticsearch")
apiv("org.elasticsearch:elasticsearch")
+ api("org.immutables:value-annotations:${"immutables".v}")
+ api("org.immutables:value:${"immutables".v}")
Review comment:
```suggestion
apiv("org.immutables:value-annotations", "immutables")
apiv("org.immutables:value", "immutables")
```
##########
File path: core/src/main/java/org/apache/calcite/sql/parser/SqlParser.java
##########
@@ -242,64 +245,72 @@ public static ConfigBuilder configBuilder(Config config) {
/**
* Interface to define the configuration for a SQL parser.
*/
+ @Value.Immutable(singleton = true)
+ @Value.Style(typeImmutable = "ImmutableSqlParserConfig")
public interface Config {
/** Default configuration. */
- Config DEFAULT = ImmutableBeans.create(Config.class)
- .withLex(Lex.ORACLE)
- .withIdentifierMaxLength(DEFAULT_IDENTIFIER_MAX_LENGTH)
- .withConformance(SqlConformanceEnum.DEFAULT)
- .withParserFactory(SqlParserImpl.FACTORY);
-
- @ImmutableBeans.Property()
- @ImmutableBeans.IntDefault(DEFAULT_IDENTIFIER_MAX_LENGTH)
- int identifierMaxLength();
+ Config DEFAULT = ImmutableSqlParserConfig.of();
+ @Value.Default default int identifierMaxLength() {
+ return DEFAULT_IDENTIFIER_MAX_LENGTH;
+ }
+ //Quoting.DOUBLE_QUOTE, Casing.TO_UPPER, Casing.UNCHANGED, true,
+ // CharLiteralStyle.STANDARD
Review comment:
This looks odd
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] jacques-n commented on a change in pull request #2531: [CALCITE-4787] Replace ImmutableBeans with Immutables
Posted by GitBox <gi...@apache.org>.
jacques-n commented on a change in pull request #2531:
URL: https://github.com/apache/calcite/pull/2531#discussion_r719679419
##########
File path: 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
Review comment:
You were right. IDE (not)helpfulness. Fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] jacques-n edited a comment on pull request #2531: [CALCITE-4787] Replace ImmutableBeans with Immutables
Posted by GitBox <gi...@apache.org>.
jacques-n edited a comment on pull request #2531:
URL: https://github.com/apache/calcite/pull/2531#issuecomment-928039556
Thanks @vlsi, that resolved it. Just needed annotationProcessor, ~~not testAnnotationProcessor~~.
Scratch that, testAnnotationProcessor is also required.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] jacques-n commented on a change in pull request #2531: [CALCITE-4787] Replace ImmutableBeans with Immutables
Posted by GitBox <gi...@apache.org>.
jacques-n commented on a change in pull request #2531:
URL: https://github.com/apache/calcite/pull/2531#discussion_r717128470
##########
File path: 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;
Review comment:
Modified. This isn't included anymore
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] julianhyde commented on a change in pull request #2531: [CALCITE-4787] Replace ImmutableBeans with Immutables
Posted by GitBox <gi...@apache.org>.
julianhyde commented on a change in pull request #2531:
URL: https://github.com/apache/calcite/pull/2531#discussion_r714383792
##########
File path: core/build.gradle.kts
##########
@@ -81,6 +83,8 @@ dependencies {
testImplementation(kotlin("test"))
testImplementation(kotlin("test-junit5"))
testRuntimeOnly("org.slf4j:slf4j-log4j12")
+ testAnnotationProcessor("org.immutables:value:2.8.8")
Review comment:
Ultimately versions go in root directory's `build.properties`. You will need to declare the dependency in `bom/build.gradle.kts`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] asereda-gs commented on a change in pull request #2531: [CALCITE-4787] Replace ImmutableBeans with Immutables
Posted by GitBox <gi...@apache.org>.
asereda-gs commented on a change in pull request #2531:
URL: https://github.com/apache/calcite/pull/2531#discussion_r714376800
##########
File path: core/build.gradle.kts
##########
@@ -81,6 +83,8 @@ dependencies {
testImplementation(kotlin("test"))
testImplementation(kotlin("test-junit5"))
testRuntimeOnly("org.slf4j:slf4j-log4j12")
+ testAnnotationProcessor("org.immutables:value:2.8.8")
Review comment:
Probably should have same immutables version for `annotationProcessor` and `testAnnotationProcessor`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] jcamachor edited a comment on pull request #2531: [CALCITE-4787] Replace ImmutableBeans with Immutables
Posted by GitBox <gi...@apache.org>.
jcamachor edited a comment on pull request #2531:
URL: https://github.com/apache/calcite/pull/2531#issuecomment-923421571
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] jacques-n commented on pull request #2531: [CALCITE-4787] Replace ImmutableBeans with Immutables
Posted by GitBox <gi...@apache.org>.
jacques-n commented on pull request #2531:
URL: https://github.com/apache/calcite/pull/2531#issuecomment-923489356
> 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!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] laurentgo commented on a change in pull request #2531: [CALCITE-4787] Replace ImmutableBeans with Immutables
Posted by GitBox <gi...@apache.org>.
laurentgo commented on a change in pull request #2531:
URL: https://github.com/apache/calcite/pull/2531#discussion_r712712042
##########
File path: core/build.gradle.kts
##########
@@ -61,6 +61,8 @@ dependencies {
implementation("commons-io:commons-io")
implementation("org.codehaus.janino:commons-compiler")
implementation("org.codehaus.janino:janino")
+ annotationProcessor("org.immutables:value:2.8.2")
Review comment:
Although, website still mentions 2.8.2, the latest release according to Maven Central and Github is 2.8.8 (with 2.9.0 being rc'ed)
##########
File path: 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);
Review comment:
I guess this is a slight change of behavior as this is not a constant but a new instance each time?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2531: [CALCITE-4787] Replace ImmutableBeans with Immutables
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2531:
URL: https://github.com/apache/calcite/pull/2531#discussion_r720180924
##########
File path: core/build.gradle.kts
##########
@@ -187,6 +191,40 @@ ide {
generatedSource(javaCCTest, "test")
}
+fun JavaCompile.configureAnnotationSet(sourceSet: SourceSet) {
+ source = sourceSet.java
+ classpath = sourceSet.compileClasspath
+ destinationDirectory.set(options.generatedSourceOutputDirectory)
+ 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")) }
Review comment:
I belive `hasTask` condition should be reversed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] jacques-n commented on a change in pull request #2531: [CALCITE-4787] Replace ImmutableBeans with Immutables
Posted by GitBox <gi...@apache.org>.
jacques-n commented on a change in pull request #2531:
URL: https://github.com/apache/calcite/pull/2531#discussion_r717128555
##########
File path: core/build.gradle.kts
##########
@@ -61,6 +61,8 @@ dependencies {
implementation("commons-io:commons-io")
implementation("org.codehaus.janino:commons-compiler")
implementation("org.codehaus.janino:janino")
+ annotationProcessor("org.immutables:value:2.8.2")
Review comment:
fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] jacques-n commented on pull request #2531: [CALCITE-4787] Replace ImmutableBeans with Immutables
Posted by GitBox <gi...@apache.org>.
jacques-n commented on pull request #2531:
URL: https://github.com/apache/calcite/pull/2531#issuecomment-925165013
Just a small correction @julianhyde
> Removing the `with` methods is a massive negative because of the disruption to dependent projects.
That's incorrect. The `with` methods remain without any disruption. Only the `RelRule.as` method and the `RelRule.EMPTY` would be removed. The latter is removed because it doesn't make sense without the `as` method.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] jacques-n commented on a change in pull request #2531: [CALCITE-4787] Replace ImmutableBeans with Immutables
Posted by GitBox <gi...@apache.org>.
jacques-n commented on a change in pull request #2531:
URL: https://github.com/apache/calcite/pull/2531#discussion_r720732655
##########
File path: core/build.gradle.kts
##########
@@ -187,6 +191,40 @@ ide {
generatedSource(javaCCTest, "test")
}
+fun JavaCompile.configureAnnotationSet(sourceSet: SourceSet) {
+ source = sourceSet.java
+ classpath = sourceSet.compileClasspath
+ destinationDirectory.set(options.generatedSourceOutputDirectory)
+ 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")) }
Review comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] jacques-n commented on a change in pull request #2531: [CALCITE-4787] Replace ImmutableBeans with Immutables
Posted by GitBox <gi...@apache.org>.
jacques-n commented on a change in pull request #2531:
URL: https://github.com/apache/calcite/pull/2531#discussion_r717126725
##########
File path: core/src/main/java/org/apache/calcite/sql/parser/SqlParser.java
##########
@@ -242,64 +245,72 @@ public static ConfigBuilder configBuilder(Config config) {
/**
* Interface to define the configuration for a SQL parser.
*/
+ @Value.Immutable(singleton = true)
+ @Value.Style(typeImmutable = "ImmutableSqlParserConfig")
public interface Config {
/** Default configuration. */
- Config DEFAULT = ImmutableBeans.create(Config.class)
- .withLex(Lex.ORACLE)
- .withIdentifierMaxLength(DEFAULT_IDENTIFIER_MAX_LENGTH)
- .withConformance(SqlConformanceEnum.DEFAULT)
- .withParserFactory(SqlParserImpl.FACTORY);
-
- @ImmutableBeans.Property()
- @ImmutableBeans.IntDefault(DEFAULT_IDENTIFIER_MAX_LENGTH)
- int identifierMaxLength();
+ Config DEFAULT = ImmutableSqlParserConfig.of();
+ @Value.Default default int identifierMaxLength() {
+ return DEFAULT_IDENTIFIER_MAX_LENGTH;
+ }
+ //Quoting.DOUBLE_QUOTE, Casing.TO_UPPER, Casing.UNCHANGED, true,
+ // CharLiteralStyle.STANDARD
Review comment:
Good catch. That was a dev note that should have been deleted. Removed.
##########
File path: core/src/main/java/org/apache/calcite/rel/rules/materialize/MaterializedViewOnlyAggregateRule.java
##########
@@ -69,18 +72,25 @@ public MaterializedViewOnlyAggregateRule(RelOptRuleOperand operand,
}
/** Rule configuration. */
- public interface Config extends MaterializedViewAggregateRule.Config {
- Config DEFAULT = create(RelFactories.LOGICAL_BUILDER);
+ @Value.Immutable
+ public interface Config extends MaterializedViewAggregateRule.Config<Config> {
+ Config DEFAULT = ImmutableMaterializedViewOnlyAggregateRule.Config.builder()
+ .withRelBuilderFactory(RelFactories.LOGICAL_BUILDER)
+ .withGenerateUnionRewriting(true)
+ .withUnionRewritingPullProgram(null)
+ .withFastBailOut(false)
+ .build();
static Config create(RelBuilderFactory relBuilderFactory) {
- return MaterializedViewAggregateRule.Config.create(relBuilderFactory)
+
+ return ImmutableMaterializedViewOnlyAggregateRule.Config.builder()
Review comment:
Thanks. In most cases I use builders to build the default so from(DEFAULT) doesn't really work.
##########
File path: 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);
Review comment:
fixed.
##########
File path: 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;
Review comment:
Modified. This isn't included anymore
##########
File path: core/build.gradle.kts
##########
@@ -61,6 +61,8 @@ dependencies {
implementation("commons-io:commons-io")
implementation("org.codehaus.janino:commons-compiler")
implementation("org.codehaus.janino:janino")
+ annotationProcessor("org.immutables:value:2.8.2")
Review comment:
fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2531: [CALCITE-4787] Replace ImmutableBeans with Immutables
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2531:
URL: https://github.com/apache/calcite/pull/2531#discussion_r716969079
##########
File path: core/src/main/java/org/apache/calcite/sql/parser/SqlParser.java
##########
@@ -242,64 +245,72 @@ public static ConfigBuilder configBuilder(Config config) {
/**
* Interface to define the configuration for a SQL parser.
*/
+ @Value.Immutable(singleton = true)
+ @Value.Style(typeImmutable = "ImmutableSqlParserConfig")
public interface Config {
/** Default configuration. */
- Config DEFAULT = ImmutableBeans.create(Config.class)
- .withLex(Lex.ORACLE)
- .withIdentifierMaxLength(DEFAULT_IDENTIFIER_MAX_LENGTH)
- .withConformance(SqlConformanceEnum.DEFAULT)
- .withParserFactory(SqlParserImpl.FACTORY);
-
- @ImmutableBeans.Property()
- @ImmutableBeans.IntDefault(DEFAULT_IDENTIFIER_MAX_LENGTH)
- int identifierMaxLength();
+ Config DEFAULT = ImmutableSqlParserConfig.of();
+ @Value.Default default int identifierMaxLength() {
+ return DEFAULT_IDENTIFIER_MAX_LENGTH;
+ }
+ //Quoting.DOUBLE_QUOTE, Casing.TO_UPPER, Casing.UNCHANGED, true,
+ // CharLiteralStyle.STANDARD
Review comment:
This looks odd
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] jcamachor edited a comment on pull request #2531: [CALCITE-4787] Replace ImmutableBeans with Immutables
Posted by GitBox <gi...@apache.org>.
jcamachor edited a comment on pull request #2531:
URL: https://github.com/apache/calcite/pull/2531#issuecomment-923453477
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] jacques-n edited a comment on pull request #2531: [CALCITE-4787] Replace ImmutableBeans with Immutables
Posted by GitBox <gi...@apache.org>.
jacques-n edited a comment on pull request #2531:
URL: https://github.com/apache/calcite/pull/2531#issuecomment-925165013
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] jacques-n commented on a change in pull request #2531: [CALCITE-4787] Replace ImmutableBeans with Immutables
Posted by GitBox <gi...@apache.org>.
jacques-n commented on a change in pull request #2531:
URL: https://github.com/apache/calcite/pull/2531#discussion_r719652552
##########
File path: 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
Review comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] jacques-n commented on a change in pull request #2531: [CALCITE-4787] Replace ImmutableBeans with Immutables
Posted by GitBox <gi...@apache.org>.
jacques-n commented on a change in pull request #2531:
URL: https://github.com/apache/calcite/pull/2531#discussion_r719679870
##########
File path: core/build.gradle.kts
##########
@@ -187,6 +192,40 @@ ide {
generatedSource(javaCCTest, "test")
}
+fun JavaCompile.configureAnnotationSet(sourceSet: SourceSet) {
+ source = sourceSet.java
+ classpath = sourceSet.compileClasspath
+ destinationDirectory.set(options.generatedSourceOutputDirectory)
+ options.compilerArgs.add("-proc:only")
+ org.gradle.api.plugins.internal.JvmPluginsHelper.configureAnnotationProcessorPath(sourceSet, sourceSet.java, options, project)
+}
+
+val annotationProcessorMain by tasks.registering(JavaCompile::class) {
+ dependsOn(javaCCMain)
+ configureAnnotationSet(sourceSets.main.get())
+ // only if we aren't running compileJava, since doing twice fails.
+ onlyIf { tasks.findByPath("compileJava")?.enabled != true }
Review comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2531: [CALCITE-4787] Replace ImmutableBeans with Immutables
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2531:
URL: https://github.com/apache/calcite/pull/2531#discussion_r719671666
##########
File path: core/build.gradle.kts
##########
@@ -187,6 +192,40 @@ ide {
generatedSource(javaCCTest, "test")
}
+fun JavaCompile.configureAnnotationSet(sourceSet: SourceSet) {
+ source = sourceSet.java
+ classpath = sourceSet.compileClasspath
+ destinationDirectory.set(options.generatedSourceOutputDirectory)
+ options.compilerArgs.add("-proc:only")
+ org.gradle.api.plugins.internal.JvmPluginsHelper.configureAnnotationProcessorPath(sourceSet, sourceSet.java, options, project)
+}
+
+val annotationProcessorMain by tasks.registering(JavaCompile::class) {
+ dependsOn(javaCCMain)
+ configureAnnotationSet(sourceSets.main.get())
+ // only if we aren't running compileJava, since doing twice fails.
+ onlyIf { project.gradle.taskGraph.hasTask("compileJava") }
Review comment:
by the way, `compileJava` is available from `sourceSet.getCompileTaskName("java")`, so `onlyIf` could be moved to `configureAnnotationSet`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] asereda-gs commented on a change in pull request #2531: [CALCITE-4787] Replace ImmutableBeans with Immutables
Posted by GitBox <gi...@apache.org>.
asereda-gs commented on a change in pull request #2531:
URL: https://github.com/apache/calcite/pull/2531#discussion_r714380726
##########
File path: core/src/main/java/org/apache/calcite/rel/rules/materialize/MaterializedViewOnlyAggregateRule.java
##########
@@ -69,18 +72,25 @@ public MaterializedViewOnlyAggregateRule(RelOptRuleOperand operand,
}
/** Rule configuration. */
- public interface Config extends MaterializedViewAggregateRule.Config {
- Config DEFAULT = create(RelFactories.LOGICAL_BUILDER);
+ @Value.Immutable
+ public interface Config extends MaterializedViewAggregateRule.Config<Config> {
+ Config DEFAULT = ImmutableMaterializedViewOnlyAggregateRule.Config.builder()
+ .withRelBuilderFactory(RelFactories.LOGICAL_BUILDER)
+ .withGenerateUnionRewriting(true)
+ .withUnionRewritingPullProgram(null)
+ .withFastBailOut(false)
+ .build();
static Config create(RelBuilderFactory relBuilderFactory) {
- return MaterializedViewAggregateRule.Config.create(relBuilderFactory)
+
+ return ImmutableMaterializedViewOnlyAggregateRule.Config.builder()
Review comment:
You can also do build from `DEFAULT` instance:
```java
ImmutableMaterializedViewOnlyAggregateRule.Config.builder()
.from(DEFAULT)
.withRelBuilderFactory(relBuilderFactory)
.withOperandSupplier(b -> b.operand(Aggregate.class).anyInputs())
.withDescription("MaterializedViewAggregateRule(Aggregate)")
.build();
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] asereda-gs commented on pull request #2531: [CALCITE-4787] Replace ImmutableBeans with Immutables
Posted by GitBox <gi...@apache.org>.
asereda-gs commented on pull request #2531:
URL: https://github.com/apache/calcite/pull/2531#issuecomment-925188499
> 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:
```java
ImmutableBean.builder()
.from(other) // merges attribute value into builder
.value("foo")
.build();
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] vlsi edited a comment on pull request #2531: [CALCITE-4787] Replace ImmutableBeans with Immutables
Posted by GitBox <gi...@apache.org>.
vlsi edited a comment on pull request #2531:
URL: https://github.com/apache/calcite/pull/2531#issuecomment-927589394
@jacques-n , please try the following in https://github.com/apache/calcite/blob/e533f4ba9566ecd5b989f255e03bdeea3e812204/build.gradle.kts#L302-L306
```diff
plugins.withId("java-library") {
dependencies {
+ "annotationProcessor"(platform(project(":bom")))
"implementation"(platform(project(":bom")))
}
}
```
I am not sure if `testAnnotationProcessor` is needed there or not.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] asereda-gs commented on pull request #2531: [CALCITE-4787] Replace ImmutableBeans with Immutables
Posted by GitBox <gi...@apache.org>.
asereda-gs commented on pull request #2531:
URL: https://github.com/apache/calcite/pull/2531#issuecomment-925472150
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] julianhyde edited a comment on pull request #2531: [CALCITE-4787] Replace ImmutableBeans with Immutables
Posted by GitBox <gi...@apache.org>.
julianhyde edited a comment on pull request #2531:
URL: https://github.com/apache/calcite/pull/2531#issuecomment-925176021
(I removed this comment. Let's have the conversation in JIRA.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] jacques-n edited a comment on pull request #2531: [CALCITE-4787] Replace ImmutableBeans with Immutables
Posted by GitBox <gi...@apache.org>.
jacques-n edited a comment on pull request #2531:
URL: https://github.com/apache/calcite/pull/2531#issuecomment-925172716
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on pull request #2531: [CALCITE-4787] Replace ImmutableBeans with Immutables
Posted by GitBox <gi...@apache.org>.
vlsi commented on pull request #2531:
URL: https://github.com/apache/calcite/pull/2531#issuecomment-925147816
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)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2531: [CALCITE-4787] Replace ImmutableBeans with Immutables
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2531:
URL: https://github.com/apache/calcite/pull/2531#discussion_r716960147
##########
File path: bom/build.gradle.kts
##########
@@ -124,6 +124,8 @@ dependencies {
apiv("org.elasticsearch.client:elasticsearch-rest-client", "elasticsearch")
apiv("org.elasticsearch.plugin:transport-netty4-client", "elasticsearch")
apiv("org.elasticsearch:elasticsearch")
+ api("org.immutables:value-annotations:${"immutables".v}")
+ api("org.immutables:value:${"immutables".v}")
Review comment:
```suggestion
apiv("org.immutables:value-annotations", "immutables")
apiv("org.immutables:value", "immutables")
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] jacques-n commented on a change in pull request #2531: [CALCITE-4787] Replace ImmutableBeans with Immutables
Posted by GitBox <gi...@apache.org>.
jacques-n commented on a change in pull request #2531:
URL: https://github.com/apache/calcite/pull/2531#discussion_r717127054
##########
File path: core/src/main/java/org/apache/calcite/rel/rules/materialize/MaterializedViewOnlyAggregateRule.java
##########
@@ -69,18 +72,25 @@ public MaterializedViewOnlyAggregateRule(RelOptRuleOperand operand,
}
/** Rule configuration. */
- public interface Config extends MaterializedViewAggregateRule.Config {
- Config DEFAULT = create(RelFactories.LOGICAL_BUILDER);
+ @Value.Immutable
+ public interface Config extends MaterializedViewAggregateRule.Config<Config> {
+ Config DEFAULT = ImmutableMaterializedViewOnlyAggregateRule.Config.builder()
+ .withRelBuilderFactory(RelFactories.LOGICAL_BUILDER)
+ .withGenerateUnionRewriting(true)
+ .withUnionRewritingPullProgram(null)
+ .withFastBailOut(false)
+ .build();
static Config create(RelBuilderFactory relBuilderFactory) {
- return MaterializedViewAggregateRule.Config.create(relBuilderFactory)
+
+ return ImmutableMaterializedViewOnlyAggregateRule.Config.builder()
Review comment:
Thanks. In most cases I use builders to build the default so from(DEFAULT) doesn't really work.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2531: [CALCITE-4787] Replace ImmutableBeans with Immutables
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2531:
URL: https://github.com/apache/calcite/pull/2531#discussion_r718199388
##########
File path: core/build.gradle.kts
##########
@@ -187,6 +194,65 @@ ide {
generatedSource(javaCCTest, "test")
}
+fun configureAnnotationSet(task: JavaCompile, sourceSet: SourceSet) {
+ with(task) {
+ source = sourceSet.java
+ classpath = sourceSet.compileClasspath
+ destinationDirectory.set(options.generatedSourceOutputDirectory)
+ options.compilerArgs.add("-proc:only")
+ org.gradle.api.plugins.internal.JvmPluginsHelper.configureAnnotationProcessorPath(sourceSet, sourceSet.java, options, project)
+ }
+}
Review comment:
```suggestion
fun JavaCompile.configureAnnotationSet(sourceSet: SourceSet) {
source = sourceSet.java
classpath = sourceSet.compileClasspath
destinationDirectory.set(options.generatedSourceOutputDirectory)
options.compilerArgs.add("-proc:only")
org.gradle.api.plugins.internal.JvmPluginsHelper.configureAnnotationProcessorPath(sourceSet, sourceSet.java, options, project)
}
```
`destinationDirectory` should be "directory with the generated class files", so `options.generatedSourceOutputDirectory` looks wrong here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2531: [CALCITE-4787] Replace ImmutableBeans with Immutables
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2531:
URL: https://github.com/apache/calcite/pull/2531#discussion_r719591114
##########
File path: core/build.gradle.kts
##########
@@ -187,6 +192,40 @@ ide {
generatedSource(javaCCTest, "test")
}
+fun JavaCompile.configureAnnotationSet(sourceSet: SourceSet) {
+ source = sourceSet.java
+ classpath = sourceSet.compileClasspath
+ destinationDirectory.set(options.generatedSourceOutputDirectory)
+ options.compilerArgs.add("-proc:only")
+ org.gradle.api.plugins.internal.JvmPluginsHelper.configureAnnotationProcessorPath(sourceSet, sourceSet.java, options, project)
+}
+
+val annotationProcessorMain by tasks.registering(JavaCompile::class) {
+ dependsOn(javaCCMain)
+ configureAnnotationSet(sourceSets.main.get())
+ // only if we aren't running compileJava, since doing twice fails.
+ onlyIf { tasks.findByPath("compileJava")?.enabled != true }
Review comment:
`tasks.findByPath("compileJava")?.enabled` is verifying if the task exists, however, what we need to check is whether task is a part of the task graph (e.g. if it was requested by the user).
The condition should be like `project.gradle.taskGraph.hasTask("compileJava")` or something like project.gradle.taskGraph { if (compileJava is in the graph) { disable proc-only task}`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on pull request #2531: [CALCITE-4787] Replace ImmutableBeans with Immutables
Posted by GitBox <gi...@apache.org>.
vlsi commented on pull request #2531:
URL: https://github.com/apache/calcite/pull/2531#issuecomment-927589394
@jacques-n , please try the following in https://github.com/apache/calcite/blob/e533f4ba9566ecd5b989f255e03bdeea3e812204/build.gradle.kts#L302-L306
```kotlin
plugins.withId("java-library") {
dependencies {
"annotationProcessor"(platform(project(":bom")))
"implementation"(platform(project(":bom")))
}
}
```
I am not sure if `testAnnotationProcessor` is needed there or not.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] jacques-n commented on a change in pull request #2531: [CALCITE-4787] Replace ImmutableBeans with Immutables
Posted by GitBox <gi...@apache.org>.
jacques-n commented on a change in pull request #2531:
URL: https://github.com/apache/calcite/pull/2531#discussion_r717126725
##########
File path: core/src/main/java/org/apache/calcite/sql/parser/SqlParser.java
##########
@@ -242,64 +245,72 @@ public static ConfigBuilder configBuilder(Config config) {
/**
* Interface to define the configuration for a SQL parser.
*/
+ @Value.Immutable(singleton = true)
+ @Value.Style(typeImmutable = "ImmutableSqlParserConfig")
public interface Config {
/** Default configuration. */
- Config DEFAULT = ImmutableBeans.create(Config.class)
- .withLex(Lex.ORACLE)
- .withIdentifierMaxLength(DEFAULT_IDENTIFIER_MAX_LENGTH)
- .withConformance(SqlConformanceEnum.DEFAULT)
- .withParserFactory(SqlParserImpl.FACTORY);
-
- @ImmutableBeans.Property()
- @ImmutableBeans.IntDefault(DEFAULT_IDENTIFIER_MAX_LENGTH)
- int identifierMaxLength();
+ Config DEFAULT = ImmutableSqlParserConfig.of();
+ @Value.Default default int identifierMaxLength() {
+ return DEFAULT_IDENTIFIER_MAX_LENGTH;
+ }
+ //Quoting.DOUBLE_QUOTE, Casing.TO_UPPER, Casing.UNCHANGED, true,
+ // CharLiteralStyle.STANDARD
Review comment:
Good catch. That was a dev note that should have been deleted. Removed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] jacques-n commented on a change in pull request #2531: [CALCITE-4787] Replace ImmutableBeans with Immutables
Posted by GitBox <gi...@apache.org>.
jacques-n commented on a change in pull request #2531:
URL: https://github.com/apache/calcite/pull/2531#discussion_r719679203
##########
File path: core/build.gradle.kts
##########
@@ -187,6 +192,40 @@ ide {
generatedSource(javaCCTest, "test")
}
+fun JavaCompile.configureAnnotationSet(sourceSet: SourceSet) {
+ source = sourceSet.java
+ classpath = sourceSet.compileClasspath
+ destinationDirectory.set(options.generatedSourceOutputDirectory)
+ options.compilerArgs.add("-proc:only")
+ org.gradle.api.plugins.internal.JvmPluginsHelper.configureAnnotationProcessorPath(sourceSet, sourceSet.java, options, project)
+}
+
+val annotationProcessorMain by tasks.registering(JavaCompile::class) {
+ dependsOn(javaCCMain)
+ configureAnnotationSet(sourceSets.main.get())
+ // only if we aren't running compileJava, since doing twice fails.
+ onlyIf { project.gradle.taskGraph.hasTask("compileJava") }
Review comment:
Nice, updated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2531: [CALCITE-4787] Replace ImmutableBeans with Immutables
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2531:
URL: https://github.com/apache/calcite/pull/2531#discussion_r719649634
##########
File path: 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
Review comment:
this `onlyIf` looks weird
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org