Build: Run Iceberg with JDK 17 #7391

singhpk234 · 2023-04-20T21:16:47Z

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.

Fixes: #7290

cc @jackye1995

singhpk234 · 2023-04-20T21:18:23Z

api/src/main/java/org/apache/iceberg/expressions/ExpressionUtil.java

  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)");


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.

Why 10 instead of 9?

ACK, counting mistake :P

Would TIME also be affected by this issue?

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

build.gradle

singhpk234 · 2023-04-21T04:08:25Z

Have added java 17 CI for all engine's as well, as part of commit 1bc99b8, 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, we can reduce the waiting time by the idea, i proposed idea a while back as part of reducing the load on main iceberg repo here (#5153 (comment)) :
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.

....4/spark/src/test/java/org/apache/iceberg/spark/source/TestMetadataTableReadableMetrics.java

....3/spark/src/test/java/org/apache/iceberg/spark/source/TestMetadataTableReadableMetrics.java

build.gradle

.github/workflows/delta-conversion-ci.yml

jackye1995 · 2023-04-22T07:15:08Z

build.gradle

+} else if (JavaVersion.current() == JavaVersion.VERSION_17) {
+  project.ext.jdkVersion = '17'
+  project.ext.extraJvmArgs = ["-XX:+IgnoreUnrecognizedVMOptions",
+                              "--add-opens", "java.base/java.io=ALL-UNNAMED",


Do we need to open all these?

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

build.gradle

....4/spark/src/test/java/org/apache/iceberg/spark/source/TestMetadataTableReadableMetrics.java

jackye1995 · 2023-04-24T20:55:20Z

build.gradle

+                              "--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"]


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.

jackye1995

Thanks for the work! Looks good to me, except for some open questions waiting for other comments from other people.

nastra

this looks mostly good to me (I've also double-checked whether we can specify all JVM args in gradle.properties via org.gradle.jvmargs but those aren't properly passed down).
I think it would be good to also double-check the difference in the sizes that required the test changes

singhpk234 · 2023-04-25T15:34:04Z

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 for the same, basically JDK 8 was reporting column size for binaryColfor 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.

jackye1995 · 2023-04-25T16:45:09Z

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})?)?");


sorry forgot to ask, why is this changed from 6 to 9? Does JDK17 change default precision of timestamp or something like that?

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: #7391 (comment)

jackye1995

waiting for CI to complete

jackye1995 · 2023-04-25T22:25:57Z

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.

* Updated README.md #7391 It's just a simple change in my thinking. * Reduce writing I like that it's shorter. Co-authored-by: Fokko Driesprong <fokko@apache.org> --------- Co-authored-by: Fokko Driesprong <fokko@apache.org>

* Updated README.md apache#7391 It's just a simple change in my thinking. * Reduce writing I like that it's shorter. Co-authored-by: Fokko Driesprong <fokko@apache.org> --------- Co-authored-by: Fokko Driesprong <fokko@apache.org>

github-actions bot added API build INFRA labels Apr 20, 2023

singhpk234 commented Apr 20, 2023

View reviewed changes

rdblue reviewed Apr 20, 2023

View reviewed changes

build.gradle Outdated Show resolved Hide resolved

rdblue changed the title ~~Build / Infra: Run Iceberg Java Test with JDK 17~~ Build: Run Iceberg Java Test with JDK 17 Apr 20, 2023

github-actions bot added MR spark labels Apr 21, 2023

singhpk234 commented Apr 21, 2023

View reviewed changes

....4/spark/src/test/java/org/apache/iceberg/spark/source/TestMetadataTableReadableMetrics.java Outdated Show resolved Hide resolved

singhpk234 changed the title ~~Build: Run Iceberg Java Test with JDK 17~~ Build: Run Iceberg with JDK 17 Apr 21, 2023

Fokko reviewed Apr 21, 2023

View reviewed changes

....3/spark/src/test/java/org/apache/iceberg/spark/source/TestMetadataTableReadableMetrics.java Outdated Show resolved Hide resolved

nastra reviewed Apr 21, 2023

View reviewed changes

build.gradle Outdated Show resolved Hide resolved

singhpk234 force-pushed the enhancement/run-iceberg-java-with-jdk17 branch from 1bc99b8 to 2c9a60e Compare April 21, 2023 19:21

singhpk234 requested review from rdblue, Fokko and nastra April 21, 2023 20:06

singhpk234 force-pushed the enhancement/run-iceberg-java-with-jdk17 branch 2 times, most recently from 791541f to b5d247d Compare April 22, 2023 04:23

jackye1995 reviewed Apr 22, 2023

View reviewed changes

.github/workflows/delta-conversion-ci.yml Outdated Show resolved Hide resolved

jackye1995 reviewed Apr 22, 2023

View reviewed changes

build.gradle Outdated Show resolved Hide resolved

singhpk234 force-pushed the enhancement/run-iceberg-java-with-jdk17 branch from b5d247d to d2faa94 Compare April 23, 2023 07:52

Prashant Singh added 7 commits April 23, 2023 08:46

java-17 add

f776dca

update gradle prop

c6f1195

address review feedback

3fc591d

add java 17 for all engines ci

1e64055

refactor gradles and address review feedback

8dd29ca

make metadata metric test more robust

b51b34c

Address review feedback

9bcbc9d

singhpk234 force-pushed the enhancement/run-iceberg-java-with-jdk17 branch from d2faa94 to 9bcbc9d Compare April 23, 2023 16:55

github-actions bot removed the MR label Apr 23, 2023

jackye1995 reviewed Apr 23, 2023

View reviewed changes

build.gradle Show resolved Hide resolved

Add jvm flags for aws integ test

1fd914f

jackye1995 reviewed Apr 24, 2023

View reviewed changes

....4/spark/src/test/java/org/apache/iceberg/spark/source/TestMetadataTableReadableMetrics.java Outdated Show resolved Hide resolved

jackye1995 reviewed Apr 24, 2023

View reviewed changes

Address review feedback

3d2fae1

singhpk234 force-pushed the enhancement/run-iceberg-java-with-jdk17 branch from 4ef73e0 to 3d2fae1 Compare April 24, 2023 21:43

jackye1995 approved these changes Apr 24, 2023

View reviewed changes

nastra approved these changes Apr 25, 2023

View reviewed changes

jackye1995 reviewed Apr 25, 2023

View reviewed changes

Add time regex

82fec3d

jackye1995 approved these changes Apr 25, 2023

View reviewed changes

jackye1995 merged commit f70e52c into apache:master Apr 25, 2023

911432 mentioned this pull request Apr 26, 2023

Updated README.md to support Java 17 #7434

Merged

manisin pushed a commit to Snowflake-Labs/iceberg that referenced this pull request May 9, 2023

Build: Run Iceberg with JDK 17 (apache#7391)

f03f339

singhpk234 mentioned this pull request Jun 13, 2024

Run Flink tests on Java 17 too #10477

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Build: Run Iceberg with JDK 17 #7391

Build: Run Iceberg with JDK 17 #7391

singhpk234 commented Apr 20, 2023 •

edited by jackye1995

Loading

singhpk234 Apr 20, 2023

rdblue Apr 20, 2023

singhpk234 Apr 21, 2023

jackye1995 Apr 25, 2023

singhpk234 Apr 25, 2023

singhpk234 commented Apr 21, 2023 •

edited

Loading

jackye1995 Apr 22, 2023

singhpk234 Apr 23, 2023

jackye1995 Apr 24, 2023

jackye1995 left a comment

nastra left a comment

singhpk234 commented Apr 25, 2023

jackye1995 Apr 25, 2023

singhpk234 Apr 25, 2023

jackye1995 left a comment

jackye1995 commented Apr 25, 2023

Build: Run Iceberg with JDK 17 #7391

Build: Run Iceberg with JDK 17 #7391

Conversation

singhpk234 commented Apr 20, 2023 • edited by jackye1995 Loading

About the change

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

singhpk234 commented Apr 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackye1995 left a comment

Choose a reason for hiding this comment

nastra left a comment

Choose a reason for hiding this comment

singhpk234 commented Apr 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackye1995 left a comment

Choose a reason for hiding this comment

jackye1995 commented Apr 25, 2023

singhpk234 commented Apr 20, 2023 •

edited by jackye1995

Loading

singhpk234 commented Apr 21, 2023 •

edited

Loading