You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "singhpk234 (via GitHub)" <gi...@apache.org> on 2023/04/20 21:16:48 UTC
[GitHub] [iceberg] singhpk234 opened a new pull request, #7391: Build / Infra: Run Iceberg Java Test with JDK 17
singhpk234 opened a new pull request, #7391:
URL: https://github.com/apache/iceberg/pull/7391
### About the change
This change adds a CI to run iceberg with JDK 17, considering now most of the engines are adding support for jdk 17 for ex : trino, spark.
cc @jackye1995
--
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] singhpk234 commented on pull request #7391: Build: Run Iceberg with JDK 17
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on PR #7391:
URL: https://github.com/apache/iceberg/pull/7391#issuecomment-1522009282
> I think it would be good to also double-check the difference in the sizes that required the test changes
+1 here is the [analysis](https://github.com/apache/iceberg/pull/7391#discussion_r1173527238) for the same, basically JDK 8 was reporting column size for `binaryCol`for 44 where as JDK 17 was reporting it as `43`, these stats were directly taken from the parquet footer hence to make the size / stats of the parquet file written env independent, I directly read the columnSize from dataFile itself, and made the changes 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] singhpk234 commented on a diff in pull request #7391: Build: Run Iceberg with JDK 17
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #7391:
URL: https://github.com/apache/iceberg/pull/7391#discussion_r1175557969
##########
spark/v3.4/spark/src/test/java/org/apache/iceberg/spark/source/TestMetadataTableReadableMetrics.java:
##########
@@ -76,6 +77,8 @@ public class TestMetadataTableReadableMetrics extends SparkTestBaseWithCatalog {
optional(8, "fixedCol", Types.FixedType.ofLength(3)),
optional(9, "binaryCol", Types.BinaryType.get()));
+ private DataFile dataFile;
Review Comment:
Making this as a class member helped as we create the data file and table obj both in the createPrimitiveTable and createNestedTable methods directly, which makes both table / data file obj non-accessible in the ut, by making it a class member we can avoid logic of recreation of these obj in the UT and directly reference it in the test. Hence made it like above. Please let me know your thoughts.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] singhpk234 commented on a diff in pull request #7391: Build: Run Iceberg Java Test with JDK 17
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #7391:
URL: https://github.com/apache/iceberg/pull/7391#discussion_r1173200067
##########
build.gradle:
##########
@@ -715,6 +735,11 @@ project(':iceberg-parquet') {
}
project(':iceberg-arrow') {
+ test {
+ if (JavaVersion.current() == JavaVersion.VERSION_17) {
+ jvmArgs += ["-Xmx1024m", "-XX:+IgnoreUnrecognizedVMOptions", "--add-opens", "java.base/java.lang.invoke=ALL-UNNAMED", "--add-opens", "java.base/java.io=ALL-UNNAMED", "--add-opens", "java.base/java.lang.reflect=ALL-UNNAMED", "--add-opens", "java.base/java.lang=ALL-UNNAMED", "--add-opens", "java.base/java.math=ALL-UNNAMED", "--add-opens", "java.base/java.net=ALL-UNNAMED", "--add-opens", "java.base/java.nio=ALL-UNNAMED", "--add-opens", "java.base/java.text=ALL-UNNAMED", "--add-opens", "java.base/java.time=ALL-UNNAMED", "--add-opens", "java.base/java.util.concurrent.atomic=ALL-UNNAMED", "--add-opens", "java.base/java.util.concurrent=ALL-UNNAMED", "--add-opens", "java.base/java.util.regex=ALL-UNNAMED", "--add-opens", "java.base/java.util=ALL-UNNAMED", "--add-opens", "java.base/jdk.internal.ref=ALL-UNNAMED", "--add-opens", "java.base/jdk.internal.reflect=ALL-UNNAMED", "--add-opens", "java.sql/java.sql=ALL-UNNAMED"]
Review Comment:
Arrow test were failing without these flags, for ex:
```
org.apache.iceberg.arrow.vectorized.ArrowReaderTest > testHasNextIsIdempotent FAILED
java.lang.NoClassDefFoundError: Could not initialize class org.apache.arrow.memory.util.MemoryUtil
at org.apache.arrow.memory.util.MemoryUtil.<clinit>(MemoryUtil.java:138)
... 16 more
Caused by:
java.lang.reflect.InaccessibleObjectException: Unable to make field long java.nio.Buffer.address accessible: module java.base does not "opens java.nio" to unnamed module @99e937b
```
hence it required jdk flags like `--add-opens=java.base/java.io=ALL-UNNAMED` hence added the same here, not all flags mentioned here are required here though, just added the superset 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] singhpk234 commented on a diff in pull request #7391: Build: Run Iceberg Java Test with JDK 17
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #7391:
URL: https://github.com/apache/iceberg/pull/7391#discussion_r1173200067
##########
build.gradle:
##########
@@ -715,6 +735,11 @@ project(':iceberg-parquet') {
}
project(':iceberg-arrow') {
+ test {
+ if (JavaVersion.current() == JavaVersion.VERSION_17) {
+ jvmArgs += ["-Xmx1024m", "-XX:+IgnoreUnrecognizedVMOptions", "--add-opens", "java.base/java.lang.invoke=ALL-UNNAMED", "--add-opens", "java.base/java.io=ALL-UNNAMED", "--add-opens", "java.base/java.lang.reflect=ALL-UNNAMED", "--add-opens", "java.base/java.lang=ALL-UNNAMED", "--add-opens", "java.base/java.math=ALL-UNNAMED", "--add-opens", "java.base/java.net=ALL-UNNAMED", "--add-opens", "java.base/java.nio=ALL-UNNAMED", "--add-opens", "java.base/java.text=ALL-UNNAMED", "--add-opens", "java.base/java.time=ALL-UNNAMED", "--add-opens", "java.base/java.util.concurrent.atomic=ALL-UNNAMED", "--add-opens", "java.base/java.util.concurrent=ALL-UNNAMED", "--add-opens", "java.base/java.util.regex=ALL-UNNAMED", "--add-opens", "java.base/java.util=ALL-UNNAMED", "--add-opens", "java.base/jdk.internal.ref=ALL-UNNAMED", "--add-opens", "java.base/jdk.internal.reflect=ALL-UNNAMED", "--add-opens", "java.sql/java.sql=ALL-UNNAMED"]
Review Comment:
Arrow test were failing without these flags, for ex:
```
org.apache.iceberg.arrow.vectorized.ArrowReaderTest > testHasNextIsIdempotent FAILED
java.lang.NoClassDefFoundError: Could not initialize class org.apache.arrow.memory.util.MemoryUtil
at org.apache.arrow.memory.util.MemoryUtil.<clinit>(MemoryUtil.java:138)
... 16 more
Caused by:
java.lang.reflect.InaccessibleObjectException: Unable to make field long java.nio.Buffer.address accessible: module java.base does not "opens java.nio" to unnamed module @99e937b
```
hence it required jdk flags like `--add-opens=java.base/java.io=ALL-UNNAMED` hence added the same 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] jackye1995 commented on a diff in pull request #7391: Build: Run Iceberg with JDK 17
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on code in PR #7391:
URL: https://github.com/apache/iceberg/pull/7391#discussion_r1174318068
##########
build.gradle:
##########
@@ -68,10 +68,36 @@ try {
if (JavaVersion.current() == JavaVersion.VERSION_1_8) {
project.ext.jdkVersion = '8'
+ project.ext.extraJvmArgs = []
} else if (JavaVersion.current() == JavaVersion.VERSION_11) {
project.ext.jdkVersion = '11'
+ project.ext.extraJvmArgs = []
+} else if (JavaVersion.current() == JavaVersion.VERSION_17) {
+ project.ext.jdkVersion = '17'
+ project.ext.extraJvmArgs = ["-XX:+IgnoreUnrecognizedVMOptions",
+ "--add-opens", "java.base/java.io=ALL-UNNAMED",
Review Comment:
Do we need to open all these?
--
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] singhpk234 commented on a diff in pull request #7391: Build: Run Iceberg with JDK 17
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #7391:
URL: https://github.com/apache/iceberg/pull/7391#discussion_r1174529831
##########
build.gradle:
##########
@@ -68,10 +68,36 @@ try {
if (JavaVersion.current() == JavaVersion.VERSION_1_8) {
project.ext.jdkVersion = '8'
+ project.ext.extraJvmArgs = []
} else if (JavaVersion.current() == JavaVersion.VERSION_11) {
project.ext.jdkVersion = '11'
+ project.ext.extraJvmArgs = []
+} else if (JavaVersion.current() == JavaVersion.VERSION_17) {
+ project.ext.jdkVersion = '17'
+ project.ext.extraJvmArgs = ["-XX:+IgnoreUnrecognizedVMOptions",
+ "--add-opens", "java.base/java.io=ALL-UNNAMED",
Review Comment:
I collected these flag by debugging individual failures of ut's from test projects (spark / arrow / aws (kryo)) this is a superset i would say we may define only the relevant one's per package, but since there was no harm in passing additional flags hence passed all the flags in all the projects. please let me know your thoughts if the above makes sense, happy to add required flags per projects
--
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] singhpk234 commented on a diff in pull request #7391: Build: Run Iceberg with JDK 17
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #7391:
URL: https://github.com/apache/iceberg/pull/7391#discussion_r1174530232
##########
build.gradle:
##########
@@ -271,6 +297,9 @@ project(':iceberg-bundled-guava') {
}
project(':iceberg-api') {
+ test {
Review Comment:
I tried a lot of options for ex : setting this in `org.gradle.jvmargs` or setting it in `DEFAULT_JVM_OPTS` or `GRADLE_OPTS` but the only way i could make it work was via above, I am open to all suggestions 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] singhpk234 commented on a diff in pull request #7391: Build: Run Iceberg with JDK 17
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #7391:
URL: https://github.com/apache/iceberg/pull/7391#discussion_r1174281647
##########
spark/v3.4/spark/src/test/java/org/apache/iceberg/spark/source/TestMetadataTableReadableMetrics.java:
##########
@@ -190,6 +191,7 @@ private GenericRecord createNestedRecord(Long longCol, Double doubleCol) {
}
@Test
+ @Ignore
Review Comment:
made the changes to directly fetch column size from data file created, to get rid of hard coded sizes
--
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] singhpk234 commented on a diff in pull request #7391: Build / Infra: Run Iceberg Java Test with JDK 17
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #7391:
URL: https://github.com/apache/iceberg/pull/7391#discussion_r1173100570
##########
api/src/main/java/org/apache/iceberg/expressions/ExpressionUtil.java:
##########
@@ -47,10 +47,10 @@ public class ExpressionUtil {
private static final Pattern DATE = Pattern.compile("\\d{4}-\\d{2}-\\d{2}");
private static final Pattern TIME = Pattern.compile("\\d{2}:\\d{2}(:\\d{2}(.\\d{1,6})?)?");
private static final Pattern TIMESTAMP =
- Pattern.compile("\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}(:\\d{2}(.\\d{1,6})?)?");
+ Pattern.compile("\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}(:\\d{2}(.\\d{1,10})?)?");
private static final Pattern TIMESTAMPTZ =
Pattern.compile(
- "\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}(:\\d{2}(.\\d{1,6})?)?([-+]\\d{2}:\\d{2}|Z)");
+ "\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}(:\\d{2}(.\\d{1,10})?)?([-+]\\d{2}:\\d{2}|Z)");
Review Comment:
when running with java 17, i get `2023-04-20T19:17:17.748170628Z` when using OffsetTime class, it only happens with me in ubuntu and not in my developement mac.
As we need to support both the env's hence adjusted the regex to account for the same.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on a diff in pull request #7391: Build: Run Iceberg with JDK 17
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on code in PR #7391:
URL: https://github.com/apache/iceberg/pull/7391#discussion_r1175548330
##########
spark/v3.4/spark/src/test/java/org/apache/iceberg/spark/source/TestMetadataTableReadableMetrics.java:
##########
@@ -76,6 +77,8 @@ public class TestMetadataTableReadableMetrics extends SparkTestBaseWithCatalog {
optional(8, "fixedCol", Types.FixedType.ofLength(3)),
optional(9, "binaryCol", Types.BinaryType.get()));
+ private DataFile dataFile;
Review Comment:
Why do we need to make this change? I thought as long as we remove the hard-coded stats it's good enough. If it's only for styling, it feels to me that having this as a local variable is better compared to a shared uninitialized instance variable across different tests.
--
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] singhpk234 commented on a diff in pull request #7391: Build: Run Iceberg with JDK 17
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #7391:
URL: https://github.com/apache/iceberg/pull/7391#discussion_r1176892250
##########
api/src/main/java/org/apache/iceberg/expressions/ExpressionUtil.java:
##########
@@ -47,10 +47,10 @@ public class ExpressionUtil {
private static final Pattern DATE = Pattern.compile("\\d{4}-\\d{2}-\\d{2}");
private static final Pattern TIME = Pattern.compile("\\d{2}:\\d{2}(:\\d{2}(.\\d{1,6})?)?");
private static final Pattern TIMESTAMP =
- Pattern.compile("\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}(:\\d{2}(.\\d{1,6})?)?");
+ Pattern.compile("\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}(:\\d{2}(.\\d{1,10})?)?");
private static final Pattern TIMESTAMPTZ =
Pattern.compile(
- "\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}(:\\d{2}(.\\d{1,6})?)?([-+]\\d{2}:\\d{2}|Z)");
+ "\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}(:\\d{2}(.\\d{1,10})?)?([-+]\\d{2}:\\d{2}|Z)");
Review Comment:
Actually it would be 9 places even for java 8, looks like we were not hitting this case earlier
```
public static final DateTimeFormatter ISO_LOCAL_TIME;
static {
ISO_LOCAL_TIME = new DateTimeFormatterBuilder()
.appendValue(HOUR_OF_DAY, 2)
.appendLiteral(':')
.appendValue(MINUTE_OF_HOUR, 2)
.optionalStart()
.appendLiteral(':')
.appendValue(SECOND_OF_MINUTE, 2)
.optionalStart()
.appendFraction(NANO_OF_SECOND, 0, 9, true)
.toFormatter(ResolverStyle.STRICT, null);
}
```
https://github.com/corretto/corretto-8/blob/develop/jdk/src/share/classes/java/time/format/DateTimeFormatter.java#L811
fixed the hard coded test in time as well as updated the regex
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on a diff in pull request #7391: Build: Run Iceberg with JDK 17
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on code in PR #7391:
URL: https://github.com/apache/iceberg/pull/7391#discussion_r1175787399
##########
spark/v3.4/spark/src/test/java/org/apache/iceberg/spark/source/TestMetadataTableReadableMetrics.java:
##########
@@ -76,6 +77,8 @@ public class TestMetadataTableReadableMetrics extends SparkTestBaseWithCatalog {
optional(8, "fixedCol", Types.FixedType.ofLength(3)),
optional(9, "binaryCol", Types.BinaryType.get()));
+ private DataFile dataFile;
Review Comment:
I see. I read the full tests, technically we could also let `createPrimitiveTable` and `createNestedTable` return `Pair<Table, DataFile>` so the individual methods could access both the table (which is not really used today) and also the associated data files. I think this has the benefit that if we want to enable parallel execution of tests it won't get in our way. What do you think?
--
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 diff in pull request #7391: Build / Infra: Run Iceberg Java Test with JDK 17
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #7391:
URL: https://github.com/apache/iceberg/pull/7391#discussion_r1173173330
##########
build.gradle:
##########
@@ -715,6 +735,11 @@ project(':iceberg-parquet') {
}
project(':iceberg-arrow') {
+ test {
+ if (JavaVersion.current() == JavaVersion.VERSION_17) {
+ jvmArgs += ["-Xmx1024m", "-XX:+IgnoreUnrecognizedVMOptions", "--add-opens", "java.base/java.lang.invoke=ALL-UNNAMED", "--add-opens", "java.base/java.io=ALL-UNNAMED", "--add-opens", "java.base/java.lang.reflect=ALL-UNNAMED", "--add-opens", "java.base/java.lang=ALL-UNNAMED", "--add-opens", "java.base/java.math=ALL-UNNAMED", "--add-opens", "java.base/java.net=ALL-UNNAMED", "--add-opens", "java.base/java.nio=ALL-UNNAMED", "--add-opens", "java.base/java.text=ALL-UNNAMED", "--add-opens", "java.base/java.time=ALL-UNNAMED", "--add-opens", "java.base/java.util.concurrent.atomic=ALL-UNNAMED", "--add-opens", "java.base/java.util.concurrent=ALL-UNNAMED", "--add-opens", "java.base/java.util.regex=ALL-UNNAMED", "--add-opens", "java.base/java.util=ALL-UNNAMED", "--add-opens", "java.base/jdk.internal.ref=ALL-UNNAMED", "--add-opens", "java.base/jdk.internal.reflect=ALL-UNNAMED", "--add-opens", "java.sql/java.sql=ALL-UNNAMED"]
Review Comment:
Can you explain why these are needed?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on a diff in pull request #7391: Build: Run Iceberg with JDK 17
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on code in PR #7391:
URL: https://github.com/apache/iceberg/pull/7391#discussion_r1174596700
##########
build.gradle:
##########
@@ -271,6 +297,9 @@ project(':iceberg-bundled-guava') {
}
project(':iceberg-api') {
+ test {
Review Comment:
I was thinking if we could configure them all at allProjects or subPtojects hook https://github.com/apache/iceberg/blob/master/build.gradle#L200. If not, I cannot think of a better way either
--
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 diff in pull request #7391: Build: Run Iceberg with JDK 17
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7391:
URL: https://github.com/apache/iceberg/pull/7391#discussion_r1173858209
##########
build.gradle:
##########
@@ -271,6 +273,11 @@ project(':iceberg-bundled-guava') {
}
project(':iceberg-api') {
+ test {
+ if (JavaVersion.current() == JavaVersion.VERSION_17) {
Review Comment:
I think it would be great to define the JVM args in a single place so that all subprojects use them automatically when running with JDK17
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 merged pull request #7391: Build: Run Iceberg with JDK 17
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 merged PR #7391:
URL: https://github.com/apache/iceberg/pull/7391
--
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] singhpk234 commented on a diff in pull request #7391: Build: Run Iceberg Java Test with JDK 17
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #7391:
URL: https://github.com/apache/iceberg/pull/7391#discussion_r1173200201
##########
api/src/main/java/org/apache/iceberg/expressions/ExpressionUtil.java:
##########
@@ -47,10 +47,10 @@ public class ExpressionUtil {
private static final Pattern DATE = Pattern.compile("\\d{4}-\\d{2}-\\d{2}");
private static final Pattern TIME = Pattern.compile("\\d{2}:\\d{2}(:\\d{2}(.\\d{1,6})?)?");
private static final Pattern TIMESTAMP =
- Pattern.compile("\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}(:\\d{2}(.\\d{1,6})?)?");
+ Pattern.compile("\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}(:\\d{2}(.\\d{1,10})?)?");
private static final Pattern TIMESTAMPTZ =
Pattern.compile(
- "\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}(:\\d{2}(.\\d{1,6})?)?([-+]\\d{2}:\\d{2}|Z)");
+ "\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}(:\\d{2}(.\\d{1,10})?)?([-+]\\d{2}:\\d{2}|Z)");
Review Comment:
ACK, counting mistake :P
--
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] singhpk234 commented on a diff in pull request #7391: Build: Run Iceberg with JDK 17
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #7391:
URL: https://github.com/apache/iceberg/pull/7391#discussion_r1174057374
##########
build.gradle:
##########
@@ -715,6 +735,11 @@ project(':iceberg-parquet') {
}
project(':iceberg-arrow') {
+ test {
+ if (JavaVersion.current() == JavaVersion.VERSION_17) {
+ jvmArgs += ["-Xmx1024m", "-XX:+IgnoreUnrecognizedVMOptions", "--add-opens", "java.base/java.lang.invoke=ALL-UNNAMED", "--add-opens", "java.base/java.io=ALL-UNNAMED", "--add-opens", "java.base/java.lang.reflect=ALL-UNNAMED", "--add-opens", "java.base/java.lang=ALL-UNNAMED", "--add-opens", "java.base/java.math=ALL-UNNAMED", "--add-opens", "java.base/java.net=ALL-UNNAMED", "--add-opens", "java.base/java.nio=ALL-UNNAMED", "--add-opens", "java.base/java.text=ALL-UNNAMED", "--add-opens", "java.base/java.time=ALL-UNNAMED", "--add-opens", "java.base/java.util.concurrent.atomic=ALL-UNNAMED", "--add-opens", "java.base/java.util.concurrent=ALL-UNNAMED", "--add-opens", "java.base/java.util.regex=ALL-UNNAMED", "--add-opens", "java.base/java.util=ALL-UNNAMED", "--add-opens", "java.base/jdk.internal.ref=ALL-UNNAMED", "--add-opens", "java.base/jdk.internal.reflect=ALL-UNNAMED", "--add-opens", "java.sql/java.sql=ALL-UNNAMED"]
Review Comment:
ACK
--
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] singhpk234 commented on a diff in pull request #7391: Build: Run Iceberg with JDK 17
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #7391:
URL: https://github.com/apache/iceberg/pull/7391#discussion_r1174095844
##########
build.gradle:
##########
@@ -715,6 +735,11 @@ project(':iceberg-parquet') {
}
project(':iceberg-arrow') {
+ test {
+ if (JavaVersion.current() == JavaVersion.VERSION_17) {
+ jvmArgs += ["-Xmx1024m", "-XX:+IgnoreUnrecognizedVMOptions", "--add-opens", "java.base/java.lang.invoke=ALL-UNNAMED", "--add-opens", "java.base/java.io=ALL-UNNAMED", "--add-opens", "java.base/java.lang.reflect=ALL-UNNAMED", "--add-opens", "java.base/java.lang=ALL-UNNAMED", "--add-opens", "java.base/java.math=ALL-UNNAMED", "--add-opens", "java.base/java.net=ALL-UNNAMED", "--add-opens", "java.base/java.nio=ALL-UNNAMED", "--add-opens", "java.base/java.text=ALL-UNNAMED", "--add-opens", "java.base/java.time=ALL-UNNAMED", "--add-opens", "java.base/java.util.concurrent.atomic=ALL-UNNAMED", "--add-opens", "java.base/java.util.concurrent=ALL-UNNAMED", "--add-opens", "java.base/java.util.regex=ALL-UNNAMED", "--add-opens", "java.base/java.util=ALL-UNNAMED", "--add-opens", "java.base/jdk.internal.ref=ALL-UNNAMED", "--add-opens", "java.base/jdk.internal.reflect=ALL-UNNAMED", "--add-opens", "java.sql/java.sql=ALL-UNNAMED"]
Review Comment:
Done
##########
build.gradle:
##########
@@ -271,6 +273,11 @@ project(':iceberg-bundled-guava') {
}
project(':iceberg-api') {
+ test {
+ if (JavaVersion.current() == JavaVersion.VERSION_17) {
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] singhpk234 commented on a diff in pull request #7391: Build: Run Iceberg with JDK 17
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #7391:
URL: https://github.com/apache/iceberg/pull/7391#discussion_r1173528240
##########
spark/v3.3/spark/src/test/java/org/apache/iceberg/spark/source/TestMetadataTableReadableMetrics.java:
##########
@@ -190,6 +191,7 @@ private GenericRecord createNestedRecord(Long longCol, Double doubleCol) {
}
@Test
+ @Ignore
Review Comment:
Definitely !
--
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] Fokko commented on a diff in pull request #7391: Build: Run Iceberg with JDK 17
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #7391:
URL: https://github.com/apache/iceberg/pull/7391#discussion_r1173390615
##########
build.gradle:
##########
@@ -715,6 +735,11 @@ project(':iceberg-parquet') {
}
project(':iceberg-arrow') {
+ test {
+ if (JavaVersion.current() == JavaVersion.VERSION_17) {
+ jvmArgs += ["-Xmx1024m", "-XX:+IgnoreUnrecognizedVMOptions", "--add-opens", "java.base/java.lang.invoke=ALL-UNNAMED", "--add-opens", "java.base/java.io=ALL-UNNAMED", "--add-opens", "java.base/java.lang.reflect=ALL-UNNAMED", "--add-opens", "java.base/java.lang=ALL-UNNAMED", "--add-opens", "java.base/java.math=ALL-UNNAMED", "--add-opens", "java.base/java.net=ALL-UNNAMED", "--add-opens", "java.base/java.nio=ALL-UNNAMED", "--add-opens", "java.base/java.text=ALL-UNNAMED", "--add-opens", "java.base/java.time=ALL-UNNAMED", "--add-opens", "java.base/java.util.concurrent.atomic=ALL-UNNAMED", "--add-opens", "java.base/java.util.concurrent=ALL-UNNAMED", "--add-opens", "java.base/java.util.regex=ALL-UNNAMED", "--add-opens", "java.base/java.util=ALL-UNNAMED", "--add-opens", "java.base/jdk.internal.ref=ALL-UNNAMED", "--add-opens", "java.base/jdk.internal.reflect=ALL-UNNAMED", "--add-opens", "java.sql/java.sql=ALL-UNNAMED"]
Review Comment:
Can you add newlines?
--
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] singhpk234 commented on a diff in pull request #7391: Build: Run Iceberg Java Test with JDK 17
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #7391:
URL: https://github.com/apache/iceberg/pull/7391#discussion_r1173288917
##########
spark/v3.4/spark/src/test/java/org/apache/iceberg/spark/source/TestMetadataTableReadableMetrics.java:
##########
@@ -190,6 +191,7 @@ private GenericRecord createNestedRecord(Long longCol, Double doubleCol) {
}
@Test
+ @Ignore
Review Comment:
Have disabled this UT purposefully as it failed in one of my [CI runs](https://github.com/singhpk234/iceberg/actions/runs/4759943553/jobs/8459727045) with error below :
```
TestMetadataTableReadableMetrics > testPrimitiveColumns FAILED
java.lang.AssertionError: Row should match: row 1 col 1 (nested col 1) (nested col 2) contents should match expected:<44> but was:<43>
at org.junit.Assert.fail(Assert.java:89)
at org.junit.Assert.failNotEquals(Assert.java:835)
at org.junit.Assert.assertEquals(Assert.java:120)
at org.apache.iceberg.spark.SparkTestHelperBase.assertEquals(SparkTestHelperBase.java:84)
at org.apache.iceberg.spark.SparkTestHelperBase.assertEquals(SparkTestHelperBase.java:81)
at org.apache.iceberg.spark.SparkTestHelperBase.assertEquals(SparkTestHelperBase.java:81)
at org.apache.iceberg.spark.SparkTestHelperBase.assertEquals(SparkTestHelperBase.java:66)
at org.apache.iceberg.spark.source.TestMetadataTableReadableMetrics.testPrimitiveColumns(TestMetadataTableReadableMetrics.java:232)
```
will take a deeper look, shortly as the failure 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] singhpk234 commented on a diff in pull request #7391: Build: Run Iceberg Java Test with JDK 17
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #7391:
URL: https://github.com/apache/iceberg/pull/7391#discussion_r1173288917
##########
spark/v3.4/spark/src/test/java/org/apache/iceberg/spark/source/TestMetadataTableReadableMetrics.java:
##########
@@ -190,6 +191,7 @@ private GenericRecord createNestedRecord(Long longCol, Double doubleCol) {
}
@Test
+ @Ignore
Review Comment:
Have disabled this UT purposefully as it failed in one of my CI runs with error below :
```
TestMetadataTableReadableMetrics > testPrimitiveColumns FAILED
java.lang.AssertionError: Row should match: row 1 col 1 (nested col 1) (nested col 2) contents should match expected:<44> but was:<43>
at org.junit.Assert.fail(Assert.java:89)
at org.junit.Assert.failNotEquals(Assert.java:835)
at org.junit.Assert.assertEquals(Assert.java:120)
at org.apache.iceberg.spark.SparkTestHelperBase.assertEquals(SparkTestHelperBase.java:84)
at org.apache.iceberg.spark.SparkTestHelperBase.assertEquals(SparkTestHelperBase.java:81)
at org.apache.iceberg.spark.SparkTestHelperBase.assertEquals(SparkTestHelperBase.java:81)
at org.apache.iceberg.spark.SparkTestHelperBase.assertEquals(SparkTestHelperBase.java:66)
at org.apache.iceberg.spark.source.TestMetadataTableReadableMetrics.testPrimitiveColumns(TestMetadataTableReadableMetrics.java:232)
```
will take a deeper look, shortly as the failure 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] singhpk234 commented on a diff in pull request #7391: Build: Run Iceberg with JDK 17
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #7391:
URL: https://github.com/apache/iceberg/pull/7391#discussion_r1173527238
##########
spark/v3.4/spark/src/test/java/org/apache/iceberg/spark/source/TestMetadataTableReadableMetrics.java:
##########
@@ -190,6 +191,7 @@ private GenericRecord createNestedRecord(Long longCol, Double doubleCol) {
}
@Test
+ @Ignore
Review Comment:
This is actually interesting and happening only in java 17 env :
basically the parquet file written has diff `column_size` value in `files` metadata table, in java 8 it's 44 and in java 17 it's 43.
effectively this line here produces diff results:
https://github.com/apache/iceberg/blob/d04efee702fcdcdbe3659c12f7442f5000aa246a/parquet/src/main/java/org/apache/iceberg/parquet/ParquetUtil.java#L127
Is it because java 17 provides better compression than java 8 ? as per this blog : https://dkomanov.medium.com/java-compression-performance-fb373078cfde
since this is a value we are getting directly from parquet footer, so effectively we are reading what were writing stats are not getting messed up in between, which seems correct to me. But I might be wrong here, will wait for other folks feedback here if there is deeper investigation 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] Fokko commented on a diff in pull request #7391: Build: Run Iceberg with JDK 17
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #7391:
URL: https://github.com/apache/iceberg/pull/7391#discussion_r1173392100
##########
spark/v3.3/spark/src/test/java/org/apache/iceberg/spark/source/TestMetadataTableReadableMetrics.java:
##########
@@ -190,6 +191,7 @@ private GenericRecord createNestedRecord(Long longCol, Double doubleCol) {
}
@Test
+ @Ignore
Review Comment:
I think we should fix the UTs before adding JDK17, otherwise, it looks like JDK17 is supported but it might have bugs
--
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] singhpk234 commented on a diff in pull request #7391: Build: Run Iceberg Java Test with JDK 17
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #7391:
URL: https://github.com/apache/iceberg/pull/7391#discussion_r1173200067
##########
build.gradle:
##########
@@ -715,6 +735,11 @@ project(':iceberg-parquet') {
}
project(':iceberg-arrow') {
+ test {
+ if (JavaVersion.current() == JavaVersion.VERSION_17) {
+ jvmArgs += ["-Xmx1024m", "-XX:+IgnoreUnrecognizedVMOptions", "--add-opens", "java.base/java.lang.invoke=ALL-UNNAMED", "--add-opens", "java.base/java.io=ALL-UNNAMED", "--add-opens", "java.base/java.lang.reflect=ALL-UNNAMED", "--add-opens", "java.base/java.lang=ALL-UNNAMED", "--add-opens", "java.base/java.math=ALL-UNNAMED", "--add-opens", "java.base/java.net=ALL-UNNAMED", "--add-opens", "java.base/java.nio=ALL-UNNAMED", "--add-opens", "java.base/java.text=ALL-UNNAMED", "--add-opens", "java.base/java.time=ALL-UNNAMED", "--add-opens", "java.base/java.util.concurrent.atomic=ALL-UNNAMED", "--add-opens", "java.base/java.util.concurrent=ALL-UNNAMED", "--add-opens", "java.base/java.util.regex=ALL-UNNAMED", "--add-opens", "java.base/java.util=ALL-UNNAMED", "--add-opens", "java.base/jdk.internal.ref=ALL-UNNAMED", "--add-opens", "java.base/jdk.internal.reflect=ALL-UNNAMED", "--add-opens", "java.sql/java.sql=ALL-UNNAMED"]
Review Comment:
Arrow test were failing without these flags, for ex:
```
org.apache.iceberg.arrow.vectorized.ArrowReaderTest > testHasNextIsIdempotent FAILED
java.lang.NoClassDefFoundError: Could not initialize class org.apache.arrow.memory.util.MemoryUtil
at org.apache.arrow.memory.util.MemoryUtil.<clinit>(MemoryUtil.java:138)
... 16 more
Caused by:
java.lang.reflect.InaccessibleObjectException: Unable to make field long java.nio.Buffer.address accessible: module java.base does not "opens java.nio" to unnamed module @99e937b
```
hence it required jdk flags like `--add-opens=java.base/java.nio=ALL-UNNAMED` hence added the same here, not all flags mentioned here are required here though, just added the superset 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] singhpk234 commented on a diff in pull request #7391: Build: Run Iceberg with JDK 17
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #7391:
URL: https://github.com/apache/iceberg/pull/7391#discussion_r1175799017
##########
spark/v3.4/spark/src/test/java/org/apache/iceberg/spark/source/TestMetadataTableReadableMetrics.java:
##########
@@ -76,6 +77,8 @@ public class TestMetadataTableReadableMetrics extends SparkTestBaseWithCatalog {
optional(8, "fixedCol", Types.FixedType.ofLength(3)),
optional(9, "binaryCol", Types.BinaryType.get()));
+ private DataFile dataFile;
Review Comment:
ACK, +1 on parallel execution.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on a diff in pull request #7391: Build: Run Iceberg with JDK 17
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on code in PR #7391:
URL: https://github.com/apache/iceberg/pull/7391#discussion_r1176847338
##########
api/src/main/java/org/apache/iceberg/expressions/ExpressionUtil.java:
##########
@@ -47,10 +47,10 @@ public class ExpressionUtil {
private static final Pattern DATE = Pattern.compile("\\d{4}-\\d{2}-\\d{2}");
private static final Pattern TIME = Pattern.compile("\\d{2}:\\d{2}(:\\d{2}(.\\d{1,6})?)?");
private static final Pattern TIMESTAMP =
- Pattern.compile("\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}(:\\d{2}(.\\d{1,6})?)?");
+ Pattern.compile("\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}(:\\d{2}(.\\d{1,10})?)?");
private static final Pattern TIMESTAMPTZ =
Pattern.compile(
- "\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}(:\\d{2}(.\\d{1,6})?)?([-+]\\d{2}:\\d{2}|Z)");
+ "\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}(:\\d{2}(.\\d{1,10})?)?([-+]\\d{2}:\\d{2}|Z)");
Review Comment:
Would `TIME` also be affected by this issue?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on a diff in pull request #7391: Build: Run Iceberg with JDK 17
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on code in PR #7391:
URL: https://github.com/apache/iceberg/pull/7391#discussion_r1176783348
##########
api/src/main/java/org/apache/iceberg/expressions/ExpressionUtil.java:
##########
@@ -47,10 +47,10 @@ public class ExpressionUtil {
private static final Pattern DATE = Pattern.compile("\\d{4}-\\d{2}-\\d{2}");
private static final Pattern TIME = Pattern.compile("\\d{2}:\\d{2}(:\\d{2}(.\\d{1,6})?)?");
private static final Pattern TIMESTAMP =
- Pattern.compile("\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}(:\\d{2}(.\\d{1,6})?)?");
+ Pattern.compile("\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}(:\\d{2}(.\\d{1,9})?)?");
Review Comment:
sorry forgot to ask, why is this changed from 6 to 9? Does JDK17 change default precision of timestamp or something like that?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: 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] singhpk234 commented on a diff in pull request #7391: Build: Run Iceberg with JDK 17
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #7391:
URL: https://github.com/apache/iceberg/pull/7391#discussion_r1176826642
##########
api/src/main/java/org/apache/iceberg/expressions/ExpressionUtil.java:
##########
@@ -47,10 +47,10 @@ public class ExpressionUtil {
private static final Pattern DATE = Pattern.compile("\\d{4}-\\d{2}-\\d{2}");
private static final Pattern TIME = Pattern.compile("\\d{2}:\\d{2}(:\\d{2}(.\\d{1,6})?)?");
private static final Pattern TIMESTAMP =
- Pattern.compile("\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}(:\\d{2}(.\\d{1,6})?)?");
+ Pattern.compile("\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}(:\\d{2}(.\\d{1,9})?)?");
Review Comment:
yes with my mac on using JDK 17 existing regex would pass, but with ubuntu with JDK 17, was getting `2023-04-20T19:17:17.748170628Z` more details here: https://github.com/apache/iceberg/pull/7391#discussion_r1173100570
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on a diff in pull request #7391: Build: Run Iceberg with JDK 17
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on code in PR #7391:
URL: https://github.com/apache/iceberg/pull/7391#discussion_r1174318889
##########
build.gradle:
##########
@@ -271,6 +297,9 @@ project(':iceberg-bundled-guava') {
}
project(':iceberg-api') {
+ test {
Review Comment:
Is there a way to configure all jvm args instead of individually?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on a diff in pull request #7391: Build: Run Iceberg with JDK 17
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on code in PR #7391:
URL: https://github.com/apache/iceberg/pull/7391#discussion_r1174615249
##########
build.gradle:
##########
@@ -558,6 +586,7 @@ project(':iceberg-delta-lake') {
task integrationTest(type: Test) {
testClassesDirs = sourceSets.integration.output.classesDirs
classpath = sourceSets.integration.runtimeClasspath
+ jvmArgs += project.property('extraJvmArgs')
Review Comment:
There are a few other places for integration tests we should also add, although they are not run in CI
--
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] singhpk234 commented on a diff in pull request #7391: Build: Run Iceberg with JDK 17
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #7391:
URL: https://github.com/apache/iceberg/pull/7391#discussion_r1174613740
##########
build.gradle:
##########
@@ -271,6 +297,9 @@ project(':iceberg-bundled-guava') {
}
project(':iceberg-api') {
+ test {
Review Comment:
This works, except for integration test Task, thanks for the suggestion, have made the changes for remaining 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] singhpk234 commented on pull request #7391: Build: Run Iceberg Java Test with JDK 17
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on PR #7391:
URL: https://github.com/apache/iceberg/pull/7391#issuecomment-1517230277
Have added java 17 CI for all engine's as well, as part of commit [1bc99b8](https://github.com/apache/iceberg/pull/7391/commits/1bc99b805afe823604d9058b632af3135b3b5d1f), except Flink couldn't find minimum flink version since when java 17 is supported, found this issue https://issues.apache.org/jira/browse/FLINK-15736, but it seems un-resolved (cc @stevenzwu, who might have more context here)
---
This has now added new CI's, i proposed idea a while back to reduce the load on main iceberg repo here (https://github.com/apache/iceberg/issues/5153#issuecomment-1179510026) :
The idea is based on apache spark :
```
This is what Apache Spark does presently :
We create a PR and our repository triggers the workflow. Our PR uses the resources allocated to us for testing.
Apache Spark repository finds our workflow, and links it in a comment in our PR
```
We can do the same for iceberg, i.e run ci on our fork and iceberg just finds the run and attaches the workflow to our pr in iceberg repo.
As per spark folks :
```
Spark was one of the projects that uses the GitHub resources most in ASF, and now it's one of the lowest after this change :-).
```
Happy to create a pr to do same for iceberg.
--
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] singhpk234 commented on a diff in pull request #7391: Build: Run Iceberg with JDK 17
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #7391:
URL: https://github.com/apache/iceberg/pull/7391#discussion_r1174056807
##########
spark/v3.3/spark/src/test/java/org/apache/iceberg/spark/source/TestMetadataTableReadableMetrics.java:
##########
@@ -190,6 +191,7 @@ private GenericRecord createNestedRecord(Long longCol, Double doubleCol) {
}
@Test
+ @Ignore
Review Comment:
Added the analysis of this failure here : https://github.com/apache/iceberg/pull/7391#discussion_r1173527238, both are the same UT just in different spark verions
##########
build.gradle:
##########
@@ -271,6 +273,11 @@ project(':iceberg-bundled-guava') {
}
project(':iceberg-api') {
+ test {
+ if (JavaVersion.current() == JavaVersion.VERSION_17) {
Review Comment:
ACK
--
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 diff in pull request #7391: Build / Infra: Run Iceberg Java Test with JDK 17
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #7391:
URL: https://github.com/apache/iceberg/pull/7391#discussion_r1173173035
##########
api/src/main/java/org/apache/iceberg/expressions/ExpressionUtil.java:
##########
@@ -47,10 +47,10 @@ public class ExpressionUtil {
private static final Pattern DATE = Pattern.compile("\\d{4}-\\d{2}-\\d{2}");
private static final Pattern TIME = Pattern.compile("\\d{2}:\\d{2}(:\\d{2}(.\\d{1,6})?)?");
private static final Pattern TIMESTAMP =
- Pattern.compile("\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}(:\\d{2}(.\\d{1,6})?)?");
+ Pattern.compile("\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}(:\\d{2}(.\\d{1,10})?)?");
private static final Pattern TIMESTAMPTZ =
Pattern.compile(
- "\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}(:\\d{2}(.\\d{1,6})?)?([-+]\\d{2}:\\d{2}|Z)");
+ "\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}(:\\d{2}(.\\d{1,10})?)?([-+]\\d{2}:\\d{2}|Z)");
Review Comment:
Why 10 instead of 9?
--
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] Fokko commented on a diff in pull request #7391: Build: Run Iceberg with JDK 17
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #7391:
URL: https://github.com/apache/iceberg/pull/7391#discussion_r1173392100
##########
spark/v3.3/spark/src/test/java/org/apache/iceberg/spark/source/TestMetadataTableReadableMetrics.java:
##########
@@ -190,6 +191,7 @@ private GenericRecord createNestedRecord(Long longCol, Double doubleCol) {
}
@Test
+ @Ignore
Review Comment:
I think we should fix the UTs before adding JDK17, otherwise, it looks like JDK17 is supported but it might have weird quirks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on a diff in pull request #7391: Build: Run Iceberg with JDK 17
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on code in PR #7391:
URL: https://github.com/apache/iceberg/pull/7391#discussion_r1174317840
##########
.github/workflows/delta-conversion-ci.yml:
##########
@@ -88,7 +88,7 @@ jobs:
runs-on: ubuntu-22.04
strategy:
matrix:
- jvm: [ 8, 11 ]
+ jvm: [ 8, 11, 17 ]
Review Comment:
Nit: extra space in the end
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on a diff in pull request #7391: Build: Run Iceberg with JDK 17
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on code in PR #7391:
URL: https://github.com/apache/iceberg/pull/7391#discussion_r1175773589
##########
build.gradle:
##########
@@ -68,10 +68,36 @@ try {
if (JavaVersion.current() == JavaVersion.VERSION_1_8) {
project.ext.jdkVersion = '8'
+ project.ext.extraJvmArgs = []
} else if (JavaVersion.current() == JavaVersion.VERSION_11) {
project.ext.jdkVersion = '11'
+ project.ext.extraJvmArgs = []
+} else if (JavaVersion.current() == JavaVersion.VERSION_17) {
+ project.ext.jdkVersion = '17'
+ project.ext.extraJvmArgs = ["-XX:+IgnoreUnrecognizedVMOptions",
+ "--add-opens", "java.base/java.io=ALL-UNNAMED",
+ "--add-opens", "java.base/java.lang.invoke=ALL-UNNAMED",
+ "--add-opens", "java.base/java.lang.reflect=ALL-UNNAMED",
+ "--add-opens", "java.base/java.lang=ALL-UNNAMED",
+ "--add-opens", "java.base/java.math=ALL-UNNAMED",
+ "--add-opens", "java.base/java.net=ALL-UNNAMED",
+ "--add-opens", "java.base/java.nio=ALL-UNNAMED",
+ "--add-opens", "java.base/java.text=ALL-UNNAMED",
+ "--add-opens", "java.base/java.time=ALL-UNNAMED",
+ "--add-opens", "java.base/java.util.concurrent.atomic=ALL-UNNAMED",
+ "--add-opens", "java.base/java.util.concurrent=ALL-UNNAMED",
+ "--add-opens", "java.base/java.util.regex=ALL-UNNAMED",
+ "--add-opens", "java.base/java.util=ALL-UNNAMED",
+ "--add-opens", "java.base/jdk.internal.ref=ALL-UNNAMED",
+ "--add-opens", "java.base/jdk.internal.reflect=ALL-UNNAMED",
+ "--add-opens", "java.sql/java.sql=ALL-UNNAMED",
+ "--add-opens", "java.base/sun.util.calendar=ALL-UNNAMED",
+ "--add-opens", "java.base/sun.nio.ch=ALL-UNNAMED",
+ "--add-opens", "java.base/sun.nio.cs=ALL-UNNAMED",
+ "--add-opens", "java.base/sun.security.action=ALL-UNNAMED",
+ "--add-opens", "java.base/sun.util.calendar=ALL-UNNAMED"]
Review Comment:
maybe we could provide a list of classes to open and generate the `--add-opens` and `=ALL-UNNAMED` boilerplates. But that might be over-engineering, and I am fine as is. Curious what other people think.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on pull request #7391: Build: Run Iceberg with JDK 17
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on PR #7391:
URL: https://github.com/apache/iceberg/pull/7391#issuecomment-1522498949
Thanks for the work and everyone's review! Looks like everything is passing, and all comments are addressed. I will go ahead and merge it, we can fix anything remaining in follow-up PRs.
--
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