Run selected tests with Java 19 #15709

findepi · 2023-01-13T13:13:49Z

In order to future-proof us for Java update, run selected tests (trino-main) with JDK 19 too.

This updates some tests due to Double.toString() and Float.toString() changes in JDK 19.

findepi · 2023-01-13T13:14:55Z

I expected this to fail at this point, in ManageTestResources, because ExecutorService is now AutoCloseable too.
Easy to fix, but want to first see the ci is working properly for this change.

findepi · 2023-01-13T13:15:03Z

.github/workflows/ci.yml

@@ -576,6 +577,8 @@ jobs:
                github.event.client_payload.pull_request.head.sha == github.event.client_payload.slash_command.args.named.sha &&
                format('refs/pull/{0}/head', github.event.client_payload.pull_request.number) || '' }}
      - uses: ./.github/actions/setup
+        with:
+          java-version: ${{ matrix.jdk != '' && matrix.jdk || '17' }}


cc @nineinchnick

findepi · 2023-01-13T16:36:52Z

Expectedly, https://github.com/trinodb/trino/actions/runs/3911618479/jobs/6686349744

2023-01-13T09:40:03.216-0600	INFO	main	stderr	FATAL: io.trino.testng.services.ManageTestResources: 
	Test instance field has value that looks like a resource
	    Test class: class io.trino.execution.TestCommitTask
	    Instance: io.trino.execution.TestCommitTask@62eaacdd
	    Field: private java.util.concurrent.ExecutorService io.trino.execution.TestCommitTask.executor
	    Value: java.util.concurrent.ThreadPoolExecutor@18449ee3[Running, pool size = 0, active threads = 0, queued tasks = 0, completed tasks = 0]
	    Test execution stage: BEFORE_CLASS
	    Matching rule: isAutoCloseable

	Resources must not be allocated in a field initializer or a test constructor,
	and must be freed when test class is completed.
	Depending which rule has been violated, if the reported class holds on
	to resources only conditionally, you can add @ResourcePresence to declare that.

findepi · 2023-01-13T16:39:28Z

CI #13633

hashhar · 2023-01-13T17:39:53Z

What about compilation of plugins too? Maven-checks would be nice to have with 19 too.

martint · 2023-01-13T23:50:31Z

How does it affect CI run time?

hashhar · 2023-01-16T07:18:19Z

How does it affect CI run time?

Adds 23 mins. ci / test (core/trino-main, 19) (pull_request) Failing after 23m

findepi · 2023-01-16T10:32:03Z

What about compilation of plugins too?

It would come at an even higher the cost. We're not ready to stop testing with 17.

Think of this PR just as a foot in a door for Java 19 compat, we're not going thru the door yet.

Maven-checks would be nice to have with 19 too.

That would be mostly testing the maven plugins themselves, not our code.

findepi · 2023-01-16T11:18:26Z

I updated some tests

ExecutorService interface new close method caused a challenge for assertAllMethodsOverridden(ListeningExecutorService.class, DecoratingListeningExecutorService.class) cc @losipiuk @kokosing
new BigDecimal(a_double) hasn't changed between version, but we use BigDecimal.valueOf(a_double) when casting to decimal, and this has changed. I updated expected results in tests, I hope this is the right thing to do, @martint .

losipiuk

ExecutorService change LGTM

findepi · 2023-01-16T11:48:48Z

ExecutorService change LGTM

Thanks @losipiuk for reviewing it. I added reference to google/guava#6296 in the code comment.

findepi · 2023-01-16T16:35:07Z

core/trino-main/src/test/java/io/trino/type/TestArrayOperators.java

-                .isEqualTo("[3.14,1.0E-323,1.0E308,\"NaN\",\"Infinity\",\"-Infinity\",null]");
+                .isEqualTo(Runtime.version().feature() >= 19
+                        ? "[3.14,9.9E-324,1.0E308,\"NaN\",\"Infinity\",\"-Infinity\",null]"
+                        : "[3.14,1.0E-323,1.0E308,\"NaN\",\"Infinity\",\"-Infinity\",null]");


double a = 1.0E-323; double b = 9.9E-324; System.out.println(a == b); // true

in both Java 17 and 19. I.e. same value, different representation.

In order to future-proof us for Java update, run selected tests (`trino-main`) with JDK 19 too.

martint · 2023-01-16T21:49:41Z

core/trino-main/src/test/java/io/trino/type/TestDecimalCasts.java

@@ -267,7 +267,9 @@ public void testDoubleToShortDecimalCasts()
        assertDecimalFunction("CAST(DOUBLE '-1234567890.0' AS DECIMAL(20,10))", decimal("-1234567890.0000000000", createDecimalType(20, 10)));
        assertDecimalFunction("CAST(DOUBLE '1234567890.0' AS DECIMAL(30,20))", decimal("1234567890.00000000000000000000", createDecimalType(30, 20)));
        assertDecimalFunction("CAST(DOUBLE '-1234567890.0' AS DECIMAL(30,20))", decimal("-1234567890.00000000000000000000", createDecimalType(30, 20)));
-        assertDecimalFunction("CAST(DOUBLE '123456789123456784' AS DECIMAL(18,0))", decimal("123456789123456784", createDecimalType(18)));
+        assertDecimalFunction("CAST(DOUBLE '123456789123456784' AS DECIMAL(18,0))", Runtime.version().feature() >= 19


This seems like a bug somewhere since the resulting numbers are different. If the problem is that the input value doesn't have a precise representation, then we should change the test to have one that does. Ideally, we shouldn't be gating all tests based on the version of the runtime. It makes it harder to reason about correctness.

In any case, it's a behavior change. For same input, same expression Trino on Java 19 produces different results. I am grateful to the test author for writing this test, otherwise I wouldn't know the behavior changed.

Ideally, we shouldn't be gating all tests based on the version of the runtime.

If the results depend on runtime version, then it's a necessity.
The alternative of changing/reducing test coverage isn't exactly as exciting to me.

It makes it harder to reason about correctness.

Agreed!
Fortunately it's easy to compare 17's and 19's expected values, but yet it's a human's job to do that.

OTOH it's temporary. Once we move to 19+ as required runtime, we will remove the conditionality here.

This seems like a bug somewhere since the resulting numbers are different

If we call it a bug, it would be a correctness bug, right?

FWIW here are some details

double input = 123456789123456784d; // 17: "17" // 18: "18" // 19: "19" System.out.println(Runtime.version().feature()); // This is equivalent to part of io.trino.spi.type.DecimalConversions#internalDoubleToLongDecimal // 17: "123456789123456784000000000000000000" // 18: "123456789123456784000000000000000000" // 19: "123456789123456780000000000000000000" System.out.println(BigDecimal.valueOf(input).setScale(18, HALF_UP).unscaledValue()); // Some more details on the above expression: // 17: "123456789123456784" // 18: "123456789123456784" // 19: "1.2345678912345678E+17" System.out.println(BigDecimal.valueOf(input)); // java.math.BigDecimal.valueOf(double) uses Double.toString (in Java 17, 18 and 19) // 17: "1.23456789123456784E17" // 18: "1.23456789123456784E17" // 19: "1.2345678912345678E17" System.out.println(Double.toString(input));

Is this https://bugs.openjdk.org/browse/JDK-4511638 ? This is considered a bug in JDK, fixed in 19

So this is expected for Double/Float.toString to have a different ("more" correct) representation in the 19

Thanks @wendigo for the pointer!

findepi · 2023-01-17T15:38:43Z

I am going to merge this as-is. It's a almost purely test-only change, and there were no concerns about non-test part of that. The problems uncovered are not something I believe is a bug, but in any case they can be fixed as a followup.

Alternative would be

remove test coverage so that behavioral change is not visible (i am sure change visibility is more important than nice formatting of test cases though)
not merge this, and keep zero coverage for Java 19 (but some community members want to run on Java 19 already, and it's nice to help them so we're ready later)
decide whether old or new behavior is correct (for floating points it can be "old", "new", "both" or "none"), and make Trino do same thing irrespective of runtime first, before merging this PR (but this can be as well taken care of in a followup; also having test coverage first would help reviewing such a product change, as we would have CI exercising it for two runtimes)

cla-bot bot added the cla-signed label Jan 13, 2023

findepi commented Jan 13, 2023

View reviewed changes

nineinchnick approved these changes Jan 13, 2023

View reviewed changes

findepi force-pushed the findepi/j19 branch from 56c6c62 to 569b302 Compare January 13, 2023 16:39

losipiuk approved these changes Jan 13, 2023

View reviewed changes

findepi changed the title ~~Run selected tetss with Java 19~~ Run selected tests with Java 19 Jan 13, 2023

findepi force-pushed the findepi/j19 branch from 569b302 to 7ad91f6 Compare January 13, 2023 16:41

findepi force-pushed the findepi/j19 branch from 7ad91f6 to b019399 Compare January 16, 2023 11:16

losipiuk approved these changes Jan 16, 2023

View reviewed changes

findepi force-pushed the findepi/j19 branch from b019399 to 519b4cf Compare January 16, 2023 11:48

findepi force-pushed the findepi/j19 branch from 519b4cf to 7518abc Compare January 16, 2023 16:34

findepi commented Jan 16, 2023

View reviewed changes

Test trino-main with Java 19

bd63e1a

In order to future-proof us for Java update, run selected tests (`trino-main`) with JDK 19 too.

findepi force-pushed the findepi/j19 branch from 7518abc to bd63e1a Compare January 16, 2023 16:36

martint reviewed Jan 16, 2023

View reviewed changes

findepi merged commit 9ce592b into trinodb:master Jan 18, 2023

findepi deleted the findepi/j19 branch January 18, 2023 11:09

github-actions bot added this to the 406 milestone Jan 18, 2023

colebow mentioned this pull request Jan 18, 2023

Add Trino 406 release notes #15625

Merged

findepi mentioned this pull request May 13, 2023

Put testing with with JDK 20 on hold #17491

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run selected tests with Java 19 #15709

Run selected tests with Java 19 #15709

findepi commented Jan 13, 2023 •

edited by electrum

Loading

findepi commented Jan 13, 2023

findepi Jan 13, 2023

findepi commented Jan 13, 2023

findepi commented Jan 13, 2023

hashhar commented Jan 13, 2023

martint commented Jan 13, 2023

hashhar commented Jan 16, 2023

findepi commented Jan 16, 2023

findepi commented Jan 16, 2023

losipiuk left a comment

findepi commented Jan 16, 2023

findepi Jan 16, 2023

martint Jan 16, 2023

findepi Jan 17, 2023

findepi Jan 17, 2023

findepi Jan 17, 2023

wendigo Jan 17, 2023

wendigo Jan 17, 2023

findepi Jan 18, 2023

findepi commented Jan 17, 2023

Run selected tests with Java 19 #15709

Run selected tests with Java 19 #15709

Conversation

findepi commented Jan 13, 2023 • edited by electrum Loading

findepi commented Jan 13, 2023

Choose a reason for hiding this comment

findepi commented Jan 13, 2023

findepi commented Jan 13, 2023

hashhar commented Jan 13, 2023

martint commented Jan 13, 2023

hashhar commented Jan 16, 2023

findepi commented Jan 16, 2023

findepi commented Jan 16, 2023

losipiuk left a comment

Choose a reason for hiding this comment

findepi commented Jan 16, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

findepi commented Jan 17, 2023

findepi commented Jan 13, 2023 •

edited by electrum

Loading