-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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.
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
[Native] Introduce functional native test framework using TestContainers #23094
Conversation
df275f3
to
9a30dd5
Compare
e2bd85d
to
19e13fb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! (docs)
70a5951
to
2424368
Compare
This looks good, but actually most of the expressions in this query runner will be evaluated by the coordinator. Would it be possible to add TPCH data and relevant tests? |
3a65dd3
to
7174734
Compare
7174734
to
fcbd763
Compare
@tdcmeehan We have added a few relevant tests with tables using the TPCH connector. Can you please have a look? |
fcbd763
to
308d021
Compare
308d021
to
0c24e6d
Compare
...to-native-execution/src/test/java/com/facebook/presto/nativeworker/ContainerQueryRunner.java
Outdated
Show resolved
Hide resolved
...to-native-execution/src/test/java/com/facebook/presto/nativeworker/ContainerQueryRunner.java
Outdated
Show resolved
Hide resolved
...to-native-execution/src/test/java/com/facebook/presto/nativeworker/ContainerQueryRunner.java
Outdated
Show resolved
Hide resolved
...xecution/src/test/java/com/facebook/presto/nativeworker/TestPrestoContainerBasicQueries.java
Show resolved
Hide resolved
0c24e6d
to
b67de20
Compare
...to-native-execution/src/test/java/com/facebook/presto/nativeworker/ContainerQueryRunner.java
Outdated
Show resolved
Hide resolved
...tive-execution/src/test/java/com/facebook/presto/nativeworker/ContainerQueryRunnerUtils.java
Outdated
Show resolved
Hide resolved
...tive-execution/src/test/java/com/facebook/presto/nativeworker/ContainerQueryRunnerUtils.java
Outdated
Show resolved
Hide resolved
...tive-execution/src/test/java/com/facebook/presto/nativeworker/ContainerQueryRunnerUtils.java
Outdated
Show resolved
Hide resolved
...tive-execution/src/test/java/com/facebook/presto/nativeworker/ContainerQueryRunnerUtils.java
Outdated
Show resolved
Hide resolved
...tive-execution/src/test/java/com/facebook/presto/nativeworker/ContainerQueryRunnerUtils.java
Outdated
Show resolved
Hide resolved
...tive-execution/src/test/java/com/facebook/presto/nativeworker/ContainerQueryRunnerUtils.java
Outdated
Show resolved
Hide resolved
...tive-execution/src/test/java/com/facebook/presto/nativeworker/ContainerQueryRunnerUtils.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Joe-Abraham. Have couple of comments.
...to-tests/src/main/java/com/facebook/presto/tests/AbstractTestNativeTpchConnectorQueries.java
Show resolved
Hide resolved
...xecution/src/test/java/com/facebook/presto/nativeworker/TestPrestoContainerBasicQueries.java
Outdated
Show resolved
Hide resolved
...xecution/src/test/java/com/facebook/presto/nativeworker/TestPrestoContainerBasicQueries.java
Outdated
Show resolved
Hide resolved
...to-native-execution/src/test/java/com/facebook/presto/nativeworker/ContainerQueryRunner.java
Outdated
Show resolved
Hide resolved
...tive-execution/src/test/java/com/facebook/presto/nativeworker/ContainerQueryRunnerUtils.java
Outdated
Show resolved
Hide resolved
...tive-execution/src/test/java/com/facebook/presto/nativeworker/ContainerQueryRunnerUtils.java
Outdated
Show resolved
Hide resolved
...tive-execution/src/test/java/com/facebook/presto/nativeworker/ContainerQueryRunnerUtils.java
Outdated
Show resolved
Hide resolved
...tive-execution/src/test/java/com/facebook/presto/nativeworker/ContainerQueryRunnerUtils.java
Outdated
Show resolved
Hide resolved
94ba486
to
90d7497
Compare
b928bc5
to
311da10
Compare
311da10
to
8011b8c
Compare
TimeUnit.SECONDS.sleep(5); | ||
} | ||
catch (InterruptedException e) { | ||
throw new RuntimeException(e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or just continue on?
execResult = coordinator.execInContainer(command); | ||
} | ||
catch (IOException e) { | ||
fail("Presto CLI failed with error message: " + e.getMessage()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's clearer to the end user to let the exception bubble up and fail the test than convert it into a fail call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Override
public MaterializedResult execute(Session session, String sql) { ... }
Can we add exceptions to an override method? If so, aren't we required to edit its base class and the other children of the parent class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this will also introduce many changes in com/facebook/presto/sql/query/QueryAssertions.java
wherever the execute gets called.
...to-native-execution/src/test/java/com/facebook/presto/nativeworker/ContainerQueryRunner.java
Outdated
Show resolved
Hide resolved
...to-native-execution/src/test/java/com/facebook/presto/nativeworker/ContainerQueryRunner.java
Outdated
Show resolved
Hide resolved
...tive-execution/src/test/java/com/facebook/presto/nativeworker/ContainerQueryRunnerUtils.java
Outdated
Show resolved
Hide resolved
...tive-execution/src/test/java/com/facebook/presto/nativeworker/ContainerQueryRunnerUtils.java
Outdated
Show resolved
Hide resolved
...tive-execution/src/test/java/com/facebook/presto/nativeworker/ContainerQueryRunnerUtils.java
Outdated
Show resolved
Hide resolved
...tive-execution/src/test/java/com/facebook/presto/nativeworker/ContainerQueryRunnerUtils.java
Outdated
Show resolved
Hide resolved
...tive-execution/src/test/java/com/facebook/presto/nativeworker/ContainerQueryRunnerUtils.java
Outdated
Show resolved
Hide resolved
extends AbstractTestQueryFramework | ||
{ | ||
@Test | ||
public void testTpchTinyTables() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use a data provider to break this up into tests that an pass and fail independently.
15bb23d
to
a90d325
Compare
a90d325
to
ecb833b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple of nits, otherwise LGTM
return ContainerQueryRunnerUtils.toMaterializedResult(execResult.getStdout()); | ||
} | ||
catch (IOException | InterruptedException e) { | ||
fail("Execute failed with error message: " + e.getMessage()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rethrow the IOException
as UncheckedIOException
. For InterruptedException
, just rethrow as RuntimeException
.
return Boolean.parseBoolean(value); | ||
} | ||
else { | ||
fail("Unsupported type: " + type); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just throw here
fail("Unsupported type: " + type); | |
throw new UnsupportedOperationException("Unsupported type: " + type); |
ecb833b
to
13f39a9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two more minor nits
private final String catalog; | ||
private final String schema; | ||
private final int numberOfWorkers; | ||
Logger logger = Logger.getLogger(ContainerQueryRunner.class.getName()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logger logger = Logger.getLogger(ContainerQueryRunner.class.getName()); | |
private static Logger logger = Logger.getLogger(ContainerQueryRunner.class.getName()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
modified it
this.schema = schema; | ||
this.numberOfWorkers = numberOfWorkers; | ||
|
||
// TODO: This framework is tested only in Ubuntu x86_64, as there is no support to run the native docker images in ARM based system, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you create an issue for this once it's merged?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The work is in progress #23275
13f39a9
to
053c38d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lazy review. +1
Resolves: #23086