You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2021/10/08 17:43:08 UTC
[GitHub] [iceberg] rdblue opened a new pull request #3256: Build: Move Spark version modules under spark directory.
rdblue opened a new pull request #3256:
URL: https://github.com/apache/iceberg/pull/3256
This is another option for refactoring the Spark build. This is similar to #3237, but it also moves Spark projects into separate build.gradle files and allows building multiple Spark versions in parallel.
This removes Spark 3.1 testing from the Spark 3.0 project because 3.0 will be copied to produce a 3.1 build. Otherwise, Spark builds are unchanged to keep changes small.
There should also be further refactoring after these build changes to copy the contents of `iceberg-spark` into each version to ensure all classes are compiled against the correct version of Spark and Scala.
This is the new layout:
```
├── spark
│ ├── v2.4
│ │ ├── spark2
│ │ └── spark-runtime
│ └── v3.0
│ ├── spark3
│ │ └── src
│ ├── spark3-extensions
│ │ └── src
│ └── spark3-runtime
│ └── src
```
Work to do after this:
* Refactor `iceberg-spark` and move into version-specific modules
* Add a v3.1 build
* Refactoring to remove modules that are no longer needed, like `spark3-extensions`
* Apply similar refactoring to `jmh.gradle`
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on a change in pull request #3256: Build: Move Spark version modules under spark directory
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #3256:
URL: https://github.com/apache/iceberg/pull/3256#discussion_r727299658
##########
File path: settings.gradle
##########
@@ -55,21 +52,44 @@ project(':arrow').name = 'iceberg-arrow'
project(':parquet').name = 'iceberg-parquet'
project(':bundled-guava').name = 'iceberg-bundled-guava'
project(':spark').name = 'iceberg-spark'
-project(':spark3').name = 'iceberg-spark3'
-project(':spark3-extensions').name = 'iceberg-spark3-extensions'
-project(':spark3-runtime').name = 'iceberg-spark3-runtime'
project(':pig').name = 'iceberg-pig'
project(':hive-metastore').name = 'iceberg-hive-metastore'
project(':nessie').name = 'iceberg-nessie'
+String defaultSparkVersions = "2.4,3.0"
+List<String> knownSparkVersions = ["2.4", "3.0"]
+List<String> sparkVersions = (System.getProperty("sparkVersions") != null ? System.getProperty("sparkVersions") : defaultSparkVersions).split(",")
+
+if (!knownSparkVersions.containsAll(sparkVersions)) {
+ throw new GradleException("Found unsupported Spark versions: " + (sparkVersions - knownSparkVersions))
+}
+
+if (sparkVersions.contains("3.0")) {
Review comment:
I am fine either way.
##########
File path: spark/build.gradle
##########
@@ -0,0 +1,97 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+project(':iceberg-spark') {
+ configurations.all {
+ resolutionStrategy {
+ // Spark 2.4.4 can only use the below datanucleus version, the versions introduced
+ // by Hive 2.3.6 will meet lots of unexpected issues, so here force to use the versions
+ // introduced by Hive 1.2.1.
+ force 'org.datanucleus:datanucleus-api-jdo:3.2.6'
+ force 'org.datanucleus:datanucleus-core:3.2.10'
+ force 'org.datanucleus:datanucleus-rdbms:3.2.9'
+ }
+ }
+
+ dependencies {
+ implementation project(path: ':iceberg-bundled-guava', configuration: 'shadow')
+ api project(':iceberg-api')
+ implementation project(':iceberg-common')
+ implementation project(':iceberg-core')
+ api project(':iceberg-data')
+ implementation project(':iceberg-orc')
+ implementation project(':iceberg-parquet')
+ implementation project(':iceberg-arrow')
+ implementation project(':iceberg-hive-metastore')
+
+ compileOnly "com.google.errorprone:error_prone_annotations"
+ compileOnly "org.apache.avro:avro"
+ compileOnly("org.apache.spark:spark-hive_2.11") {
+ exclude group: 'org.apache.avro', module: 'avro'
+ }
+
+ implementation("org.apache.orc:orc-core::nohive") {
+ exclude group: 'org.apache.hadoop'
+ exclude group: 'commons-lang'
+ // These artifacts are shaded and included in the orc-core fat jar
+ exclude group: 'com.google.protobuf', module: 'protobuf-java'
+ exclude group: 'org.apache.hive', module: 'hive-storage-api'
+ }
+
+ implementation("org.apache.arrow:arrow-vector") {
+ exclude group: 'io.netty', module: 'netty-buffer'
+ exclude group: 'io.netty', module: 'netty-common'
+ exclude group: 'com.google.code.findbugs', module: 'jsr305'
+ }
+
+ testImplementation("org.apache.hadoop:hadoop-minicluster") {
+ exclude group: 'org.apache.avro', module: 'avro'
+ }
+ testImplementation project(path: ':iceberg-hive-metastore', configuration: 'testArtifacts')
+ testImplementation project(path: ':iceberg-api', configuration: 'testArtifacts')
+ testImplementation project(path: ':iceberg-core', configuration: 'testArtifacts')
+ testImplementation project(path: ':iceberg-data', configuration: 'testArtifacts')
+ }
+
+ test {
+ // For vectorized reads
+ // Allow unsafe memory access to avoid the costly check arrow does to check if index is within bounds
+ systemProperty("arrow.enable_unsafe_memory_access", "true")
+ // Disable expensive null check for every get(index) call.
+ // Iceberg manages nullability checks itself instead of relying on arrow.
+ systemProperty("arrow.enable_null_check_for_get", "false")
+
+ // Vectorized reads need more memory
+ maxHeapSize '2500m'
+ }
+}
+
+// add enabled Spark version modules to the build
+project.ext.defaultSparkVersions = "2.4,3.0"
Review comment:
I was confused initially as I did not recognize we still kept the common module. The more I look, the more it seems we should get rid of 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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on a change in pull request #3256: Build: Move Spark version modules under spark directory
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #3256:
URL: https://github.com/apache/iceberg/pull/3256#discussion_r726538685
##########
File path: jmh.gradle
##########
@@ -19,9 +19,9 @@
def jmhProjects = []
if (JavaVersion.current() == JavaVersion.VERSION_1_8) {
- jmhProjects = [ project("iceberg-spark2"), project("iceberg-spark3") ]
+ jmhProjects = [ project(":iceberg-spark:iceberg-spark2"), project(":iceberg-spark:iceberg-spark3") ]
Review comment:
Does it have to be `:iceberg-spark:spark2` and `:iceberg-spark:spark3`?
##########
File path: jmh.gradle
##########
@@ -19,9 +19,9 @@
def jmhProjects = []
if (JavaVersion.current() == JavaVersion.VERSION_1_8) {
- jmhProjects = [ project("iceberg-spark2"), project("iceberg-spark3") ]
+ jmhProjects = [ project(":iceberg-spark:iceberg-spark2"), project(":iceberg-spark:iceberg-spark3") ]
Review comment:
Doesn't it have to be `:iceberg-spark:spark2` and `:iceberg-spark:spark3`?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #3256: Build: Move Spark version modules under spark directory
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3256:
URL: https://github.com/apache/iceberg/pull/3256#discussion_r726604201
##########
File path: spark/build.gradle
##########
@@ -0,0 +1,97 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+project(':iceberg-spark') {
+ configurations.all {
+ resolutionStrategy {
+ // Spark 2.4.4 can only use the below datanucleus version, the versions introduced
+ // by Hive 2.3.6 will meet lots of unexpected issues, so here force to use the versions
+ // introduced by Hive 1.2.1.
+ force 'org.datanucleus:datanucleus-api-jdo:3.2.6'
+ force 'org.datanucleus:datanucleus-core:3.2.10'
+ force 'org.datanucleus:datanucleus-rdbms:3.2.9'
+ }
+ }
+
+ dependencies {
+ implementation project(path: ':iceberg-bundled-guava', configuration: 'shadow')
+ api project(':iceberg-api')
+ implementation project(':iceberg-common')
+ implementation project(':iceberg-core')
+ api project(':iceberg-data')
+ implementation project(':iceberg-orc')
+ implementation project(':iceberg-parquet')
+ implementation project(':iceberg-arrow')
+ implementation project(':iceberg-hive-metastore')
+
+ compileOnly "com.google.errorprone:error_prone_annotations"
+ compileOnly "org.apache.avro:avro"
+ compileOnly("org.apache.spark:spark-hive_2.11") {
+ exclude group: 'org.apache.avro', module: 'avro'
+ }
+
+ implementation("org.apache.orc:orc-core::nohive") {
+ exclude group: 'org.apache.hadoop'
+ exclude group: 'commons-lang'
+ // These artifacts are shaded and included in the orc-core fat jar
+ exclude group: 'com.google.protobuf', module: 'protobuf-java'
+ exclude group: 'org.apache.hive', module: 'hive-storage-api'
+ }
+
+ implementation("org.apache.arrow:arrow-vector") {
+ exclude group: 'io.netty', module: 'netty-buffer'
+ exclude group: 'io.netty', module: 'netty-common'
+ exclude group: 'com.google.code.findbugs', module: 'jsr305'
+ }
+
+ testImplementation("org.apache.hadoop:hadoop-minicluster") {
+ exclude group: 'org.apache.avro', module: 'avro'
+ }
+ testImplementation project(path: ':iceberg-hive-metastore', configuration: 'testArtifacts')
+ testImplementation project(path: ':iceberg-api', configuration: 'testArtifacts')
+ testImplementation project(path: ':iceberg-core', configuration: 'testArtifacts')
+ testImplementation project(path: ':iceberg-data', configuration: 'testArtifacts')
+ }
+
+ test {
+ // For vectorized reads
+ // Allow unsafe memory access to avoid the costly check arrow does to check if index is within bounds
+ systemProperty("arrow.enable_unsafe_memory_access", "true")
+ // Disable expensive null check for every get(index) call.
+ // Iceberg manages nullability checks itself instead of relying on arrow.
+ systemProperty("arrow.enable_null_check_for_get", "false")
+
+ // Vectorized reads need more memory
+ maxHeapSize '2500m'
+ }
+}
+
+// add enabled Spark version modules to the build
+project.ext.defaultSparkVersions = "2.4,3.0"
Review comment:
We can probably share the default versions and known versions. But we do need the `if` statements here to load the correct build files if the projects were defined.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on a change in pull request #3256: Build: Move Spark version modules under spark directory
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #3256:
URL: https://github.com/apache/iceberg/pull/3256#discussion_r726543327
##########
File path: settings.gradle
##########
@@ -55,21 +52,44 @@ project(':arrow').name = 'iceberg-arrow'
project(':parquet').name = 'iceberg-parquet'
project(':bundled-guava').name = 'iceberg-bundled-guava'
project(':spark').name = 'iceberg-spark'
-project(':spark3').name = 'iceberg-spark3'
-project(':spark3-extensions').name = 'iceberg-spark3-extensions'
-project(':spark3-runtime').name = 'iceberg-spark3-runtime'
project(':pig').name = 'iceberg-pig'
project(':hive-metastore').name = 'iceberg-hive-metastore'
project(':nessie').name = 'iceberg-nessie'
+String defaultSparkVersions = "2.4,3.0"
+List<String> knownSparkVersions = ["2.4", "3.0"]
+List<String> sparkVersions = (System.getProperty("sparkVersions") != null ? System.getProperty("sparkVersions") : defaultSparkVersions).split(",")
+
+if (!knownSparkVersions.containsAll(sparkVersions)) {
+ throw new GradleException("Found unsupported Spark versions: " + (sparkVersions - knownSparkVersions))
+}
+
+if (sparkVersions.contains("3.0")) {
+ include ':iceberg-spark:spark3'
+ include ':iceberg-spark:spark3-extensions'
+ include ':iceberg-spark:spark3-runtime'
+ project(':iceberg-spark:spark3').projectDir = file('spark/v3.0/spark3')
+ project(':iceberg-spark:spark3').name = 'iceberg-spark3'
+ project(':iceberg-spark:spark3-extensions').projectDir = file('spark/v3.0/spark3-extensions')
+ project(':iceberg-spark:spark3-extensions').name = 'iceberg-spark3-extensions'
+ project(':iceberg-spark:spark3-runtime').projectDir = file('spark/v3.0/spark3-runtime')
+ project(':iceberg-spark:spark3-runtime').name = 'iceberg-spark3-runtime'
+}
+
if (JavaVersion.current() == JavaVersion.VERSION_1_8) {
Review comment:
Do we need to throw an exception if someone asks for 2.4 and has JDK 11?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue merged pull request #3256: Build: Move Spark version modules under spark directory
Posted by GitBox <gi...@apache.org>.
rdblue merged pull request #3256:
URL: https://github.com/apache/iceberg/pull/3256
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue edited a comment on pull request #3256: Build: Move Spark version modules under spark directory
Posted by GitBox <gi...@apache.org>.
rdblue edited a comment on pull request #3256:
URL: https://github.com/apache/iceberg/pull/3256#issuecomment-941094431
> My original thought in #3237 was that we can define the rule that the shared code can continue to live, and any code that cannot build againt all versions should then be refactored to different version modules.
I think that compiling against different versions of Spark is beginning to be a significant risk due to changes across versions. That's only going to get worse when we start adding flags for Scala version as well and continue to add more extensions that touch Spark internals. I don't think it's a good idea to try to refactor common classes out unless they don't have a compile dependency on Spark. And in that case, we can more them to core like the `Writer` classes.
> . . . The intention is again to minimize duplicated code by minimizing the number of version folders.
I think we're actually better off the other way. What we have now does some pretty crazy things to deal with multiple Spark versions and I think we'll end up with cleaner code if we just duplicate it. This is one of the reasons why I think we want these build changes.
> In #3237 I had another scalaVersion system property to allow setting a flexible Scala version. Do you think that is something worth adding?
Yes, but as a follow-up. This is just a PR to put a basic structure in place and we will continue to improve it in future commits.
> What would be the naming scheme for modules after v3.0?
We don't need to decide this now, since this doesn't introduce new modules yet. I'd like to focus on getting this in with minimal changes since it's already so large!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue removed a comment on pull request #3256: Build: Move Spark version modules under spark directory
Posted by GitBox <gi...@apache.org>.
rdblue removed a comment on pull request #3256:
URL: https://github.com/apache/iceberg/pull/3256#issuecomment-941123660
> My original thought in #3237 was that we can define the rule that the shared code can continue to live, and any code that cannot build againt all versions should then be refactored to different version modules.
I think that compiling against different versions of Spark is beginning to be a significant risk due to changes across versions. That's only going to get worse when we start adding flags for Scala version as well and continue to add more extensions that touch Spark internals. I don't think it's a good idea to try to refactor common classes out unless they don't have a compile dependency on Spark. And in that case, we can more them to core like the `Writer` classes.
> . . . The intention is again to minimize duplicated code by minimizing the number of version folders.
I think we're actually better off the other way. What we have now does some pretty crazy things to deal with multiple Spark versions and I think we'll end up with cleaner code if we just duplicate it. This is one of the reasons why I think we the build change.
> In #3237 I had another scalaVersion system property to allow setting a flexible Scala version. Do you think that is something worth adding?
Yes, but as a follow-up. This is just a PR to put a basic structure in place and we will continue to improve it in future commits.
> What would be the naming scheme for modules after v3.0?
We don't need to decide this now, since this doesn't introduce new modules yet. I'd like to focus on getting this in with minimal changes since it's already so large!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on pull request #3256: Build: Move Spark version modules under spark directory
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on pull request #3256:
URL: https://github.com/apache/iceberg/pull/3256#issuecomment-941180930
We should probably wait with merging any other Spark related PRs before this is in.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #3256: Build: Move Spark version modules under spark directory
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3256:
URL: https://github.com/apache/iceberg/pull/3256#discussion_r730068789
##########
File path: spark/build.gradle
##########
@@ -0,0 +1,97 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+project(':iceberg-spark') {
+ configurations.all {
+ resolutionStrategy {
+ // Spark 2.4.4 can only use the below datanucleus version, the versions introduced
+ // by Hive 2.3.6 will meet lots of unexpected issues, so here force to use the versions
+ // introduced by Hive 1.2.1.
+ force 'org.datanucleus:datanucleus-api-jdo:3.2.6'
+ force 'org.datanucleus:datanucleus-core:3.2.10'
+ force 'org.datanucleus:datanucleus-rdbms:3.2.9'
+ }
+ }
+
+ dependencies {
+ implementation project(path: ':iceberg-bundled-guava', configuration: 'shadow')
+ api project(':iceberg-api')
+ implementation project(':iceberg-common')
+ implementation project(':iceberg-core')
+ api project(':iceberg-data')
+ implementation project(':iceberg-orc')
+ implementation project(':iceberg-parquet')
+ implementation project(':iceberg-arrow')
+ implementation project(':iceberg-hive-metastore')
+
+ compileOnly "com.google.errorprone:error_prone_annotations"
+ compileOnly "org.apache.avro:avro"
+ compileOnly("org.apache.spark:spark-hive_2.11") {
+ exclude group: 'org.apache.avro', module: 'avro'
+ }
+
+ implementation("org.apache.orc:orc-core::nohive") {
+ exclude group: 'org.apache.hadoop'
+ exclude group: 'commons-lang'
+ // These artifacts are shaded and included in the orc-core fat jar
+ exclude group: 'com.google.protobuf', module: 'protobuf-java'
+ exclude group: 'org.apache.hive', module: 'hive-storage-api'
+ }
+
+ implementation("org.apache.arrow:arrow-vector") {
+ exclude group: 'io.netty', module: 'netty-buffer'
+ exclude group: 'io.netty', module: 'netty-common'
+ exclude group: 'com.google.code.findbugs', module: 'jsr305'
+ }
+
+ testImplementation("org.apache.hadoop:hadoop-minicluster") {
+ exclude group: 'org.apache.avro', module: 'avro'
+ }
+ testImplementation project(path: ':iceberg-hive-metastore', configuration: 'testArtifacts')
+ testImplementation project(path: ':iceberg-api', configuration: 'testArtifacts')
+ testImplementation project(path: ':iceberg-core', configuration: 'testArtifacts')
+ testImplementation project(path: ':iceberg-data', configuration: 'testArtifacts')
+ }
+
+ test {
+ // For vectorized reads
+ // Allow unsafe memory access to avoid the costly check arrow does to check if index is within bounds
+ systemProperty("arrow.enable_unsafe_memory_access", "true")
+ // Disable expensive null check for every get(index) call.
+ // Iceberg manages nullability checks itself instead of relying on arrow.
+ systemProperty("arrow.enable_null_check_for_get", "false")
+
+ // Vectorized reads need more memory
+ maxHeapSize '2500m'
+ }
+}
+
+// add enabled Spark version modules to the build
+project.ext.defaultSparkVersions = "2.4,3.0"
Review comment:
I moved the known and default Spark version lists into `gradle.properties` so that they are shared.
As for getting rid of the common module, I'm all for that. I just want to make this as small as possible.
##########
File path: jmh.gradle
##########
@@ -17,15 +17,22 @@
* under the License.
*/
-def jmhProjects = []
-if (JavaVersion.current() == JavaVersion.VERSION_1_8) {
- jmhProjects = [ project("iceberg-spark2"), project("iceberg-spark3") ]
-} else if (JavaVersion.current() == JavaVersion.VERSION_11) {
- jmhProjects = [ project("iceberg-spark3") ]
-} else {
+if (jdkVersion != '8' && jdkVersion != '11') {
throw new GradleException("The JMH benchamrks must be run with JDK 8 or JDK 11")
}
+def knownSparkVersions = System.getProperty("knownSparkVersions").split(",")
Review comment:
Fixed. I also removed it from spark/build.gradle.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #3256: Build: Move Spark version modules under spark directory
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3256:
URL: https://github.com/apache/iceberg/pull/3256#discussion_r727220184
##########
File path: settings.gradle
##########
@@ -55,21 +52,44 @@ project(':arrow').name = 'iceberg-arrow'
project(':parquet').name = 'iceberg-parquet'
project(':bundled-guava').name = 'iceberg-bundled-guava'
project(':spark').name = 'iceberg-spark'
-project(':spark3').name = 'iceberg-spark3'
-project(':spark3-extensions').name = 'iceberg-spark3-extensions'
-project(':spark3-runtime').name = 'iceberg-spark3-runtime'
project(':pig').name = 'iceberg-pig'
project(':hive-metastore').name = 'iceberg-hive-metastore'
project(':nessie').name = 'iceberg-nessie'
+String defaultSparkVersions = "2.4,3.0"
Review comment:
We need to look into how to deduplicate this. And we can avoid parsing it as well.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #3256: Build: Move Spark version modules under spark directory
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #3256:
URL: https://github.com/apache/iceberg/pull/3256#issuecomment-941123660
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on pull request #3256: Build: Move Spark version modules under spark directory
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on pull request #3256:
URL: https://github.com/apache/iceberg/pull/3256#issuecomment-940455683
> 1. should we continue to keep the existing iceberg-spark module as a common module across all versions
While I am generally against sharing code between any versions of Spark, I am 50/50 here. We could probably have a common module for very basic things but then we won't compile that code against the exact Spark version we will be running with. Since the main development will be done against the most recent version, I'd slightly prefer duplicating the classes as then we won't have to reason how public and stable Spark APIs that are used in that common module are.
> The definition is that build version is the actual version we build Spark, and source version is the smallest version that would be forward compatible until we create the next source version.
I'd vote for no compatibility between minor versions, only patch releases. We tried that in the past with 3.0 and 3.1 and it slows down the development and makes the life much harder. We are using Spark APIs that change frequently. I'd say having a module for each minor Spark version would be better. I think there will be major features in each upcoming minor Spark version that we will want to consume ASAP.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on pull request #3256: Build: Move Spark version modules under spark directory
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on pull request #3256:
URL: https://github.com/apache/iceberg/pull/3256#issuecomment-940464300
@RussellSpitzer @szehon-ho @flyrain, any thoughts?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on a change in pull request #3256: Build: Move Spark version modules under spark directory
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #3256:
URL: https://github.com/apache/iceberg/pull/3256#discussion_r726556131
##########
File path: spark/build.gradle
##########
@@ -0,0 +1,97 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+project(':iceberg-spark') {
+ configurations.all {
Review comment:
Is it supposed to include common configs?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on a change in pull request #3256: Build: Move Spark version modules under spark directory
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #3256:
URL: https://github.com/apache/iceberg/pull/3256#discussion_r726575721
##########
File path: settings.gradle
##########
@@ -55,21 +52,44 @@ project(':arrow').name = 'iceberg-arrow'
project(':parquet').name = 'iceberg-parquet'
project(':bundled-guava').name = 'iceberg-bundled-guava'
project(':spark').name = 'iceberg-spark'
-project(':spark3').name = 'iceberg-spark3'
-project(':spark3-extensions').name = 'iceberg-spark3-extensions'
-project(':spark3-runtime').name = 'iceberg-spark3-runtime'
project(':pig').name = 'iceberg-pig'
project(':hive-metastore').name = 'iceberg-hive-metastore'
project(':nessie').name = 'iceberg-nessie'
+String defaultSparkVersions = "2.4,3.0"
+List<String> knownSparkVersions = ["2.4", "3.0"]
+List<String> sparkVersions = (System.getProperty("sparkVersions") != null ? System.getProperty("sparkVersions") : defaultSparkVersions).split(",")
+
+if (!knownSparkVersions.containsAll(sparkVersions)) {
+ throw new GradleException("Found unsupported Spark versions: " + (sparkVersions - knownSparkVersions))
+}
+
+if (sparkVersions.contains("3.0")) {
Review comment:
+1 for just add 3.2 next, that's why I was trying to introduce the concept of build and src versions.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #3256: Build: Move Spark version modules under spark directory
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3256:
URL: https://github.com/apache/iceberg/pull/3256#discussion_r726603924
##########
File path: spark/build.gradle
##########
@@ -0,0 +1,97 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+project(':iceberg-spark') {
+ configurations.all {
Review comment:
One more thing: I don't think that this common Spark module needs to exist in the long term. For Spark 3.2, I think we should move all the code in iceberg-spark into the 3.2 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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on a change in pull request #3256: Build: Move Spark version modules under spark directory
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #3256:
URL: https://github.com/apache/iceberg/pull/3256#discussion_r727313457
##########
File path: spark/build.gradle
##########
@@ -0,0 +1,97 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+project(':iceberg-spark') {
+ configurations.all {
+ resolutionStrategy {
+ // Spark 2.4.4 can only use the below datanucleus version, the versions introduced
+ // by Hive 2.3.6 will meet lots of unexpected issues, so here force to use the versions
+ // introduced by Hive 1.2.1.
+ force 'org.datanucleus:datanucleus-api-jdo:3.2.6'
+ force 'org.datanucleus:datanucleus-core:3.2.10'
+ force 'org.datanucleus:datanucleus-rdbms:3.2.9'
+ }
+ }
+
+ dependencies {
+ implementation project(path: ':iceberg-bundled-guava', configuration: 'shadow')
+ api project(':iceberg-api')
+ implementation project(':iceberg-common')
+ implementation project(':iceberg-core')
+ api project(':iceberg-data')
+ implementation project(':iceberg-orc')
+ implementation project(':iceberg-parquet')
+ implementation project(':iceberg-arrow')
+ implementation project(':iceberg-hive-metastore')
+
+ compileOnly "com.google.errorprone:error_prone_annotations"
+ compileOnly "org.apache.avro:avro"
+ compileOnly("org.apache.spark:spark-hive_2.11") {
+ exclude group: 'org.apache.avro', module: 'avro'
+ }
+
+ implementation("org.apache.orc:orc-core::nohive") {
+ exclude group: 'org.apache.hadoop'
+ exclude group: 'commons-lang'
+ // These artifacts are shaded and included in the orc-core fat jar
+ exclude group: 'com.google.protobuf', module: 'protobuf-java'
+ exclude group: 'org.apache.hive', module: 'hive-storage-api'
+ }
+
+ implementation("org.apache.arrow:arrow-vector") {
+ exclude group: 'io.netty', module: 'netty-buffer'
+ exclude group: 'io.netty', module: 'netty-common'
+ exclude group: 'com.google.code.findbugs', module: 'jsr305'
+ }
+
+ testImplementation("org.apache.hadoop:hadoop-minicluster") {
+ exclude group: 'org.apache.avro', module: 'avro'
+ }
+ testImplementation project(path: ':iceberg-hive-metastore', configuration: 'testArtifacts')
+ testImplementation project(path: ':iceberg-api', configuration: 'testArtifacts')
+ testImplementation project(path: ':iceberg-core', configuration: 'testArtifacts')
+ testImplementation project(path: ':iceberg-data', configuration: 'testArtifacts')
+ }
+
+ test {
+ // For vectorized reads
+ // Allow unsafe memory access to avoid the costly check arrow does to check if index is within bounds
+ systemProperty("arrow.enable_unsafe_memory_access", "true")
+ // Disable expensive null check for every get(index) call.
+ // Iceberg manages nullability checks itself instead of relying on arrow.
+ systemProperty("arrow.enable_null_check_for_get", "false")
+
+ // Vectorized reads need more memory
+ maxHeapSize '2500m'
+ }
+}
+
+// add enabled Spark version modules to the build
+project.ext.defaultSparkVersions = "2.4,3.0"
Review comment:
I was confused initially as I did not recognize we still kept the common module. The more I look, the more it seems we should get rid of 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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi edited a comment on pull request #3256: Build: Move Spark version modules under spark directory
Posted by GitBox <gi...@apache.org>.
aokolnychyi edited a comment on pull request #3256:
URL: https://github.com/apache/iceberg/pull/3256#issuecomment-940455683
> 1. should we continue to keep the existing iceberg-spark module as a common module across all versions
While I am generally against sharing code between any versions of Spark, I am 50/50 here. We could probably have a common module for very basic things but then we won't compile that code against the exact Spark version we will be running with. Since the main development will be done against the most recent version, I'd slightly prefer duplicating the classes as then we won't have to reason how public and stable Spark API, which are used in that common module, are.
> The definition is that build version is the actual version we build Spark, and source version is the smallest version that would be forward compatible until we create the next source version.
I'd vote for no compatibility between minor versions, only patch releases. We tried that in the past with 3.0 and 3.1 and it slows down the development and makes the life much harder. We are using Spark APIs that change frequently. I'd say having a module for each minor Spark version would be better. I think there will be major features in each upcoming minor Spark version that we will want to consume ASAP.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #3256: Build: Move Spark version modules under spark directory
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3256:
URL: https://github.com/apache/iceberg/pull/3256#discussion_r726601852
##########
File path: settings.gradle
##########
@@ -55,21 +52,44 @@ project(':arrow').name = 'iceberg-arrow'
project(':parquet').name = 'iceberg-parquet'
project(':bundled-guava').name = 'iceberg-bundled-guava'
project(':spark').name = 'iceberg-spark'
-project(':spark3').name = 'iceberg-spark3'
-project(':spark3-extensions').name = 'iceberg-spark3-extensions'
-project(':spark3-runtime').name = 'iceberg-spark3-runtime'
project(':pig').name = 'iceberg-pig'
project(':hive-metastore').name = 'iceberg-hive-metastore'
project(':nessie').name = 'iceberg-nessie'
+String defaultSparkVersions = "2.4,3.0"
+List<String> knownSparkVersions = ["2.4", "3.0"]
+List<String> sparkVersions = (System.getProperty("sparkVersions") != null ? System.getProperty("sparkVersions") : defaultSparkVersions).split(",")
+
+if (!knownSparkVersions.containsAll(sparkVersions)) {
+ throw new GradleException("Found unsupported Spark versions: " + (sparkVersions - knownSparkVersions))
+}
+
+if (sparkVersions.contains("3.0")) {
Review comment:
Let me think about this a bit more. I think that we should be able to run the tests and integration tests against 3.1. I think the easy way to do that is to introduce a 3.1 build directory. Maybe it can share source with 3.0 though? I just want to avoid having extra ways of calling tests for just one build of Spark. Ideally, we'd add actions to test each version of Spark individually.
##########
File path: settings.gradle
##########
@@ -55,21 +52,44 @@ project(':arrow').name = 'iceberg-arrow'
project(':parquet').name = 'iceberg-parquet'
project(':bundled-guava').name = 'iceberg-bundled-guava'
project(':spark').name = 'iceberg-spark'
-project(':spark3').name = 'iceberg-spark3'
-project(':spark3-extensions').name = 'iceberg-spark3-extensions'
-project(':spark3-runtime').name = 'iceberg-spark3-runtime'
project(':pig').name = 'iceberg-pig'
project(':hive-metastore').name = 'iceberg-hive-metastore'
project(':nessie').name = 'iceberg-nessie'
+String defaultSparkVersions = "2.4,3.0"
+List<String> knownSparkVersions = ["2.4", "3.0"]
+List<String> sparkVersions = (System.getProperty("sparkVersions") != null ? System.getProperty("sparkVersions") : defaultSparkVersions).split(",")
+
+if (!knownSparkVersions.containsAll(sparkVersions)) {
+ throw new GradleException("Found unsupported Spark versions: " + (sparkVersions - knownSparkVersions))
+}
+
+if (sparkVersions.contains("3.0")) {
Review comment:
Let me think about this a bit more. I think that we should be able to run the tests and integration tests against 3.1. I think the easy way to do that is to introduce a 3.1 build directory. Maybe it can share source with 3.0 though? I just want to avoid having extra ways of calling tests for just one build of Spark. Ideally, we'd add on set of actions to test each version of Spark individually.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #3256: Build: Move Spark version modules under spark directory
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3256:
URL: https://github.com/apache/iceberg/pull/3256#discussion_r726603540
##########
File path: spark/build.gradle
##########
@@ -0,0 +1,97 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+project(':iceberg-spark') {
+ configurations.all {
Review comment:
I didn't change the individual builds, other than to remove the Spark 3.1 parts of the Spark 3.0 build. We can refactor these as needed once the change is done.
But, I still think it's a good trade-off to avoid trying to share code. The version numbers may change across releases, or maybe Spark starts shading something and we no longer need to ship it. I think we should not worry about what's in some common config and just update the individual builds as 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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #3256: Build: Move Spark version modules under spark directory
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #3256:
URL: https://github.com/apache/iceberg/pull/3256#issuecomment-941123660
> My original thought in #3237 was that we can define the rule that the shared code can continue to live, and any code that cannot build againt all versions should then be refactored to different version modules.
I think that compiling against different versions of Spark is beginning to be a significant risk due to changes across versions. That's only going to get worse when we start adding flags for Scala version as well and continue to add more extensions that touch Spark internals. I don't think it's a good idea to try to refactor common classes out unless they don't have a compile dependency on Spark. And in that case, we can more them to core like the `Writer` classes.
> . . . The intention is again to minimize duplicated code by minimizing the number of version folders.
I think we're actually better off the other way. What we have now does some pretty crazy things to deal with multiple Spark versions and I think we'll end up with cleaner code if we just duplicate it. This is one of the reasons why I think we the build change.
> In #3237 I had another scalaVersion system property to allow setting a flexible Scala version. Do you think that is something worth adding?
Yes, but as a follow-up. This is just a PR to put a basic structure in place and we will continue to improve it in future commits.
> What would be the naming scheme for modules after v3.0?
We don't need to decide this now, since this doesn't introduce new modules yet. I'd like to focus on getting this in with minimal changes since it's already so large!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #3256: Build: Move Spark version modules under spark directory
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #3256:
URL: https://github.com/apache/iceberg/pull/3256#issuecomment-944617852
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on a change in pull request #3256: Build: Move Spark version modules under spark directory
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #3256:
URL: https://github.com/apache/iceberg/pull/3256#discussion_r726555035
##########
File path: spark/build.gradle
##########
@@ -0,0 +1,97 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+project(':iceberg-spark') {
+ configurations.all {
+ resolutionStrategy {
+ // Spark 2.4.4 can only use the below datanucleus version, the versions introduced
+ // by Hive 2.3.6 will meet lots of unexpected issues, so here force to use the versions
+ // introduced by Hive 1.2.1.
+ force 'org.datanucleus:datanucleus-api-jdo:3.2.6'
+ force 'org.datanucleus:datanucleus-core:3.2.10'
+ force 'org.datanucleus:datanucleus-rdbms:3.2.9'
+ }
+ }
+
+ dependencies {
+ implementation project(path: ':iceberg-bundled-guava', configuration: 'shadow')
+ api project(':iceberg-api')
+ implementation project(':iceberg-common')
+ implementation project(':iceberg-core')
+ api project(':iceberg-data')
+ implementation project(':iceberg-orc')
+ implementation project(':iceberg-parquet')
+ implementation project(':iceberg-arrow')
+ implementation project(':iceberg-hive-metastore')
+
+ compileOnly "com.google.errorprone:error_prone_annotations"
+ compileOnly "org.apache.avro:avro"
+ compileOnly("org.apache.spark:spark-hive_2.11") {
+ exclude group: 'org.apache.avro', module: 'avro'
+ }
+
+ implementation("org.apache.orc:orc-core::nohive") {
+ exclude group: 'org.apache.hadoop'
+ exclude group: 'commons-lang'
+ // These artifacts are shaded and included in the orc-core fat jar
+ exclude group: 'com.google.protobuf', module: 'protobuf-java'
+ exclude group: 'org.apache.hive', module: 'hive-storage-api'
+ }
+
+ implementation("org.apache.arrow:arrow-vector") {
+ exclude group: 'io.netty', module: 'netty-buffer'
+ exclude group: 'io.netty', module: 'netty-common'
+ exclude group: 'com.google.code.findbugs', module: 'jsr305'
+ }
+
+ testImplementation("org.apache.hadoop:hadoop-minicluster") {
+ exclude group: 'org.apache.avro', module: 'avro'
+ }
+ testImplementation project(path: ':iceberg-hive-metastore', configuration: 'testArtifacts')
+ testImplementation project(path: ':iceberg-api', configuration: 'testArtifacts')
+ testImplementation project(path: ':iceberg-core', configuration: 'testArtifacts')
+ testImplementation project(path: ':iceberg-data', configuration: 'testArtifacts')
+ }
+
+ test {
+ // For vectorized reads
+ // Allow unsafe memory access to avoid the costly check arrow does to check if index is within bounds
+ systemProperty("arrow.enable_unsafe_memory_access", "true")
+ // Disable expensive null check for every get(index) call.
+ // Iceberg manages nullability checks itself instead of relying on arrow.
+ systemProperty("arrow.enable_null_check_for_get", "false")
+
+ // Vectorized reads need more memory
+ maxHeapSize '2500m'
+ }
+}
+
+// add enabled Spark version modules to the build
+project.ext.defaultSparkVersions = "2.4,3.0"
Review comment:
Similar to the question above, can we just keep this part in this particular `build.gradle`?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on a change in pull request #3256: Build: Move Spark version modules under spark directory
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #3256:
URL: https://github.com/apache/iceberg/pull/3256#discussion_r726554678
##########
File path: spark/build.gradle
##########
@@ -0,0 +1,97 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+project(':iceberg-spark') {
+ configurations.all {
Review comment:
Why do we need all these dependencies if the same is also defined for each specific version?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #3256: Build: Move Spark version modules under spark directory
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #3256:
URL: https://github.com/apache/iceberg/pull/3256#discussion_r726676480
##########
File path: settings.gradle
##########
@@ -55,21 +52,44 @@ project(':arrow').name = 'iceberg-arrow'
project(':parquet').name = 'iceberg-parquet'
project(':bundled-guava').name = 'iceberg-bundled-guava'
project(':spark').name = 'iceberg-spark'
-project(':spark3').name = 'iceberg-spark3'
-project(':spark3-extensions').name = 'iceberg-spark3-extensions'
-project(':spark3-runtime').name = 'iceberg-spark3-runtime'
project(':pig').name = 'iceberg-pig'
project(':hive-metastore').name = 'iceberg-hive-metastore'
project(':nessie').name = 'iceberg-nessie'
+String defaultSparkVersions = "2.4,3.0"
Review comment:
Looks like we are defining this as a local variable and as a project property? Also should we store this as a list? Seems a little odd that we split it to turn it into a list immediately after?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on pull request #3256: Build: Move Spark version modules under spark directory
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on pull request #3256:
URL: https://github.com/apache/iceberg/pull/3256#issuecomment-940264386
Thanks for the PR! There are a few aspects I'd like to discuss:
> 1. should we continue to keep the existing iceberg-spark module as a common module across all versions
My original thought in #3237 was that we can define the rule that the shared code can continue to live, and any code that cannot build againt all versions should then be refactored to different version modules. The intention is to minimize duplicated code across version modules, but I don't have enough context yet if this is worth the effort, maybe after refactoring the current code to build across Spark 2 and 3, we don't have much shared code anyway.
> 2. should we have v3.1 if v3.0 and v3.1 can all be built against v3.0
In #3237 I have the concept of a build version and source version.
The definition is that build version is the actual version we build Spark, and source version is the smallest version that would be forward compatible until we create the next source version.
So in this case, we will have source version v2.4 that can build Spark 2.4.x, source version v3.0 that can build Spark 3.0.x and 3.1.x. And we will have a v3.2 for build versions v3.2.x and forward until we cannot build against v3.2 source.
The intention is again to minimize duplicated code by minimizing the number of version folders.
> 3. How would we handle Scala version in this architecture?
In #3237 I had another `scalaVersion` system property to allow setting a flexible Scala version. Do you think that is something worth adding? It doesn't need to be refactored in this PR, just want to know what you think about Scala versioning.
> 4. What would be the naming scheme for modules after v3.0?
This is just something not yet shown in this PR, for example for v3.2, would the package names be something like `:iceberg-spark:spark32-runtime`, or `:iceberg-spark:spark3-2-runtime`, or something else?
Also combining with Scala version consideration, if we support that, should we also publish artifacts for all Scala versions we support like what Spark does?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a change in pull request #3256: Build: Move Spark version modules under spark directory
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #3256:
URL: https://github.com/apache/iceberg/pull/3256#discussion_r725532356
##########
File path: spark/build.gradle
##########
@@ -0,0 +1,97 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+project(':iceberg-spark') {
+ configurations.all {
+ resolutionStrategy {
+ // Spark 2.4.4 can only use the below datanucleus version, the versions introduced
+ // by Hive 2.3.6 will meet lots of unexpected issues, so here force to use the versions
+ // introduced by Hive 1.2.1.
+ force 'org.datanucleus:datanucleus-api-jdo:3.2.6'
+ force 'org.datanucleus:datanucleus-core:3.2.10'
+ force 'org.datanucleus:datanucleus-rdbms:3.2.9'
+ }
+ }
+
+ dependencies {
+ implementation project(path: ':iceberg-bundled-guava', configuration: 'shadow')
+ api project(':iceberg-api')
+ implementation project(':iceberg-common')
+ implementation project(':iceberg-core')
+ api project(':iceberg-data')
+ implementation project(':iceberg-orc')
+ implementation project(':iceberg-parquet')
+ implementation project(':iceberg-arrow')
+ implementation project(':iceberg-hive-metastore')
+
+ compileOnly "com.google.errorprone:error_prone_annotations"
+ compileOnly "org.apache.avro:avro"
+ compileOnly("org.apache.spark:spark-hive_2.11") {
+ exclude group: 'org.apache.avro', module: 'avro'
+ }
+
+ implementation("org.apache.orc:orc-core::nohive") {
Review comment:
Is there an extra `:` before `nohive`?
Edit: Spoke to soon - the double colon exists in the old build file as well.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #3256: Build: Move Spark version modules under spark directory
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #3256:
URL: https://github.com/apache/iceberg/pull/3256#discussion_r726673564
##########
File path: jmh.gradle
##########
@@ -19,9 +19,9 @@
def jmhProjects = []
if (JavaVersion.current() == JavaVersion.VERSION_1_8) {
- jmhProjects = [ project("iceberg-spark2"), project("iceberg-spark3") ]
+ jmhProjects = [ project(":iceberg-spark:iceberg-spark2"), project(":iceberg-spark:iceberg-spark3") ]
Review comment:
I was wondering why do this with a def since I think we only need this as a local variable,
I was messing around writing new tests for ZOrdering and subbed out this code with
```groovy
List<Project> jmhProjects = (JavaVersion.current() == JavaVersion.VERSION_1_8) ?
[project("iceberg-spark2"), project("iceberg-spark3")] : [project("iceberg-spark3")]
```
I think if we want to keep the version erroring we could do
```groovy
List<Project> jmhProjects = []
switch (JavaVersion.current()) {
case JavaVersion.VERSION_1_8:
jmhProjects = [project("iceberg-spark2"), project("iceberg-spark3")]
break;
case JavaVersion.VERSION_11:
jmhProjects = [project("iceberg-spark3")]
break;
default:
throw new GradleException("The JMH benchamrks must be run with JDK 8 or JDK 11")
}
```
Anyway just a suggestion, feel free to ignore
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #3256: Build: Move Spark version modules under spark directory
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #3256:
URL: https://github.com/apache/iceberg/pull/3256#issuecomment-941215358
Yeah, looks like since we're moving files around this hits commit conflicts really quickly. We should try to get consensus around the file moves and structure as soon as possible and then get this in. That's partly why I wanted to make this first commit do as little as possible. We can add more refactoring later, but it's easiest to do this as multiple focused commits with only one big file move!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue removed a comment on pull request #3256: Build: Move Spark version modules under spark directory
Posted by GitBox <gi...@apache.org>.
rdblue removed a comment on pull request #3256:
URL: https://github.com/apache/iceberg/pull/3256#issuecomment-941123660
> My original thought in #3237 was that we can define the rule that the shared code can continue to live, and any code that cannot build againt all versions should then be refactored to different version modules.
I think that compiling against different versions of Spark is beginning to be a significant risk due to changes across versions. That's only going to get worse when we start adding flags for Scala version as well and continue to add more extensions that touch Spark internals. I don't think it's a good idea to try to refactor common classes out unless they don't have a compile dependency on Spark. And in that case, we can more them to core like the `Writer` classes.
> . . . The intention is again to minimize duplicated code by minimizing the number of version folders.
I think we're actually better off the other way. What we have now does some pretty crazy things to deal with multiple Spark versions and I think we'll end up with cleaner code if we just duplicate it. This is one of the reasons why I think we the build change.
> In #3237 I had another scalaVersion system property to allow setting a flexible Scala version. Do you think that is something worth adding?
Yes, but as a follow-up. This is just a PR to put a basic structure in place and we will continue to improve it in future commits.
> What would be the naming scheme for modules after v3.0?
We don't need to decide this now, since this doesn't introduce new modules yet. I'd like to focus on getting this in with minimal changes since it's already so large!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on a change in pull request #3256: Build: Move Spark version modules under spark directory
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #3256:
URL: https://github.com/apache/iceberg/pull/3256#discussion_r726579213
##########
File path: spark/build.gradle
##########
@@ -0,0 +1,97 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+project(':iceberg-spark') {
+ configurations.all {
+ resolutionStrategy {
+ // Spark 2.4.4 can only use the below datanucleus version, the versions introduced
+ // by Hive 2.3.6 will meet lots of unexpected issues, so here force to use the versions
+ // introduced by Hive 1.2.1.
+ force 'org.datanucleus:datanucleus-api-jdo:3.2.6'
+ force 'org.datanucleus:datanucleus-core:3.2.10'
+ force 'org.datanucleus:datanucleus-rdbms:3.2.9'
+ }
+ }
+
+ dependencies {
+ implementation project(path: ':iceberg-bundled-guava', configuration: 'shadow')
+ api project(':iceberg-api')
+ implementation project(':iceberg-common')
+ implementation project(':iceberg-core')
+ api project(':iceberg-data')
+ implementation project(':iceberg-orc')
+ implementation project(':iceberg-parquet')
+ implementation project(':iceberg-arrow')
+ implementation project(':iceberg-hive-metastore')
+
+ compileOnly "com.google.errorprone:error_prone_annotations"
+ compileOnly "org.apache.avro:avro"
+ compileOnly("org.apache.spark:spark-hive_2.11") {
+ exclude group: 'org.apache.avro', module: 'avro'
+ }
+
+ implementation("org.apache.orc:orc-core::nohive") {
+ exclude group: 'org.apache.hadoop'
+ exclude group: 'commons-lang'
+ // These artifacts are shaded and included in the orc-core fat jar
+ exclude group: 'com.google.protobuf', module: 'protobuf-java'
+ exclude group: 'org.apache.hive', module: 'hive-storage-api'
+ }
+
+ implementation("org.apache.arrow:arrow-vector") {
+ exclude group: 'io.netty', module: 'netty-buffer'
+ exclude group: 'io.netty', module: 'netty-common'
+ exclude group: 'com.google.code.findbugs', module: 'jsr305'
+ }
+
+ testImplementation("org.apache.hadoop:hadoop-minicluster") {
+ exclude group: 'org.apache.avro', module: 'avro'
+ }
+ testImplementation project(path: ':iceberg-hive-metastore', configuration: 'testArtifacts')
+ testImplementation project(path: ':iceberg-api', configuration: 'testArtifacts')
+ testImplementation project(path: ':iceberg-core', configuration: 'testArtifacts')
+ testImplementation project(path: ':iceberg-data', configuration: 'testArtifacts')
+ }
+
+ test {
+ // For vectorized reads
+ // Allow unsafe memory access to avoid the costly check arrow does to check if index is within bounds
+ systemProperty("arrow.enable_unsafe_memory_access", "true")
+ // Disable expensive null check for every get(index) call.
+ // Iceberg manages nullability checks itself instead of relying on arrow.
+ systemProperty("arrow.enable_null_check_for_get", "false")
+
+ // Vectorized reads need more memory
+ maxHeapSize '2500m'
+ }
+}
+
+// add enabled Spark version modules to the build
+project.ext.defaultSparkVersions = "2.4,3.0"
Review comment:
Since we have parameterization in `settings.gradle`, do we really need the this build file once the code is moved into subfolders?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #3256: Build: Move Spark version modules under spark directory
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3256:
URL: https://github.com/apache/iceberg/pull/3256#discussion_r726602584
##########
File path: settings.gradle
##########
@@ -55,21 +52,44 @@ project(':arrow').name = 'iceberg-arrow'
project(':parquet').name = 'iceberg-parquet'
project(':bundled-guava').name = 'iceberg-bundled-guava'
project(':spark').name = 'iceberg-spark'
-project(':spark3').name = 'iceberg-spark3'
-project(':spark3-extensions').name = 'iceberg-spark3-extensions'
-project(':spark3-runtime').name = 'iceberg-spark3-runtime'
project(':pig').name = 'iceberg-pig'
project(':hive-metastore').name = 'iceberg-hive-metastore'
project(':nessie').name = 'iceberg-nessie'
+String defaultSparkVersions = "2.4,3.0"
+List<String> knownSparkVersions = ["2.4", "3.0"]
+List<String> sparkVersions = (System.getProperty("sparkVersions") != null ? System.getProperty("sparkVersions") : defaultSparkVersions).split(",")
+
+if (!knownSparkVersions.containsAll(sparkVersions)) {
+ throw new GradleException("Found unsupported Spark versions: " + (sparkVersions - knownSparkVersions))
+}
+
+if (sparkVersions.contains("3.0")) {
+ include ':iceberg-spark:spark3'
+ include ':iceberg-spark:spark3-extensions'
+ include ':iceberg-spark:spark3-runtime'
+ project(':iceberg-spark:spark3').projectDir = file('spark/v3.0/spark3')
+ project(':iceberg-spark:spark3').name = 'iceberg-spark3'
+ project(':iceberg-spark:spark3-extensions').projectDir = file('spark/v3.0/spark3-extensions')
+ project(':iceberg-spark:spark3-extensions').name = 'iceberg-spark3-extensions'
+ project(':iceberg-spark:spark3-runtime').projectDir = file('spark/v3.0/spark3-runtime')
+ project(':iceberg-spark:spark3-runtime').name = 'iceberg-spark3-runtime'
+}
+
if (JavaVersion.current() == JavaVersion.VERSION_1_8) {
Review comment:
We do that in the Spark 2.4.x `build.gradle`: https://github.com/apache/iceberg/pull/3256/files#diff-7938d5bc8beec0f7ef2243ba77a3947bfcc529814249a422ff4c60d48a4a2e45R21
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #3256: Build: Move Spark version modules under spark directory
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #3256:
URL: https://github.com/apache/iceberg/pull/3256#discussion_r726673564
##########
File path: jmh.gradle
##########
@@ -19,9 +19,9 @@
def jmhProjects = []
if (JavaVersion.current() == JavaVersion.VERSION_1_8) {
- jmhProjects = [ project("iceberg-spark2"), project("iceberg-spark3") ]
+ jmhProjects = [ project(":iceberg-spark:iceberg-spark2"), project(":iceberg-spark:iceberg-spark3") ]
Review comment:
I was messing around writing new tests for ZOrdering and subbed out this code with
```groovy
List<Project> jmhProjects = (JavaVersion.current() == JavaVersion.VERSION_1_8) ?
[project("iceberg-spark2"), project("iceberg-spark3")] : [project("iceberg-spark3")]
```
I think if we want to keep the version erroring we could do
```groovy
List<Project> jmhProjects = []
switch (JavaVersion.current()) {
case JavaVersion.VERSION_1_8:
jmhProjects = [project("iceberg-spark2"), project("iceberg-spark3")]
break;
case JavaVersion.VERSION_11:
jmhProjects = [project("iceberg-spark3")]
break;
default:
throw new GradleException("The JMH benchamrks must be run with JDK 8 or JDK 11")
}
```
Anyway just a suggestion, feel free to ignore
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a change in pull request #3256: Build: Move Spark version modules under spark directory
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #3256:
URL: https://github.com/apache/iceberg/pull/3256#discussion_r725532356
##########
File path: spark/build.gradle
##########
@@ -0,0 +1,97 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+project(':iceberg-spark') {
+ configurations.all {
+ resolutionStrategy {
+ // Spark 2.4.4 can only use the below datanucleus version, the versions introduced
+ // by Hive 2.3.6 will meet lots of unexpected issues, so here force to use the versions
+ // introduced by Hive 1.2.1.
+ force 'org.datanucleus:datanucleus-api-jdo:3.2.6'
+ force 'org.datanucleus:datanucleus-core:3.2.10'
+ force 'org.datanucleus:datanucleus-rdbms:3.2.9'
+ }
+ }
+
+ dependencies {
+ implementation project(path: ':iceberg-bundled-guava', configuration: 'shadow')
+ api project(':iceberg-api')
+ implementation project(':iceberg-common')
+ implementation project(':iceberg-core')
+ api project(':iceberg-data')
+ implementation project(':iceberg-orc')
+ implementation project(':iceberg-parquet')
+ implementation project(':iceberg-arrow')
+ implementation project(':iceberg-hive-metastore')
+
+ compileOnly "com.google.errorprone:error_prone_annotations"
+ compileOnly "org.apache.avro:avro"
+ compileOnly("org.apache.spark:spark-hive_2.11") {
+ exclude group: 'org.apache.avro', module: 'avro'
+ }
+
+ implementation("org.apache.orc:orc-core::nohive") {
Review comment:
Is there an extra `:` before `nohive`?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue edited a comment on pull request #3256: Build: Move Spark version modules under spark directory
Posted by GitBox <gi...@apache.org>.
rdblue edited a comment on pull request #3256:
URL: https://github.com/apache/iceberg/pull/3256#issuecomment-941094431
> My original thought in #3237 was that we can define the rule that the shared code can continue to live, and any code that cannot build againt all versions should then be refactored to different version modules.
I think that compiling against different versions of Spark is beginning to be a significant risk due to changes across versions. That's only going to get worse when we start adding flags for Scala version as well and continue to add more extensions that touch Spark internals. I don't think it's a good idea to try to refactor common classes out unless they don't have a compile dependency on Spark. And in that case, we can more them to core like the `Writer` classes.
> . . . The intention is again to minimize duplicated code by minimizing the number of version folders.
I think we're actually better off the other way. What we have now does some pretty crazy things to deal with multiple Spark versions and I think we'll end up with cleaner code if we just duplicate it. This is one of the reasons why I think we want these build changes.
> In #3237 I had another scalaVersion system property to allow setting a flexible Scala version. Do you think that is something worth adding?
Yes, but as a follow-up. This is just a PR to put a basic structure in place and we will continue to improve it in future commits.
> What would be the naming scheme for modules after v3.0?
We don't need to decide this now, since this doesn't introduce new modules yet. I'd like to focus on getting this in with minimal changes since it's already so large!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on pull request #3256: Build: Move Spark version modules under spark directory
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on pull request #3256:
URL: https://github.com/apache/iceberg/pull/3256#issuecomment-941180930
We should probably wait with merging any other Spark related PRs before this is in.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #3256: Build: Move Spark version modules under spark directory
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3256:
URL: https://github.com/apache/iceberg/pull/3256#discussion_r727221577
##########
File path: jmh.gradle
##########
@@ -19,9 +19,9 @@
def jmhProjects = []
if (JavaVersion.current() == JavaVersion.VERSION_1_8) {
- jmhProjects = [ project("iceberg-spark2"), project("iceberg-spark3") ]
+ jmhProjects = [ project(":iceberg-spark:iceberg-spark2"), project(":iceberg-spark:iceberg-spark3") ]
Review comment:
Thanks, @RussellSpitzer! That's one of the things we needed to update.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on a change in pull request #3256: Build: Move Spark version modules under spark directory
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #3256:
URL: https://github.com/apache/iceberg/pull/3256#discussion_r730071369
##########
File path: jmh.gradle
##########
@@ -17,15 +17,22 @@
* under the License.
*/
-def jmhProjects = []
-if (JavaVersion.current() == JavaVersion.VERSION_1_8) {
- jmhProjects = [ project("iceberg-spark2"), project("iceberg-spark3") ]
-} else if (JavaVersion.current() == JavaVersion.VERSION_11) {
- jmhProjects = [ project("iceberg-spark3") ]
-} else {
+if (jdkVersion != '8' && jdkVersion != '11') {
throw new GradleException("The JMH benchamrks must be run with JDK 8 or JDK 11")
}
+def knownSparkVersions = System.getProperty("knownSparkVersions").split(",")
Review comment:
Do we use `knownSparkVersions` in jmh?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on a change in pull request #3256: Build: Move Spark version modules under spark directory
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #3256:
URL: https://github.com/apache/iceberg/pull/3256#discussion_r726571137
##########
File path: spark/build.gradle
##########
@@ -0,0 +1,97 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+project(':iceberg-spark') {
+ configurations.all {
Review comment:
Or is it just temporary and will be removed once we move the code into subfolders?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on a change in pull request #3256: Build: Move Spark version modules under spark directory
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #3256:
URL: https://github.com/apache/iceberg/pull/3256#discussion_r726542449
##########
File path: settings.gradle
##########
@@ -55,21 +52,44 @@ project(':arrow').name = 'iceberg-arrow'
project(':parquet').name = 'iceberg-parquet'
project(':bundled-guava').name = 'iceberg-bundled-guava'
project(':spark').name = 'iceberg-spark'
-project(':spark3').name = 'iceberg-spark3'
-project(':spark3-extensions').name = 'iceberg-spark3-extensions'
-project(':spark3-runtime').name = 'iceberg-spark3-runtime'
project(':pig').name = 'iceberg-pig'
project(':hive-metastore').name = 'iceberg-hive-metastore'
project(':nessie').name = 'iceberg-nessie'
+String defaultSparkVersions = "2.4,3.0"
+List<String> knownSparkVersions = ["2.4", "3.0"]
+List<String> sparkVersions = (System.getProperty("sparkVersions") != null ? System.getProperty("sparkVersions") : defaultSparkVersions).split(",")
+
+if (!knownSparkVersions.containsAll(sparkVersions)) {
+ throw new GradleException("Found unsupported Spark versions: " + (sparkVersions - knownSparkVersions))
+}
+
+if (sparkVersions.contains("3.0")) {
Review comment:
Do we even need to add 3.1 module? Seems like we should be fine with 3.0 only unless someone in the community wants to leverage some 3.1 APIs?
I'd probably just add 3.2 next and focus on that version.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #3256: Build: Move Spark version modules under spark directory
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3256:
URL: https://github.com/apache/iceberg/pull/3256#discussion_r726598139
##########
File path: jmh.gradle
##########
@@ -19,9 +19,9 @@
def jmhProjects = []
if (JavaVersion.current() == JavaVersion.VERSION_1_8) {
- jmhProjects = [ project("iceberg-spark2"), project("iceberg-spark3") ]
+ jmhProjects = [ project(":iceberg-spark:iceberg-spark2"), project(":iceberg-spark:iceberg-spark3") ]
Review comment:
No, all modules include `iceberg-` at the start. That way all of the Jar names don't change.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on a change in pull request #3256: Build: Move Spark version modules under spark directory
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #3256:
URL: https://github.com/apache/iceberg/pull/3256#discussion_r727299658
##########
File path: settings.gradle
##########
@@ -55,21 +52,44 @@ project(':arrow').name = 'iceberg-arrow'
project(':parquet').name = 'iceberg-parquet'
project(':bundled-guava').name = 'iceberg-bundled-guava'
project(':spark').name = 'iceberg-spark'
-project(':spark3').name = 'iceberg-spark3'
-project(':spark3-extensions').name = 'iceberg-spark3-extensions'
-project(':spark3-runtime').name = 'iceberg-spark3-runtime'
project(':pig').name = 'iceberg-pig'
project(':hive-metastore').name = 'iceberg-hive-metastore'
project(':nessie').name = 'iceberg-nessie'
+String defaultSparkVersions = "2.4,3.0"
+List<String> knownSparkVersions = ["2.4", "3.0"]
+List<String> sparkVersions = (System.getProperty("sparkVersions") != null ? System.getProperty("sparkVersions") : defaultSparkVersions).split(",")
+
+if (!knownSparkVersions.containsAll(sparkVersions)) {
+ throw new GradleException("Found unsupported Spark versions: " + (sparkVersions - knownSparkVersions))
+}
+
+if (sparkVersions.contains("3.0")) {
Review comment:
I am fine either way.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on pull request #3256: Build: Move Spark version modules under spark directory
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on pull request #3256:
URL: https://github.com/apache/iceberg/pull/3256#issuecomment-944620075
A separate 3.1 sounds reasonable to me. It will follow the new naming, right? With the Spark and Scala versions in 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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #3256: Build: Move Spark version modules under spark directory
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #3256:
URL: https://github.com/apache/iceberg/pull/3256#issuecomment-941094431
> My original thought in #3237 was that we can define the rule that the shared code can continue to live, and any code that cannot build againt all versions should then be refactored to different version modules.
I think that compiling against different versions of Spark is beginning to be a significant risk due to changes across versions. That's only going to get worse when we start adding flags for Scala version as well and continue to add more extensions that touch Spark internals. I don't think it's a good idea to try to refactor common classes out unless they don't have a compile dependency on Spark. And in that case, we can more them to core like the `Writer` classes.
> . . . The intention is again to minimize duplicated code by minimizing the number of version folders.
I think we're actually better off the other way. What we have now does some pretty crazy things to deal with multiple Spark versions and I think we'll end up with cleaner code if we just duplicate it. This is one of the reasons why I think we the build change.
> In #3237 I had another scalaVersion system property to allow setting a flexible Scala version. Do you think that is something worth adding?
Yes, but as a follow-up. This is just a PR to put a basic structure in place and we will continue to improve it in future commits.
> What would be the naming scheme for modules after v3.0?
We don't need to decide this now, since this doesn't introduce new modules yet. I'd like to focus on getting this in with minimal changes since it's already so large!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org