Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Run selected tests with Java 19 #15709

Merged
merged 1 commit into from
Jan 18, 2023
Merged

Run selected tests with Java 19 #15709

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -513,6 +513,7 @@ jobs:
include:
- { modules: [ client/trino-jdbc, plugin/trino-base-jdbc, plugin/trino-thrift, plugin/trino-memory ] }
- { modules: core/trino-main }
- { modules: core/trino-main, jdk: 19 }
- { modules: plugin/trino-accumulo }
- { modules: plugin/trino-bigquery }
- { modules: plugin/trino-cassandra }
Expand Down Expand Up @@ -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' }}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @nineinchnick

- name: Cleanup node
# This is required as a virtual environment update 20210219.1 left too little space for MemSQL to work
if: matrix.modules == 'plugin/trino-singlestore'
Expand Down
45 changes: 45 additions & 0 deletions core/trino-main/src/main/java/io/trino/dispatcher/DecoratingListeningExecutorService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,21 +17,49 @@
import com.google.common.util.concurrent.ListenableFuture;
import com.google.common.util.concurrent.ListeningExecutorService;

import javax.annotation.Nullable;

import java.lang.invoke.MethodHandle;
import java.lang.invoke.MethodHandles;
import java.lang.reflect.Method;
import java.util.Collection;
import java.util.List;
import java.util.concurrent.Callable;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Future;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.TimeoutException;

import static com.google.common.base.Throwables.throwIfUnchecked;
import static com.google.common.collect.ImmutableList.toImmutableList;
import static java.util.Objects.requireNonNull;

public class DecoratingListeningExecutorService
extends ForwardingListeningExecutorService
implements ListeningExecutorService
{
// TODO remove after requiring Java 19+ for runtime.
private static final @Nullable MethodHandle CLOSE_METHOD;

static {
Method closeMethod;
try {
closeMethod = ExecutorService.class.getMethod("close");
}
catch (NoSuchMethodException e) {
closeMethod = null;
}
try {
CLOSE_METHOD = closeMethod != null
? MethodHandles.lookup().unreflect(closeMethod)
: null;
}
catch (IllegalAccessException e) {
throw new RuntimeException(e);
}
}

private final ListeningExecutorService delegate;
private final TaskDecorator decorator;

Expand Down Expand Up @@ -142,6 +170,23 @@ public boolean awaitTermination(long timeout, TimeUnit unit)
return super.awaitTermination(timeout, unit);
}

// TODO This is temporary, until Guava's ForwardingExecutorService has the method in their interface. See https://github.com/google/guava/issues/6296
//@Override
public void close()
{
if (CLOSE_METHOD == null) {
throw new UnsupportedOperationException("ExecutorService.close has close() method since Java 19. " +
"The DecoratingListeningExecutorService supports the method only when run with Java 19 runtime.");
}
try {
CLOSE_METHOD.invoke(delegate());
}
catch (Throwable e) {
throwIfUnchecked(e);
throw new RuntimeException(e);
}
}

public interface TaskDecorator
{
Runnable decorate(Runnable command);
Expand Down
8 changes: 6 additions & 2 deletions core/trino-main/src/test/java/io/trino/sql/planner/TestUnwrapCastInComparison.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -628,12 +628,16 @@ public void testNoEffect()
// INTEGER->REAL implicit cast is not injective if the real constant is >= 2^23 and <= 2^31 - 1
testUnwrap("integer", "a = REAL '8388608'", "CAST(a AS REAL) = REAL '8388608.0'");

testUnwrap("integer", "a = REAL '2147483647'", "CAST(a AS REAL) = REAL '2.14748365E9'");
testUnwrap("integer", "a = REAL '2147483647'", Runtime.version().feature() >= 19
? "CAST(a AS REAL) = REAL '2.1474836E9'"
: "CAST(a AS REAL) = REAL '2.14748365E9'");

// INTEGER->REAL implicit cast is not injective if the real constant is <= -2^23 and >= -2^31 + 1
testUnwrap("integer", "a = REAL '-8388608'", "CAST(a AS REAL) = REAL '-8388608.0'");

testUnwrap("integer", "a = REAL '-2147483647'", "CAST(a AS REAL) = REAL '-2.14748365E9'");
testUnwrap("integer", "a = REAL '-2147483647'", Runtime.version().feature() >= 19
? "CAST(a AS REAL) = REAL '-2.1474836E9'"
: "CAST(a AS REAL) = REAL '-2.14748365E9'");

// DECIMAL(p)->DOUBLE not injective for p > 15
testUnwrap("decimal(16)", "a = DOUBLE '1'", "CAST(a AS DOUBLE) = 1E0");
Expand Down
4 changes: 3 additions & 1 deletion core/trino-main/src/test/java/io/trino/type/TestArrayOperators.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,7 +318,9 @@ public void testArrayToJson()
assertThat(assertions.expression("CAST(a AS JSON)")
.binding("a", "ARRAY[3.14E0, 1e-323, 1e308, nan(), infinity(), -infinity(), null]"))
.hasType(JSON)
.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]");
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

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.


assertThat(assertions.expression("CAST(a AS JSON)")
.binding("a", "ARRAY[DECIMAL '3.14', null]"))
Expand Down
56 changes: 42 additions & 14 deletions core/trino-main/src/test/java/io/trino/type/TestDecimalCasts.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @wendigo for the pointer!

? decimal("123456789123456780", createDecimalType(18))
: decimal("123456789123456784", createDecimalType(18)));
assertDecimalFunction("CAST(DOUBLE '123456789.123456790' AS DECIMAL(18,9))", decimal("123456789.123456790", createDecimalType(18, 9)));

// test rounding
Expand Down Expand Up @@ -367,18 +369,42 @@ public void testFloatToDecimalCasts()
assertDecimalFunction("CAST(REAL '1000' AS DECIMAL(4,0))", decimal("1000", createDecimalType(4)));
assertDecimalFunction("CAST(REAL '1000.01' AS DECIMAL(7,2))", decimal("01000.01", createDecimalType(7, 2)));
assertDecimalFunction("CAST(REAL '-234.0' AS DECIMAL(3,0))", decimal("-234", createDecimalType(3)));
assertDecimalFunction("CAST(REAL '12345678400000000' AS DECIMAL(17,0))", decimal("12345678400000000", createDecimalType(17)));
assertDecimalFunction("CAST(REAL '-12345678400000000' AS DECIMAL(17,0))", decimal("-12345678400000000", createDecimalType(17)));
assertDecimalFunction("CAST(REAL '1234567940' AS DECIMAL(20,10))", decimal("1234567940.0000000000", createDecimalType(20, 10)));
assertDecimalFunction("CAST(REAL '-1234567940' AS DECIMAL(20,10))", decimal("-1234567940.0000000000", createDecimalType(20, 10)));
assertDecimalFunction("CAST(REAL '1234567940' AS DECIMAL(30,20))", decimal("1234567940.00000000000000000000", createDecimalType(30, 20)));
assertDecimalFunction("CAST(REAL '-1234567940' AS DECIMAL(30,20))", decimal("-1234567940.00000000000000000000", createDecimalType(30, 20)));
assertDecimalFunction("CAST(REAL '123456790519087104' AS DECIMAL(18,0))", decimal("123456791000000000", createDecimalType(18)));
assertDecimalFunction("CAST(REAL '-123456790519087104' AS DECIMAL(18,0))", decimal("-123456791000000000", createDecimalType(18)));
assertDecimalFunction("CAST(REAL '123456790519087104' AS DECIMAL(20,2))", decimal("123456791000000000.00", createDecimalType(20, 2)));
assertDecimalFunction("CAST(REAL '-123456790519087104' AS DECIMAL(20,2))", decimal("-123456791000000000.00", createDecimalType(20, 2)));
assertDecimalFunction("CAST(REAL '1234567905190871' AS DECIMAL(18,2))", decimal("1234567950000000.00", createDecimalType(18, 2)));
assertDecimalFunction("CAST(REAL '-1234567905190871' AS DECIMAL(18,2))", decimal("-1234567950000000.00", createDecimalType(18, 2)));
assertDecimalFunction("CAST(REAL '12345678400000000' AS DECIMAL(17,0))", Runtime.version().feature() >= 19
? decimal("12345678000000000", createDecimalType(17))
: decimal("12345678400000000", createDecimalType(17)));
assertDecimalFunction("CAST(REAL '-12345678400000000' AS DECIMAL(17,0))", Runtime.version().feature() >= 19
? decimal("-12345678000000000", createDecimalType(17))
: decimal("-12345678400000000", createDecimalType(17)));
assertDecimalFunction("CAST(REAL '1234567940' AS DECIMAL(20,10))", Runtime.version().feature() >= 19
? decimal("1234568000.0000000000", createDecimalType(20, 10))
: decimal("1234567940.0000000000", createDecimalType(20, 10)));
assertDecimalFunction("CAST(REAL '-1234567940' AS DECIMAL(20,10))", Runtime.version().feature() >= 19
? decimal("-1234568000.0000000000", createDecimalType(20, 10))
: decimal("-1234567940.0000000000", createDecimalType(20, 10)));
assertDecimalFunction("CAST(REAL '1234567940' AS DECIMAL(30,20))", Runtime.version().feature() >= 19
? decimal("1234568000.00000000000000000000", createDecimalType(30, 20))
: decimal("1234567940.00000000000000000000", createDecimalType(30, 20)));
assertDecimalFunction("CAST(REAL '-1234567940' AS DECIMAL(30,20))", Runtime.version().feature() >= 19
? decimal("-1234568000.00000000000000000000", createDecimalType(30, 20))
: decimal("-1234567940.00000000000000000000", createDecimalType(30, 20)));
assertDecimalFunction("CAST(REAL '123456790519087104' AS DECIMAL(18,0))", Runtime.version().feature() >= 19
? decimal("123456790000000000", createDecimalType(18))
: decimal("123456791000000000", createDecimalType(18)));
assertDecimalFunction("CAST(REAL '-123456790519087104' AS DECIMAL(18,0))", Runtime.version().feature() >= 19
? decimal("-123456790000000000", createDecimalType(18))
: decimal("-123456791000000000", createDecimalType(18)));
assertDecimalFunction("CAST(REAL '123456790519087104' AS DECIMAL(20,2))", Runtime.version().feature() >= 19
? decimal("123456790000000000.00", createDecimalType(20, 2))
: decimal("123456791000000000.00", createDecimalType(20, 2)));
assertDecimalFunction("CAST(REAL '-123456790519087104' AS DECIMAL(20,2))", Runtime.version().feature() >= 19
? decimal("-123456790000000000.00", createDecimalType(20, 2))
: decimal("-123456791000000000.00", createDecimalType(20, 2)));
assertDecimalFunction("CAST(REAL '1234567905190871' AS DECIMAL(18,2))", Runtime.version().feature() >= 19
? decimal("1234568000000000.00", createDecimalType(18, 2))
: decimal("1234567950000000.00", createDecimalType(18, 2)));
assertDecimalFunction("CAST(REAL '-1234567905190871' AS DECIMAL(18,2))", Runtime.version().feature() >= 19
? decimal("-1234568000000000.00", createDecimalType(18, 2))
: decimal("-1234567950000000.00", createDecimalType(18, 2)));
assertDecimalFunction("CAST(REAL '1456213.432632456' AS DECIMAL(18,9))", decimal("001456213.400000000", createDecimalType(18, 9)));

// test roundtrip
Expand All @@ -390,7 +416,9 @@ public void testFloatToDecimalCasts()
assertInvalidCast("CAST(REAL '234.0' AS DECIMAL(2,0))", "Cannot cast REAL '234.0' to DECIMAL(2, 0)");
assertInvalidCast("CAST(REAL '1000.01' AS DECIMAL(5,2))", "Cannot cast REAL '1000.01' to DECIMAL(5, 2)");
assertInvalidCast("CAST(REAL '-234.0' AS DECIMAL(2,0))", "Cannot cast REAL '-234.0' to DECIMAL(2, 0)");
assertInvalidCast("CAST(REAL '98765430784.0' AS DECIMAL(20, 10))", "Cannot cast REAL '9.8765431E10' to DECIMAL(20, 10)");
assertInvalidCast("CAST(REAL '98765430784.0' AS DECIMAL(20, 10))", Runtime.version().feature() >= 19
? "Cannot cast REAL '9.876543E10' to DECIMAL(20, 10)"
: "Cannot cast REAL '9.8765431E10' to DECIMAL(20, 10)");

assertInvalidCast("CAST(CAST(nan() as REAL) AS DECIMAL(10,5))", "Cannot cast REAL 'NaN' to DECIMAL(10, 5)");
assertInvalidCast("CAST(CAST(infinity() as REAL) AS DECIMAL(10,1))", "Cannot cast REAL 'Infinity' to DECIMAL(10, 1)");
Expand Down
8 changes: 6 additions & 2 deletions core/trino-main/src/test/java/io/trino/type/TestMapOperators.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,7 +278,9 @@ public void testMapToJson()
assertThat(assertions.expression("cast(a as JSON)")
.binding("a", "MAP(ARRAY[1e-323,1e308,nan()], ARRAY[-323,308,null])"))
.hasType(JSON)
.isEqualTo("{\"1.0E-323\":-323,\"1.0E308\":308,\"NaN\":null}");
.isEqualTo(Runtime.version().feature() >= 19
? "{\"1.0E308\":308,\"9.9E-324\":-323,\"NaN\":null}"
: "{\"1.0E-323\":-323,\"1.0E308\":308,\"NaN\":null}");

assertThat(assertions.expression("cast(a as JSON)")
.binding("a", "MAP(ARRAY[DECIMAL '3.14', DECIMAL '0.01'], ARRAY[0.14, null])"))
Expand Down Expand Up @@ -329,7 +331,9 @@ public void testMapToJson()
assertThat(assertions.expression("cast(a as JSON)")
.binding("a", "MAP(ARRAY[1, 2, 3, 5, 8, 13, 21], ARRAY[3.14E0, 1e-323, 1e308, nan(), infinity(), -infinity(), null])"))
.hasType(JSON)
.isEqualTo("{\"1\":3.14,\"13\":\"-Infinity\",\"2\":1.0E-323,\"21\":null,\"3\":1.0E308,\"5\":\"NaN\",\"8\":\"Infinity\"}");
.isEqualTo(Runtime.version().feature() >= 19
? "{\"1\":3.14,\"13\":\"-Infinity\",\"2\":9.9E-324,\"21\":null,\"3\":1.0E308,\"5\":\"NaN\",\"8\":\"Infinity\"}"
: "{\"1\":3.14,\"13\":\"-Infinity\",\"2\":1.0E-323,\"21\":null,\"3\":1.0E308,\"5\":\"NaN\",\"8\":\"Infinity\"}");

assertThat(assertions.expression("cast(a as JSON)")
.binding("a", "MAP(ARRAY[1, 2], ARRAY[DECIMAL '3.14', null])"))
Expand Down
5 changes: 5 additions & 0 deletions ...ng/trino-testing-services/src/main/java/io/trino/testng/services/ManageTestResources.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,6 +250,11 @@ private static Rule isAutoCloseable()
if (!(value instanceof AutoCloseable)) {
return false;
}
if (value instanceof ExecutorService) {
// Covered by isLeftoverExecutorService.
// ExecutorService is AutoCloseable since Java 19.
return false;
}

// Disallow AutoCloseable instances, even if not started, before class is run.
// Allocation of such resources generally correlates with too eager initialization.
Expand Down