Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Upgrade Transport to use API from Trino v406 #128
Upgrade Transport to use API from Trino v406 #128
Changes from 19 commits
31303a0
94472c5
4dd7dec
af417d8
9d5f073
9b73f39
0a7631a
8b71131
ebeec1c
555b84b
774ecc2
566e70e
5281f97
91a9e01
581cb5e
917a5ae
da0c0ba
13588af
82286fb
04c68e3
2722cbe
97f8777
29b067f
f795e24
a414767
d5260de
75e2e3a
d1ff33a
b18c82e
8d38fc5
213178a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
doesn't this module use TestNG? why is JUnit 5 added here?
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.
Yes, this module use TestNG. However, as the tests for Trino use the class of
QueryAssertions
from Trino, if this dependency is not added, the tests for Trino will fail because of the following error: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.
Could you add the next step for it? Sounds like it's an issue of Trino
QueryAssertions
? Maybe add a TODO item here and other places?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.
+1 disabling the tests does not seem like the right course of action here
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.
Are you able to run the failed test with
QueryAssertion
now? It was not yet clear whyQueryAssertion
failed previously with the old way usingSqlScalarFunction
. Annotation-driven functions had no issue withQueryAssertion
.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 test still fails with
QueryAssertions
. BrieflyQueryAssertions
requires to compare the outputs from two queries with the same UDF, however in case of binary UDF, two queries results in the execution of different code paths in Trino which makes the inputs to UDF are different, so the outputs of the queries are different. The details are added in the comments of the code.I have created a TODO issue to reenable them after the root cause is found in Trino and fixed as shown in #131
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 don't understand this change. Previously we asserted that NPE was thrown. Now you seem to be asserting that NPE is thrown only when
trinoTest
is false? But whentrinoTest
is true we do expect the exception to be thrown, don't we?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 code of this test are executed for Trino, Hive and Spark. Originally it expects a NullPointerException is thrown during the execution of looking up a null value in a file by all three query engines. However, Trino v406 does not throw NullPointerException but returns a null value in this case. Therefore the code change removes the expected exception from @test annotation, and only checks if a NullPointerException is caught if it is not in case of Trino.
Also I have already added some comments to explain the reason in the code.