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/07/15 11:33:29 UTC
[GitHub] [iceberg] nastra opened a new pull request #2826: Upgrade to Gradle 7
nastra opened a new pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826
Note that the `compile` / `runtime` / `testRuntime` / `testCompile` configurations were removed and replaced with `implementation` / `runtimeOnly` / `testRuntimeOnly` / `testImplementation`.
See also https://docs.gradle.org/current/userguide/upgrading_version_6.html#sec:configuration_removal for additional details.
The build currently fails because we need https://github.com/palantir/gradle-baseline/issues/1819 so that the `gradle-baseline` plugin works with Gradle 7.
--
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 #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r676750640
##########
File path: arrow/src/main/java/org/apache/iceberg/arrow/vectorized/parquet/VectorizedParquetDefinitionLevelReader.java
##########
@@ -305,7 +305,7 @@ public void nextDictEncodedBatch(
protected abstract void nextVal(
FieldVector vector, int idx, ValuesAsBytesReader valuesReader, int typeWidth, byte[] byteArray);
protected abstract void nextDictEncodedVal(
- FieldVector vector, int startOffset, VectorizedDictionaryEncodedParquetValuesReader reader,
+ FieldVector vector, int idx, VectorizedDictionaryEncodedParquetValuesReader reader,
Review comment:
Do you know why this is needed now? I thought we already caught variable names that shadow field names.
--
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 #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r677678017
##########
File path: build.gradle
##########
@@ -247,13 +250,30 @@ project(':iceberg-data') {
exclude group: 'org.slf4j', module: 'slf4j-log4j12'
}
- testCompile("org.apache.hadoop:hadoop-client") {
+ implementation("org.apache.orc:orc-core::nohive") {
Review comment:
This adds implementation dependencies for org.apache.avro:avro and org.apache.orc:orc-core. Why isn't there one for org.apache.parquet?
Can you be more specific about what you're saying with implementation vs compile? When are transitive dependencies included and when are they not?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: 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] nastra commented on a change in pull request #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
nastra commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r696340309
##########
File path: build.gradle
##########
@@ -401,15 +441,15 @@ project(':iceberg-flink-runtime') {
}
dependencies {
- compile project(':iceberg-flink')
- compile project(':iceberg-aws')
- compile(project(':iceberg-nessie')) {
+ implementation project(':iceberg-flink')
+ implementation project(':iceberg-aws')
+ implementation(project(':iceberg-nessie')) {
exclude group: 'com.google.code.findbugs', module: 'jsr305'
}
}
shadowJar {
- configurations = [project.configurations.compile]
+ configurations = [project.configurations.runtimeClasspath]
Review comment:
yes this now contains `runtimeOnly` as well as can be seen in https://github.com/apache/iceberg/pull/2826#discussion_r681794627
--
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 #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r696077805
##########
File path: build.gradle
##########
@@ -174,7 +175,7 @@ project(':iceberg-bundled-guava') {
shadowJar {
classifier null
- configurations = [project.configurations.compileOnly]
+ configurations = [project.configurations.compileClasspath]
Review comment:
I am still not sure I fully understood. So `compileClasspath` includes `compileOnly` as well as `implementation`. However, previously we only pulled `compileOnly`. We did not pull `compile` (now `implementation`) before.
--
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 #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r690838931
##########
File path: baseline.gradle
##########
@@ -47,7 +47,8 @@ subprojects {
// So we apply Spotless manually to get a similar effect to baseline-format, but change the
// import order.
- pluginManager.withPlugin('com.diffplug.gradle.spotless') {
+
Review comment:
nit: extra line?
##########
File path: baseline.gradle
##########
@@ -36,7 +36,7 @@ subprojects {
apply plugin: 'com.palantir.baseline-checkstyle'
apply plugin: 'com.palantir.baseline-error-prone'
}
- apply plugin: 'com.palantir.baseline-scalastyle'
+ apply plugin: 'com.github.alisiikh.scalastyle'
Review comment:
How widely is this plugin used? What's the license? I did not find that in their GitHub.
##########
File path: hive3/src/main/java/org/apache/iceberg/mr/hive/vector/CompatibilityHiveVectorUtils.java
##########
@@ -59,7 +59,7 @@ private CompatibilityHiveVectorUtils() {
* Returns serialized mapwork instance from a job conf - ported from Hive source code LlapHiveUtils#findMapWork
*
* @param job JobConf instance
- * @return
+ * @return A serialized {@link MapWork} based on the given job conf
Review comment:
nit: We usually start with a lower case after `@return`.
##########
File path: baseline.gradle
##########
@@ -82,4 +83,15 @@ subprojects {
)
}
}
-}
+
+ pluginManager.withPlugin('com.github.alisiikh.scalastyle') {
+ scalastyle {
+ config = file("${rootDir}/project/scalastyle_config.xml")
+ inputEncoding = 'UTF-8'
+ outputEncoding = 'UTF-8'
+ failOnWarning = false
+ verbose = false
+ quiet = false
+ }
+ }
+}
Review comment:
nit: missing an empty line at the end of the file?
##########
File path: mr/src/test/java/org/apache/iceberg/mr/hive/TestTables.java
##########
@@ -115,7 +115,7 @@ public String catalogName() {
/**
* The table properties string needed for the CREATE TABLE ... commands,
* like "TBLPROPERTIES('iceberg.catalog'='mycatalog')
- * @return
+ * @return The tables properties string, such as "TBLPROPERTIES('iceberg.catalog'='mycatalog')
Review comment:
I think quotes are not aligned too
##########
File path: mr/src/test/java/org/apache/iceberg/mr/hive/TestTables.java
##########
@@ -115,7 +115,7 @@ public String catalogName() {
/**
* The table properties string needed for the CREATE TABLE ... commands,
* like "TBLPROPERTIES('iceberg.catalog'='mycatalog')
- * @return
+ * @return The tables properties string, such as "TBLPROPERTIES('iceberg.catalog'='mycatalog')
Review comment:
nit: same here
##########
File path: core/src/test/java/org/apache/iceberg/TestTableMetadata.java
##########
@@ -90,11 +90,11 @@ public void testJsonConversion() throws Exception {
long previousSnapshotId = System.currentTimeMillis() - new Random(1234).nextInt(3600);
Snapshot previousSnapshot = new BaseSnapshot(
ops.io(), previousSnapshotId, null, previousSnapshotId, null, null, null, ImmutableList.of(
- new GenericManifestFile(localInput("file:/tmp/manfiest.1.avro"), SPEC_5.specId())));
+ new GenericManifestFile(localInput("file:/tmp/manfiest.1.avro"), SPEC_5.specId())));
Review comment:
Are these required?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: 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 #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r681794627
##########
File path: build.gradle
##########
@@ -174,7 +175,7 @@ project(':iceberg-bundled-guava') {
shadowJar {
classifier null
- configurations = [project.configurations.compileOnly]
+ configurations = [project.configurations.compileClasspath]
Review comment:
this is because Compile is now basically compileOnly
--
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 #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#issuecomment-900758469
@kbendick @flyrain @karuppayya, could you help reviewing this one too if you have time?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: 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] nastra commented on pull request #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
nastra commented on pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#issuecomment-899313851
@rdblue given that Iceberg 0.12.0 is out, I think now would be a good time to revisit this PR and get it merged.
--
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] nastra commented on a change in pull request #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
nastra commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r691491938
##########
File path: gradlew
##########
@@ -28,7 +44,7 @@ APP_NAME="Gradle"
APP_BASE_NAME=`basename "$0"`
# Add default JVM options here. You can also use JAVA_OPTS and GRADLE_OPTS to pass JVM options to this script.
-DEFAULT_JVM_OPTS=""
+DEFAULT_JVM_OPTS='"-Xmx64m" "-Xms64m"'
Review comment:
Those changes are generated automatically by Gradle when doing an upgrade
--
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 #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r696087897
##########
File path: build.gradle
##########
@@ -174,7 +175,7 @@ project(':iceberg-bundled-guava') {
shadowJar {
classifier null
- configurations = [project.configurations.compileOnly]
+ configurations = [project.configurations.compileClasspath]
Review comment:
Previously we wanted to add everything that was only in the compilation phase which in the past could only be defined as "CompileOnly"
Now everything in compilation is "CompileClasspath"
--
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] flyrain commented on a change in pull request #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
flyrain commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r691464151
##########
File path: gradlew.bat
##########
@@ -0,0 +1,89 @@
+@rem
+@rem Copyright 2015 the original author or authors.
+@rem
+@rem Licensed under the Apache License, Version 2.0 (the "License");
+@rem you may not use this file except in compliance with the License.
+@rem You may obtain a copy of the License at
+@rem
+@rem https://www.apache.org/licenses/LICENSE-2.0
+@rem
+@rem Unless required by applicable law or agreed to in writing, software
+@rem distributed under the License is distributed on an "AS IS" BASIS,
+@rem WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+@rem See the License for the specific language governing permissions and
+@rem limitations under the License.
+@rem
+
+@if "%DEBUG%" == "" @echo off
+@rem ##########################################################################
+@rem
+@rem Gradle startup script for Windows
+@rem
+@rem ##########################################################################
+
+@rem Set local scope for the variables with windows NT shell
+if "%OS%"=="Windows_NT" setlocal
+
+set DIRNAME=%~dp0
+if "%DIRNAME%" == "" set DIRNAME=.
+set APP_BASE_NAME=%~n0
+set APP_HOME=%DIRNAME%
+
+@rem Resolve any "." and ".." in APP_HOME to make it shorter.
+for %%i in ("%APP_HOME%") do set APP_HOME=%%~fi
+
+@rem Add default JVM options here. You can also use JAVA_OPTS and GRADLE_OPTS to pass JVM options to this script.
+set DEFAULT_JVM_OPTS="-Xmx64m" "-Xms64m"
Review comment:
Same here for `xmx`
--
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 #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r681839251
##########
File path: core/src/main/java/org/apache/iceberg/FindFiles.java
##########
@@ -103,7 +103,7 @@ public Builder asOfTime(long timestampMillis) {
// case, there is no valid snapshot to read.
Preconditions.checkArgument(lastSnapshotId != null,
"Cannot find a snapshot older than %s",
- DATE_FORMAT.format(LocalDateTime.ofInstant(Instant.ofEpochMilli(timestampMillis), ZoneId.systemDefault())));
Review comment:
Another slight change in behavior here ... I would probably save these for another PR unless we really need it here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: 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 #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#issuecomment-905866352
This generally looks good to me. We are changing a lot in terms of dependencies so it is hard to spot everything. I left a few questions but other than that, I'd support getting this in.
This was change clearly required a substantial amount of work. Thanks, @nastra!
--
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] nastra commented on a change in pull request #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
nastra commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r678046138
##########
File path: build.gradle
##########
@@ -247,13 +250,30 @@ project(':iceberg-data') {
exclude group: 'org.slf4j', module: 'slf4j-log4j12'
}
- testCompile("org.apache.hadoop:hadoop-client") {
+ implementation("org.apache.orc:orc-core::nohive") {
Review comment:
> Can you be more specific about what you're saying with implementation vs compile? When are transitive dependencies included and when are they not?
You could technically just replace the old `compile` configuration with the new `api` configuration, but according to [Gradle docs](https://docs.gradle.org/current/userguide/java_library_plugin.html#java_library_plugin) it is better to use `implementation`, since compilation will be faster and other dependencies don't **leak** into your classpath during compilation. Below is the section from the Gradle docs that I'm referring to:
> Dependencies appearing in the api configurations will be transitively exposed to consumers of the library, and as such will appear on the compile classpath of consumers. Dependencies found in the implementation configuration will, on the other hand, not be exposed to consumers, and therefore not leak into the consumers' compile classpath. This comes with several benefits:
>* dependencies do not leak into the compile classpath of consumers anymore, so you will never accidentally depend on a transitive dependency
>* faster compilation thanks to reduced classpath size
>* less recompilations when implementation dependencies change: consumers would not need to be recompiled
>* cleaner publishing: when used in conjunction with the new maven-publish plugin, Java libraries produce POM files that distinguish exactly between what is required to compile against the library and what is required to use the library at runtime (in other words, don’t mix what is needed to compile the library itself and what is needed to compile against the library).
--
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] nastra commented on a change in pull request #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
nastra commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r678045366
##########
File path: build.gradle
##########
@@ -247,13 +250,30 @@ project(':iceberg-data') {
exclude group: 'org.slf4j', module: 'slf4j-log4j12'
}
- testCompile("org.apache.hadoop:hadoop-client") {
+ implementation("org.apache.orc:orc-core::nohive") {
Review comment:
> This adds implementation dependencies for org.apache.avro:avro and org.apache.orc:orc-core. Why isn't there one for org.apache.parquet?
Judging by the 3 parquet imports that are being used in `:iceberg-data`, we have:
```
import org.apache.parquet.hadoop.ParquetFileReader;
import org.apache.parquet.hadoop.metadata.ParquetMetadata;
```
These 2 are satisfied by `org.apache.parquet:parquet-hadoop`. and then `import org.apache.parquet.Preconditions` (which should probably be replaced with `org.apache.iceberg.relocated.com.google.common.base.Preconditions`), which is being satisfied by `org.apache.parquet:parquet-common`. Both of these libs are part of `org.apache.parquet:parquet-avro` as can be seen below:
```
\--- org.apache.parquet:parquet-avro -> 1.12.0
+--- org.apache.parquet:parquet-column:1.12.0
| +--- org.apache.parquet:parquet-common:1.12.0
| | +--- org.apache.parquet:parquet-format-structures:1.12.0
| | | +--- org.slf4j:slf4j-api:1.7.22 -> 1.7.25
| | | \--- javax.annotation:javax.annotation-api:1.3.2
| | +--- org.slf4j:slf4j-api:1.7.22 -> 1.7.25
| | \--- org.apache.yetus:audience-annotations:0.12.0
| \--- org.apache.parquet:parquet-encoding:1.12.0
| \--- org.apache.parquet:parquet-common:1.12.0 (*)
+--- org.apache.parquet:parquet-hadoop:1.12.0
| +--- org.apache.parquet:parquet-column:1.12.0 (*)
| +--- org.apache.parquet:parquet-format-structures:1.12.0 (*)
| +--- org.apache.parquet:parquet-jackson:1.12.0
| +--- org.xerial.snappy:snappy-java:1.1.8
| +--- commons-pool:commons-pool:1.6
| \--- com.github.luben:zstd-jni:1.4.9-1
\--- org.apache.parquet:parquet-format-structures:1.12.0 (*)
```
Thus `org.apache.parquet:parquet-avro` carries everything we need for `:iceberg-data` in this case. Does that answer your question?
--
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] nastra commented on a change in pull request #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
nastra commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r696339319
##########
File path: baseline.gradle
##########
@@ -36,7 +36,7 @@ subprojects {
apply plugin: 'com.palantir.baseline-checkstyle'
apply plugin: 'com.palantir.baseline-error-prone'
}
- apply plugin: 'com.palantir.baseline-scalastyle'
+ apply plugin: 'com.github.alisiikh.scalastyle'
Review comment:
+1 on using Spotless (I was actually planning on working on this in the next month or so)
--
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] nastra commented on a change in pull request #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
nastra commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r677161667
##########
File path: build.gradle
##########
@@ -247,13 +250,30 @@ project(':iceberg-data') {
exclude group: 'org.slf4j', module: 'slf4j-log4j12'
}
- testCompile("org.apache.hadoop:hadoop-client") {
+ implementation("org.apache.orc:orc-core::nohive") {
Review comment:
> These are no longer pulled in via iceberg-orc and iceberg-core?
correct, because `implementation` allows now more fine-grained control over dependencies being pulled in, which wasn't the case previously with `compile` (you got more dependencies than you were asking for). Now one could probably achieve the same thing by pulling in certain dependencies via `api`, but that means you're making those dependencies explicitly part of your API. The Gradle docs also state that one should generally prefer `implementation` over `api`, meaning it is better to explicitly state all the dependencies that are really required, so that you don't get surprises with non-expected dependencies and such.
> Do we need an implementation dependency on Parquet as well?
already added it in https://github.com/nastra/iceberg/blob/6c308fd66a9f33ce31b5abf8de61dd8269fda4b5/build.gradle#L261
--
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] nastra commented on a change in pull request #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
nastra commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r677158451
##########
File path: build.gradle
##########
@@ -174,7 +175,7 @@ project(':iceberg-bundled-guava') {
shadowJar {
classifier null
- configurations = [project.configurations.compileOnly]
+ configurations = [project.configurations.compileClasspath]
Review comment:
yes that's correct. I made sure that the generated Jars are identical
--
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] nastra commented on a change in pull request #2826: Upgrade to Gradle 7
Posted by GitBox <gi...@apache.org>.
nastra commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r673867234
##########
File path: core/src/main/java/org/apache/iceberg/BaseTransaction.java
##########
@@ -454,6 +454,7 @@ public TableMetadata refresh() {
}
@Override
+ @SuppressWarnings("ConsistentOverrides")
public void commit(TableMetadata underlyingBase, TableMetadata metadata) {
Review comment:
renaming `underlyingBase` to `base` here does conflict and shadows the `base` variable that's defined in the class itself
--
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 #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r675667832
##########
File path: api/src/main/java/org/apache/iceberg/expressions/StrictMetricsEvaluator.java
##########
@@ -429,7 +429,7 @@ public Boolean or(Boolean leftResult, Boolean rightResult) {
}
private boolean canContainNulls(Integer id) {
- return nullCounts == null || (nullCounts.containsKey(id) && nullCounts.get(id) > 0);
+ return nullCounts == null || nullCounts.containsKey(id) && nullCounts.get(id) > 0;
Review comment:
I think it is much better to have the parentheses so that the logic is clear to a reader, even if it is identical to Java's precedence. Can you update the style config and revert these changes?
--
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 #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r696070195
##########
File path: baseline.gradle
##########
@@ -36,7 +36,7 @@ subprojects {
apply plugin: 'com.palantir.baseline-checkstyle'
apply plugin: 'com.palantir.baseline-error-prone'
}
- apply plugin: 'com.palantir.baseline-scalastyle'
+ apply plugin: 'com.github.alisiikh.scalastyle'
Review comment:
We can use third party plugins like this. While I don't see a license for it, there is no reason to think that using it is prohibited because it is published through the [Gradle plugin repository](https://plugins.gradle.org/plugin/com.github.alisiikh.scalastyle).
The main concern we have with licensing is redistributing code. That is, when you download our release tarball you also get a copy of some software, or when you download a Jar from maven central you get a compiled copy. That redistribution makes the artifact that is downloaded a derivative work.
This is different because we simply reference something in config and whoever is using that config downloads and uses it. We are basically telling a user how to build but not distributing the plugin, and the user downloads the plugin when building this project. In that case, the license is between the user downloading and running the plugin and the plugin author. That's why we have a `gradlew` script that downloads gradle when building and we don't check the gradle wrapper Jar into our source repo.
--
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] nastra commented on a change in pull request #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
nastra commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r690999266
##########
File path: core/src/test/java/org/apache/iceberg/TestTableMetadata.java
##########
@@ -90,11 +90,11 @@ public void testJsonConversion() throws Exception {
long previousSnapshotId = System.currentTimeMillis() - new Random(1234).nextInt(3600);
Snapshot previousSnapshot = new BaseSnapshot(
ops.io(), previousSnapshotId, null, previousSnapshotId, null, null, null, ImmutableList.of(
- new GenericManifestFile(localInput("file:/tmp/manfiest.1.avro"), SPEC_5.specId())));
+ new GenericManifestFile(localInput("file:/tmp/manfiest.1.avro"), SPEC_5.specId())));
Review comment:
yes these are required
##########
File path: hive3/src/main/java/org/apache/iceberg/mr/hive/vector/CompatibilityHiveVectorUtils.java
##########
@@ -59,7 +59,7 @@ private CompatibilityHiveVectorUtils() {
* Returns serialized mapwork instance from a job conf - ported from Hive source code LlapHiveUtils#findMapWork
*
* @param job JobConf instance
- * @return
+ * @return A serialized {@link MapWork} based on the given job conf
Review comment:
I searched across the codebase and it seems that it's a bit inconsistent in terms of upper/lowercase. I generally think that something like this would be better if addressed by Checkstyle (but not sure if checkstyle supports such a check). Otherwise stuff like this will essentially pop up in lots of PR reviews
##########
File path: mr/src/test/java/org/apache/iceberg/mr/hive/TestTables.java
##########
@@ -115,7 +115,7 @@ public String catalogName() {
/**
* The table properties string needed for the CREATE TABLE ... commands,
* like "TBLPROPERTIES('iceberg.catalog'='mycatalog')
- * @return
+ * @return The tables properties string, such as "TBLPROPERTIES('iceberg.catalog'='mycatalog')
Review comment:
ah this is just a blind copy/paste from the javadoc description a few lines above without actually me checking whether it had missing pieces..will 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] RussellSpitzer commented on a change in pull request #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r696085579
##########
File path: build.gradle
##########
@@ -174,7 +175,7 @@ project(':iceberg-bundled-guava') {
shadowJar {
classifier null
- configurations = [project.configurations.compileOnly]
+ configurations = [project.configurations.compileClasspath]
Review comment:
The old layout
--
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 #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r696063329
##########
File path: arrow/src/main/java/org/apache/iceberg/arrow/vectorized/parquet/VectorizedParquetDefinitionLevelReader.java
##########
@@ -305,7 +305,7 @@ public void nextDictEncodedBatch(
protected abstract void nextVal(
FieldVector vector, int idx, ValuesAsBytesReader valuesReader, int typeWidth, byte[] byteArray);
protected abstract void nextDictEncodedVal(
- FieldVector vector, int startOffset, VectorizedDictionaryEncodedParquetValuesReader reader,
+ FieldVector vector, int idx, VectorizedDictionaryEncodedParquetValuesReader reader,
Review comment:
I think we can resolve this one.
--
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] flyrain commented on a change in pull request #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
flyrain commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r691463863
##########
File path: gradlew
##########
@@ -28,7 +44,7 @@ APP_NAME="Gradle"
APP_BASE_NAME=`basename "$0"`
# Add default JVM options here. You can also use JAVA_OPTS and GRADLE_OPTS to pass JVM options to this script.
-DEFAULT_JVM_OPTS=""
+DEFAULT_JVM_OPTS='"-Xmx64m" "-Xms64m"'
Review comment:
Is it necessary to set xmx here since we got this `org.gradle.jvmargs=-Xmx768m` in file gradle.properties? If we are going to use 768m as the xmx, we should just remove the 64m setting to avoid confusion.
--
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 #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r696078201
##########
File path: build.gradle
##########
@@ -174,7 +175,7 @@ project(':iceberg-bundled-guava') {
shadowJar {
classifier null
- configurations = [project.configurations.compileOnly]
+ configurations = [project.configurations.compileClasspath]
Review comment:
Did `compileOnly` change too?
--
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] nastra commented on a change in pull request #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
nastra commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r692683812
##########
File path: baseline.gradle
##########
@@ -47,7 +47,8 @@ subprojects {
// So we apply Spotless manually to get a similar effect to baseline-format, but change the
// import order.
- pluginManager.withPlugin('com.diffplug.gradle.spotless') {
+
Review comment:
fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: 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] nastra commented on a change in pull request #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
nastra commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r678045366
##########
File path: build.gradle
##########
@@ -247,13 +250,30 @@ project(':iceberg-data') {
exclude group: 'org.slf4j', module: 'slf4j-log4j12'
}
- testCompile("org.apache.hadoop:hadoop-client") {
+ implementation("org.apache.orc:orc-core::nohive") {
Review comment:
> This adds implementation dependencies for org.apache.avro:avro and org.apache.orc:orc-core. Why isn't there one for org.apache.parquet?
Judging by the 3 parquet imports that are being used in `:iceberg-data`, we have:
```
import org.apache.parquet.hadoop.ParquetFileReader;
import org.apache.parquet.hadoop.metadata.ParquetMetadata;
```
These 2 are satisfied by `org.apache.parquet:parquet-hadoop`. and then `import org.apache.parquet.Preconditions` (which should probably be replaced with `org.apache.iceberg.relocated.com.google.common.base.Preconditions`), which is being satisfied by `org.apache.parquet:parquet-common`. Both of these libs are part of `org.apache.parquet:parquet-avro` as can be seen below:
```
\--- org.apache.parquet:parquet-avro -> 1.12.0
+--- org.apache.parquet:parquet-column:1.12.0
| +--- org.apache.parquet:parquet-common:1.12.0
| | +--- org.apache.parquet:parquet-format-structures:1.12.0
| | | +--- org.slf4j:slf4j-api:1.7.22 -> 1.7.25
| | | \--- javax.annotation:javax.annotation-api:1.3.2
| | +--- org.slf4j:slf4j-api:1.7.22 -> 1.7.25
| | \--- org.apache.yetus:audience-annotations:0.12.0
| \--- org.apache.parquet:parquet-encoding:1.12.0
| \--- org.apache.parquet:parquet-common:1.12.0 (*)
+--- org.apache.parquet:parquet-hadoop:1.12.0
| +--- org.apache.parquet:parquet-column:1.12.0 (*)
| +--- org.apache.parquet:parquet-format-structures:1.12.0 (*)
| +--- org.apache.parquet:parquet-jackson:1.12.0
| +--- org.xerial.snappy:snappy-java:1.1.8
| +--- commons-pool:commons-pool:1.6
| \--- com.github.luben:zstd-jni:1.4.9-1
\--- org.apache.parquet:parquet-format-structures:1.12.0 (*)
```
Thus `org.apache.parquet:parquet-avro` carries everything we need for `:iceberg-data` in this case. Does that answer your question?
> Can you be more specific about what you're saying with implementation vs compile? When are transitive dependencies included and when are they not?
You could technically just replace the old `compile` configuration with the new `api` configuration, but according to [Gradle docs](https://docs.gradle.org/current/userguide/java_library_plugin.html#java_library_plugin) it is better to use `implementation`, since compilation will be faster and other dependencies don't **leak** into your classpath during compilation. Below is the section from the Gradle docs that I'm referring to:
> Dependencies appearing in the api configurations will be transitively exposed to consumers of the library, and as such will appear on the compile classpath of consumers. Dependencies found in the implementation configuration will, on the other hand, not be exposed to consumers, and therefore not leak into the consumers' compile classpath. This comes with several benefits:
>* dependencies do not leak into the compile classpath of consumers anymore, so you will never accidentally depend on a transitive dependency
>* faster compilation thanks to reduced classpath size
less recompilations when implementation dependencies change: consumers would not need to be recompiled
cleaner publishing: when used in conjunction with the new maven-publish plugin, Java libraries produce POM files that distinguish exactly between what is required to compile against the library and what is required to use the library at runtime (in other words, don’t mix what is needed to compile the library itself and what is needed to compile against the library).
--
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] nastra commented on a change in pull request #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
nastra commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r690999266
##########
File path: core/src/test/java/org/apache/iceberg/TestTableMetadata.java
##########
@@ -90,11 +90,11 @@ public void testJsonConversion() throws Exception {
long previousSnapshotId = System.currentTimeMillis() - new Random(1234).nextInt(3600);
Snapshot previousSnapshot = new BaseSnapshot(
ops.io(), previousSnapshotId, null, previousSnapshotId, null, null, null, ImmutableList.of(
- new GenericManifestFile(localInput("file:/tmp/manfiest.1.avro"), SPEC_5.specId())));
+ new GenericManifestFile(localInput("file:/tmp/manfiest.1.avro"), SPEC_5.specId())));
Review comment:
yes these are required
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: 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 #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#issuecomment-900758469
@kbendick @flyrain @karuppayya, could you help reviewing this one too if you have time?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: 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] nastra commented on a change in pull request #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
nastra commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r707955200
##########
File path: build.gradle
##########
@@ -1166,10 +1281,11 @@ project(':iceberg-spark3-runtime') {
}
dependencies {
- compile project(':iceberg-spark3')
- compile project(':iceberg-spark3-extensions')
- compile project(':iceberg-aws')
- compile(project(':iceberg-nessie')) {
+ implementation project(':iceberg-api')
Review comment:
sgtm
##########
File path: build.gradle
##########
@@ -201,44 +202,46 @@ project(':iceberg-bundled-guava') {
project(':iceberg-api') {
dependencies {
- compile project(path: ':iceberg-bundled-guava', configuration: 'shadow')
- compileOnly "com.google.errorprone:error_prone_annotations:2.3.3"
- testCompile "org.apache.avro:avro"
+ implementation project(path: ':iceberg-bundled-guava', configuration: 'shadow')
+ compileOnly "com.google.errorprone:error_prone_annotations"
+ testImplementation "org.apache.avro:avro"
}
}
project(':iceberg-common') {
dependencies {
- compile project(path: ':iceberg-bundled-guava', configuration: 'shadow')
+ implementation project(path: ':iceberg-bundled-guava', configuration: 'shadow')
}
}
project(':iceberg-core') {
dependencies {
- compile project(':iceberg-api')
- compile project(':iceberg-common')
+ implementation project(':iceberg-api')
+ implementation project(':iceberg-common')
+ implementation project(path: ':iceberg-bundled-guava', configuration: 'shadow')
- compile("org.apache.avro:avro") {
+ implementation("org.apache.avro:avro") {
exclude group: 'org.tukaani' // xz compression is not supported
}
- compile "com.fasterxml.jackson.core:jackson-databind"
- compile "com.fasterxml.jackson.core:jackson-core"
- compile "com.github.ben-manes.caffeine:caffeine"
+ implementation "com.fasterxml.jackson.core:jackson-databind"
+ implementation "com.fasterxml.jackson.core:jackson-core"
+ implementation "com.github.ben-manes.caffeine:caffeine"
compileOnly("org.apache.hadoop:hadoop-client") {
exclude group: 'org.apache.avro', module: 'avro'
exclude group: 'org.slf4j', module: 'slf4j-log4j12'
}
- testCompile "org.xerial:sqlite-jdbc"
- testCompile project(path: ':iceberg-api', configuration: 'testArtifacts')
+ testImplementation "org.xerial:sqlite-jdbc"
+ testImplementation project(path: ':iceberg-api', configuration: 'testArtifacts')
}
}
project(':iceberg-data') {
dependencies {
- compile project(':iceberg-api')
- compile project(':iceberg-core')
+ implementation project(path: ':iceberg-bundled-guava', configuration: 'shadow')
+ implementation project(':iceberg-api')
Review comment:
sgtm
--
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 #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r707424996
##########
File path: build.gradle
##########
@@ -201,44 +202,46 @@ project(':iceberg-bundled-guava') {
project(':iceberg-api') {
dependencies {
- compile project(path: ':iceberg-bundled-guava', configuration: 'shadow')
- compileOnly "com.google.errorprone:error_prone_annotations:2.3.3"
- testCompile "org.apache.avro:avro"
+ implementation project(path: ':iceberg-bundled-guava', configuration: 'shadow')
+ compileOnly "com.google.errorprone:error_prone_annotations"
+ testImplementation "org.apache.avro:avro"
}
}
project(':iceberg-common') {
dependencies {
- compile project(path: ':iceberg-bundled-guava', configuration: 'shadow')
+ implementation project(path: ':iceberg-bundled-guava', configuration: 'shadow')
}
}
project(':iceberg-core') {
dependencies {
- compile project(':iceberg-api')
- compile project(':iceberg-common')
+ implementation project(':iceberg-api')
+ implementation project(':iceberg-common')
+ implementation project(path: ':iceberg-bundled-guava', configuration: 'shadow')
- compile("org.apache.avro:avro") {
+ implementation("org.apache.avro:avro") {
exclude group: 'org.tukaani' // xz compression is not supported
}
- compile "com.fasterxml.jackson.core:jackson-databind"
- compile "com.fasterxml.jackson.core:jackson-core"
- compile "com.github.ben-manes.caffeine:caffeine"
+ implementation "com.fasterxml.jackson.core:jackson-databind"
+ implementation "com.fasterxml.jackson.core:jackson-core"
+ implementation "com.github.ben-manes.caffeine:caffeine"
compileOnly("org.apache.hadoop:hadoop-client") {
exclude group: 'org.apache.avro', module: 'avro'
exclude group: 'org.slf4j', module: 'slf4j-log4j12'
}
- testCompile "org.xerial:sqlite-jdbc"
- testCompile project(path: ':iceberg-api', configuration: 'testArtifacts')
+ testImplementation "org.xerial:sqlite-jdbc"
+ testImplementation project(path: ':iceberg-api', configuration: 'testArtifacts')
}
}
project(':iceberg-data') {
dependencies {
- compile project(':iceberg-api')
- compile project(':iceberg-core')
+ implementation project(path: ':iceberg-bundled-guava', configuration: 'shadow')
+ implementation project(':iceberg-api')
Review comment:
I think this should be an `api` dependency because these classes are used in the API of `iceberg-data`. For example, `iceberg-api` includes the expressions, which are passed into `IcebergGenerics`.
--
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 #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r707429471
##########
File path: build.gradle
##########
@@ -1166,10 +1281,11 @@ project(':iceberg-spark3-runtime') {
}
dependencies {
- compile project(':iceberg-spark3')
- compile project(':iceberg-spark3-extensions')
- compile project(':iceberg-aws')
- compile(project(':iceberg-nessie')) {
+ implementation project(':iceberg-api')
Review comment:
I think this should be an `api` dependency here and most other places.
--
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] nastra commented on a change in pull request #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
nastra commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r707979771
##########
File path: build.gradle
##########
@@ -1219,6 +1336,15 @@ project(':iceberg-spark3-runtime') {
// relocate Antlr runtime and related deps to shade Iceberg specific version
relocate 'org.antlr.v4.runtime', 'org.apache.iceberg.shaded.org.antlr.v4.runtime'
+ dependencies {
+ exclude 'org/antlr/runtime/**/*'
+ exclude 'org/abego/treelayout/**/*'
+ exclude 'com/ibm/icu/**/*'
+ exclude 'javax/json/**/*'
+ exclude 'org/glassfish/**/*'
+ exclude 'org/stringtemplate/**/*'
+ }
Review comment:
oh you're totally right. I moved stuff to the exclude list of the `implementation` configuration and we're getting the same results
--
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 #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#issuecomment-922011071
Merged. Thanks for working through all of the issues to get this done, @nastra!
--
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 #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r708746749
##########
File path: build.gradle
##########
@@ -498,24 +543,26 @@ project(':iceberg-hive-metastore') {
exclude group: 'org.slf4j', module: 'slf4j-log4j12'
}
- testCompile project(path: ':iceberg-api', configuration: 'testArtifacts')
+ testImplementation project(path: ':iceberg-api', configuration: 'testArtifacts')
}
}
project(':iceberg-mr') {
configurations {
- testCompile {
+ testImplementation {
exclude group: 'org.apache.parquet', module: 'parquet-hadoop-bundle'
}
}
dependencies {
- compile project(':iceberg-api')
- compile project(':iceberg-core')
- compile project(':iceberg-data')
- compile project(':iceberg-hive-metastore')
- compile project(':iceberg-orc')
- compile project(':iceberg-parquet')
+ implementation project(path: ':iceberg-bundled-guava', configuration: 'shadow')
+ api project(':iceberg-api')
+ implementation project(':iceberg-common')
+ implementation project(':iceberg-core')
+ implementation project(':iceberg-data')
Review comment:
This should also be an API dependency because the `InputFormat` produces Iceberg classes in iceberg-data.
--
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 #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r696096936
##########
File path: build.gradle
##########
@@ -401,15 +441,15 @@ project(':iceberg-flink-runtime') {
}
dependencies {
- compile project(':iceberg-flink')
- compile project(':iceberg-aws')
- compile(project(':iceberg-nessie')) {
+ implementation project(':iceberg-flink')
+ implementation project(':iceberg-aws')
+ implementation(project(':iceberg-nessie')) {
exclude group: 'com.google.code.findbugs', module: 'jsr305'
}
}
shadowJar {
- configurations = [project.configurations.compile]
+ configurations = [project.configurations.runtimeClasspath]
Review comment:
Does this now include `runtimeOnly` 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] aokolnychyi commented on pull request #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#issuecomment-905863665
Do we need to check in `gradle-wrapper.jar`? Wasn't it downloaded before?
--
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] cwsteinbach commented on a change in pull request #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
cwsteinbach commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r696074362
##########
File path: baseline.gradle
##########
@@ -36,7 +36,7 @@ subprojects {
apply plugin: 'com.palantir.baseline-checkstyle'
apply plugin: 'com.palantir.baseline-error-prone'
}
- apply plugin: 'com.palantir.baseline-scalastyle'
+ apply plugin: 'com.github.alisiikh.scalastyle'
Review comment:
My advice is to switch to Spotless for formatting Scala. We should use it for formatting Python and Java too, but that can wait until later.
--
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] nastra commented on a change in pull request #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
nastra commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r696338108
##########
File path: build.gradle
##########
@@ -174,7 +175,7 @@ project(':iceberg-bundled-guava') {
shadowJar {
classifier null
- configurations = [project.configurations.compileOnly]
+ configurations = [project.configurations.compileClasspath]
Review comment:
@RussellSpitzer I think you explained it quite 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] nastra commented on a change in pull request #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
nastra commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r691002090
##########
File path: mr/src/test/java/org/apache/iceberg/mr/hive/TestTables.java
##########
@@ -115,7 +115,7 @@ public String catalogName() {
/**
* The table properties string needed for the CREATE TABLE ... commands,
* like "TBLPROPERTIES('iceberg.catalog'='mycatalog')
- * @return
+ * @return The tables properties string, such as "TBLPROPERTIES('iceberg.catalog'='mycatalog')
Review comment:
ah this is just a blind copy/paste from the javadoc description a few lines above without actually me checking whether it had missing pieces..will 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] nastra commented on a change in pull request #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
nastra commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r696327470
##########
File path: gradle/wrapper/gradle-wrapper.properties
##########
@@ -1,24 +1,5 @@
-#
Review comment:
this was automatically deleted by the gradle upgrade functionality. I looked at some other projects and they also didn't have a license header in this file. When one does an upgrade via [./gradlew wrapper --gradle-version 7.2](https://docs.gradle.org/current/userguide/gradle_wrapper.html#sec:upgrading_wrapper) then Gradle usually also upgrades shell scripts and other stuff
--
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 #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r696097430
##########
File path: build.gradle
##########
@@ -312,13 +334,14 @@ project(':iceberg-aws') {
project(':iceberg-flink') {
Review comment:
@openinx @stevenzwu @JingsongLi, could you check the Flink part?
--
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] nastra commented on a change in pull request #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
nastra commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r697209647
##########
File path: gradle/wrapper/gradle-wrapper.properties
##########
@@ -1,24 +1,5 @@
-#
Review comment:
I thought it's only the checkstyle rule that makes sure there's a license header on **java/ts** files (https://github.com/apache/iceberg/blob/970e8aac60081c7d286ab0f7e12e428b3309dd34/.baseline/checkstyle/checkstyle.xml#L18-L21).
Based on [this comment](https://github.com/gradle/gradle/issues/2852#issuecomment-347204733) I would probably leave out the license header from the properties file, since it's _just_ a configuration file.
--
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 #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#issuecomment-918276763
> Do we need to check in gradle-wrapper.jar? Wasn't it downloaded before?
We do not want to check in the gradle wrapper Jar because it conflicts with Apache release policy. We can't release a source tarball with the Jar in it and have to update the wrapper to download the Jar the first time it runs.
--
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] nastra commented on a change in pull request #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
nastra commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r707953067
##########
File path: build.gradle
##########
@@ -247,13 +250,30 @@ project(':iceberg-data') {
exclude group: 'org.slf4j', module: 'slf4j-log4j12'
}
- testCompile("org.apache.hadoop:hadoop-client") {
+ 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.parquet:parquet-avro") {
+ exclude group: 'org.apache.avro', module: 'avro'
+ // already shaded by Parquet
+ exclude group: 'it.unimi.dsi'
+ exclude group: 'org.codehaus.jackson'
+ }
+
+ compileOnly "org.apache.avro:avro"
Review comment:
it seems that `org.apache.avro:avro` was mostly used as a `compileOnly` dependency in other places in this file, therefore I thought having it as `compileOnly` here would be good here 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] nastra commented on a change in pull request #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
nastra commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r691492268
##########
File path: gradlew.bat
##########
@@ -0,0 +1,89 @@
+@rem
+@rem Copyright 2015 the original author or authors.
+@rem
+@rem Licensed under the Apache License, Version 2.0 (the "License");
+@rem you may not use this file except in compliance with the License.
+@rem You may obtain a copy of the License at
+@rem
+@rem https://www.apache.org/licenses/LICENSE-2.0
+@rem
+@rem Unless required by applicable law or agreed to in writing, software
+@rem distributed under the License is distributed on an "AS IS" BASIS,
+@rem WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+@rem See the License for the specific language governing permissions and
+@rem limitations under the License.
+@rem
+
+@if "%DEBUG%" == "" @echo off
+@rem ##########################################################################
+@rem
+@rem Gradle startup script for Windows
+@rem
+@rem ##########################################################################
+
+@rem Set local scope for the variables with windows NT shell
+if "%OS%"=="Windows_NT" setlocal
+
+set DIRNAME=%~dp0
+if "%DIRNAME%" == "" set DIRNAME=.
+set APP_BASE_NAME=%~n0
+set APP_HOME=%DIRNAME%
+
+@rem Resolve any "." and ".." in APP_HOME to make it shorter.
+for %%i in ("%APP_HOME%") do set APP_HOME=%%~fi
+
+@rem Add default JVM options here. You can also use JAVA_OPTS and GRADLE_OPTS to pass JVM options to this script.
+set DEFAULT_JVM_OPTS="-Xmx64m" "-Xms64m"
Review comment:
This is also generated by Gradle itself when doing the upgrade
--
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] nastra commented on a change in pull request #2826: Upgrade to Gradle 7
Posted by GitBox <gi...@apache.org>.
nastra commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r673867234
##########
File path: core/src/main/java/org/apache/iceberg/BaseTransaction.java
##########
@@ -454,6 +454,7 @@ public TableMetadata refresh() {
}
@Override
+ @SuppressWarnings("ConsistentOverrides")
public void commit(TableMetadata underlyingBase, TableMetadata metadata) {
Review comment:
renaming `underlyingBase` to `base` here does conflict and shadows the `base` variable that's defined in the class itself. I think it makes more sense to suppress the warning here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: 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 #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
aokolnychyi edited a comment on pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#issuecomment-900758469
@raptond @kbendick @flyrain @karuppayya, could you help reviewing this one too if you have time?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: 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] snazy commented on a change in pull request #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
snazy commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r674245564
##########
File path: build.gradle
##########
@@ -656,27 +707,37 @@ if (jdkVersion == '8') {
exclude group: 'org.pentaho' // missing dependency
exclude group: 'org.slf4j', module: 'slf4j-log4j12'
}
+
+ compileOnly("org.apache.orc:orc-core::nohive") {
Review comment:
This kind of dependency w/ excludes is repeated. I think, it would be cleaner to add a new configuration named something like `orc-core-nohive` so so and add that configuration as a dependency instead of the repeated lines. WDYT?
--
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 #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r681794627
##########
File path: build.gradle
##########
@@ -174,7 +175,7 @@ project(':iceberg-bundled-guava') {
shadowJar {
classifier null
- configurations = [project.configurations.compileOnly]
+ configurations = [project.configurations.compileClasspath]
Review comment:
this is because CompileClasspath is now basically compileOnly
--
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] nastra commented on a change in pull request #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
nastra commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r692810673
##########
File path: gradle/wrapper/gradle-wrapper.properties
##########
@@ -19,6 +19,6 @@
distributionBase=GRADLE_USER_HOME
distributionPath=wrapper/dists
+distributionUrl=https\://services.gradle.org/distributions/gradle-7.1.1-bin.zip
Review comment:
good point, yes that should be easy to upgrade now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: 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 #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r696967226
##########
File path: gradle/wrapper/gradle-wrapper.properties
##########
@@ -1,24 +1,5 @@
-#
Review comment:
Should we add it back? I thought we had a license checker (rat) that will fail if the license is missing.
If this won't cause that to fail, then I don't have a strong opinion here (as we don't really need a license on our cradle-wrappers file in my opinion, though I'm not an expert on licensing issues).
--
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] nastra commented on a change in pull request #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
nastra commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r708866670
##########
File path: build.gradle
##########
@@ -498,24 +543,26 @@ project(':iceberg-hive-metastore') {
exclude group: 'org.slf4j', module: 'slf4j-log4j12'
}
- testCompile project(path: ':iceberg-api', configuration: 'testArtifacts')
+ testImplementation project(path: ':iceberg-api', configuration: 'testArtifacts')
}
}
project(':iceberg-mr') {
configurations {
- testCompile {
+ testImplementation {
exclude group: 'org.apache.parquet', module: 'parquet-hadoop-bundle'
}
}
dependencies {
- compile project(':iceberg-api')
- compile project(':iceberg-core')
- compile project(':iceberg-data')
- compile project(':iceberg-hive-metastore')
- compile project(':iceberg-orc')
- compile project(':iceberg-parquet')
+ implementation project(path: ':iceberg-bundled-guava', configuration: 'shadow')
+ api project(':iceberg-api')
+ implementation project(':iceberg-common')
+ implementation project(':iceberg-core')
+ implementation project(':iceberg-data')
Review comment:
good point, updated this and `iceberg-spark`/`iceberg-flink` to an `api` dependency, since those were also using iceberg-data class in their API
--
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] nastra commented on a change in pull request #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
nastra commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r677589501
##########
File path: build.gradle
##########
@@ -247,13 +250,30 @@ project(':iceberg-data') {
exclude group: 'org.slf4j', module: 'slf4j-log4j12'
}
- testCompile("org.apache.hadoop:hadoop-client") {
+ implementation("org.apache.orc:orc-core::nohive") {
Review comment:
or did you mean the one defined in https://github.com/nastra/iceberg/blob/6c308fd66a9f33ce31b5abf8de61dd8269fda4b5/build.gradle#L245?
--
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 #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r682853260
##########
File path: core/src/main/java/org/apache/iceberg/FindFiles.java
##########
@@ -103,7 +103,7 @@ public Builder asOfTime(long timestampMillis) {
// case, there is no valid snapshot to read.
Preconditions.checkArgument(lastSnapshotId != null,
"Cannot find a snapshot older than %s",
- DATE_FORMAT.format(LocalDateTime.ofInstant(Instant.ofEpochMilli(timestampMillis), ZoneId.systemDefault())));
Review comment:
could we just skip that rule? We do for some other error prone rules. I'm fine with maybe making this change later just seems unrelated
--
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] nastra commented on a change in pull request #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
nastra commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r692810274
##########
File path: mr/src/test/java/org/apache/iceberg/mr/hive/TestTables.java
##########
@@ -114,8 +114,8 @@ public String catalogName() {
/**
* The table properties string needed for the CREATE TABLE ... commands,
- * like "TBLPROPERTIES('iceberg.catalog'='mycatalog')
- * @return
+ * like TBLPROPERTIES('iceberg.catalog'='mycatalog')
+ * @return the tables properties string, such as TBLPROPERTIES('iceberg.catalog'='mycatalog')
Review comment:
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: 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 #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r682853260
##########
File path: core/src/main/java/org/apache/iceberg/FindFiles.java
##########
@@ -103,7 +103,7 @@ public Builder asOfTime(long timestampMillis) {
// case, there is no valid snapshot to read.
Preconditions.checkArgument(lastSnapshotId != null,
"Cannot find a snapshot older than %s",
- DATE_FORMAT.format(LocalDateTime.ofInstant(Instant.ofEpochMilli(timestampMillis), ZoneId.systemDefault())));
Review comment:
could we just skip that rule? We do for some other error prone rules. I'm fine with maybe making this change later just seems unrelated
--
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] nastra commented on a change in pull request #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
nastra commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r692683835
##########
File path: baseline.gradle
##########
@@ -82,4 +83,15 @@ subprojects {
)
}
}
-}
+
+ pluginManager.withPlugin('com.github.alisiikh.scalastyle') {
+ scalastyle {
+ config = file("${rootDir}/project/scalastyle_config.xml")
+ inputEncoding = 'UTF-8'
+ outputEncoding = 'UTF-8'
+ failOnWarning = false
+ verbose = false
+ quiet = false
+ }
+ }
+}
Review comment:
fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: 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] nastra commented on a change in pull request #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
nastra commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r677157207
##########
File path: arrow/src/main/java/org/apache/iceberg/arrow/vectorized/parquet/VectorizedParquetDefinitionLevelReader.java
##########
@@ -305,7 +305,7 @@ public void nextDictEncodedBatch(
protected abstract void nextVal(
FieldVector vector, int idx, ValuesAsBytesReader valuesReader, int typeWidth, byte[] byteArray);
protected abstract void nextDictEncodedVal(
- FieldVector vector, int startOffset, VectorizedDictionaryEncodedParquetValuesReader reader,
+ FieldVector vector, int idx, VectorizedDictionaryEncodedParquetValuesReader reader,
Review comment:
this is due to `ConsistentOverrides` from the additional ErrorProne checks in https://github.com/palantir/gradle-baseline#baseline-error-prone-checks
--
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] nastra commented on a change in pull request #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
nastra commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r692689770
##########
File path: mr/src/test/java/org/apache/iceberg/mr/hive/TestTables.java
##########
@@ -115,7 +115,7 @@ public String catalogName() {
/**
* The table properties string needed for the CREATE TABLE ... commands,
* like "TBLPROPERTIES('iceberg.catalog'='mycatalog')
- * @return
+ * @return The tables properties string, such as "TBLPROPERTIES('iceberg.catalog'='mycatalog')
Review comment:
fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: 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] snazy commented on a change in pull request #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
snazy commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r692793731
##########
File path: gradle/wrapper/gradle-wrapper.properties
##########
@@ -19,6 +19,6 @@
distributionBase=GRADLE_USER_HOME
distributionPath=wrapper/dists
+distributionUrl=https\://services.gradle.org/distributions/gradle-7.1.1-bin.zip
Review comment:
7.2 has been released. Is it easy to use that version instead of 7.1.1?
##########
File path: mr/src/test/java/org/apache/iceberg/mr/hive/TestTables.java
##########
@@ -114,8 +114,8 @@ public String catalogName() {
/**
* The table properties string needed for the CREATE TABLE ... commands,
- * like "TBLPROPERTIES('iceberg.catalog'='mycatalog')
- * @return
+ * like TBLPROPERTIES('iceberg.catalog'='mycatalog')
+ * @return the tables properties string, such as TBLPROPERTIES('iceberg.catalog'='mycatalog')
Review comment:
nit: add `{@code ` around that TBLPROPERTIES
--
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 #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#issuecomment-905864464
@rdblue, could you check if your comments were resolved?
--
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] nastra commented on a change in pull request #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
nastra commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r692685500
##########
File path: baseline.gradle
##########
@@ -36,7 +36,7 @@ subprojects {
apply plugin: 'com.palantir.baseline-checkstyle'
apply plugin: 'com.palantir.baseline-error-prone'
}
- apply plugin: 'com.palantir.baseline-scalastyle'
+ apply plugin: 'com.github.alisiikh.scalastyle'
Review comment:
I haven't found any license either, but `com.palantir.baseline-scalastyle` is essentially using `org.github.ngbinh.scalastyle` (see https://github.com/palantir/gradle-baseline/blob/develop/gradle-baseline-java/build.gradle#L15) and `com.github.alisiikh.scalastyle` is a fork of it (I guess because `org.github.ngbinh.scalastyle` was updated in 2018 the last time)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: 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 #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r681823671
##########
File path: build.gradle
##########
@@ -570,15 +619,15 @@ if (jdkVersion == '8') {
tasks.jar.dependsOn tasks.shadowJar
dependencies {
- compile project(':iceberg-data')
- compile project(':iceberg-orc')
+ implementation project(':iceberg-data')
+ implementation project(':iceberg-orc')
testCompileOnly project(path: ':iceberg-data', configuration: 'testArtifacts')
testCompileOnly project(path: ':iceberg-orc', configuration: 'testArtifacts')
}
shadowJar {
- configurations = [project.configurations.compile, project.configurations.compileOnly, project.configurations.testCompileOnly]
Review comment:
Why did this have a dependency on test compile deps ... that doesn't sound right.
--
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] nastra commented on a change in pull request #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
nastra commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r692689898
##########
File path: hive3/src/main/java/org/apache/iceberg/mr/hive/vector/CompatibilityHiveVectorUtils.java
##########
@@ -59,7 +59,7 @@ private CompatibilityHiveVectorUtils() {
* Returns serialized mapwork instance from a job conf - ported from Hive source code LlapHiveUtils#findMapWork
*
* @param job JobConf instance
- * @return
+ * @return A serialized {@link MapWork} based on the given job conf
Review comment:
fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: 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 #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r707426115
##########
File path: build.gradle
##########
@@ -247,13 +250,30 @@ project(':iceberg-data') {
exclude group: 'org.slf4j', module: 'slf4j-log4j12'
}
- testCompile("org.apache.hadoop:hadoop-client") {
+ 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.parquet:parquet-avro") {
+ exclude group: 'org.apache.avro', module: 'avro'
+ // already shaded by Parquet
+ exclude group: 'it.unimi.dsi'
+ exclude group: 'org.codehaus.jackson'
+ }
+
+ compileOnly "org.apache.avro:avro"
Review comment:
Why is Avro `compileOnly` when both `parquet-avro` and `orc-core` are `implementation`?
--
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 #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r707444680
##########
File path: build.gradle
##########
@@ -1219,6 +1336,15 @@ project(':iceberg-spark3-runtime') {
// relocate Antlr runtime and related deps to shade Iceberg specific version
relocate 'org.antlr.v4.runtime', 'org.apache.iceberg.shaded.org.antlr.v4.runtime'
+ dependencies {
+ exclude 'org/antlr/runtime/**/*'
+ exclude 'org/abego/treelayout/**/*'
+ exclude 'com/ibm/icu/**/*'
+ exclude 'javax/json/**/*'
+ exclude 'org/glassfish/**/*'
+ exclude 'org/stringtemplate/**/*'
+ }
Review comment:
It is suspicious to me that this is now necessary. I think it is because we are shading `runtimeClasspath` now rather than `compile`. I think that change is mostly correct because `implementation` dependencies are no longer transitive unless you look at the runtime classpath. But some extra dependencies must be leaking in from the other changes.
I think the right way to exclude these is to add them to the `implementation` excludes above, not to exclude specific packages.
--
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 #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
rdblue merged pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826
--
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 #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r696102631
##########
File path: gradle/wrapper/gradle-wrapper.properties
##########
@@ -1,24 +1,5 @@
-#
Review comment:
Do we delete the license on purpose?
--
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] nastra commented on a change in pull request #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
nastra commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r681831553
##########
File path: build.gradle
##########
@@ -570,15 +619,15 @@ if (jdkVersion == '8') {
tasks.jar.dependsOn tasks.shadowJar
dependencies {
- compile project(':iceberg-data')
- compile project(':iceberg-orc')
+ implementation project(':iceberg-data')
+ implementation project(':iceberg-orc')
testCompileOnly project(path: ':iceberg-data', configuration: 'testArtifacts')
testCompileOnly project(path: ':iceberg-orc', configuration: 'testArtifacts')
}
shadowJar {
- configurations = [project.configurations.compile, project.configurations.compileOnly, project.configurations.testCompileOnly]
Review comment:
don't know why it had this dependency but I agree that it wasn't correct
--
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] nastra commented on a change in pull request #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
nastra commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r696327470
##########
File path: gradle/wrapper/gradle-wrapper.properties
##########
@@ -1,24 +1,5 @@
-#
Review comment:
this was automatically deleted by the gradle upgrade functionality. I looked at some other projects and they also didn't have a license header in this file
--
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 #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r676752903
##########
File path: build.gradle
##########
@@ -174,7 +175,7 @@ project(':iceberg-bundled-guava') {
shadowJar {
classifier null
- configurations = [project.configurations.compileOnly]
+ configurations = [project.configurations.compileClasspath]
Review comment:
Is this correct? We include `compileOnly` to avoid including `compile` or `implementation` dependencies that are expected in the runtime classpath.
--
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] nastra commented on a change in pull request #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
nastra commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r676327356
##########
File path: api/src/main/java/org/apache/iceberg/expressions/StrictMetricsEvaluator.java
##########
@@ -429,7 +429,7 @@ public Boolean or(Boolean leftResult, Boolean rightResult) {
}
private boolean canContainNulls(Integer id) {
- return nullCounts == null || (nullCounts.containsKey(id) && nullCounts.get(id) > 0);
+ return nullCounts == null || nullCounts.containsKey(id) && nullCounts.get(id) > 0;
Review comment:
@rdblue pushed a new version where the `UnnecessaryParentheses` checkstyle module was removed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: 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 #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r696081826
##########
File path: baseline.gradle
##########
@@ -36,7 +36,7 @@ subprojects {
apply plugin: 'com.palantir.baseline-checkstyle'
apply plugin: 'com.palantir.baseline-error-prone'
}
- apply plugin: 'com.palantir.baseline-scalastyle'
+ apply plugin: 'com.github.alisiikh.scalastyle'
Review comment:
Ok, sounds like we can use this for now to unblock this PR and then consider `Spotless` as an alternative.
--
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] nastra commented on a change in pull request #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
nastra commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r691001361
##########
File path: hive3/src/main/java/org/apache/iceberg/mr/hive/vector/CompatibilityHiveVectorUtils.java
##########
@@ -59,7 +59,7 @@ private CompatibilityHiveVectorUtils() {
* Returns serialized mapwork instance from a job conf - ported from Hive source code LlapHiveUtils#findMapWork
*
* @param job JobConf instance
- * @return
+ * @return A serialized {@link MapWork} based on the given job conf
Review comment:
I searched across the codebase and it seems that it's a bit inconsistent in terms of upper/lowercase. I generally think that something like this would be better if addressed by Checkstyle (but not sure if checkstyle supports such a check). Otherwise stuff like this will essentially pop up in lots of PR reviews
--
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] nastra commented on pull request #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
nastra commented on pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#issuecomment-906136723
> Do we need to check in `gradle-wrapper.jar`? Wasn't it downloaded before?
I checked the history and the wrapper jar was removed from the project https://github.com/apache/iceberg/pull/639. However, according to the [Gradle docs](https://docs.gradle.org/current/userguide/gradle_wrapper.html#sec:adding_wrapper) it's expected that all of the wrapper files (including the jar) end up in source control and get replaced in source control once an upgrade of Gradle is done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: 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 #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r696090871
##########
File path: build.gradle
##########
@@ -174,7 +175,7 @@ project(':iceberg-bundled-guava') {
shadowJar {
classifier null
- configurations = [project.configurations.compileOnly]
+ configurations = [project.configurations.compileClasspath]
Review comment:
@nastra Can you explain better?
--
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] nastra edited a comment on pull request #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
nastra edited a comment on pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#issuecomment-884718583
> Can you compare the generated pom's from the current build-files and see whether there are noteworthy differences to the pom's generated with this change? And whether the generated shadow-jar(s) still have the same content and pom's? Issues in there can be tricky to investigate later.
Yes I did compare those already and I noticed that some runtime jars where slightly **bigger** on this branch because `exclude '...'` isn't properly working due to https://github.com/johnrengelman/shadow/issues/505. Pushed 4e33561fd7c256d7c42e06d93d7f7e64fd5d6877 that fixes this
--
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 #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r681823671
##########
File path: build.gradle
##########
@@ -570,15 +619,15 @@ if (jdkVersion == '8') {
tasks.jar.dependsOn tasks.shadowJar
dependencies {
- compile project(':iceberg-data')
- compile project(':iceberg-orc')
+ implementation project(':iceberg-data')
+ implementation project(':iceberg-orc')
testCompileOnly project(path: ':iceberg-data', configuration: 'testArtifacts')
testCompileOnly project(path: ':iceberg-orc', configuration: 'testArtifacts')
}
shadowJar {
- configurations = [project.configurations.compile, project.configurations.compileOnly, project.configurations.testCompileOnly]
Review comment:
Why did this have a dependency on test compile deps ... that doesn't sound right. The change makes sense here to me but just want to make sure we know why it was this way before.
--
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 #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r696064401
##########
File path: baseline.gradle
##########
@@ -36,7 +36,7 @@ subprojects {
apply plugin: 'com.palantir.baseline-checkstyle'
apply plugin: 'com.palantir.baseline-error-prone'
}
- apply plugin: 'com.palantir.baseline-scalastyle'
+ apply plugin: 'com.github.alisiikh.scalastyle'
Review comment:
@cwsteinbach @rdblue @danielcweeks, could you advise here, folks?
--
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] nastra commented on pull request #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
nastra commented on pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#issuecomment-884718583
> Can you compare the generated pom's from the current build-files and see whether there are noteworthy differences to the pom's generated with this change? And whether the generated shadow-jar(s) still have the same content and pom's? Issues in there can be tricky to investigate later.
Yes I did compare those already and I noticed that the jars of the `xx-runtime.jar` is slightly **bigger** on this branch because `exclude '...'` isn't properly working due to https://github.com/johnrengelman/shadow/issues/505
--
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] nastra commented on a change in pull request #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
nastra commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r677161667
##########
File path: build.gradle
##########
@@ -247,13 +250,30 @@ project(':iceberg-data') {
exclude group: 'org.slf4j', module: 'slf4j-log4j12'
}
- testCompile("org.apache.hadoop:hadoop-client") {
+ implementation("org.apache.orc:orc-core::nohive") {
Review comment:
> These are no longer pulled in via iceberg-orc and iceberg-core?
correct, because `implementation` allows now more fine-grained control over dependencies being pulled in, which wasn't the case previously with `compile` (you got more dependencies than you were asking for). Now one could probably achieve the same thing by pulling in certain dependencies via `api`, but that means you're making those dependencies explicitly part of your API. The Gradle docs also state that one should generally prefer `implementation` over `api`, meaning it is better to explicitly state all the dependencies that are really required, so that you don't get surprises with non-expected dependencies and such.
> Do we need an implementation dependency on Parquet as well?
already added it in https://github.com/nastra/iceberg/blob/6c308fd66a9f33ce31b5abf8de61dd8269fda4b5/build.gradle#L261
--
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 #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r682876432
##########
File path: core/src/main/java/org/apache/iceberg/FindFiles.java
##########
@@ -103,7 +103,7 @@ public Builder asOfTime(long timestampMillis) {
// case, there is no valid snapshot to read.
Preconditions.checkArgument(lastSnapshotId != null,
"Cannot find a snapshot older than %s",
- DATE_FORMAT.format(LocalDateTime.ofInstant(Instant.ofEpochMilli(timestampMillis), ZoneId.systemDefault())));
Review comment:
I'm okay fixing this here. Seems minor.
--
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 #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r690838931
##########
File path: baseline.gradle
##########
@@ -47,7 +47,8 @@ subprojects {
// So we apply Spotless manually to get a similar effect to baseline-format, but change the
// import order.
- pluginManager.withPlugin('com.diffplug.gradle.spotless') {
+
Review comment:
nit: extra line?
##########
File path: baseline.gradle
##########
@@ -36,7 +36,7 @@ subprojects {
apply plugin: 'com.palantir.baseline-checkstyle'
apply plugin: 'com.palantir.baseline-error-prone'
}
- apply plugin: 'com.palantir.baseline-scalastyle'
+ apply plugin: 'com.github.alisiikh.scalastyle'
Review comment:
How widely is this plugin used? What's the license? I did not find that in their GitHub.
##########
File path: hive3/src/main/java/org/apache/iceberg/mr/hive/vector/CompatibilityHiveVectorUtils.java
##########
@@ -59,7 +59,7 @@ private CompatibilityHiveVectorUtils() {
* Returns serialized mapwork instance from a job conf - ported from Hive source code LlapHiveUtils#findMapWork
*
* @param job JobConf instance
- * @return
+ * @return A serialized {@link MapWork} based on the given job conf
Review comment:
nit: We usually start with a lower case after `@return`.
##########
File path: baseline.gradle
##########
@@ -82,4 +83,15 @@ subprojects {
)
}
}
-}
+
+ pluginManager.withPlugin('com.github.alisiikh.scalastyle') {
+ scalastyle {
+ config = file("${rootDir}/project/scalastyle_config.xml")
+ inputEncoding = 'UTF-8'
+ outputEncoding = 'UTF-8'
+ failOnWarning = false
+ verbose = false
+ quiet = false
+ }
+ }
+}
Review comment:
nit: missing an empty line at the end of the file?
##########
File path: mr/src/test/java/org/apache/iceberg/mr/hive/TestTables.java
##########
@@ -115,7 +115,7 @@ public String catalogName() {
/**
* The table properties string needed for the CREATE TABLE ... commands,
* like "TBLPROPERTIES('iceberg.catalog'='mycatalog')
- * @return
+ * @return The tables properties string, such as "TBLPROPERTIES('iceberg.catalog'='mycatalog')
Review comment:
I think quotes are not aligned too
##########
File path: mr/src/test/java/org/apache/iceberg/mr/hive/TestTables.java
##########
@@ -115,7 +115,7 @@ public String catalogName() {
/**
* The table properties string needed for the CREATE TABLE ... commands,
* like "TBLPROPERTIES('iceberg.catalog'='mycatalog')
- * @return
+ * @return The tables properties string, such as "TBLPROPERTIES('iceberg.catalog'='mycatalog')
Review comment:
nit: same here
##########
File path: core/src/test/java/org/apache/iceberg/TestTableMetadata.java
##########
@@ -90,11 +90,11 @@ public void testJsonConversion() throws Exception {
long previousSnapshotId = System.currentTimeMillis() - new Random(1234).nextInt(3600);
Snapshot previousSnapshot = new BaseSnapshot(
ops.io(), previousSnapshotId, null, previousSnapshotId, null, null, null, ImmutableList.of(
- new GenericManifestFile(localInput("file:/tmp/manfiest.1.avro"), SPEC_5.specId())));
+ new GenericManifestFile(localInput("file:/tmp/manfiest.1.avro"), SPEC_5.specId())));
Review comment:
Are these required?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: 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 #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r676754289
##########
File path: build.gradle
##########
@@ -247,13 +250,30 @@ project(':iceberg-data') {
exclude group: 'org.slf4j', module: 'slf4j-log4j12'
}
- testCompile("org.apache.hadoop:hadoop-client") {
+ implementation("org.apache.orc:orc-core::nohive") {
Review comment:
These are no longer pulled in via iceberg-orc and iceberg-core? What about Parquet? Do we need an implementation dependency on Parquet 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] nastra commented on pull request #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
nastra commented on pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#issuecomment-891943246
> Looks good to me, If it is possible I would try to separate out the build changes from the format changes if possible. Like first doing a bump of baseline if we can do that without the gradle change then doing the gradle change.
>
> But I don't have strong feelings about that, and if all of our artifacts are the same before and after this Patch I think we are ok. I would recommend we double check the spark31 - spark tests to make sure their runtime class-paths are still correct.
I tried to do that by creating 4 commits and keeping relevant things related to each other within a particular commit. I'm not sure doing the commits in reverse order is better here as I see those as logical steps: 1) Upgrade to Gradle 7 with related plugins that don't work otherwise 2) fix formatting or errorprone stuff separately due to newer versions in separate commits
--
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 #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
aokolnychyi edited a comment on pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#issuecomment-900758469
@raptond @kbendick @flyrain @karuppayya, could you help reviewing this one too if you have time?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: 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 #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r681794627
##########
File path: build.gradle
##########
@@ -174,7 +175,7 @@ project(':iceberg-bundled-guava') {
shadowJar {
classifier null
- configurations = [project.configurations.compileOnly]
+ configurations = [project.configurations.compileClasspath]
Review comment:
this is because CompileClasspath is now basically compileOnly
Implementation is the new "compile"
--
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] nastra commented on pull request #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
nastra commented on pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#issuecomment-918285549
> > Do we need to check in gradle-wrapper.jar? Wasn't it downloaded before?
>
> We do not want to check in the gradle wrapper Jar because it conflicts with Apache release policy. We can't release a source tarball with the Jar in it and have to update the wrapper to download the Jar the first time it runs.
Last time I checked locally, the wrapper wasn't downloaded automatically (not sure if they changed that). I just removed the wrapper jar and CI fails because the wrapper jar is missing
```
Error: Could not find or load main class org.gradle.wrapper.GradleWrapperMain
Error: Process completed with exit code 1.
```
--
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] nastra edited a comment on pull request #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
nastra edited a comment on pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#issuecomment-918285549
> > Do we need to check in gradle-wrapper.jar? Wasn't it downloaded before?
>
> We do not want to check in the gradle wrapper Jar because it conflicts with Apache release policy. We can't release a source tarball with the Jar in it and have to update the wrapper to download the Jar the first time it runs.
@rdblue Last time I checked locally, the wrapper wasn't downloaded automatically (not sure if they changed that). I just removed the wrapper jar and CI fails because the wrapper jar is missing
```
Error: Could not find or load main class org.gradle.wrapper.GradleWrapperMain
Error: Process completed with exit code 1.
```
--
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 #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r681837282
##########
File path: core/src/main/java/org/apache/iceberg/BaseTableScan.java
##########
@@ -306,6 +306,6 @@ private Schema lazyColumnProjection() {
}
private static String formatTimestampMillis(long millis) {
- return DATE_FORMAT.format(LocalDateTime.ofInstant(Instant.ofEpochMilli(millis), ZoneId.systemDefault()));
Review comment:
This is a slightly different behavior ... we used to use the System Default and now we are doing UTC?
--
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 #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r682876432
##########
File path: core/src/main/java/org/apache/iceberg/FindFiles.java
##########
@@ -103,7 +103,7 @@ public Builder asOfTime(long timestampMillis) {
// case, there is no valid snapshot to read.
Preconditions.checkArgument(lastSnapshotId != null,
"Cannot find a snapshot older than %s",
- DATE_FORMAT.format(LocalDateTime.ofInstant(Instant.ofEpochMilli(timestampMillis), ZoneId.systemDefault())));
Review comment:
I'm okay fixing this here. Seems minor.
--
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] nastra commented on a change in pull request #2826: Build: Upgrade to Gradle 7.x
Posted by GitBox <gi...@apache.org>.
nastra commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r681842716
##########
File path: core/src/main/java/org/apache/iceberg/BaseTableScan.java
##########
@@ -306,6 +306,6 @@ private Schema lazyColumnProjection() {
}
private static String formatTimestampMillis(long millis) {
- return DATE_FORMAT.format(LocalDateTime.ofInstant(Instant.ofEpochMilli(millis), ZoneId.systemDefault()));
Review comment:
yes, this was an error reported from the newer error-prone checks
> com.palantir.baseline-error-prone uses a newer error-prone version (and does additional checks), so a few changes were necessary to fix JavaTimeSystemDefaultTimeZone / ConsistentOverrides errors
##########
File path: core/src/main/java/org/apache/iceberg/FindFiles.java
##########
@@ -103,7 +103,7 @@ public Builder asOfTime(long timestampMillis) {
// case, there is no valid snapshot to read.
Preconditions.checkArgument(lastSnapshotId != null,
"Cannot find a snapshot older than %s",
- DATE_FORMAT.format(LocalDateTime.ofInstant(Instant.ofEpochMilli(timestampMillis), ZoneId.systemDefault())));
Review comment:
yes, this was an error reported from the newer error-prone checks
> com.palantir.baseline-error-prone uses a newer error-prone version (and does additional checks), so a few changes were necessary to fix JavaTimeSystemDefaultTimeZone / ConsistentOverrides errors
--
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